Skip to content

[Concurrency] Improve diagnostics for missing await #37263

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 2 commits into from
May 5, 2021
Merged

[Concurrency] Improve diagnostics for missing await #37263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8e78c8b
[CSApply] Propagate 'async' from contextual types to closures, unless…
hborla May 3, 2021
b6a3434
[Concurrency] Emit a tailored note for a missing await if the call is…
hborla May 5, 2021
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
33 changes: 33 additions & 0 deletions lib/Sema/CSApply.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
#include "swift/AST/ASTVisitor.h"
#include "swift/AST/ASTWalker.h"
#include "swift/AST/ClangModuleLoader.h"
#include "swift/AST/Effects.h"
#include "swift/AST/ExistentialLayout.h"
#include "swift/AST/GenericEnvironment.h"
#include "swift/AST/GenericSignature.h"
Expand Down Expand Up @@ -7043,6 +7044,38 @@ Expr *ExprRewriter::coerceToType(Expr *expr, Type toType,
}
}

/// Whether the given effect should be propagated to a closure expression.
auto shouldApplyEffect = [&](EffectKind kind) -> bool {
auto last = locator.last();
if (!(last && last->is<LocatorPathElt::ApplyArgToParam>()))
return true;

// The effect should not be applied if the closure is an argument
// to a function where that effect is polymorphic.
if (auto *call = getAsExpr<ApplyExpr>(locator.getAnchor())) {
if (auto *declRef = dyn_cast<DeclRefExpr>(call->getFn())) {
if (auto *fn = dyn_cast<AbstractFunctionDecl>(declRef->getDecl()))
return !fn->hasPolymorphicEffect(kind);
}
}

return true;
};

// If we have a ClosureExpr, we can safely propagate 'async' to the closure.
fromEI = fromFunc->getExtInfo();
if (toEI.isAsync() && !fromEI.isAsync() && shouldApplyEffect(EffectKind::Async)) {
auto newFromFuncType = fromFunc->withExtInfo(fromEI.withAsync());
if (applyTypeToClosureExpr(cs, expr, newFromFuncType)) {
fromFunc = newFromFuncType->castTo<FunctionType>();

// Propagating 'async' might have satisfied the entire conversion.
// If so, we're done, otherwise keep converting.
if (fromFunc->isEqual(toType))
return expr;
}
}

// If we have a ClosureExpr, then we can safely propagate the 'no escape'
// bit to the closure without invalidating prior analysis.
fromEI = fromFunc->getExtInfo();
Expand Down
3 changes: 2 additions & 1 deletion lib/Sema/TypeCheckConcurrency.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1722,7 +1722,8 @@ namespace {
}

// Mark as implicitly async.
apply->setImplicitlyAsync(true);
if (!fnType->getExtInfo().isAsync())
apply->setImplicitlyAsync(true);

// If we don't need to check for sendability, we're done.
if (!shouldDiagnoseNonSendableViolations(ctx.LangOpts))
Expand Down
14 changes: 12 additions & 2 deletions lib/Sema/TypeCheckEffects.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2871,8 +2871,18 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
}
continue;
}
Ctx.Diags.diagnose(diag.expr.getStartLoc(),
diag::async_access_without_await, 0);

auto *call = dyn_cast<ApplyExpr>(&diag.expr);
if (call && call->implicitlyAsync()) {
// Emit a tailored note if the call is implicitly async, meaning the
// callee is isolated to an actor.
auto callee = call->getCalledValue();
Ctx.Diags.diagnose(diag.expr.getStartLoc(), diag::actor_isolated_sync_func,
callee->getDescriptiveKind(), callee->getName());
} else {
Ctx.Diags.diagnose(diag.expr.getStartLoc(),
diag::async_access_without_await, 0);
}

