From 1aaba1f2853673ff41397275771899de7308198a Mon Sep 17 00:00:00 2001 From: Doug Gregor Date: Wed, 5 Jun 2024 22:44:44 -0700 Subject: [PATCH] Don't check sendability of witnesses for `@preconcurrency` conformances When a witness is part of a `@preconcurrency` conformance, suppress Sendable checking for that witness because we assume that the caller is correctly invoking this witness from within the right isolation domain. This property is dynamically checked. Fixes https://github.com/apple/swift/issues/74057. --- lib/Sema/TypeCheckProtocol.cpp | 61 ++++++++++--------- lib/Sema/TypeCheckProtocol.h | 6 +- .../preconcurrency_conformances.swift | 23 ++++--- 3 files changed, 52 insertions(+), 38 deletions(-) diff --git a/lib/Sema/TypeCheckProtocol.cpp b/lib/Sema/TypeCheckProtocol.cpp index d66b35c1b6881..0785b7ef65069 100644 --- a/lib/Sema/TypeCheckProtocol.cpp +++ b/lib/Sema/TypeCheckProtocol.cpp @@ -3090,7 +3090,8 @@ static bool hasExplicitGlobalActorAttr(ValueDecl *decl) { std::optional ConformanceChecker::checkActorIsolation(ValueDecl *requirement, - ValueDecl *witness) { + ValueDecl *witness, + bool &usesPreconcurrency) { // Determine the isolation of the requirement itself. auto requirementIsolation = getActorIsolation(requirement); @@ -3136,9 +3137,16 @@ ConformanceChecker::checkActorIsolation(ValueDecl *requirement, return std::nullopt; case ActorReferenceResult::ExitsActorToNonisolated: - diagnoseNonSendableTypesInReference( - /*base=*/nullptr, getDeclRefInContext(witness), - DC, loc, SendableCheckReason::Conformance); + if (!isPreconcurrency) { + diagnoseNonSendableTypesInReference( + /*base=*/nullptr, getDeclRefInContext(witness), + DC, loc, SendableCheckReason::Conformance); + } else { + // We depended on @preconcurrency since we were exiting an isolation + // domain. + usesPreconcurrency = true; + } + return std::nullopt; case ActorReferenceResult::EntersActor: // Handled below. @@ -3220,19 +3228,24 @@ ConformanceChecker::checkActorIsolation(ValueDecl *requirement, witness->getAttrs().hasAttribute()) return std::nullopt; - // Check that the results of the witnessing method are sendable - diagnoseNonSendableTypesInReference( - /*base=*/nullptr, getDeclRefInContext(witness), - DC, loc, SendableCheckReason::Conformance, - getActorIsolation(witness), FunctionCheckKind::Results); - - // If this requirement is a function, check that its parameters are Sendable as well - if (isa(requirement)) { + if (!isPreconcurrency) { + // Check that the results of the witnessing method are sendable diagnoseNonSendableTypesInReference( - /*base=*/nullptr, getDeclRefInContext(requirement), - requirement->getInnermostDeclContext(), requirement->getLoc(), - SendableCheckReason::Conformance, getActorIsolation(witness), - FunctionCheckKind::Params, loc); + /*base=*/nullptr, getDeclRefInContext(witness), + DC, loc, SendableCheckReason::Conformance, + getActorIsolation(witness), FunctionCheckKind::Results); + + // If this requirement is a function, check that its parameters are Sendable as well + if (isa(requirement)) { + diagnoseNonSendableTypesInReference( + /*base=*/nullptr, getDeclRefInContext(requirement), + requirement->getInnermostDeclContext(), requirement->getLoc(), + SendableCheckReason::Conformance, getActorIsolation(witness), + FunctionCheckKind::Params, loc); + } + } else { + // We depended on @preconcurrency to suppress Sendable checking. + usesPreconcurrency = true; } // If the witness is accessible across actors, we don't need to consider it @@ -4955,7 +4968,7 @@ hasInvalidTypeInConformanceContext(const ValueDecl *requirement, } void ConformanceChecker::resolveValueWitnesses() { - bool usesPreconcurrencyConformance = false; + bool usesPreconcurrency = false; for (auto *requirement : Proto->getProtocolRequirements()) { // Associated type requirements handled elsewhere. @@ -4974,16 +4987,8 @@ void ConformanceChecker::resolveValueWitnesses() { // Check actor isolation. If we need to enter into the actor's // isolation within the witness thunk, record that. - if (auto enteringIsolation = checkActorIsolation(requirement, witness)) { - // Only @preconcurrency conformances allow entering isolation - // when neither requirement nor witness are async by adding a - // runtime precondition that witness is always called on the - // expected executor. - if (Conformance->isPreconcurrency()) { - usesPreconcurrencyConformance = - !isAsyncDecl(requirement) && !isAsyncDecl(witness); - } - + if (auto enteringIsolation = checkActorIsolation(requirement, witness, + usesPreconcurrency)) { Conformance->overrideWitness( requirement, Conformance->getWitnessUncached(requirement) @@ -5134,7 +5139,7 @@ void ConformanceChecker::resolveValueWitnesses() { } } - if (Conformance->isPreconcurrency() && !usesPreconcurrencyConformance) { + if (Conformance->isPreconcurrency() && !usesPreconcurrency) { auto diag = DC->getASTContext().Diags.diagnose( Conformance->getLoc(), diag::preconcurrency_conformance_not_used, Proto->getDeclaredInterfaceType()); diff --git a/lib/Sema/TypeCheckProtocol.h b/lib/Sema/TypeCheckProtocol.h index 35da8cdf260a6..004417af5d020 100644 --- a/lib/Sema/TypeCheckProtocol.h +++ b/lib/Sema/TypeCheckProtocol.h @@ -130,11 +130,15 @@ class ConformanceChecker : public WitnessChecker { /// Check that the witness and requirement have compatible actor contexts. /// + /// \param usesPreconcurrency Will be set true if the conformance is + /// @preconcurrency and we made use of that fact. + /// /// \returns the isolation that needs to be enforced to invoke the witness /// from the requirement, used when entering an actor-isolated synchronous /// witness from an asynchronous requirement. std::optional checkActorIsolation(ValueDecl *requirement, - ValueDecl *witness); + ValueDecl *witness, + bool &usesPreconcurrency); /// Enforce restrictions on non-final classes witnessing requirements /// involving the protocol 'Self' type. diff --git a/test/Concurrency/preconcurrency_conformances.swift b/test/Concurrency/preconcurrency_conformances.swift index 969688ba02ff0..42a953fc8e8ff 100644 --- a/test/Concurrency/preconcurrency_conformances.swift +++ b/test/Concurrency/preconcurrency_conformances.swift @@ -35,28 +35,36 @@ extension TestPreconcurrencyAttr : @preconcurrency Q { // Ok } class NonSendable {} -// expected-note@-1 6 {{class 'NonSendable' does not conform to the 'Sendable' protocol}} protocol TestSendability { var x: NonSendable { get } func test(_: NonSendable?) -> [NonSendable] } -// Make sure that preconcurrency conformances don't suppress Sendable diagnostics +// Make sure that preconcurrency conformances suppress Sendable diagnostics, +// because @preconcurrency assumes that the witness will always be called +// from within the same isolation domain (with a dynamic check). @MainActor struct Value : @preconcurrency TestSendability { var x: NonSendable { NonSendable() } - // expected-warning@-1 {{non-sendable type 'NonSendable' in conformance of main actor-isolated property 'x' to protocol requirement cannot cross actor boundary}} - // expected-note@-2 2 {{property declared here}} + // expected-note@-1 2 {{property declared here}} - // expected-warning@+2 {{non-sendable type '[NonSendable]' returned by main actor-isolated instance method 'test' satisfying protocol requirement cannot cross actor boundary}} - // expected-warning@+1 {{non-sendable type 'NonSendable?' in parameter of the protocol requirement satisfied by main actor-isolated instance method 'test' cannot cross actor boundary}} func test(_: NonSendable?) -> [NonSendable] { // expected-note@-1 2 {{calls to instance method 'test' from outside of its actor context are implicitly asynchronous}} [] } } +// Make sure we don't spuriously say the @preconcurrency is unnecessary. +@MainActor +struct OtherValue : @preconcurrency TestSendability { + var x: NonSendable { NonSendable() } + + nonisolated func test(_: NonSendable?) -> [NonSendable] { + [] + } +} + // Make sure that references to actor isolated witness is diagnosed // expected-note@+1 2 {{add '@MainActor' to make global function 'test(value:)' part of global actor 'MainActor'}} @@ -73,10 +81,7 @@ actor MyActor { extension MyActor : @preconcurrency TestSendability { var x: NonSendable { NonSendable() } - // expected-warning@-1 {{non-sendable type 'NonSendable' in conformance of actor-isolated property 'x' to protocol requirement cannot cross actor boundary}} - // expected-warning@+2 {{non-sendable type '[NonSendable]' returned by actor-isolated instance method 'test' satisfying protocol requirement cannot cross actor boundary}} - // expected-warning@+1 {{non-sendable type 'NonSendable?' in parameter of the protocol requirement satisfied by actor-isolated instance method 'test' cannot cross actor boundary}} func test(_: NonSendable?) -> [NonSendable] { [] }