Skip to content

[TypeCheckEffects] Stage in new effects checker errors for statement expressions as warnings until Swift 6. #70820

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
Jan 11, 2024
Merged

[TypeCheckEffects] Stage in new effects checker errors for statement expressions as warnings until Swift 6. #70820

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
2468bb0
[TypeCheckEffects] Do not diagnose missing try/await in `async let`
hborla Jan 10, 2024
9fc7032
[TypeCheckEffects] Redundant effects markers on statement expressions…
hborla Jan 10, 2024
cc79dea
[TypeCheckEffects] Downgrade new effects checker errors on if/switch …
hborla Jan 10, 2024
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
4 changes: 2 additions & 2 deletions include/swift/AST/DiagnosticsSema.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1288,8 +1288,8 @@ ERROR(single_value_stmt_branch_must_end_in_result,none,
ERROR(cannot_jump_in_single_value_stmt,none,
"cannot '%0' in '%1' when used as expression",
(StmtKind, StmtKind))
ERROR(effect_marker_on_single_value_stmt,none,
"'%0' may not be used on '%1' expression", (StringRef, StmtKind))
WARNING(effect_marker_on_single_value_stmt,none,
"'%0' has no effect on '%1' expression", (StringRef, StmtKind))
ERROR(out_of_place_then_stmt,none,
"'then' may only appear as the last statement in an 'if', 'switch', or "
"'do' expression", ())
Expand Down
52 changes: 41 additions & 11 deletions lib/Sema/TypeCheckEffects.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2220,9 +2220,10 @@ class Context {
}

void diagnoseUncoveredThrowSite(ASTContext &ctx, ASTNode E,
const PotentialEffectReason &reason) {
const Classification &classification) {
auto &Diags = ctx.Diags;
auto message = diag::throwing_call_without_try;
const auto &reason = classification.getThrowReason();
auto reasonKind = reason.getKind();

bool suggestTryFixIt = reasonKind == PotentialEffectReason::Kind::Apply;
Expand Down Expand Up @@ -2262,7 +2263,8 @@ class Context {
}
}

Diags.diagnose(loc, message).highlight(highlight);
Diags.diagnose(loc, message).highlight(highlight)
.warnUntilSwiftVersionIf(classification.shouldDowngradeToWarning(), 6);
maybeAddRethrowsNote(Diags, loc, reason);

// If this is a call without expected 'try[?|!]', like this:
Expand Down Expand Up @@ -2562,6 +2564,15 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>

/// Do we have any 'await's in this context?
HasAnyAwait = 0x80,

/// Are we in an 'async let' initializer context?
InAsyncLet = 0x100,

/// Does an enclosing 'if' or 'switch' expr have a 'try'?
StmtExprCoversTry = 0x200,

/// Does an enclosing 'if' or 'switch' expr have an 'await'?
StmtExprCoversAwait = 0x400,
};
private:
unsigned Bits;
Expand Down Expand Up @@ -2707,8 +2718,7 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
}

void enterAsyncLet() {
Self.Flags.set(ContextFlags::IsTryCovered);
Self.Flags.set(ContextFlags::IsAsyncCovered);
Self.Flags.set(ContextFlags::InAsyncLet);
}

void refineLocalContext(Context newContext) {
Expand All @@ -2730,6 +2740,9 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
Self.Flags.reset();
Self.MaxThrowingKind = ConditionalEffectKind::None;

Self.Flags.mergeFrom(ContextFlags::StmtExprCoversTry, OldFlags);
Self.Flags.mergeFrom(ContextFlags::StmtExprCoversAwait, OldFlags);

// Suppress 'try' coverage checking within a single level of
// do/catch in debugger functions.
if (OldFlags.has(ContextFlags::IsTopLevelDebuggerFunction))
Expand All @@ -2747,6 +2760,17 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
// 'async'.
}

