Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't check sendability of witnesses for @preconcurrency conformances #74172

Merged
merged 1 commit into from
Jun 7, 2024
Merged

Don't check sendability of witnesses for @preconcurrency conformances #74172

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 33 additions & 28 deletions lib/Sema/TypeCheckProtocol.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3090,7 +3090,8 @@ static bool hasExplicitGlobalActorAttr(ValueDecl *decl) {

std::optional<ActorIsolation>
ConformanceChecker::checkActorIsolation(ValueDecl *requirement,
ValueDecl *witness) {
ValueDecl *witness,
bool &usesPreconcurrency) {

// Determine the isolation of the requirement itself.
auto requirementIsolation = getActorIsolation(requirement);
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -3220,19 +3228,24 @@ ConformanceChecker::checkActorIsolation(ValueDecl *requirement,
witness->getAttrs().hasAttribute<NonisolatedAttr>())
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<AbstractFunctionDecl>(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<AbstractFunctionDecl>(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
Expand Down Expand Up @@ -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.
Expand All @@ -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)
Expand Down Expand Up @@ -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());
Expand Down
6 changes: 5 additions & 1 deletion lib/Sema/TypeCheckProtocol.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<ActorIsolation> checkActorIsolation(ValueDecl *requirement,
ValueDecl *witness);
ValueDecl *witness,
bool &usesPreconcurrency);

/// Enforce restrictions on non-final classes witnessing requirements
/// involving the protocol 'Self' type.
Expand Down
23 changes: 14 additions & 9 deletions test/Concurrency/preconcurrency_conformances.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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'}}
Expand All @@ -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] {
[]
}
Expand Down