Merge pull request #78738 from calda/cal--fix-nested-weak-self-issue

Fix issue where implicit self was unexpectedly not allowed in nested weak self closure in Swift 6 mode

Author: hamishknight

lib/Sema/MiscDiagnostics.cpp

@@ -1840,16 +1840,14 @@ class ImplicitSelfUsageChecker : public BaseDiagnosticWalker {
       return selfDeclAllowsImplicitSelf510(DRE, ty, inClosure);
 
     return selfDeclAllowsImplicitSelf(DRE->getDecl(), ty, inClosure,
-                                      /*validateParentClosures:*/ true,
-                                      /*validateSelfRebindings:*/ true);
+                                      /*isValidatingParentClosures:*/ false);
   }
 
   /// Whether or not implicit self is allowed for this implicit self decl
   static bool selfDeclAllowsImplicitSelf(const ValueDecl *selfDecl,
                                          const Type captureType,
                                          const AbstractClosureExpr *inClosure,
-                                         bool validateParentClosures,
-                                         bool validateSelfRebindings) {
+                                         bool isValidatingParentClosures) {
     ASTContext &ctx = inClosure->getASTContext();
 
     auto requiresSelfQualification =
@@ -1883,28 +1881,32 @@ class ImplicitSelfUsageChecker : public BaseDiagnosticWalker {
       }
     }
 
-    // If the self decl comes from a conditional statement, validate
-    // that it is an allowed `guard let self` or `if let self` condition.
+    // If the self decl refers to a weak capture, then implicit self is not
+    // allowed. Self must be unwrapped in a `guard let self` / `if let self`
+    // condition first. This is usually enforced by the type checker, but
+    // isn't in some cases (e.g. when calling a method on `Optional<Self>`).
+    //  - When validating implicit self usage in a nested closure, it's not
+    //    necessary for self to be unwrapped in this parent closure. If self
+    //    isn't unwrapped correctly in the nested closure, we would have
+    //    already noticed when validating that closure.
+    if (selfDecl->getInterfaceType()->is<WeakStorageType>() &&
+        !isValidatingParentClosures) {
+      return false;
+    }
+
+    // If the self decl refers to an invalid unwrapping conditon like
+    // `guard let self = somethingOtherThanSelf`, then implicit self is always
+    // disallowed.
     //  - Even if this closure doesn't have a `weak self` capture, it could
     //    be a closure nested in some parent closure with a `weak self`
     //    capture, so we should always validate the conditional statement
     //    that defines self if present.
