[Sema] Catch use-before-declarations in nested closures

Previously we would allow these in Sema and diagnose them in SILGen,
but allowing them in Sema is unsound because it means the constraint
system ends up kicking interface type requests for declarations that
should be type-checked as part of the closure itself. Adjust the
name lookup logic to look through parent closures when detecting
invalid forward references.

For now we don't fallback to an outer result on encountering a
use-before-declaration to preserve the current behavior. I'm planning
on changing that in the next commit though.

rdar://74430478

Author: hamishknight

lib/Sema/PreCheckTarget.cpp

@@ -393,29 +393,34 @@ static bool isMemberChainTail(Expr *expr, Expr *parent, MemberChainKind kind) {
 }
 
 static bool isValidForwardReference(ValueDecl *D, DeclContext *DC,
-                                    ValueDecl **localDeclAfterUse) {
-  *localDeclAfterUse = nullptr;
-
-  // References to variables injected by lldb are always valid.
-  if (isa<VarDecl>(D) && cast<VarDecl>(D)->isDebuggerVar())
+                                    ValueDecl *&localDeclAfterUse,
+                                    bool &shouldUseOuterResults) {
+  // Only VarDecls require declaration before use.
+  auto *VD = dyn_cast<VarDecl>(D);
+  if (!VD)
     return true;
 
-  // If we find something in the current context, it must be a forward
-  // reference, because otherwise if it was in scope, it would have
-  // been returned by the call to ASTScope::lookupLocalDecls() above.
-  if (D->getDeclContext()->isLocalContext()) {
-    do {
-      if (D->getDeclContext() == DC) {
-        *localDeclAfterUse = D;
-        return false;
-      }
+  // Non-local and variables injected by lldb are always valid.
+  auto *varDC = VD->getDeclContext();
+  if (!varDC->isLocalContext() || VD->isDebuggerVar())
+    return true;
 
-      // If we're inside of a 'defer' context, walk up to the parent
-      // and check again. We don't want 'defer' bodies to forward
-      // reference bindings in the immediate outer scope.
-    } while (isa<FuncDecl>(DC) &&
-             cast<FuncDecl>(DC)->isDeferBody() &&
-             (DC = DC->getParent()));
+  while (true) {
+    if (varDC == DC) {
+      localDeclAfterUse = VD;
+      return false;
+    }
+    if (isa<AbstractClosureExpr>(DC) ||
+        (isa<FuncDecl>(DC) && cast<FuncDecl>(DC)->isDeferBody())) {
+      // If we cross a closure, don't allow falling back to an outer result if
+      // we have a forward reference. This preserves the behavior prior to
+      // diagnosing this in Sema.
+      if (isa<AbstractClosureExpr>(DC))
+        shouldUseOuterResults = false;
+      DC = DC->getParent();
+      continue;
+    }
+    break;
   }
   return true;
 }
@@ -587,19 +592,20 @@ static Expr *resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE, DeclContext *DC,
     Lookup = TypeChecker::lookupUnqualified(DC, LookupName, Loc, lookupOptions);
 
     ValueDecl *localDeclAfterUse = nullptr;
-    AllDeclRefs =
-        findNonMembers(Lookup.innerResults(), UDRE->getRefKind(),
-                       /*breakOnMember=*/true, ResultValues,
-                       [&](ValueDecl *D) {
-                         return isValidForwardReference(D, DC, &localDeclAfterUse);
-                       });
+    bool shouldUseOuterResults = true;
+    AllDeclRefs = findNonMembers(
+        Lookup.innerResults(), UDRE->getRefKind(),
+        /*breakOnMember=*/true, ResultValues, [&](ValueDecl *D) {
+          return isValidForwardReference(D, DC, localDeclAfterUse,
+                                         shouldUseOuterResults);
+        });
 
     // If local declaration after use is found, check outer results for
     // better matching candidates.
     if (ResultValues.empty() && localDeclAfterUse) {
       auto innerDecl = localDeclAfterUse;
       while (localDeclAfterUse) {
-        if (Lookup.outerResults().empty()) {
+        if (!shouldUseOuterResults || Lookup.outerResults().empty()) {
           Context.Diags.diagnose(Loc, diag::use_local_before_declaration, Name);
           Context.Diags.diagnose(innerDecl, diag::decl_declared_here,
                                  localDeclAfterUse);
@@ -609,12 +615,12 @@ static Expr *resolveDeclRefExpr(UnresolvedDeclRefExpr *UDRE, DeclContext *DC,
         Lookup.shiftDownResults();
         ResultValues.clear();
         localDeclAfterUse = nullptr;
-        AllDeclRefs =
-            findNonMembers(Lookup.innerResults(), UDRE->getRefKind(),
-                           /*breakOnMember=*/true, ResultValues,
-                           [&](ValueDecl *D) {
-                             return isValidForwardReference(D, DC, &localDeclAfterUse);
-                           });
+        AllDeclRefs = findNonMembers(
+            Lookup.innerResults(), UDRE->getRefKind(),
+            /*breakOnMember=*/true, ResultValues, [&](ValueDecl *D) {
+              return isValidForwardReference(D, DC, localDeclAfterUse,
+                                             shouldUseOuterResults);
+            });
       }
     }
   }

test/NameLookup/name_lookup.swift

@@ -643,8 +643,8 @@ struct PatternBindingWithTwoVars3 { var x = y, y = x }
 
 // https://github.com/apple/swift/issues/51518
 do {
-  let closure1 = { closure2() } // expected-error {{circular reference}} expected-note {{through reference here}}
-  let closure2 = { closure1() } // expected-note {{through reference here}} expected-note {{through reference here}}
+  let closure1 = { closure2() } // expected-error {{use of local variable 'closure2' before its declaration}}
+  let closure2 = { closure1() } // expected-note {{'closure2' declared here}}
 }
 
 func color(with value: Int) -> Int {

test/NameLookup/use_before_declaration_shadowing.swift

@@ -0,0 +1,73 @@
+// RUN: %target-typecheck-verify-swift
+// RUN: %target-typecheck-verify-swift -parse-as-library
+
+func testLocal() {
+  // The first `y` here is considered the inner result.
+  do {
+    let y = ""
+    do {
+      let _: String = y
+      let y = 0
+      _ = y
+    }
+  }
+  do {
+    let y = ""
+    do {
+      _ = {
+        let _: String = y
+      }
+      let y = 0
+      _ = y
+    }
+  }
+  do {
+    let y = ""
+    _ = {
+      _ = {
+        let _: String = y
+      }
+      let y = 0
+      _ = y
+    }
+  }
+  do {
+    let y = ""
+    func bar() {
+      _ = {
+        let _: String = y
+      }
+      let y = 0
+      _ = y
+    }
+  }
+}
+
+let topLevelString = ""
+
+func testTopLevel() {
+  // Here 'topLevelString' is now an outer result.
+  do {
+    let _: String = topLevelString
+    let topLevelString = 0
+    _ = topLevelString
+  }
+  do {
+    _ = {
+      let _: String = topLevelString // expected-error {{use of local variable 'topLevelString' before its declaration}}
+    }
+    let topLevelString = 0 // expected-note {{'topLevelString' declared here}}
+  }
+  _ = {
+    _ = {
+      let _: String = topLevelString // expected-error {{use of local variable 'topLevelString' before its declaration}}
+    }
+    let topLevelString = 0 // expected-note {{'topLevelString' declared here}}
+  }
+  func bar() {
+    _ = {
+      let _: String = topLevelString // expected-error {{use of local variable 'topLevelString' before its declaration}}
+    }
+    let topLevelString = 0 // expected-note {{'topLevelString' declared here}}
+  }
+}

test/SILGen/capture_order.swift

@@ -109,16 +109,6 @@ func transitiveForwardCapture3() {
   }
 }
 
-func captureInClosure() {
-  let x = { (i: Int) in // expected-error {{closure captures 'currentTotal' before it is declared}}
-    currentTotal += i // expected-note {{captured here}}
-  }
-
-  var currentTotal = 0 // expected-note {{captured value declared here}}
-
-  _ = x
-}
-
 /// Regression tests
 
 // https://github.com/apple/swift/issues/47389
@@ -183,16 +173,6 @@ func f_57097() {
   // expected-warning@-1 {{variable 'r' was never mutated; consider changing to 'let' constant}}
 }
 
-class class77933460 {}
-
-func func77933460() {
-  var obj: class77933460 = { obj }()
-  // expected-error@-1 {{closure captures 'obj' before it is declared}}
-  // expected-note@-2 {{captured here}}
-  // expected-note@-3 {{captured value declared here}}
-  // expected-warning@-4 {{variable 'obj' was never mutated; consider changing to 'let' constant}}
-}
-
 // MARK: - Forward Declared Lets
 
 // https://github.com/swiftlang/swift/issues/84909
@@ -208,110 +188,6 @@ func global_fwd(_ a: () -> Any) -> Any { a() }
 func global_gen_fwd<T>(_ g: () -> T) -> T { g() }
 func global_fwd_p(_ p: () -> any P) -> any P { p() }
 
-func forward_declared_let_captures() {
-  do {
-    let bad: Any = { bad }()
-    // expected-error@-1 {{closure captures 'bad' before it is declared}}
-    // expected-note@-2 {{captured here}}
-    // expected-note@-3 {{captured value declared here}}
-  }
-
-  do {
-    func fwd(_ i: () -> Any) -> Any { i() }
-    let bad = fwd { bad }
-    // expected-error@-1 {{closure captures 'bad' before it is declared}}
-    // expected-note@-2 {{captured here}}
-    // expected-note@-3 {{captured value declared here}}
-  }
-
-  do {
-    let bad = global_fwd { bad }
-    // expected-error@-1 {{closure captures 'bad' before it is declared}}
-    // expected-note@-2 {{captured here}}
-    // expected-note@-3 {{captured value declared here}}
-  }
-
-  do {
-    let bad: Any = global_gen_fwd { bad }
-    // expected-error@-1 {{closure captures 'bad' before it is declared}}
-    // expected-note@-2 {{captured here}}
-    // expected-note@-3 {{captured value declared here}}
-  }
-
-  do {
-    let bad: Any = E.static_gen_fwd { bad }
-    // expected-error@-1 {{closure captures 'bad' before it is declared}}
-    // expected-note@-2 {{captured here}}
-    // expected-note@-3 {{captured value declared here}}
-  }
-
-  do {
-    let badNested: Any = global_fwd { { [badNested] in badNested }() }
-    // expected-error@-1 {{closure captures 'badNested' before it is declared}}
-    // expected-note@-2 {{captured here}}
-    // expected-note@-3 {{captured value declared here}}
-  }
-
-  do {
-    let badOpt: Any? = { () -> Any? in badOpt }()
-    // expected-error@-1 {{closure captures 'badOpt' before it is declared}}
-    // expected-note@-2 {{captured here}}
-    // expected-note@-3 {{captured value declared here}}
-  }
-
-  do {
-    let badTup: (Any, Any) = { (badTup.0, badTup.1) }()
-    // expected-error@-1 {{closure captures 'badTup' before it is declared}}
-    // expected-note@-2 {{captured here}}
-    // expected-note@-3 {{captured value declared here}}
-  }
-
-  do {
-    let badTup: (Int, Any) = { (badTup.0, badTup.1) }()
-    // expected-error@-1 {{closure captures 'badTup' before it is declared}}
-    // expected-note@-2 {{captured here}}
-    // expected-note@-3 {{captured value declared here}}
-  }
-
-  do {
-    let (badTup3, badTup4): (Any, Any) = { (badTup4, badTup3) }()
-    // expected-error@-1 {{closure captures 'badTup4' before it is declared}}
-    // expected-note@-2 {{captured here}}
-    // expected-note@-3 {{captured value declared here}}
-    // expected-error@-4 {{closure captures 'badTup3' before it is declared}}
-    // expected-note@-5 {{captured here}}
-    // expected-note@-6 {{captured value declared here}}
-  }
-
-  do {
-    struct S { var p: Any }
-    let badStruct: S = { S(p: badStruct.p) }()
-    // expected-error@-1 {{closure captures 'badStruct' before it is declared}}
-    // expected-note@-2 {{captured here}}
-    // expected-note@-3 {{captured value declared here}}
-  }
-
-  do {
-    enum EE {
-      case boring
-      case weird(Any)
-      case strange(Any)
-    }
-
-    let badEnum: EE = { .weird(EE.strange(badEnum)) }()
-    // expected-error@-1 {{closure captures 'badEnum' before it is declared}}
-    // expected-note@-2 {{captured here}}
-    // expected-note@-3 {{captured value declared here}}
-  }
-
-  do {
-    let badproto: any P = global_fwd_p { badproto }
-    // expected-error@-1 {{closure captures 'badproto' before it is declared}}
-    // expected-note@-2 {{captured here}}
-    // expected-note@-3 {{captured value declared here}}
-  }
-}
-
 func forward_declared_let_captures_local_fn() {
   do {
     func bad_local_f() -> Any { bad }
@@ -441,10 +317,13 @@ func forward_declared_let_captures_local_fn() {
   }
 }
 
-func forward_declared_local_lazy_captures() {
-  // runtime stack overflow
-  lazy var infiniteRecurse: Any = { infiniteRecurse }()
-
-  // function that returns itself
-  lazy var hmm: () -> Any = { hmm }
-}
+// FIXME: Currently they crash SILGen (TypeConverter-setCaptureTypeExpansionContext-e72208.swift)
+//func forward_declared_local_lazy_captures() {
+//  // runtime stack overflow
+//  var _infiniteRecurse: Any { infiniteRecurse }
+//  lazy var infiniteRecurse = _infiniteRecurse
+//
+//  // function that returns itself
+//  func _hmm() -> Any { hmm }
+//  lazy var hmm = _hmm
+//}

test/SILOptimizer/closure_lifetime_fixup_undef.swift

@@ -1,13 +1,14 @@
-// RUN: not %target-swift-frontend %s -sil-verify-all -c 2>&1 | %FileCheck %s
+// RUN: %target-typecheck-verify-swift
 
 // Report the error but don't crash.
-// CHECK: error: closure captures 'stringList' before it is declared
 
 class TestUndefined {
   private var stringList: [String]!
 
   func dontCrash(strings: [String]) {
     assert(stringList.allSatisfy({ $0 == stringList.first!}))
+    // expected-error@-1 {{use of local variable 'stringList' before its declaration}}
     let stringList = strings.filter({ $0 == "a" })
+    // expected-note@-1 {{'stringList' declared here}}
   }
 }

test/SILOptimizer/definite_init_closures_fail.swift

@@ -65,3 +65,7 @@ struct Generic<T : P> {
   } // expected-error {{return from initializer without initializing all stored properties}}
 }
 
+func captureUninitialized() {
+  let fn: () -> Void // expected-note {{constant defined here}}
+  fn = { fn() } // expected-error {{constant 'fn' captured by a closure before being initialized}}
+}