Skip to content

Commit 68301f7

Browse files
hamishknighthamishknight
committed
[Sema] Warn on redundant try/awaits for if/switch expressions
Look through `try`/`await` markers when looking for out of place if/switch expressions, and customize the effect checking diagnostic such that we warn that `try`/`await` are redundant on `if`/`switch` expressions.
1 parent 5e70912 commit 68301f7

File tree

5 files changed

+118
-14
lines changed

5 files changed

+118
-14
lines changed

‎include/swift/AST/DiagnosticsSema.def

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1100,6 +1100,8 @@ ERROR(single_value_stmt_branch_must_end_in_throw,none,
11001100
ERROR(cannot_jump_in_single_value_stmt,none,
11011101
"cannot '%0' in '%1' when used as expression",
11021102
(StmtKind, StmtKind))
1103+
WARNING(redundant_effect_marker_on_single_value_stmt,none,
1104+
"'%0' on an '%1' expression has no effect", (StringRef, StmtKind))
11031105

11041106
ERROR(did_not_call_function_value,none,
11051107
"function value was used as a property; add () to call it",

‎lib/Sema/MiscDiagnostics.cpp

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3816,6 +3816,15 @@ class SingleValueStmtUsageChecker final : public ASTWalker {
38163816
E = CE->getSubExpr();
38173817
continue;
38183818
}
3819+
// Look through try/await. We'll warn on these during effect checking.
3820+
if (auto *TE = dyn_cast<AnyTryExpr>(E)) {
3821+
E = TE->getSubExpr();
3822+
continue;
3823+
}
3824+
if (auto *AE = dyn_cast<AwaitExpr>(E)) {
3825+
E = AE->getSubExpr();
3826+
continue;
3827+
}
38193828
break;
38203829
}
38213830
if (auto *SVE = dyn_cast<SingleValueStmtExpr>(E))

‎lib/Sema/TypeCheckEffects.cpp

Lines changed: 35 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2720,11 +2720,12 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
27202720
// course we're in a context that could never handle an 'async'. Then, we
27212721
// produce an error.
27222722
if (!Flags.has(ContextFlags::HasAnyAsyncSite)) {
2723-
if (CurContext.handlesAsync(ConditionalEffectKind::Conditional))
2724-
Ctx.Diags.diagnose(E->getAwaitLoc(), diag::no_async_in_await);
2725-
else
2723+
if (CurContext.handlesAsync(ConditionalEffectKind::Conditional)) {
2724+
diagnoseRedundantAwait(E);
2725+
} else {
27262726
CurContext.diagnoseUnhandledAsyncSite(Ctx.Diags, E, None,
27272727
/*forAwait=*/ true);
2728+
}
27282729
}
27292730

27302731
// Inform the parent of the walk that an 'await' exists here.
@@ -2742,7 +2743,7 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
27422743
// Warn about 'try' expressions that weren't actually needed.
27432744
if (!Flags.has(ContextFlags::HasTryThrowSite)) {
27442745
if (!E->isImplicit())
2745-
Ctx.Diags.diagnose(E->getTryLoc(), diag::no_throw_in_try);
2746+
diagnoseRedundantTry(E);
27462747

27472748
// Diagnose all the call sites within a single unhandled 'try'
27482749
// at the same time.
@@ -2762,9 +2763,8 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
27622763
E->getSubExpr()->walk(*this);
27632764

27642765
// Warn about 'try' expressions that weren't actually needed.
2765-
if (!Flags.has(ContextFlags::HasTryThrowSite)) {
2766-
Ctx.Diags.diagnose(E->getLoc(), diag::no_throw_in_try);
2767-
}
2766+
if (!Flags.has(ContextFlags::HasTryThrowSite))
2767+
diagnoseRedundantTry(E);
27682768

27692769
scope.preserveCoverageFromOptionalOrForcedTryOperand();
27702770
return ShouldNotRecurse;
@@ -2778,9 +2778,8 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
27782778
E->getSubExpr()->walk(*this);
27792779

27802780
// Warn about 'try' expressions that weren't actually needed.
2781-
if (!Flags.has(ContextFlags::HasTryThrowSite)) {
2782-
Ctx.Diags.diagnose(E->getLoc(), diag::no_throw_in_try);
2783-
}
2781+
if (!Flags.has(ContextFlags::HasTryThrowSite))
2782+
diagnoseRedundantTry(E);
27842783

27852784
scope.preserveCoverageFromOptionalOrForcedTryOperand();
27862785
return ShouldNotRecurse;
@@ -2818,6 +2817,32 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
28182817
return ShouldRecurse;
28192818
}
28202819

2820+
void diagnoseRedundantTry(AnyTryExpr *E) const {
2821+
auto *semanticExpr = E->getSemanticsProvidingExpr();
2822+
if (auto *SVE = dyn_cast<SingleValueStmtExpr>(semanticExpr)) {
2823+
// For an if/switch expression, produce a custom diagnostic.
2824+
Ctx.Diags.diagnose(E->getTryLoc(),
2825+
diag::redundant_effect_marker_on_single_value_stmt,
2826+
"try", SVE->getStmt()->getKind())
2827+
.highlight(E->getTryLoc());
2828+
return;
2829+
}
2830+
Ctx.Diags.diagnose(E->getTryLoc(), diag::no_throw_in_try);
2831+
}
2832+
2833+
void diagnoseRedundantAwait(AwaitExpr *E) const {
2834+
auto *semanticExpr = E->getSemanticsProvidingExpr();
2835+
if (auto *SVE = dyn_cast<SingleValueStmtExpr>(semanticExpr)) {
2836+
// For an if/switch expression, produce a custom diagnostic.
2837+
Ctx.Diags.diagnose(E->getAwaitLoc(),
2838+
diag::redundant_effect_marker_on_single_value_stmt,
2839+
"await", SVE->getStmt()->getKind())
2840+
.highlight(E->getAwaitLoc());
2841+
return;
2842+
}
2843+
Ctx.Diags.diagnose(E->getAwaitLoc(), diag::no_async_in_await);
2844+
}
2845+
28212846
void diagnoseUncoveredAsyncSite(const Expr *anchor) const {
28222847
auto asyncPointIter = uncoveredAsync.find(anchor);
28232848
if (asyncPointIter == uncoveredAsync.end())

‎test/expr/unary/if_expr.swift

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// RUN: %target-typecheck-verify-swift -enable-experimental-feature IfSwitchExpressions
1+
// RUN: %target-typecheck-verify-swift -enable-experimental-feature IfSwitchExpressions -disable-availability-checking
22

33
// MARK: Functions
44

@@ -384,7 +384,7 @@ func stmts() {
384384

385385
if try if .random() { true } else { false } {}
386386
// expected-error@-1 {{'if' may only be used as expression in return, throw, or as the source of an assignment}}
387-
// expected-warning@-2 {{no calls to throwing functions occur within 'try' expression}}
387+
// expected-warning@-2 {{'try' on an 'if' expression has no effect}}
388388

389389
// expected-error@+1 {{'if' may only be used as expression in return, throw, or as the source of an assignment}}
390390
guard if .random() { true } else { false } else {
@@ -667,3 +667,37 @@ func return4() throws -> Int {
667667
}
668668
return i
669669
}
670+
671+
// MARK: Effect specifiers
672+
673+
func tryIf1() -> Int {
674+
try if .random() { 0 } else { 1 }
675+
// expected-warning@-1 {{'try' on an 'if' expression has no effect}}
676+
}
677+
678+
func tryIf2() -> Int {
679+
let x = try if .random() { 0 } else { 1 }
680+
// expected-warning@-1 {{'try' on an 'if' expression has no effect}}
681+
return x
682+
}
683+
684+
func tryIf3() -> Int {
685+
return try if .random() { 0 } else { 1 }
686+
// expected-warning@-1 {{'try' on an 'if' expression has no effect}}
687+
}
688+
689+
func awaitIf1() async -> Int {
690+
await if .random() { 0 } else { 1 }
691+
// expected-warning@-1 {{'await' on an 'if' expression has no effect}}
692+
}
693+
694+
func awaitIf2() async -> Int {
695+
let x = await if .random() { 0 } else { 1 }
696+
// expected-warning@-1 {{'await' on an 'if' expression has no effect}}
697+
return x
698+
}
699+
700+
func awaitIf3() async -> Int {
701+
return await if .random() { 0 } else { 1 }
702+
// expected-warning@-1 {{'await' on an 'if' expression has no effect}}
703+
}

‎test/expr/unary/switch_expr.swift

Lines changed: 36 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// RUN: %target-typecheck-verify-swift -enable-experimental-feature IfSwitchExpressions
1+
// RUN: %target-typecheck-verify-swift -enable-experimental-feature IfSwitchExpressions -disable-availability-checking
22

33
// MARK: Functions
44

@@ -501,7 +501,7 @@ func stmts() {
501501
// expected-error@-1 {{'switch' may only be used as expression in return, throw, or as the source of an assignment}}
502502

503503
if try switch Bool.random() { case true: true case false: true } {}
504-
// expected-warning@-1 {{no calls to throwing functions occur within 'try' expression}}
504+
// expected-warning@-1 {{'try' on an 'switch' expression has no effect}}
505505
// expected-error@-2 {{'switch' may only be used as expression in return, throw, or as the source of an assignment}}
506506

507507
// expected-error@+1 {{'switch' may only be used as expression in return, throw, or as the source of an assignment}}
@@ -877,3 +877,37 @@ func continueToInner() -> Int {
877877
1 // expected-warning {{integer literal is unused}}
878878
}
879879
}
880+
881+
// MARK: Effect specifiers
882+
883+
func trySwitch1() -> Int {
884+
try switch Bool.random() { case true: 0 case false: 1 }
885+
// expected-warning@-1 {{'try' on an 'switch' expression has no effect}}
886+
}
887+
888+
func trySwitch2() -> Int {
889+
let x = try switch Bool.random() { case true: 0 case false: 1 }
890+
// expected-warning@-1 {{'try' on an 'switch' expression has no effect}}
891+
return x
892+
}
893+
894+
func trySwitch3() -> Int {
895+
return try switch Bool.random() { case true: 0 case false: 1 }
896+
// expected-warning@-1 {{'try' on an 'switch' expression has no effect}}
897+
}
898+
899+
func awaitSwitch1() async -> Int {
900+
await switch Bool.random() { case true: 0 case false: 1 }
901+
// expected-warning@-1 {{'await' on an 'switch' expression has no effect}}
902+
}
903+
904+
func awaitSwitch2() async -> Int {
905+
let x = await switch Bool.random() { case true: 0 case false: 1 }
906+
// expected-warning@-1 {{'await' on an 'switch' expression has no effect}}
907+
return x
908+
}
909+
910+
func awaitSwitch3() async -> Int {
911+
return await switch Bool.random() { case true: 0 case false: 1 }
912+
// expected-warning@-1 {{'await' on an 'switch' expression has no effect}}
913+
}

0 commit comments

Comments
 (0)