[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.

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 context, 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;
 }
@@ -589,19 +594,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);
@@ -611,12 +617,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
@@ -182,13 +172,3 @@ func f_57097() {
   var r = g() // expected-note {{captured value declared here}}
   // 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}}
-}

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}}
+}

test/Sema/diag_use_before_declaration.swift

@@ -80,6 +80,24 @@ func nested_scope_3() {
   }
 }
 
+func captureInClosure() {
+  let x = { (i: Int) in
+    currentTotal += i // expected-error {{use of local variable 'currentTotal' before its declaration}}
+  }
+
+  var currentTotal = 0 // expected-note {{'currentTotal' declared here}}
+
+  _ = x
+}
+
+class class77933460 {}
+
+func func77933460() {
+  var obj: class77933460 = { obj }()
+  // expected-error@-1 {{use of local variable 'obj' before its declaration}}
+  // expected-note@-2 {{'obj' declared here}}
+}
+
 //===----------------------------------------------------------------------===//
 // Type scope
 //===----------------------------------------------------------------------===//

test/expr/closure/multi_statement.swift

@@ -358,7 +358,8 @@ func test_no_crash_with_circular_ref_due_to_error() {
       // expected-error@-1 {{consecutive statements on a line must be separated by ';'}}
       // expected-error@-2 {{'let' cannot appear nested inside another 'var' or 'let' pattern}}
       // expected-error@-3 {{cannot call value of non-function type 'Int?'}}
-      print(next)
+      // expected-note@-4 {{'next' declared here}}
+      print(next) // expected-error {{use of local variable 'next' before its declaration}}
       return x
     }
     return 0
@@ -676,20 +677,26 @@ func test_recursive_var_reference_in_multistatement_closure() {
 
   func test(optionalInt: Int?, themes: MyStruct?) {
     takeClosure {
-      let int = optionalInt { // expected-error {{cannot call value of non-function type 'Int?'}}
-        print(int)
+      let int = optionalInt {
+        // expected-error@-1 {{cannot call value of non-function type 'Int?'}}
+        // expected-note@-2 {{'int' declared here}}
+        print(int) // expected-error {{use of local variable 'int' before its declaration}}
       }
     }
 
     takeClosure {
-      let theme = themes?.someMethod() { // expected-error {{extra trailing closure passed in call}}
-        _ = theme
+      let theme = themes?.someMethod() {
+        // expected-error@-1 {{extra trailing closure passed in call}}
+        // expected-note@-2 {{'theme' declared here}}
+        _ = theme // expected-error {{use of local variable 'theme' before its declaration}}
       }
     }
 
     takeClosure {
-      let theme = themes?.filter({ $0 }) { // expected-error {{value of type 'MyStruct' has no member 'filter'}}
-        _ = theme
+      let theme = themes?.filter({ $0 }) {
+        // expected-error@-1 {{value of type 'MyStruct' has no member 'filter'}}
+        // expected-note@-2 {{'theme' declared here}}
+        _ = theme // expected-error {{use of local variable 'theme' before its declaration}}
       }
     }
   }

test/expr/expressions.swift

@@ -244,23 +244,24 @@ func test_as_2() {
   x as [] // expected-error {{expected element type}} {{9-9= <#type#>}}
 }
 
-func test_lambda() {
+func test_lambda1() {
   // A simple closure.
   var a = { (value: Int) -> () in markUsed(value+1) }
   // expected-warning@-1 {{initialization of variable 'a' was never used; consider replacing with assignment to '_' or removing it}}
+}
 
+func test_lambda2() {
   // A recursive lambda.
-  var fib = { (n: Int) -> Int in
-    // expected-warning@-1 {{variable 'fib' was never mutated; consider changing to 'let' constant}}
+  var fib = { (n: Int) -> Int in // expected-note 2{{'fib' declared here}}
     if (n < 2) {
       return n
     }
 
-    return fib(n-1)+fib(n-2)
+    return fib(n-1)+fib(n-2) // expected-error 2{{use of local variable 'fib' before its declaration}}
   }
 }
 
-func test_lambda2() {
+func test_lambda3() {
   { () -> protocol<Int> in
     // expected-error @-1 {{'protocol<...>' composition syntax has been removed and is not needed here}} {{11-24=Int}}
     // expected-error @-2 {{non-protocol, non-class type 'Int' cannot be used within a protocol-constrained type}}

test/expr/unary/if_expr.swift

@@ -1647,3 +1647,13 @@ func testCaptureList() {
   let _ = { [x = (if .random() { 0 } else { 1 })] in x }
   // expected-error@-1 {{'if' may only be used as expression in return, throw, or as the source of an assignment}}
 }
+
+func testUseBeforeDecl() throws {
+  let x = if .random() {
+    print(y) // expected-error {{use of local variable 'y' before its declaration}}
+    let y = 0 // expected-note {{'y' declared here}}
+    throw Err()
+  } else {
+    0
+  }
+}