Skip to content

[Concurrency] Fix try-await fix-it #36763

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 3 commits into from
Apr 7, 2021
Merged

[Concurrency] Fix try-await fix-it #36763

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fdbaedc
Plumb await location through to diagnostics
etcwilde Apr 5, 2021
8e63412
Emit 'try' before 'await' in fix-it
etcwilde Apr 5, 2021
45c81c2
Updating tests to verify fix-it placement
etcwilde Apr 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
28 changes: 20 additions & 8 deletions lib/Sema/TypeCheckEffects.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1424,6 +1424,9 @@ class Context {
DiagnoseErrorOnTry = b;
}

/// Stores the location of the innermost await
SourceLoc awaitLoc = SourceLoc();

/// Whether this is a function that rethrows.
bool hasPolymorphicEffect(EffectKind kind) const {
if (!Function)
Expand Down Expand Up @@ -1683,7 +1686,7 @@ class Context {
auto loc = E.getStartLoc();
SourceLoc insertLoc;
SourceRange highlight;

// Generate more specific messages in some cases.
if (auto e = dyn_cast_or_null<ApplyExpr>(E.dyn_cast<Expr*>())) {
if (isa<PrefixUnaryExpr>(e) || isa<PostfixUnaryExpr>(e) ||
Expand All @@ -1693,7 +1696,7 @@ class Context {
}
insertLoc = loc;
highlight = e->getSourceRange();

if (InterpolatedString &&
e->getCalledValue() &&
e->getCalledValue()->getBaseName() ==
Expand All @@ -1702,7 +1705,7 @@ class Context {
insertLoc = InterpolatedString->getLoc();
}
}

Diags.diagnose(loc, message).highlight(highlight);
maybeAddRethrowsNote(Diags, loc, reason);

Expand All @@ -1716,6 +1719,10 @@ class Context {
if (!suggestTryFixIt)
return;

// 'try' should go before 'await'
if (awaitLoc.isValid())
insertLoc = awaitLoc;

Diags.diagnose(loc, diag::note_forgot_try)
.fixItInsert(insertLoc, "try ");
Diags.diagnose(loc, diag::note_error_to_optional)
Expand Down Expand Up @@ -2115,14 +2122,16 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
DeclContext *OldReasyncDC;
ContextFlags OldFlags;
ConditionalEffectKind OldMaxThrowingKind;
SourceLoc OldAwaitLoc;

public:
ContextScope(CheckEffectsCoverage &self, Optional<Context> newContext)
: Self(self), OldContext(self.CurContext),
OldRethrowsDC(self.RethrowsDC),
OldReasyncDC(self.ReasyncDC),
OldFlags(self.Flags),
OldMaxThrowingKind(self.MaxThrowingKind) {
OldMaxThrowingKind(self.MaxThrowingKind),
OldAwaitLoc(self.CurContext.awaitLoc) {
if (newContext) self.CurContext = *newContext;
}

Expand All @@ -2140,9 +2149,10 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
Self.Flags.clear(ContextFlags::HasTryThrowSite);
}

void enterAwait() {
void enterAwait(SourceLoc awaitLoc) {
Self.Flags.set(ContextFlags::IsAsyncCovered);
Self.Flags.clear(ContextFlags::HasAnyAsyncSite);
Self.CurContext.awaitLoc = awaitLoc;
}

void enterAsyncLet() {
Expand Down Expand Up @@ -2239,6 +2249,7 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
Self.ReasyncDC = OldReasyncDC;
Self.Flags = OldFlags;
Self.MaxThrowingKind = OldMaxThrowingKind;
Self.CurContext.awaitLoc = OldAwaitLoc;
}
};

Expand Down Expand Up @@ -2647,12 +2658,13 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
break;
}
}

ShouldRecurse_t checkAwait(AwaitExpr *E) {

// Walk the operand.
ContextScope scope(*this, None);
scope.enterAwait();
scope.enterAwait(E->getAwaitLoc());

E->getSubExpr()->walk(*this);

// Warn about 'await' expressions that weren't actually needed, unless of
Expand All @@ -2665,7 +2677,7 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
CurContext.diagnoseUnhandledAsyncSite(Ctx.Diags, E, None,
/*forAwait=*/ true);
}

// Inform the parent of the walk that an 'await' exists here.
scope.preserveCoverageFromAwaitOperand();
return ShouldNotRecurse;
Expand Down
6 changes: 3 additions & 3 deletions test/expr/unary/async_await.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,9 @@ func testThrowingAndAsync() async throws {

// Errors
_ = await throwingAndAsync() // expected-error{{call can throw but is not marked with 'try'}}
// expected-note@-1{{did you mean to use 'try'?}}
// expected-note@-2{{did you mean to handle error as optional value?}}
// expected-note@-3{{did you mean to disable error propagation?}}
// expected-note@-1{{did you mean to use 'try'?}}{{7-7=try }}
// expected-note@-2{{did you mean to handle error as optional value?}}{{7-7=try? }}
// expected-note@-3{{did you mean to disable error propagation?}}{{7-7=try! }}
_ = try throwingAndAsync() // expected-error{{call is 'async' but is not marked with 'await'}}{{11-11=await }}
}

Expand Down