Update error messages

This patch restructures how we emit the effect errors for missing
awaits. As we're traversing the expressions, we collect all of the
locations where a missing await is, and metadata about the reason for
the error, as well as the "anchor".

The anchor is where the await should actually be placed in the
expression to fix things. The error is emitted for the whole
sub-expression instead of for the exact synchronization/call-site of the
async function. This avoids the erroneous message for

```
 _ = 32 + asyncFunc()
```

The old behaviour was to put the `await` between the `+` operator and
the call to `asyncFunc()`, but this is not a valid place to put effects.
Note; this issue is also present in the fix-it for `try` insertions and
also needs to be fixed. Instead, the anchor points between the
assignment expression and 32, labelling the whole right-side of the
assignment as being the asynchronous function. We emit notes for each
uncovered async point in the entire expression though, so that someone
can see why the expression is async.

Author: etcwilde

include/swift/AST/DiagnosticsSema.def

@@ -4253,17 +4253,17 @@ WARNING(no_throw_in_do_with_catch,none,
 //------------------------------------------------------------------------------
 // MARK: Concurrency
 //------------------------------------------------------------------------------
-ERROR(async_call_without_await,none,
-      "call is 'async' but is not marked with 'await'", ())
-ERROR(async_call_without_await_in_autoclosure,none,
-      "call is 'async' in an autoclosure argument that is not marked with 'await'", ())
-ERROR(async_call_without_await_in_async_let,none,
-      "call is 'async' in an 'async let' initializer that is not marked "
-      "with 'await'", ())
-ERROR(async_prop_access_without_await,none,
-      "property access is 'async' but is not marked with 'await'", ())
-ERROR(async_subscript_access_without_await,none,
-      "subscript access is 'async' but is not marked with 'await'", ())
+ERROR(async_expr_without_await,none,
+      "expression is 'async' but is not marked with 'await'", ())
+
+NOTE(async_access_without_await,none,
+    "%select{call|property access|subscript access|}0 is 'async'", (unsigned))
+
+NOTE(async_call_without_await_in_autoclosure,none,
+      "call is 'async' in an autoclosure argument", ())
+NOTE(async_call_without_await_in_async_let,none,
+      "call is 'async' in an 'async let' initializer", ())
+
 WARNING(no_async_in_await,none,
         "no 'async' operations occur within 'await' expression", ())
 ERROR(async_call_in_illegal_context,none,
@@ -4310,8 +4310,8 @@ ERROR(async_let_not_initialized,none,
       "'async let' binding requires an initializer expression", ())
 ERROR(async_let_no_variables,none,
       "'async let' requires at least one named variable", ())
-ERROR(async_let_without_await,none,
-      "reference to async let %0 is not marked with 'await'", (DeclName))
+NOTE(async_let_without_await,none,
+      "reference to async let %0 is 'async'", (DeclName))
 ERROR(async_let_in_illegal_context,none,
       "async let %0 cannot be referenced in "
       "%select{<<ERROR>>|a default argument|a property wrapper initializer|a property initializer|a global variable initializer|an enum case raw value|a catch pattern|a catch guard expression|a defer body}1",

lib/Sema/TypeCheckEffects.cpp

@@ -1907,67 +1907,6 @@ class Context {
     }
   }
 
-  void diagnoseUncoveredAsyncSite(ASTContext &ctx, ASTNode node,
-                                  PotentialEffectReason reason) {
-    SourceRange highlight = node.getSourceRange();
-    auto diag = diag::async_call_without_await;
-
-    switch (reason.getKind()) {
-    case PotentialEffectReason::Kind::AsyncLet:
-      // Reference to an 'async let' missing an 'await'.
-      if (auto declR = dyn_cast_or_null<DeclRefExpr>(node.dyn_cast<Expr*>())) {
-        if (auto var = dyn_cast<VarDecl>(declR->getDecl())) {
-          if (var->isAsyncLet()) {
-            ctx.Diags.diagnose(declR->getLoc(), diag::async_let_without_await,
-                               var->getName());
-            return;
-          }
-        }
-      }
-      LLVM_FALLTHROUGH; // fallthrough to a message about property access
-
-    case PotentialEffectReason::Kind::PropertyAccess:
-      diag = diag::async_prop_access_without_await;
-      break;
-
-    case PotentialEffectReason::Kind::SubscriptAccess:
-      diag = diag::async_subscript_access_without_await;
-      break;
-
-    case PotentialEffectReason::Kind::ByClosure:
-    case PotentialEffectReason::Kind::ByDefaultClosure:
-    case PotentialEffectReason::Kind::ByConformance:
-    case PotentialEffectReason::Kind::Apply: {
-      if (Function) {
-        // To produce a better error message, check if it is an autoclosure.
-        // We do not use 'Context::isAutoClosure' b/c it gives conservative
-        // answers.
-        if (auto autoclosure = dyn_cast_or_null<AutoClosureExpr>(
-                Function->getAbstractClosureExpr())) {
-          switch (autoclosure->getThunkKind()) {
-          case AutoClosureExpr::Kind::None:
-            diag = diag::async_call_without_await_in_autoclosure;
-            break;
-
-          case AutoClosureExpr::Kind::AsyncLet:
-            diag = diag::async_call_without_await_in_async_let;
-            break;
-
-          case AutoClosureExpr::Kind::SingleCurryThunk:
-          case AutoClosureExpr::Kind::DoubleCurryThunk:
-            break;
-          }
-        }
-      }
-      break;
-    }
-    };
-
-    ctx.Diags.diagnose(node.getStartLoc(), diag)
-        .fixItInsert(node.getStartLoc(), "await ")
-        .highlight(highlight);
-  }
-
   void diagnoseAsyncInIllegalContext(DiagnosticEngine &Diags, ASTNode node) {
     if (auto *e = node.dyn_cast<Expr*>()) {
       if (isa<ApplyExpr>(e)) {
@@ -2119,8 +2058,43 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
   /// context.
   ConditionalEffectKind MaxThrowingKind;
 
+  struct DiagnosticInfo {
+    DiagnosticInfo(Expr &failingExpr,
+                   PotentialEffectReason reason) :
+      reason(reason),
+      expr(failingExpr) {}
+
+    /// Reason for throwing
+    PotentialEffectReason reason;
+
+    /// Failing expression
+    Expr &expr;
+  };
+
+  SmallVector<Expr *, 4> errorOrder;
+  llvm::DenseMap<Expr *, std::vector<DiagnosticInfo>> uncoveredAsync;
   llvm::DenseMap<Expr *, Expr *> parentMap;
 
+  static bool isEffectAnchor(Expr *e) {
+    return isa<AbstractClosureExpr>(e) || isa<DiscardAssignmentExpr>(e) || isa<AssignExpr>(e);
+  }
+
+  static bool isAnchorTooEarly(Expr *e) {
+    return isa<AssignExpr>(e) || isa<DiscardAssignmentExpr>(e);
+  }
+
+  /// Find the top location where we should put the await
+  static Expr* walkToAnchor(Expr *e,
+                            llvm::DenseMap<Expr *, Expr*> &parentMap) {
+    Expr *parent = e;
+    Expr *lastParent = e;
+    while (parent && !isEffectAnchor(parent)) {
+      lastParent = parent;
+      parent = parentMap[parent];
+    }
+    return parent && !isAnchorTooEarly(parent) ? parent : lastParent;
+  }
+
   void flagInvalidCode() {
     // Suppress warnings about useless try or catch.
     Flags.set(ContextFlags::HasAnyThrowSite);
@@ -2282,6 +2256,13 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
     }
   }
 
+  ~CheckEffectsCoverage() {
+    for (Expr *anchor: errorOrder) {
+      diagnoseUncoveredAsyncSite(anchor);
+    }
+  }
+
+
   /// Mark that the current context is top-level code with
   /// throw-without-try enabled.
   void setTopLevelThrowWithoutTry() {
@@ -2638,8 +2619,17 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
       }
       // Diagnose async calls that are outside of an await context.
       else if (!Flags.has(ContextFlags::IsAsyncCovered)) {
-        CurContext.diagnoseUncoveredAsyncSite(Ctx, E,
-                                              classification.getAsyncReason());
+        Expr *expr = E.dyn_cast<Expr*>();
+        Expr *anchor = walkToAnchor(expr, parentMap);
+
+        auto key = uncoveredAsync.find(anchor);
+        if (key == uncoveredAsync.end()) {
+          uncoveredAsync.insert({anchor, {}});
+          errorOrder.push_back(anchor);
+        }
+        uncoveredAsync[anchor].emplace_back(
+            *expr,
+            classification.getAsyncReason());
       }
     }
 
@@ -2788,6 +2778,78 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
 
     return ShouldRecurse;
   }
+
+  void diagnoseUncoveredAsyncSite(const Expr *anchor) const {
+    auto asyncPointIter = uncoveredAsync.find(anchor);
+    if (asyncPointIter == uncoveredAsync.end())
+      return;
+    const std::vector<DiagnosticInfo> &errors = asyncPointIter->getSecond();
+    SourceLoc awaitInsertLoc = anchor->getStartLoc();
+    if (const AnyTryExpr *tryExpr = dyn_cast<AnyTryExpr>(anchor))
+      awaitInsertLoc = tryExpr->getSubExpr()->getStartLoc();
+    else if (const AutoClosureExpr *autoClosure = dyn_cast<AutoClosureExpr>(anchor)) {
+      if (const AnyTryExpr *tryExpr = dyn_cast<AnyTryExpr>(autoClosure->getSingleExpressionBody()))
+        awaitInsertLoc = tryExpr->getSubExpr()->getStartLoc();
+    }
+
+    Ctx.Diags.diagnose(anchor->getStartLoc(), diag::async_expr_without_await)
+      .fixItInsert(awaitInsertLoc, "await ")
+      .highlight(anchor->getSourceRange());
+
+    for (const DiagnosticInfo &diag: errors) {
+      switch (diag.reason.getKind()) {
+        case PotentialEffectReason::Kind::AsyncLet:
+          if (auto declR = dyn_cast<DeclRefExpr>(&diag.expr)) {
+            if (auto var = dyn_cast<VarDecl>(declR->getDecl())) {
+              if (var->isAsyncLet()) {
+                Ctx.Diags.diagnose(declR->getLoc(),
+                                   diag::async_let_without_await,
+                                   var->getName());
+                continue;
+              }
+            }
+          }
+          LLVM_FALLTHROUGH; // fallthrough to a message about PropertyAccess
+        case PotentialEffectReason::Kind::PropertyAccess:
+          Ctx.Diags.diagnose(diag.expr.getStartLoc(),
+                             diag::async_access_without_await, 1);
+          continue;
+
+        case PotentialEffectReason::Kind::SubscriptAccess:
+          Ctx.Diags.diagnose(diag.expr.getStartLoc(),
+                             diag::async_access_without_await, 2);
+          continue;
+
+        case PotentialEffectReason::Kind::ByClosure:
+        case PotentialEffectReason::Kind::ByDefaultClosure:
+        case PotentialEffectReason::Kind::ByConformance:
+        case PotentialEffectReason::Kind::Apply: {
+         if (auto autoclosure = dyn_cast<AutoClosureExpr>(anchor)) {
+           switch(autoclosure->getThunkKind()) {
+             case AutoClosureExpr::Kind::None:
+               Ctx.Diags.diagnose(diag.expr.getStartLoc(),
+                                  diag::async_call_without_await_in_autoclosure);
+               break;
+             case AutoClosureExpr::Kind::AsyncLet:
+               Ctx.Diags.diagnose(diag.expr.getStartLoc(),
+                                  diag::async_call_without_await_in_async_let);
+               break;
+             case AutoClosureExpr::Kind::SingleCurryThunk:
+             case AutoClosureExpr::Kind::DoubleCurryThunk:
+               Ctx.Diags.diagnose(diag.expr.getStartLoc(),
+                                  diag::async_access_without_await, 0);
+               break;
+           }
+          continue;
+         }
+         Ctx.Diags.diagnose(diag.expr.getStartLoc(),
+                            diag::async_access_without_await, 0);
+
+         continue;
+        }
+      }
+    }
+  }
 };
 
 // Find nested functions and perform effects checking on them.