Skip to content

[Concurrency] Downgrade missing await error to a warning for synchronous access to isolated global/static lets. #70151

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.

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
Dec 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 8 additions & 2 deletions lib/Sema/TypeCheckConcurrency.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -538,12 +538,18 @@ static bool varIsSafeAcrossActors(const ModuleDecl *fromModule,
}

bool swift::isLetAccessibleAnywhere(const ModuleDecl *fromModule,
VarDecl *let) {
VarDecl *let,
ActorReferenceResult::Options &options) {
auto isolation = getActorIsolation(let);
ActorReferenceResult::Options options = llvm::None;
return varIsSafeAcrossActors(fromModule, let, isolation, options);
}

bool swift::isLetAccessibleAnywhere(const ModuleDecl *fromModule,
VarDecl *let) {
ActorReferenceResult::Options options = llvm::None;
return isLetAccessibleAnywhere(fromModule, let, options);
}

namespace {
/// Describes the important parts of a partial apply thunk.
struct PartialApplyThunkInfo {
Expand Down
7 changes: 7 additions & 0 deletions lib/Sema/TypeCheckConcurrency.h
Original file line number Diff line number Diff line change
Expand Up @@ -560,6 +560,13 @@ bool isAccessibleAcrossActors(
const DeclContext *fromDC,
llvm::Optional<ReferencedActor> actorInstance = llvm::None);

/// Determines if the 'let' can be read from anywhere within the given module,
/// regardless of the isolation or async-ness of the context in which
/// the var is read.
bool isLetAccessibleAnywhere(const ModuleDecl *fromModule,
VarDecl *let,
ActorReferenceResult::Options &options);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I added this as a new overload because the original is in include/swift/AST/ActorIsolation.h and I didn't see a good reason to expose ActorReferenceResult outside of Sema.


/// Check whether given variable references to a potentially
/// isolated actor.
bool isPotentiallyIsolatedActor(
Expand Down
52 changes: 48 additions & 4 deletions lib/Sema/TypeCheckEffects.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -718,6 +718,8 @@ static bool isRethrowLikeTypedThrows(AbstractFunctionDecl *func) {
class Classification {
bool IsInvalid = false; // The AST is malformed. Don't diagnose.

bool downgradeToWarning = false;

// Throwing
ConditionalEffectKind ThrowKind = ConditionalEffectKind::None;
llvm::Optional<PotentialEffectReason> ThrowReason;
Expand Down Expand Up @@ -968,6 +970,13 @@ class Classification {
bool isInvalid() const { return IsInvalid; }
void makeInvalid() { IsInvalid = true; }

bool shouldDowngradeToWarning() const {
return downgradeToWarning;
}
void setDowngradeToWarning(bool downgrade) {
downgradeToWarning = downgrade;
}

ConditionalEffectKind getConditionalKind(EffectKind kind) const {
switch (kind) {
case EffectKind::Throws: return ThrowKind;
Expand Down Expand Up @@ -1044,6 +1053,23 @@ class ApplyClassifier {
return Classification();
}

/// Whether a missing 'await' error on accessing an async var should be
/// downgraded to a warning. This is only the case for synchronous access
/// to isolated global or static 'let' variables.
bool downgradeAsyncAccessToWarning(Decl *decl) {
if (auto *var = dyn_cast<VarDecl>(decl)) {
ActorReferenceResult::Options options = llvm::None;
// The newly-diagnosed cases are invalid regardless of the module context
// of the caller, i.e. isolated static and global 'let' variables.
auto *module = var->getDeclContext()->getParentModule();
if (!isLetAccessibleAnywhere(module, var, options)) {
return options.contains(ActorReferenceResult::Flags::Preconcurrency);
}
}

return false;
}

Classification classifyLookup(LookupExpr *E) {
auto member = E->getMember();
if (!member)
Expand All @@ -1060,6 +1086,9 @@ class ApplyClassifier {
reason = PotentialEffectReason::forSubscriptAccess();
}

classification.setDowngradeToWarning(
downgradeAsyncAccessToWarning(member.getDecl()));

classification.mergeImplicitEffects(
member.getDecl()->getASTContext(),
E->isImplicitlyAsync().has_value(), E->isImplicitlyThrows(),
Expand All @@ -1086,6 +1115,9 @@ class ApplyClassifier {
declRef, ConditionalEffectKind::Always, reason);
}

classification.setDowngradeToWarning(
downgradeAsyncAccessToWarning(E->getDecl()));

classification.mergeImplicitEffects(
E->getDeclRef().getDecl()->getASTContext(),
E->isImplicitlyAsync().has_value(), E->isImplicitlyThrows(),
Expand Down Expand Up @@ -2548,15 +2580,20 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>

struct DiagnosticInfo {
DiagnosticInfo(Expr &failingExpr,
PotentialEffectReason reason) :
PotentialEffectReason reason,
bool downgradeToWarning) :
reason(reason),
expr(failingExpr) {}
expr(failingExpr),
downgradeToWarning(downgradeToWarning) {}

/// Reason for throwing
PotentialEffectReason reason;

/// Failing expression
Expr &expr;

/// Whether the error should be downgraded to a warning.
bool downgradeToWarning;
};

SmallVector<Expr *, 4> errorOrder;
Expand Down Expand Up @@ -3113,8 +3150,9 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
CurContext.isWithinInterpolatedString());
if (uncoveredAsync.find(anchor) == uncoveredAsync.end())
errorOrder.push_back(anchor);
uncoveredAsync[anchor].emplace_back(*expr,
classification.getAsyncReason());
uncoveredAsync[anchor].emplace_back(
*expr, classification.getAsyncReason(),
classification.shouldDowngradeToWarning());
}
}

Expand Down Expand Up @@ -3335,7 +3373,13 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
awaitInsertLoc = tryExpr->getSubExpr()->getStartLoc();
}

bool downgradeToWarning = llvm::all_of(errors,
[&](DiagnosticInfo diag) -> bool {
return diag.downgradeToWarning;
});

Ctx.Diags.diagnose(anchor->getStartLoc(), diag::async_expr_without_await)
.warnUntilSwiftVersionIf(downgradeToWarning, 6)
.fixItInsert(awaitInsertLoc, "await ")
.highlight(anchor->getSourceRange());

Expand Down
23 changes: 19 additions & 4 deletions test/Concurrency/concurrent_value_checking.swift
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,8 @@
// REQUIRES: concurrency
// REQUIRES: asserts

class NotConcurrent { } // expected-note 16{{class 'NotConcurrent' does not conform to the 'Sendable' protocol}}
// expected-complete-note @-1 11{{class 'NotConcurrent' does not conform to the 'Sendable' protocol}}
class NotConcurrent { } // expected-note 18{{class 'NotConcurrent' does not conform to the 'Sendable' protocol}}
// expected-complete-note @-1 12{{class 'NotConcurrent' does not conform to the 'Sendable' protocol}}

// ----------------------------------------------------------------------
// Sendable restriction on actor operations
Expand Down Expand Up @@ -115,12 +115,27 @@ func globalAsync(_: NotConcurrent?) async {
globalSync(nil)
}

enum E {
@SomeGlobalActor static let notSafe: NotConcurrent? = nil
}

func globalTest() async {
// expected-error@+2 {{expression is 'async' but is not marked with 'await'}}
// expected-warning@+2 {{expression is 'async' but is not marked with 'await'}}
// expected-note@+1 {{property access is 'async'}}
let a = globalValue // expected-warning{{non-sendable type 'NotConcurrent?' in implicitly asynchronous access to global actor 'SomeGlobalActor'-isolated let 'globalValue' cannot cross actor boundary}}
await globalAsync(a) // expected-complete-warning{{passing argument of non-sendable type 'NotConcurrent?' into global actor 'SomeGlobalActor'-isolated context may introduce data races}}
await globalSync(a) // expected-complete-warning{{passing argument of non-sendable type 'NotConcurrent?' into global actor 'SomeGlobalActor'-isolated context may introduce data races}}

// expected-warning@+2 {{expression is 'async' but is not marked with 'await'}}
// expected-note@+1 {{property access is 'async'}}
let _ = E.notSafe // expected-warning{{non-sendable type 'NotConcurrent?' in implicitly asynchronous access to global actor 'SomeGlobalActor'-isolated static property 'notSafe' cannot cross actor boundary}}

// expected-error@+3 {{expression is 'async' but is not marked with 'await'}}
// expected-note@+2 {{call is 'async'}}
// expected-note@+1 {{property access is 'async'}}
globalAsync(E.notSafe)
// expected-complete-warning@-1 {{passing argument of non-sendable type 'NotConcurrent?' into global actor 'SomeGlobalActor'-isolated context may introduce data races}}
// expected-warning@-2 {{non-sendable type 'NotConcurrent?' in implicitly asynchronous access to global actor 'SomeGlobalActor'-isolated static property 'notSafe' cannot cross actor boundary}}
}

struct HasSubscript {
Expand All @@ -139,7 +154,7 @@ class ClassWithGlobalActorInits { // expected-note 2{{class 'ClassWithGlobalActo

@MainActor
func globalTestMain(nc: NotConcurrent) async {
// expected-error@+2 {{expression is 'async' but is not marked with 'await'}}
// expected-warning@+2 {{expression is 'async' but is not marked with 'await'}}
// expected-note@+1 {{property access is 'async'}}
let a = globalValue // expected-warning{{non-sendable type 'NotConcurrent?' in implicitly asynchronous access to global actor 'SomeGlobalActor'-isolated let 'globalValue' cannot cross actor boundary}}
await globalAsync(a) // expected-complete-warning{{passing argument of non-sendable type 'NotConcurrent?' into global actor 'SomeGlobalActor'-isolated context may introduce data races}}
Expand Down