Merge pull request #74475 from ktoso/wip-async-get-if-let

[Concurrency] if let value on async value should be banned (not crash)

Author: ktoso

lib/ASTGen/Sources/ASTGen/Stmts.swift

@@ -129,7 +129,7 @@ extension ASTGenVisitor {
       } else {
         let identifier = pat.boundName
         if identifier != nil {
-          // For `if let foo { }` Create an implicit `foo` expression as the initializer.
+          // For `if let foo { }` Create a `foo` expression as the initializer.
           let ref = BridgedDeclNameRef.createParsed(.createIdentifier(identifier))
           let loc = BridgedDeclNameLoc.createParsed(self.generateSourceLoc(node.pattern))
           initializer =
@@ -139,7 +139,6 @@ extension ASTGenVisitor {
               kind: .ordinary,
               loc: loc
             ).asExpr
-          initializer.setImplicit()
         } else {
           // FIXME: Implement.
           // For `if let foo.bar {`, diagnose and convert it to `if let _ =  foo.bar`

lib/Parse/ParseStmt.cpp

@@ -1793,8 +1793,12 @@ Parser::parseStmtConditionElement(SmallVectorImpl<StmtConditionElement> &result,
     auto declRefExpr = new (Context) UnresolvedDeclRefExpr(bindingName,
                                                            DeclRefKind::Ordinary,
                                                            loc);
-    
-    declRefExpr->setImplicit();
+    // We do NOT mark this declRefExpr as implicit because that'd avoid
+    // reporting errors if it were used in a synchronous context in
+    // 'diagnoseUnhandledAsyncSite'. There may be other ways to resolve the
+    // reporting issue that we could explore in the future.
+    //
+    // Even though implicit, the ast node has correct source location.
     Init = makeParserResult(declRefExpr);
   } else if (BindingKindStr != "case") {
     // If the pattern is present but isn't an identifier, the user wrote

lib/Sema/TypeCheckEffects.cpp

@@ -1251,6 +1251,7 @@ class ApplyClassifier {
     Classification classification;
     PotentialEffectReason reason = PotentialEffectReason::forPropertyAccess();
     ConcreteDeclRef declRef = E->getDeclRef();
+
     if (auto getter = getEffectfulGetOnlyAccessor(declRef)) {
       reason = getKindOfEffectfulProp(declRef);
       classification = Classification::forDeclRef(
@@ -2018,7 +2019,7 @@ class ApplyClassifier {
   /// Given the type of an argument, try to determine if it contains
   /// a throws/async function in a way that is permitted to cause a
   /// rethrows/reasync function to throw/async.
-  static Classification 
+  static Classification
   classifyArgumentByType(Type paramType, SubstitutionMap subs,
                          ConditionalEffectKind conditional,
                          PotentialEffectReason reason, EffectKind kind) {
@@ -2663,12 +2664,20 @@ class Context {
   }
 
   /// providing a \c kind helps tailor the emitted message.
-  void
-  diagnoseUnhandledAsyncSite(DiagnosticEngine &Diags, ASTNode node,
+  void diagnoseUnhandledAsyncSite(DiagnosticEngine &Diags, ASTNode node,
                              std::optional<PotentialEffectReason> maybeReason,
                              bool forAwait = false) {
-    if (node.isImplicit())
+    if (node.isImplicit()) {
+      // The reason we return early on implicit nodes is that sometimes we
+      // inject implicit closures, e.g. in 'async let' and we'd end up
+      // "double reporting" some errors, with no great way to make sure the
+      // "more specific diagnostic" is emitted. So instead, we avoid emitting
+      // about implicit code.
+      //
+      // Some synthesized code, like macros, are NOT marked implicit, so we will
+      // report about errors in them properly.
       return;
+    }
 
     switch (getKind()) {
     case Kind::PotentiallyHandled: {
@@ -2691,6 +2700,7 @@ class Context {
       return;
     }
   }
+
 };
 
 /// A class to walk over a local context and validate the correctness
@@ -2723,13 +2733,13 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
 
       /// Do we have a throw site using 'try' in this context?
       HasTryThrowSite = 0x10,
-      
+
       /// Are we in the context of an 'await'?
       IsAsyncCovered = 0x20,
-      
+
       /// Do we have any calls to 'async' functions in this context?
       HasAnyAsyncSite = 0x40,
-      
+
       /// Do we have any 'await's in this context?
       HasAnyAwait = 0x80,
 
@@ -3293,7 +3303,7 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
           CEC.Flags.set(ContextFlags::HasAnyThrowSite);
         return Action::Continue(E);
       }
-      
+
       PostWalkResult<Stmt *> walkToStmtPost(Stmt *S) override {
         if (isa<ThrowStmt>(S))
           CEC.Flags.set(ContextFlags::HasAnyThrowSite);
@@ -3305,7 +3315,7 @@ class CheckEffectsCoverage : public EffectsHandlingWalker<CheckEffectsCoverage>
     for (auto &clause : ICD->getClauses()) {
       // Active clauses are handled by the normal AST walk.
       if (clause.isActive) continue;
-      
+
       for (auto elt : clause.Elements)
         elt.walk(ConservativeThrowChecker(*this));
     }

test/Concurrency/effectful_properties_async_if_optional_unwrap.swift

@@ -0,0 +1,57 @@
+// RUN: %target-swift-frontend -parse-as-library -emit-sil -verify %s
+
+// REQUIRES: concurrency
+
+@available(SwiftStdlib 5.1, *)
+struct Kappa {
+  var maybeData: Int? {
+    get async { 12 }
+  }
+
+  func take() -> Int? { // expected-note 3{{add 'async' to function 'take()' to make it asynchronous}}
+
+    if let data = maybeData { // expected-error{{'async' property access in a function that does not support concurrency}}
+      return data
+    }
+
+    let x = maybeData // expected-error{{'async' property access in a function that does not support concurrency}}
+      _ = x
+
+    if let maybeData { // expected-error{{'async' property access in a function that does not support concurrency}}
+      return maybeData
+    }
+
+    return nil
+  }
+
+  func takeAsync() async -> Int? {
+
+    if let data = maybeData { // expected-error{{expression is 'async' but is not marked with 'await'}}
+      // expected-note@-1{{property access is 'async'}}
+      return data
+    }
+
+    let x = maybeData // expected-error{{expression is 'async' but is not marked with 'await'}}
+    // expected-note@-1{{property access is 'async'}}
+    _ = x
+
+    if let maybeData { // expected-error{{expression is 'async' but is not marked with 'await'}}
+      // expected-note@-1{{property access is 'async'}}
+      return maybeData
+    }
+
+    return nil
+  }
+
+  func illegalSyntax() async -> Int? {
+    // This is NOT valid syntax and we reject it properly,
+    // though we could try to could give a better message
+    if let await maybeData { // expected-error{{unwrap condition requires a valid identifier}}
+      // expected-error@-1{{pattern variable binding cannot appear in an expression}}
+      // expected-warning@-2{{no 'async' operations occur within 'await' expression}}
+      return maybeData // expected-error{{expression is 'async' but is not marked with 'await'}}
+      // expected-note@-1{{property access is 'async'}}
+    }
+
+  }
+}

test/DebugInfo/guard-let-scope3.swift

@@ -11,11 +11,12 @@ public class S {
       // CHECK: sil_scope [[X1_RHS:[0-9]+]] { loc "{{.*}}":9:19 parent [[P]]
       // CHECK: sil_scope [[X1:[0-9]+]] { loc "{{.*}}":9:19 parent [[P]]
       // CHECK: sil_scope [[X2:[0-9]+]] { loc "{{.*}}":9:29 parent [[X1]]
+      // CHECK: sil_scope [[X3:[0-9]+]] { loc "{{.*}}":9:29 parent [[X1]]
       // CHECK: sil_scope [[GUARD:[0-9]+]] { loc "{{.*}}":9:36 parent [[P]]
       // CHECK: debug_value {{.*}} : $Optional<C>, let, name "x", {{.*}}, scope [[X1]]
-      // CHECK: debug_value {{.*}} : $C, let, name "x", {{.*}}, scope [[X2]]
+      // CHECK: debug_value {{.*}} : $C, let, name "x", {{.*}}, scope [[X3]]
       // FIXME: This source location is a little wild.
-      // CHECK-NEXT: release_value{{.*}}:[[@LINE+5]]:3, scope [[X2]]
+      // CHECK-NEXT: release_value{{.*}}:[[@LINE+5]]:3, scope [[X3]]
       throw MyError()
       // CHECK: function_ref {{.*}}MyError{{.*}}:[[@LINE-1]]:13, scope [[GUARD]]
     }

test/DebugInfo/if-let-scope.swift

@@ -5,6 +5,7 @@ public func f(value: String?) {
   if let value, let value = Int(value) {
     // CHECK: sil_scope [[S1:[0-9]+]] { loc "{{.*}}":5:3
     // CHECK: sil_scope [[S2:[0-9]+]] { loc "{{.*}}":5:10
+    // CHECK: sil_scope [[S2:[0-9]+]] { loc "{{.*}}":5:10
     // CHECK: sil_scope [[S3:[0-9]+]] { loc "{{.*}}":5:29 parent [[S2]] }
     // CHECK: sil_scope [[S4:[0-9]+]] { loc "{{.*}}":5:29 parent [[S2]] }
     // CHECK: sil_scope [[S5:[0-9]+]] { loc "{{.*}}":5:40 parent [[S4]] }

test/expr/unary/async_await.swift

@@ -226,13 +226,13 @@ func testAsyncExprWithoutAwait() async {
   if let result: A = result {} // expected-error {{expression is 'async' but is not marked with 'await'}} {{22-22=await }}
   // expected-warning@-1 {{immutable value 'result' was never used; consider replacing with '_' or removing it}}
   // expected-note@-2 {{reference to async let 'result' is 'async'}}
-  if let result: A {} // expected-error {{expression is 'async' but is not marked with 'await'}} {{19-19= = await result}}
+  if let result: A {} // expected-error {{expression is 'async' but is not marked with 'await'}} {{18-18=await }}
   // expected-warning@-1 {{immutable value 'result' was never used; consider replacing with '_' or removing it}}
   // expected-note@-2 {{reference to async let 'result' is 'async'}}
   if let result = result {} // expected-error {{expression is 'async' but is not marked with 'await'}} {{19-19=await }}
   // expected-warning@-1 {{value 'result' was defined but never used; consider replacing with boolean test}}
   // expected-note@-2 {{reference to async let 'result' is 'async'}}
-  if let result {} // expected-error {{expression is 'async' but is not marked with 'await'}} {{16-16= = await result}}
+  if let result {} // expected-error {{expression is 'async' but is not marked with 'await'}} {{10-10=await }}
   // expected-warning@-1 {{value 'result' was defined but never used; consider replacing with boolean test}}
   // expected-note@-2 {{reference to async let 'result' is 'async'}}
   let a = f("a") // expected-error {{expression is 'async' but is not marked with 'await'}} {{11-11=await }}