[Sema] Error on redundant try/awaits for if/switch expressions

hamishknight · hamishknight · commit 0d55f45d95d0 · 2023-02-01T15:30:19.000Z
Look through `try`/`await` markers when looking for
out of place if/switch expressions, and customize
the effect checking diagnostic such that we error
that `try`/`await` are redundant on `if`/`switch`
expressions.
diff --git a/include/swift/AST/DiagnosticsSema.def b/include/swift/AST/DiagnosticsSema.def
@@ -1128,6 +1128,8 @@ ERROR(single_value_stmt_branch_must_end_in_throw,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))
 
 ERROR(did_not_call_function_value,none,
       "function value was used as a property; add () to call it",
diff --git a/lib/AST/Expr.cpp b/lib/AST/Expr.cpp
@@ -2530,6 +2530,16 @@ SingleValueStmtExpr::tryDigOutSingleValueStmtExpr(Expr *E) {
       E = CE->getSubExpr();
       continue;
     }
+    // Look through try/await (this is invalid, but we'll error on it in
+    // effect checking).
+    if (auto *TE = dyn_cast<AnyTryExpr>(E)) {
+      E = TE->getSubExpr();
+      continue;
+    }
+    if (auto *AE = dyn_cast<AwaitExpr>(E)) {
+      E = AE->getSubExpr();
+      continue;
+    }
     break;
   }
   return dyn_cast<SingleValueStmtExpr>(E);
diff --git a/lib/Sema/TypeCheckEffects.cpp b/lib/Sema/TypeCheckEffects.cpp
@@ -2720,11 +2720,12 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
     // course we're in a context that could never handle an 'async'. Then, we
     // produce an error.
     if (!Flags.has(ContextFlags::HasAnyAsyncSite)) {
-      if (CurContext.handlesAsync(ConditionalEffectKind::Conditional))
-        Ctx.Diags.diagnose(E->getAwaitLoc(), diag::no_async_in_await);
-      else
+      if (CurContext.handlesAsync(ConditionalEffectKind::Conditional)) {
+        diagnoseRedundantAwait(E);
+      } else {
         CurContext.diagnoseUnhandledAsyncSite(Ctx.Diags, E, None,
                                               /*forAwait=*/ true);
+      }
     }
 
     // Inform the parent of the walk that an 'await' exists here.
@@ -2742,7 +2743,7 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
     // Warn about 'try' expressions that weren't actually needed.
     if (!Flags.has(ContextFlags::HasTryThrowSite)) {
       if (!E->isImplicit())
-        Ctx.Diags.diagnose(E->getTryLoc(), diag::no_throw_in_try);
+        diagnoseRedundantTry(E);
 
     // Diagnose all the call sites within a single unhandled 'try'
     // at the same time.
@@ -2762,9 +2763,8 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
     E->getSubExpr()->walk(*this);
 
     // Warn about 'try' expressions that weren't actually needed.
-    if (!Flags.has(ContextFlags::HasTryThrowSite)) {
-      Ctx.Diags.diagnose(E->getLoc(), diag::no_throw_in_try);
-    }
+    if (!Flags.has(ContextFlags::HasTryThrowSite))
+      diagnoseRedundantTry(E);
 
     scope.preserveCoverageFromOptionalOrForcedTryOperand();
     return ShouldNotRecurse;
@@ -2778,9 +2778,8 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
     E->getSubExpr()->walk(*this);
 
     // Warn about 'try' expressions that weren't actually needed.
-    if (!Flags.has(ContextFlags::HasTryThrowSite)) {
-      Ctx.Diags.diagnose(E->getLoc(), diag::no_throw_in_try);
-    }
+    if (!Flags.has(ContextFlags::HasTryThrowSite))
+      diagnoseRedundantTry(E);
 
     scope.preserveCoverageFromOptionalOrForcedTryOperand();
     return ShouldNotRecurse;
@@ -2818,6 +2817,30 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
     return ShouldRecurse;
   }
 