-    if (validateSelfRebindings) {
-      if (auto conditionalStmt = parentConditionalStmt(selfDecl)) {
-        if (!hasValidSelfRebinding(conditionalStmt, ctx)) {
-          return false;
-        }
+    if (auto condStmt = parentConditionalStmt(selfDecl)) {
+      if (!hasValidSelfRebinding(condStmt, ctx)) {
+        return false;
       }
     }
 
-    // If this closure has a `weak self` capture, require that the
-    // closure unwraps self. If not, implicit self is not allowed
-    // in this closure or in any nested closure.
-    if (closureHasWeakSelfCapture(inClosure) &&
-        !hasValidSelfRebinding(parentConditionalStmt(selfDecl), ctx)) {
-      return false;
-    }
-
     if (auto autoclosure = dyn_cast<AutoClosureExpr>(inClosure)) {
       // Implicit self is always allowed in autoclosure thunks generated
       // during type checking. An example of this is when storing an instance
@@ -1927,7 +1929,7 @@ class ImplicitSelfUsageChecker : public BaseDiagnosticWalker {
     //  - We have to do this for all closures, even closures that typically
     //    don't require self qualificationm since an invalid self capture in
     //    a parent closure still disallows implicit self in a nested closure.
-    if (validateParentClosures) {
+    if (!isValidatingParentClosures) {
       return !implicitSelfDisallowedDueToInvalidParent(selfDecl, captureType,
                                                        inClosure);
     } else {
@@ -2048,10 +2050,14 @@ class ImplicitSelfUsageChecker : public BaseDiagnosticWalker {
         // Check whether implicit self is disallowed due to this specific
         // closure, or if its disallowed due to some parent of this closure,
         // so we can return the specific closure that is invalid.
+        //
+        // If this is a `weak self` capture, we don't need to validate that
+        // that capture has been unwrapped in a `let self = self` binding
+        // within the parent closure. A self rebinding in this inner closure
+        // is sufficient to enable implicit self.
         if (!selfDeclAllowsImplicitSelf(outerSelfDecl, captureType,
                                         outerClosure,
-                                        /*validateParentClosures:*/ false,
-                                        /*validateSelfRebindings:*/ true)) {
+                                        /*isValidatingParentClosures:*/ true)) {
           return outerClosure;
         }
 
@@ -2064,8 +2070,7 @@ class ImplicitSelfUsageChecker : public BaseDiagnosticWalker {
       // parent closures, we don't need to do that separate for this closure.
       if (validateIntermediateParents) {
         if (!selfDeclAllowsImplicitSelf(selfDecl, captureType, outerClosure,
-                                        /*validateParentClosures*/ false,
-                                        /*validateSelfRebindings*/ false)) {
+                                        /*isValidatingParentClosures*/ true)) {
           return outerClosure;
         }
       }

test/expr/closure/closures.swift

@@ -1407,6 +1407,13 @@ class TestGithubIssue69911 {
         }
       }
 
+      doVoidStuff { [weak self] in
+        guard let self = self ?? TestGithubIssue69911.staticOptional else { return }
+        doVoidStuff { [self] in // expected-warning {{capture 'self' was never used}} expeced-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}}
+          x += 1 // expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}}
+        }
+      }
+
       doVoidStuff { [weak self] in
         // Since this unwrapping is invalid, implicit self is disallowed in all nested closures:
         guard let self = self ?? TestGithubIssue69911.staticOptional else { return }
@@ -1804,3 +1811,31 @@ class TestLazyLocal {
     }
   }
 }
+
+class TestExtensionOnOptionalSelf {
+  init() {}
+}
+
+extension TestExtensionOnOptionalSelf? {
+  func foo() {
+    _ = { [weak self] in // expected-warning {{variable 'self' was written to, but never read}}
+      foo() // expected-error {{call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit}}
+    }
+
+    _ = {
+      foo()
+    }
+
+    _ = { [weak self] in // expected-warning {{variable 'self' was written to, but never read}}
+      _ = {
+        foo()
+      }
+    }
+
+    _ = { [weak self] in
+      _ = { [self] in // expected-warning {{capture 'self' was never used}}
+        foo()
+      }
+    }
+  }
+}

test/expr/closure/closures_swift6.swift

@@ -454,6 +454,92 @@ class TestGithubIssue70089 {
         }
       }
     }
+
+    func testClosuresInsideWeakSelfNotUnwrapped() {
+      // https://forums.swift.org/t/nested-weak-capture-and-implicit-self-in-swift-6/77230/1
+      doVoidStuff { [weak self] in
+        doVoidStuff { [weak self] in
+          guard let self else { return }
+          x += 1
+        }
+      }
+
+      doVoidStuff { [weak self] in
+        doVoidStuff { [weak self] in
+          doVoidStuff { [weak self] in
+            guard let self else { return }
+            doVoidStuff { [weak self] in
+              doVoidStuff { [weak self] in
+                guard let self else { return }
+                x += 1
+              }
+            }
+          }
+        }
+      }
+
+      doVoidStuff { [weak self] in
+        doVoidStuff { [weak self] in
+          guard let self else { return }
+          doVoidStuff { [self] in
+            doVoidStuff { [self] in
+              doVoidStuff { [weak self] in
+                guard let self else { return }
+                x += 1
+              }
+            }
+          }
+        }
+      }
+
+      doVoidStuff { [weak self] in
+        guard let self = self ?? TestGithubIssue70089.staticOptional else { return }
+        doVoidStuff { [weak self] in
+          guard let self else { return }
+          x += 1 // expected-error{{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}}
+        }
+      }
+
+      doVoidStuff { [weak self] in
+        doVoidStuff { [self] in
+          x += 1 // expected-error {{explicit use of 'self' is required when 'self' is optional, to make control flow explicit}} expected-note{{reference 'self?.' explicitly}}
+        }
+      }
+
+      doVoidStuff { [weak self] in
+        doVoidStuff { [self] in
+          self.x += 1 // expected-error {{value of optional type 'TestGithubIssue70089?' must be unwrapped to refer to member 'x' of wrapped base type 'TestGithubIssue70089'}} expected-note {{chain the optional using '?' to access member 'x' only for non-'nil' base values}} expected-note{{force-unwrap using '!' to abort execution if the optional value contains 'nil'}} 
+        }
+      }
+
+      doVoidStuff { [weak self] in
+        doVoidStuff { [self] in
+          self?.x += 1
+        }
+      }
+
+      doVoidStuff { [weak self] in
+        doVoidStuff { [self] in
+          guard let self else { return }
+          self.x += 1
+        }
+      }
+
+      doVoidStuff { [weak self] in
+        doVoidStuff { [self] in
+          guard let self else { return }
+          x += 1
+        }
+      }
+
+      doVoidStuff { [weak self] in
+        guard let self = self ?? TestGithubIssue70089.staticOptional else { return }
+        doVoidStuff { [self] in
+          guard let self else { return } // expected-error{{initializer for conditional binding must have Optional type, not 'TestGithubIssue70089'}}
+          x += 1 // expected-error{{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}}
+        }
+      }
+    }
 }
 
 class TestGithubIssue69911 {
@@ -522,9 +608,9 @@ class TestGithubIssue69911 {
           self.x += 1
         }
 
-        doVoidStuffNonEscaping {
+        doVoidStuffNonEscaping { // expected-note{{capture 'self' explicitly to enable implicit 'self' in this closure}}
           doVoidStuffNonEscaping {
-            x += 1 // expected-error{{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}}
+            x += 1 // expected-error{{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note{{reference 'self.' explicitly}}
             self.x += 1
           }
         }
@@ -670,7 +756,7 @@ final class AutoclosureTests {
     doVoidStuff { [weak self] in
       doVoidStuff { [self] in
         guard let self else { return }
-        method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}}
+        method()
       }
     }
 
@@ -702,13 +788,6 @@ final class AutoclosureTests {
       }
     }
 
-    doVoidStuff { [weak self] in
-      doVoidStuff { [self] in
-        guard let self else { return }
-        method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}}
-      }
-    }
-
     doVoidStuff { [weak self] in
       guard let self = self else { return }
       doVoidStuff { // expected-note {{capture 'self' explicitly to enable implicit 'self' in this closure}}
@@ -844,3 +923,118 @@ class rdar129475277 {
     }
   }
 }
