Sema: Make @concurrent not imply async on closures

The present approach is not prudent because `@concurrent` synchronous
functions, a natural extension, are a likely-to-happen future direction,
whereas the current inference rule is entirely grounded on `@concurrent`
being exclusive to async functions.

If we were to ship this rule, we would have to keep the promise for
backwards compatibility when implementing the aforementioned future
direction, replacing one inconsistency with another, and possibly
introducing new bug-prone expression checking code.

```swift
func foo(_: () -> Void) {}
func foo(_: () async -> Void) {}

// In a future without this change and  `@concurrent` synchronous
// functions accepted, the first call resolves to the first overload,
// and the second call resolves to the second, despite `@concurrent` no
// longer implying `async`.
foo { }
foo { @Concurrent in }
```

This change also drops the fix-it for removing `@concurrent` when used
on a synchronous closure. With the inference rule gone, and the
diagnosis delayed until after solution application, this error raises a
fairly balanced choice between removing the attribute and being
explicit about the effect, where a unilateral suggestion is quite
possibly more harmful than useful.

Author: AnthonyLatsis

lib/Sema/ConstraintSystem.cpp

@@ -1450,17 +1450,11 @@ FunctionType::ExtInfo ClosureEffectsRequest::evaluate(
   if (!body)
     return ASTExtInfoBuilder().withSendable(sendable).build();
 
-  // `@concurrent` attribute is only valid on asynchronous function types.
-  bool asyncFromAttr = false;
-  if (expr->getAttrs().hasAttribute<ConcurrentAttr>()) {
-    asyncFromAttr = true;
-  }
-
   auto throwFinder = FindInnerThrows(expr);
   body->walk(throwFinder);
   return ASTExtInfoBuilder()
       .withThrows(throwFinder.foundThrow(), /*FIXME:*/Type())
-      .withAsync(asyncFromAttr || bool(findAsyncNode(expr)))
+      .withAsync(bool(findAsyncNode(expr)))
       .withSendable(sendable)
       .build();
 }

lib/Sema/MiscDiagnostics.cpp

@@ -305,6 +305,11 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
         }
       }
 
+      // Performs checks on a closure.
+      if (auto *closure = dyn_cast<ClosureExpr>(E)) {
+        checkClosure(closure);
+      }
+
       // Specially diagnose some checked casts that are illegal.
       if (auto cast = dyn_cast<CheckedCastExpr>(E)) {
         checkCheckedCastExpr(cast);
@@ -1455,6 +1460,33 @@ static void diagSyntacticUseRestrictions(const Expr *E, const DeclContext *DC,
         }
       }
     }
+
+    void checkClosure(ClosureExpr *closure) {
+      if (closure->isImplicit()) {
+        return;
+      }
+
+      auto attr = closure->getAttrs().getAttribute<ConcurrentAttr>();
+      if (!attr) {
+        return;
+      }
+
+      if (closure->getAsyncLoc().isValid()) {
+        return;
+      }
+
+      if (closure->getType()->castTo<AnyFunctionType>()->isAsync()) {
+        return;
+      }
+
+      // `@concurrent` is not allowed on synchronous closures, but this is
+      // likely to change, so diagnose this here, post solution application,
+      // instead of introducing an "implies `async`" inference rule that won't
+      // make sense in the nearish future.
+      Ctx.Diags.diagnose(attr->getLocation(),
+                         diag::execution_behavior_only_on_async_closure, attr);
+      attr->setInvalid();
+    }
   };
 
   DiagnoseWalker Walker(DC, isExprStmt);

lib/Sema/TypeCheckAttr.cpp