+  void diagnoseRedundantTry(AnyTryExpr *E) const {
+    if (auto *SVE = SingleValueStmtExpr::tryDigOutSingleValueStmtExpr(E)) {
+      // For an if/switch expression, produce an error instead of a warning.
+      Ctx.Diags.diagnose(E->getTryLoc(),
+                         diag::effect_marker_on_single_value_stmt,
+                         "try", SVE->getStmt()->getKind())
+        .highlight(E->getTryLoc());
+      return;
+    }
+    Ctx.Diags.diagnose(E->getTryLoc(), diag::no_throw_in_try);
+  }
+
+  void diagnoseRedundantAwait(AwaitExpr *E) const {
+    if (auto *SVE = SingleValueStmtExpr::tryDigOutSingleValueStmtExpr(E)) {
+      // For an if/switch expression, produce an error instead of a warning.
+      Ctx.Diags.diagnose(E->getAwaitLoc(),
+                         diag::effect_marker_on_single_value_stmt,
+                         "await", SVE->getStmt()->getKind())
+        .highlight(E->getAwaitLoc());
+      return;
+    }
+    Ctx.Diags.diagnose(E->getAwaitLoc(), diag::no_async_in_await);
+  }
+
   void diagnoseUncoveredAsyncSite(const Expr *anchor) const {
     auto asyncPointIter = uncoveredAsync.find(anchor);
     if (asyncPointIter == uncoveredAsync.end())
diff --git a/test/expr/unary/if_expr.swift b/test/expr/unary/if_expr.swift
@@ -1,4 +1,4 @@
-// RUN: %target-typecheck-verify-swift
+// RUN: %target-typecheck-verify-swift -disable-availability-checking
 
 // MARK: Functions
 
@@ -440,7 +440,7 @@ func stmts() {
 
   if try if .random() { true } else { false } {}
   // expected-error@-1 {{'if' may only be used as expression in return, throw, or as the source of an assignment}}
-  // expected-warning@-2 {{no calls to throwing functions occur within 'try' expression}}
+  // expected-error@-2 {{'try' may not be used on 'if' expression}}
 
   // expected-error@+1 {{'if' may only be used as expression in return, throw, or as the source of an assignment}}
   guard if .random() { true } else { false } else {
@@ -723,3 +723,49 @@ func return4() throws -> Int {
   }
   return i
 }
+
+// MARK: Effect specifiers
+
+func tryIf1() -> Int {
+  try if .random() { 0 } else { 1 }
+  // expected-error@-1 {{'try' may not be used on 'if' expression}}
+}
+
+func tryIf2() -> Int {
+  let x = try if .random() { 0 } else { 1 }
+  // expected-error@-1 {{'try' may not be used on 'if' expression}}
+  return x
+}
+
+func tryIf3() -> Int {
+  return try if .random() { 0 } else { 1 }
+  // expected-error@-1 {{'try' may not be used on 'if' expression}}
+}
+
+func awaitIf1() async -> Int {
+  await if .random() { 0 } else { 1 }
+  // expected-error@-1 {{'await' may not be used on 'if' expression}}
+}
+
+func awaitIf2() async -> Int {
+  let x = await if .random() { 0 } else { 1 }
+  // expected-error@-1 {{'await' may not be used on 'if' expression}}
+  return x
+}
+
+func awaitIf3() async -> Int {
+  return await if .random() { 0 } else { 1 }
+  // expected-error@-1 {{'await' may not be used on 'if' expression}}
+}
+
+func tryAwaitIf1() async throws -> Int {
+  try await if .random() { 0 } else { 1 }
+  // expected-error@-1 {{'try' may not be used on 'if' expression}}
+  // expected-error@-2 {{'await' may not be used on 'if' expression}}
+}
+
+func tryAwaitIf2() async throws -> Int {
+  try await if .random() { 0 } else { 1 } as Int
+  // expected-error@-1 {{'try' may not be used on 'if' expression}}
+  // expected-error@-2 {{'await' may not be used on 'if' expression}}
+}
diff --git a/test/expr/unary/switch_expr.swift b/test/expr/unary/switch_expr.swift
@@ -1,4 +1,4 @@
-// RUN: %target-typecheck-verify-swift
+// RUN: %target-typecheck-verify-swift -disable-availability-checking
 
 // MARK: Functions
 
@@ -596,7 +596,7 @@ func stmts() {
   // expected-error@-1 {{'switch' may only be used as expression in return, throw, or as the source of an assignment}}
 
   if try switch Bool.random() { case true: true case false: true } {}
-  // expected-warning@-1 {{no calls to throwing functions occur within 'try' expression}}
+  // expected-error@-1 {{'try' may not be used on 'switch' expression}}
   // expected-error@-2 {{'switch' may only be used as expression in return, throw, or as the source of an assignment}}
 
   // expected-error@+1 {{'switch' may only be used as expression in return, throw, or as the source of an assignment}}
@@ -972,3 +972,49 @@ func continueToInner() -> Int {
     1  // expected-warning {{integer literal is unused}}
   }
 }
+
+// MARK: Effect specifiers
+
+func trySwitch1() -> Int {
+  try switch Bool.random() { case true: 0 case false: 1 }
+  // expected-error@-1 {{'try' may not be used on 'switch' expression}}
+}
+
+func trySwitch2() -> Int {
+  let x = try switch Bool.random() { case true: 0 case false: 1 }
+  // expected-error@-1 {{'try' may not be used on 'switch' expression}}
+  return x
+}
+
+func trySwitch3() -> Int {
+  return try switch Bool.random() { case true: 0 case false: 1 }
+  // expected-error@-1 {{'try' may not be used on 'switch' expression}}
+}
+
+func awaitSwitch1() async -> Int {
+  await switch Bool.random() { case true: 0 case false: 1 }
+  // expected-error@-1 {{'await' may not be used on 'switch' expression}}
+}
+
+func awaitSwitch2() async -> Int {
+  let x = await switch Bool.random() { case true: 0 case false: 1 }
+  // expected-error@-1 {{'await' may not be used on 'switch' expression}}
+  return x
+}
+
+func awaitSwitch3() async -> Int {
+  return await switch Bool.random() { case true: 0 case false: 1 }
+  // expected-error@-1 {{'await' may not be used on 'switch' expression}}
+}
+
+func tryAwaitSwitch1() async throws -> Int {
+  try await switch Bool.random() { case true: 0 case false: 1 }
+  // expected-error@-1 {{'try' may not be used on 'switch' expression}}
+  // expected-error@-2 {{'await' may not be used on 'switch' expression}}
+}
+
+func tryAwaitSwitch2() async throws -> Int {
+  try await switch Bool.random() { case true: 0 case false: 1 } as Int
+  // expected-error@-1 {{'try' may not be used on 'switch' expression}}
+  // expected-error@-2 {{'await' may not be used on 'switch' expression}}
+}