+
+class TestExtensionOnOptionalSelf {
+  init() {}
+  func bar() {}
+}
+
+extension TestExtensionOnOptionalSelf? {
+  func foo() {
+    _ = { [weak self] in
+      foo() // expected-error {{implicit use of 'self' in closure; use 'self.' to make capture semantics explicit}}
+    }
+
+    _ = {
+      foo()
+      self.foo()
+      self?.bar()
+    }
+
+    _ = { [weak self] in
+      _ = {
+        foo() // expected-error {{implicit use of 'self' in closure; use 'self.' to make capture semantics explicit}}
+        self.foo()
+        self?.bar()
+      }
+    }
+
+    _ = { [weak self] in
+      _ = { [self] in
+        foo()
+        self.foo()
+        self?.bar()
+      }
+    }
+  }
+}
+
+// non-optional self in this extension, but on a type with members defined on optional self
+extension TestExtensionOnOptionalSelf {
+  func foo() {
+    _ = { [weak self] in
+      foo() // expected-error {{implicit use of 'self' in closure; use 'self.' to make capture semantics explicit}}
+      self.foo()
+      self?.bar()
+    }
+
+    _ = { // expected-note {{capture 'self' explicitly to enable implicit 'self' in this closure}}
+      foo() // expected-error {{call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-note {{reference 'self.' explicitly}}
+      self.foo()
+    }
+
+    _ = { [weak self] in
+      _ = {
+        foo() // expected-error {{implicit use of 'self' in closure; use 'self.' to make capture semantics explicit}}
+        self.foo()
+      }
+    }
+
+    _ = { [weak self] in
+      _ = { [self] in
+        foo()
+        self.foo()
+      }
+    }
+
+    _ = { [weak self] in
+      _ = { [self] in
+        _ = { [self] in
+          foo()
+          self.foo()
+        }
+      }
+    }
+
+    _ = { [weak self] in
+      doVoidStuffNonEscaping {
+        _ = { [self] in
+          foo()
+          self.foo()
+        }
+      }
+    }
+
+    _ = { [weak self] in
+      guard case let self = self else { return }
+      _ = { [self] in
+        foo()
+      }
+    }
+  }
+}
+
+actor TestActor {
+    func setUp() {
+        doVoidStuff { [weak self] in
+            Task { [weak self] in
+                guard let self else { return }
+                await test()
+            }
+        }
+    }
+
+    @MainActor
+    func test() { }
+}
+
+class C {
+  func foo() {
+    _ = { [self] in // expected-note {{variable other than 'self' captured here under the name 'self' does not enable implicit 'self'}} expected-warning {{capture 'self' was never used}}
+      guard case let self = C() else { return }
+      _ = { [self] in
+        foo() // expected-error {{call to method 'foo' in closure requires explicit use of 'self' to make capture semantics explicit}}
+      }
+    }
+  }
+}