Skip to content

[Concurrency] Do not allow actor isolation violations in function conversions to impact constraint solving. #68685

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

Merged
merged 1 commit into from
Sep 22, 2023
Merged

[Concurrency] Do not allow actor isolation violations in function conversions to impact constraint solving. #68685

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
78 changes: 9 additions & 69 deletions lib/Sema/CSSimplify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2955,51 +2955,6 @@ static bool isPreconcurrency(ConstraintSystem &cs, DeclContext *dc) {
llvm_unreachable("unhandled DeclContext kind in isPreconcurrency");
}

/// A thin wrapper around \c swift::safeToDropGlobalActor that provides the
/// ability to infer the isolation of a closure. Not perfect but mostly works.
static bool okToRemoveGlobalActor(ConstraintSystem &cs,
DeclContext *dc,
Type globalActor, Type ty) {
auto findGlobalActorForClosure = [&](AbstractClosureExpr *ace) -> ActorIsolation {
// FIXME: Because the actor isolation checking happens after constraint
// solving, the closure expression does not yet have its actor isolation
// set, i.e., the code that would call
// `AbstractClosureExpr::setActorIsolation` hasn't run yet.
// So, I expect the existing isolation to always be set to the default.
// If the assertion below starts tripping, then this ad-hoc inference
// is no longer needed!
auto existingIso = ace->getActorIsolation();
if (existingIso != ActorIsolation::forUnspecified()) {
assert(false && "somebody set the closure's isolation already?");
return existingIso;
}

// Otherwise, do an ad-hoc inference of the closure's isolation.

// see if the closure's type has isolation.
if (auto closType = GetClosureType{cs}(ace)) {
if (auto fnTy = closType->getAs<AnyFunctionType>()) {
if (auto globActor = fnTy->getGlobalActor()) {
return ActorIsolation::forGlobalActor(globActor, /*unsafe=*/false)
.withPreconcurrency(isPreconcurrency(cs, ace));
}
}
}

// otherwise, check for an explicit annotation
if (auto *ce = dyn_cast<ClosureExpr>(ace)) {
if (auto globActor = getExplicitGlobalActor(ce)) {
return ActorIsolation::forGlobalActor(globActor, /*unsafe=*/false)
.withPreconcurrency(isPreconcurrency(cs, ace));
}
}

return existingIso;
};

return safeToDropGlobalActor(dc, globalActor, ty, findGlobalActorForClosure);
}

ConstraintSystem::TypeMatchResult
ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,
ConstraintKind kind, TypeMatchOptions flags,
Expand Down Expand Up @@ -3067,7 +3022,15 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,
return getTypeMatchFailure(locator);
}

// A global actor can be added or can match, but cannot be removed.
// Check the global-actor isolation on each of the function types. Two
// different global-actor attributes never match, but function conversions
// that add or remove global-actor attributes are okay.
//
// Some function conversions that erase global-actor isolation can admit
// data races; such violations are diagnosed by the actor isolation checker.
// We deliberately do not allow actor isolation violations to influence
// overload resolution to preserve the property that an expression can be
// re-checked against a different isolation context for isolation violations.
if (func1->getGlobalActor() || func2->getGlobalActor()) {
if (func1->getGlobalActor() && func2->getGlobalActor()) {
// If both have a global actor, match them.
Expand All @@ -3079,29 +3042,6 @@ ConstraintSystem::matchFunctionTypes(FunctionType *func1, FunctionType *func2,
if (result == SolutionKind::Error)
return getTypeMatchFailure(locator);

} else if (func1->getGlobalActor() && !func2->isAsync()) {
// Cannot remove a global actor from a synchronous function in mismatched
// DeclContext.
//
// FIXME: the ConstraintSystem's DeclContext is not always precise enough
// to give an accurate answer. We want the innermost DeclContext that
// contains the expression associated with the constraint locator.
// Sometimes the ConstraintSystem only has the enclosing function, and not
// the inner closure containing the expression. To workaround this,
// `ActorIsolationChecker::checkFunctionConversion`, has extra checking of
// function conversions specifically to account for this false positive.
// This means we may have duplicate diagnostics emitted.
if (okToRemoveGlobalActor(*this, DC, func1->getGlobalActor(), func2)) {
// FIXME: this is a bit of a hack to workaround multiple solutions
// because in these contexts it's valid to both add or remove the actor
// from these function types. At least with the score increases, we
// can bias the solver to pick the solution with fewer conversions.
increaseScore(SK_FunctionConversion, locator);

} else if (MarkGlobalActorFunction::attempt(*this, kind, func1, func2, locator)) {
return getTypeMatchFailure(locator);
}

} else if (kind < ConstraintKind::Subtype) {
return getTypeMatchFailure(locator);
} else {
Expand Down
7 changes: 6 additions & 1 deletion test/Concurrency/actor_call_implicitly_async.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,7 +299,7 @@ func blender(_ peeler : () -> Void) {
await (((wisk)))((wisk)((wisk)(1)))

blender((peelBanana))
// expected-warning@-1 2{{converting function value of type '@BananaActor () -> ()' to '() -> Void' loses global actor 'BananaActor'}}
// expected-warning@-1 {{converting function value of type '@BananaActor () -> ()' to '() -> Void' loses global actor 'BananaActor'}}

await wisk(peelBanana)
// expected-complete-warning@-1{{passing argument of non-sendable type '() -> ()' into global actor 'BananaActor'-isolated context may introduce data races}}
Expand All @@ -314,7 +314,12 @@ func blender(_ peeler : () -> Void) {

await {wisk}()(1)

// FIXME: Poor diagnostic. The issue is that the invalid function conversion
// to remove '@BananaActor' on 'wisk' cannot influence which solution is chosen.
// So, the constraint system cannot determine whether the type of this expression
// is '(Any) -> Void' or '@BananaActor (Any) -> Void'.
await (true ? wisk : {n in return})(1)
// expected-error@-1 {{type of expression is ambiguous without a type annotation}}
}

actor Chain {
Expand Down
2 changes: 1 addition & 1 deletion test/Concurrency/actor_isolation.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,7 @@ func testGlobalActorClosures() {
return 17
}

acceptConcurrentClosure { @SomeGlobalActor in 5 } // expected-warning 2{{converting function value of type '@SomeGlobalActor @Sendable () -> Int' to '@Sendable () -> Int' loses global actor 'SomeGlobalActor'}}
acceptConcurrentClosure { @SomeGlobalActor in 5 } // expected-warning {{converting function value of type '@SomeGlobalActor @Sendable () -> Int' to '@Sendable () -> Int' loses global actor 'SomeGlobalActor'}}
}

@available(SwiftStdlib 5.1, *)
Expand Down
Loading