continue;
}
Expand Down
16 changes: 9 additions & 7 deletions test/Concurrency/actor_call_implicitly_async.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,26 +207,28 @@ extension BankAccount {
func totalBalance(including other: BankAccount) async -> Int {
//expected-error@+1{{expression is 'async' but is not marked with 'await'}}{{12-12=await }}
return balance()
+ other.balance() // expected-note{{call is 'async'}}
+ other.balance() // expected-note{{calls to instance method 'balance()' from outside of its actor context are implicitly asynchronous}}
Copy link
Member

Choose a reason for hiding this comment

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

👏 Beautiful!

}

func breakAccounts(other: BankAccount) async {
// expected-error@+1{{expression is 'async' but is not marked with 'await'}}{{9-9=await }}
_ = other.deposit( // expected-note{{call is 'async'}}
other.withdraw( // expected-note{{call is 'async'}}
_ = other.deposit( // expected-note{{calls to instance method 'deposit' from outside of its actor context are implicitly asynchronous}}
other.withdraw( // expected-note{{calls to instance method 'withdraw' from outside of its actor context are implicitly asynchronous}}
self.deposit(
other.withdraw( // expected-note{{call is 'async'}}
other.balance())))) // expected-note{{call is 'async'}}
other.withdraw( // expected-note{{calls to instance method 'withdraw' from outside of its actor context are implicitly asynchronous}}
other.balance())))) // expected-note{{calls to instance method 'balance()' from outside of its actor context are implicitly asynchronous}}
}
}