@@ -8297,16 +8297,6 @@ class ClosureAttributeChecker
   }
 
   void checkExecutionBehaviorAttribute(DeclAttribute *attr) {
-    // execution behavior attribute implies `async`.
-    if (closure->hasExplicitResultType() &&
-        closure->getAsyncLoc().isInvalid()) {
-      ctx.Diags
-          .diagnose(attr->getLocation(),
-                    diag::execution_behavior_only_on_async_closure, attr)
-          .fixItRemove(attr->getRangeWithAt());
-      attr->setInvalid();
-    }
-
     if (auto actorType = getExplicitGlobalActor(closure)) {
       ctx.Diags
           .diagnose(

test/Concurrency/attr_execution/conversions.swift

@@ -93,7 +93,7 @@ do {
 
 do {
   let _: () -> Void = { @concurrent in
-    // expected-error@-1 {{invalid conversion from 'async' function of type '() async -> Void' to synchronous function type '() -> Void'}}
+    // expected-error@-1 {{cannot use @concurrent on non-async closure}}{{none}}
   }
 }
 

test/attr/execution_behavior_attrs.swift

@@ -125,17 +125,74 @@ struct IsolatedType {
   @concurrent func test() async {}
 }
 
-_ = { @concurrent in // Ok
+// `@concurrent` on closures does not contribute to `async` inference.
+do {
+  _ = { @concurrent in }
+  // expected-error@-1:9 {{cannot use @concurrent on non-async closure}}{{none}}
+  _ = { @concurrent () -> Int in }
+  // expected-error@-1:9 {{cannot use @concurrent on non-async closure}}{{none}}
+  // Explicit effect precludes effects inference from the body.
+  _ = { @concurrent () throws in await awaitMe() }
+  // expected-error@-1:9 {{cannot use @concurrent on non-async closure}}{{none}}
+  // expected-error@-2:40 {{'async' call in a function that does not support concurrency}}{{none}}
+  _ = { @concurrent () async in }
+  _ = { @concurrent in await awaitMe() }
+
+  func awaitMe() async {}
+
+  do {
+    func foo(_: () -> Void) {}
+    func foo(_: () async -> Void) async {}
+
+    func sync() {
+      foo { @concurrent in }
+      // expected-error@-1:13 {{cannot use @concurrent on non-async closure}}{{none}}
+    }
+    // expected-note@+1:10 {{add 'async' to function 'sync2()' to make it asynchronous}}{{17-17= async}}
+    func sync2() {
+      foo { @concurrent in await awaitMe() }
+      // expected-error@-1:7 {{'async' call in a function that does not support concurrency}}{{none}}
+    }
+    func async() async {
+      // Even though the context is async, the sync overload is favored because
+      // the closure is sync, so the error is expected.
+      foo { @concurrent in }
+      // expected-error@-1:13 {{cannot use @concurrent on non-async closure}}{{none}}
+
+      foo { @concurrent in await awaitMe() }
+      // expected-error@-1:7 {{expression is 'async' but is not marked with 'await'}}{{7-7=await }}
+      // expected-note@-2:7 {{call is 'async'}}{{none}}
+    }
+  }
+
+  do {
+    func foo(_: (Int) async -> Void) {}
+    func foo(_: (Int) -> Void) async {}
+
+    func sync() {
+      // OK, sync overload picked.
+      foo { @concurrent _ in }
+    }
+    func async() async {
+      foo { @concurrent _ in }
+      // expected-error@-1:13 {{cannot use @concurrent on non-async closure}}{{none}}
+      // expected-error@-2:7 {{expression is 'async' but is not marked with 'await'}}{{7-7=await }}
+      // expected-note@-3:7 {{call is 'async'}}{{none}}
+    }
+  }
+
+  do {
+    func foo<T>(_: T) {}
+
+    foo({@concurrent in })
+    // expected-error@-1:10 {{cannot use @concurrent on non-async closure}}{{none}}
+  }
 }
 
 _ = { @MainActor @concurrent in
   // expected-error@-1 {{cannot use @concurrent because function type is isolated to a global actor 'MainActor'}}
 }
 
-_ = { @concurrent () -> Int in
-  // expected-error@-1 {{@concurrent on non-async closure}}
-}
-
 // Make sure that explicit use of `@concurrent` doesn't interfere with inference of `throws` from the body.
 do {
     func acceptsThrowing(_ body: () async throws -> Void) async {