void setCoverageForSingleValueStmtExpr() {
resetCoverage();
Self.Flags.mergeFrom(ContextFlags::InAsyncLet, OldFlags);

if (OldFlags.has(ContextFlags::IsTryCovered))
Self.Flags.set(ContextFlags::StmtExprCoversTry);

if (OldFlags.has(ContextFlags::IsAsyncCovered))
Self.Flags.set(ContextFlags::StmtExprCoversAwait);
}

void preserveCoverageFromSingleValueStmtExpr() {
// We need to preserve whether we saw any throwing sites, to avoid warning
// on 'do { let x = if .random() { try ... } else { ... } } catch { ... }'
Expand Down Expand Up @@ -2918,7 +2942,7 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
// For an if/switch expression, we reset coverage such that a 'try'/'await'
// does not cover the branches.
ContextScope scope(*this, /*newContext*/ llvm::None);
scope.resetCoverage();
scope.setCoverageForSingleValueStmtExpr();
SVE->getStmt()->walk(*this);
scope.preserveCoverageFromSingleValueStmtExpr();
return ShouldNotRecurse;
Expand Down Expand Up @@ -3145,7 +3169,7 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>

ThrownErrorDestination
checkThrowAsyncSite(ASTNode E, bool requiresTry,
const Classification &classification) {
Classification &classification) {
// Suppress all diagnostics when there's an un-analyzable throw/async site.
if (classification.isInvalid()) {
Flags.set(ContextFlags::HasAnyThrowSite);
Expand Down Expand Up @@ -3173,10 +3197,13 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
classification.getAsyncReason());
}
// Diagnose async calls that are outside of an await context.
else if (!Flags.has(ContextFlags::IsAsyncCovered)) {
else if (!(Flags.has(ContextFlags::IsAsyncCovered) ||
Flags.has(ContextFlags::InAsyncLet))) {
Expr *expr = E.dyn_cast<Expr*>();
Expr *anchor = walkToAnchor(expr, parentMap,
CurContext.isWithinInterpolatedString());
if (Flags.has(ContextFlags::StmtExprCoversAwait))
classification.setDowngradeToWarning(true);
if (uncoveredAsync.find(anchor) == uncoveredAsync.end())
errorOrder.push_back(anchor);
uncoveredAsync[anchor].emplace_back(
Expand Down Expand Up @@ -3209,13 +3236,16 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
break;

bool isTryCovered =
(!requiresTry || Flags.has(ContextFlags::IsTryCovered));
(!requiresTry || Flags.has(ContextFlags::IsTryCovered) ||
Flags.has(ContextFlags::InAsyncLet));
if (!CurContext.handlesThrows(throwsKind)) {
CurContext.diagnoseUnhandledThrowSite(Ctx.Diags, E, isTryCovered,
classification.getThrowReason());
} else if (!isTryCovered) {
if (Flags.has(ContextFlags::StmtExprCoversTry))
classification.setDowngradeToWarning(true);
CurContext.diagnoseUncoveredThrowSite(Ctx, E, // we want this one to trigger
classification.getThrowReason());
classification);
} else {
return checkThrownErrorType(
E.getStartLoc(), classification.getThrownError());
Expand Down Expand Up @@ -3367,7 +3397,7 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>

void diagnoseRedundantTry(AnyTryExpr *E) const {
if (auto *SVE = SingleValueStmtExpr::tryDigOutSingleValueStmtExpr(E)) {
// For an if/switch expression, produce an error instead of a warning.
// For an if/switch expression, produce a tailored warning.
Ctx.Diags.diagnose(E->getTryLoc(),
diag::effect_marker_on_single_value_stmt,
"try", SVE->getStmt()->getKind())
Expand All @@ -3379,7 +3409,7 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>

void diagnoseRedundantAwait(AwaitExpr *E) const {
if (auto *SVE = SingleValueStmtExpr::tryDigOutSingleValueStmtExpr(E)) {
// For an if/switch expression, produce an error instead of a warning.
// For an if/switch expression, produce a tailored warning.
Ctx.Diags.diagnose(E->getAwaitLoc(),
diag::effect_marker_on_single_value_stmt,
"await", SVE->getStmt()->getKind())
Expand Down
Loading