func anotherAsyncFunc() async {
let a = BankAccount(initialDeposit: 34)
let b = BankAccount(initialDeposit: 35)

// expected-error@+1{{expression is 'async' but is not marked with 'await'}} {{7-7=await }} expected-note@+1{{call is 'async'}}
// expected-error@+2{{expression is 'async' but is not marked with 'await'}} {{7-7=await }}
// expected-note@+1{{calls to instance method 'deposit' from outside of its actor context are implicitly asynchronous}}
_ = a.deposit(1)
// expected-error@+1{{expression is 'async' but is not marked with 'await'}} {{7-7=await }} expected-note@+1{{call is 'async'}}
// expected-error@+2{{expression is 'async' but is not marked with 'await'}} {{7-7=await }}
// expected-note@+1{{calls to instance method 'balance()' from outside of its actor context are implicitly asynchronous}}
_ = b.balance()

_ = b.balance // expected-error {{actor-isolated instance method 'balance()' can only be referenced from inside the actor}}
Expand Down
29 changes: 15 additions & 14 deletions test/Concurrency/actor_isolation.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ actor MyActor: MySuperActor { // expected-error{{actor types do not support inhe
class func synchronousClass() { }
static func synchronousStatic() { }

func synchronous() -> String { text.first ?? "nothing" } // expected-note 20{{calls to instance method 'synchronous()' from outside of its actor context are implicitly asynchronous}}
func synchronous() -> String { text.first ?? "nothing" } // expected-note 19{{calls to instance method 'synchronous()' from outside of its actor context are implicitly asynchronous}}
func asynchronous() async -> String {
super.superState += 4
return synchronous()
Expand Down Expand Up @@ -202,7 +202,7 @@ extension MyActor {

_ = otherActor.synchronous()
// expected-error@-1{{expression is 'async' but is not marked with 'await'}}{{9-9=await }}
// expected-note@-2{{call is 'async'}}
// expected-note@-2{{calls to instance method 'synchronous()' from outside of its actor context are implicitly asynchronous}}
_ = await otherActor.asynchronous()
_ = otherActor.text[0]
// expected-error@-1{{expression is 'async' but is not marked with 'await'}}{{9-9=await }}
Expand All @@ -222,7 +222,7 @@ extension MyActor {

// Global actors
syncGlobalActorFunc() // expected-error{{expression is 'async' but is not marked with 'await'}}{{5-5=await }}
// expected-note@-1{{call is 'async'}}
// expected-note@-1{{calls to global function 'syncGlobalActorFunc()' from outside of its actor context are implicitly asynchronous}}

await asyncGlobalActorFunc()

Expand Down Expand Up @@ -399,7 +399,7 @@ actor Crystal {
@SomeGlobalActor func goo1() async {
let _ = goo2
// expected-error@+1{{expression is 'async' but is not marked with 'await'}}{{3-3=await }}
goo2() // expected-note{{call is 'async'}}
goo2() // expected-note{{calls to global function 'goo2()' from outside of its actor context are implicitly asynchronous}}
}
@available(macOS 9999, iOS 9999, watchOS 9999, tvOS 9999, *)
@asyncHandler @SomeOtherGlobalActor func goo2() { await goo1() }
Expand All @@ -409,7 +409,7 @@ func testGlobalActorClosures() {
let _: Int = acceptAsyncClosure { @SomeGlobalActor in
syncGlobalActorFunc()
syncOtherGlobalActorFunc() // expected-error{{expression is 'async' but is not marked with 'await'}}{{5-5=await }}
// expected-note@-1{{call is 'async'}}
// expected-note@-1{{calls to global function 'syncOtherGlobalActorFunc()' from outside of its actor context are implicitly asynchronous}}

await syncOtherGlobalActorFunc()
return 17
Expand All @@ -434,7 +434,7 @@ extension MyActor {
// expected-note@-1{{property access is 'async'}}
_ = await mutable
_ = synchronous() // expected-error{{expression is 'async' but is not marked with 'await'}}{{9-9=await }}
// expected-note@-1{{call is 'async'}}
// expected-note@-1{{calls to instance method 'synchronous()' from outside of its actor context are implicitly asynchronous}}
_ = await synchronous()
_ = text[0] // expected-error{{expression is 'async' but is not marked with 'await'}}
// expected-note@-1{{property access is 'async'}}
Expand All @@ -445,7 +445,7 @@ extension MyActor {
// we are outside of the actor instance.
_ = self.immutable
_ = self.synchronous() // expected-error{{expression is 'async' but is not marked with 'await'}}{{9-9=await }}
// expected-note@-1{{call is 'async'}}
// expected-note@-1{{calls to instance method 'synchronous()' from outside of its actor context are implicitly asynchronous}}
_ = await self.synchronous()

_ = await self.asynchronous()
Expand All @@ -461,7 +461,7 @@ extension MyActor {
// expected-note@-1{{property access is 'async'}}
_ = await super.superState
super.superMethod() // expected-error{{expression is 'async' but is not marked with 'await'}}{{5-5=await }}
// expected-note@-1{{call is 'async'}}
// expected-note@-1{{calls to instance method 'superMethod()' from outside of its actor context are implicitly asynchronous}}

await super.superMethod()
await super.superAsyncMethod()
Expand All @@ -473,7 +473,7 @@ extension MyActor {
// call asychronous methods
_ = otherActor.immutable // okay
_ = otherActor.synchronous() // expected-error{{expression is 'async' but is not marked with 'await'}}{{9-9=await }}
// expected-note@-1{{call is 'async'}}
// expected-note@-1{{calls to instance method 'synchronous()' from outside of its actor context are implicitly asynchronous}}
_ = otherActor.synchronous // expected-error{{actor-isolated instance method 'synchronous()' can only be referenced on 'self'}}
_ = await otherActor.asynchronous()
_ = otherActor.text[0] // expected-error{{expression is 'async' but is not marked with 'await'}}{{9-9=await }}
Expand Down Expand Up @@ -534,7 +534,7 @@ func testGlobalRestrictions(actor: MyActor) async {

// any kind of method can be called from outside the actor, so long as it's marked with 'await'
_ = actor.synchronous() // expected-error{{expression is 'async' but is not marked with 'await'}}{{7-7=await }}
// expected-note@-1{{call is 'async}}
// expected-note@-1{{calls to instance method 'synchronous()' from outside of its actor context are implicitly asynchronous}}
_ = actor.asynchronous() // expected-error{{expression is 'async' but is not marked with 'await'}}{{7-7=await }}
// expected-note@-1{{call is 'async'}}

Expand Down Expand Up @@ -751,7 +751,7 @@ class SomeClassWithInits {
await self.isolated() // expected-warning{{cannot use parameter 'self' with a non-sendable type 'SomeClassWithInits' from concurrently-executed code}}
self.isolated() // expected-warning{{cannot use parameter 'self' with a non-sendable type 'SomeClassWithInits' from concurrently-executed code}}
// expected-error@-1{{expression is 'async' but is not marked with 'await'}}{{7-7=await }}
// expected-note@-2{{call is 'async'}}
// expected-note@-2{{calls to instance method 'isolated()' from outside of its actor context are implicitly asynchronous}}

print(await self.mutableState) // expected-warning{{cannot use parameter 'self' with a non-sendable type 'SomeClassWithInits' from concurrently-executed code}}
}
Expand Down Expand Up @@ -791,10 +791,10 @@ func testCrossActorProtocol<T: P>(t: T) async {
await t.g()
t.f()
// expected-error@-1{{expression is 'async' but is not marked with 'await'}}{{3-3=await }}
// expected-note@-2{{call is 'async'}}
// expected-note@-2{{calls to instance method 'f()' from outside of its actor context are implicitly asynchronous}}
t.g()
// expected-error@-1{{expression is 'async' but is not marked with 'await'}}{{3-3=await }}
// expected-note@-2{{call is 'async'}}
// expected-note@-2{{calls to instance method 'g()' from outside of its actor context are implicitly asynchronous}}
}

// ----------------------------------------------------------------------
Expand All @@ -807,7 +807,8 @@ func acceptAsyncSendableClosureInheriting<T>(@_inheritActorContext _: @Sendable
extension MyActor {
func testSendableAndInheriting() {
acceptAsyncSendableClosure {
synchronous() // expected-error{{actor-isolated instance method 'synchronous()' cannot be referenced from a concurrent closure}}
synchronous() // expected-error{{expression is 'async' but is not marked with 'await'}}
// expected-note@-1{{calls to instance method 'synchronous()' from outside of its actor context are implicitly asynchronous}}
}

acceptAsyncSendableClosure {
Expand Down
4 changes: 2 additions & 2 deletions test/Concurrency/global_actor_from_ordinary_context.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ class Taylor {
func fromAsync() async {
let x = syncGlobActorFn
// expected-error@+1{{expression is 'async' but is not marked with 'await'}}{{3-3=await }}
x() // expected-note{{call is 'async'}}
x() // expected-note{{calls to let 'x' from outside of its actor context are implicitly asynchronous}}


let y = asyncGlobalActFn
Expand All @@ -120,7 +120,7 @@ func fromAsync() async {

let fn = a.method
// expected-error@+1{{expression is 'async' but is not marked with 'await'}}{{3-3=await }}
fn() //expected-note{{call is 'async}}
fn() //expected-note{{calls to let 'fn' from outside of its actor context are implicitly asynchronous}}
// expected-error@+1{{expression is 'async' but is not marked with 'await'}}{{7-7=await }}
_ = a.const_memb // expected-note{{property access is 'async'}}
// expected-error@+1{{expression is 'async' but is not marked with 'await'}}{{7-7=await }}
Expand Down
10 changes: 6 additions & 4 deletions test/Concurrency/global_actor_function_types.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,12 +130,14 @@ func testTypesConcurrencyContext() async {
let _: () -> Int = f2 // expected-error{{converting function value of type '@SomeGlobalActor () -> Int' to '() -> Int' loses global actor 'SomeGlobalActor'}}

// expected-error@+1{{expression is 'async' but is not marked with 'await'}}{{7-7=await }}
_ = f1() //expected-note{{call is 'async}}
_ = f1() //expected-note{{calls to let 'f1' from outside of its actor context are implicitly asynchronous}}
// expected-error@+1{{expression is 'async' but is not marked with 'await'}}{{7-7=await }}
_ = f2() //expected-note{{call is 'async'}}
_ = f2() //expected-note{{calls to let 'f2' from outside of its actor context are implicitly asynchronous}}
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny nit: calls to let ‘f2’ reads a little odd, could it be something like “calls of closure ‘f2’…”?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that let isn't great. I'm wondering if closure might be unexpected though. Do people think of unapplied function references as closures?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good point, hmm...I wonder if changing let to constant and var to variable would make this read nicer. calls of constant 'f2'...

Copy link
Contributor

@harlanhaskins harlanhaskins May 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

But if there's no prior art in the diagnostics for constant to refer to lets, then maybe it's not worth introducing that...

Copy link
Member

Choose a reason for hiding this comment

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

Maybe something like closure reference or a function reference depending on what we're referencing?

e.g calls to function reference 'f1' from outside...

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm going to keep thinking about this. There are other diagnostics (even near this line in the same file, basically anything that uses DescriptiveDeclKind) that use let and it's strange to read, so it's worth updating all of them together.


// expected-error@+1{{expression is 'async' but is not marked with 'await'}}{{7-7=await }}
_ = f1() + f2() // expected-note 2 {{call is 'async'}}
// expected-error@+3{{expression is 'async' but is not marked with 'await'}}{{7-7=await }}
//expected-note@+2 {{calls to let 'f1' from outside of its actor context are implicitly asynchronous}}
// expected-note@+1 {{calls to let 'f2' from outside of its actor context are implicitly asynchronous}}
_ = f1() + f2()

_ = await f1()
_ = await f2()
Expand Down
9 changes: 5 additions & 4 deletions test/Concurrency/global_actor_inference.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ class SubclassWithGlobalActors : SuperclassWithGlobalActors {
onGenericGlobalActorString() // okay

// expected-error@+1{{expression is 'async' but is not marked with 'await'}}{{5-5=await }}
onGenericGlobalActorInt() // expected-note{{call is 'async'}}
onGenericGlobalActorInt() // expected-note{{calls to instance method 'onGenericGlobalActorInt()' from outside of its actor context are implicitly asynchronous}}
}
}

Expand All @@ -253,7 +253,7 @@ class SubclassWithGlobalActors : SuperclassWithGlobalActors {

func bar() async {
// expected-error@+1{{expression is 'async' but is not marked with 'await'}}{{3-3=await }}
foo() // expected-note{{call is 'async'}}
foo() // expected-note{{calls to global function 'foo()' from outside of its actor context are implicitly asynchronous}}
}

// expected-note@+1 {{add '@SomeGlobalActor' to make global function 'barSync()' part of global actor 'SomeGlobalActor'}} {{1-1=@SomeGlobalActor }}
Expand Down Expand Up @@ -509,7 +509,7 @@ func acceptClosure<T>(_: () -> T) { }
// ----------------------------------------------------------------------
func takesUnsafeMainActor(@_unsafeMainActor fn: () -> Void) { }

@MainActor func onlyOnMainActor() { } // expected-note{{calls to global function 'onlyOnMainActor()' from outside of its actor context are implicitly asynchronous}}
@MainActor func onlyOnMainActor() { }

func useUnsafeMainActor() {
takesUnsafeMainActor {
Expand All @@ -525,7 +525,8 @@ func acceptAsyncSendableClosureInheriting<T>(@_inheritActorContext _: @Sendable

@MainActor func testCallFromMainActor() {
acceptAsyncSendableClosure {
onlyOnMainActor() // expected-error{{call to main actor-isolated global function 'onlyOnMainActor()' in a synchronous nonisolated context}}
onlyOnMainActor() // expected-error{{expression is 'async' but is not marked with 'await'}}
// expected-note@-1 {{calls to global function 'onlyOnMainActor()' from outside of its actor context are implicitly asynchronous}}
}

acceptAsyncSendableClosure {
Expand Down