Add more test cases, simplify logic

Author: calda

lib/Sema/MiscDiagnostics.cpp

@@ -1841,15 +1841,15 @@ class ImplicitSelfUsageChecker : public BaseDiagnosticWalker {
 
     return selfDeclAllowsImplicitSelf(DRE->getDecl(), ty, inClosure,
                                       /*validateParentClosures:*/ true,
-                                      /*validateSelfRebindings:*/ true);
+                                      /*validatingNestedClosure:*/ 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 validatingNestedClosure) {
     ASTContext &ctx = inClosure->getASTContext();
 
     auto requiresSelfQualification =
@@ -1883,38 +1883,35 @@ 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 `guard let self` / `if let self` conditon,
+    // we need to validate it.
+    auto selfDeclDefinedInConditionalStmt = false;
+    auto isInvalidSelfDeclRebinding = false;
+    if (auto condStmt = parentConditionalStmt(selfDecl)) {
+      selfDeclDefinedInConditionalStmt = true;
+      isInvalidSelfDeclRebinding = !hasValidSelfRebinding(condStmt, ctx);
+    }
+
+    // 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 (isInvalidSelfDeclRebinding) {
+      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)) {
-      // If a `guard let self` or `if let self` unbinding condition exists,
-      // then it must not be invalid.
-      if (auto condStmt = parentConditionalStmt(selfDecl)) {
-        if (!hasValidSelfRebinding(condStmt, ctx)) {
-          return false;
-        }
-      }
-
-      // If the self decl hasn't been unwrapped, then this closure is
-      // invalid unless non-unwrapped weak self is explicitly enabled
-      // in this situation.
-      else if (validateSelfRebindings) {
-        return false;
-      }
+    // Within a closure with a `[weak self]` capture, implicit self is only
+    // allowed if self has been unwrapped in a previous conditional statement.
+    //  - 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.
+    if (closureHasWeakSelfCapture(inClosure) &&
+        !selfDeclDefinedInConditionalStmt && !validatingNestedClosure) {
+      return false;
     }
 
     if (auto autoclosure = dyn_cast<AutoClosureExpr>(inClosure)) {
@@ -2068,7 +2065,7 @@ class ImplicitSelfUsageChecker : public BaseDiagnosticWalker {
         if (!selfDeclAllowsImplicitSelf(outerSelfDecl, captureType,
                                         outerClosure,
                                         /*validateParentClosures:*/ false,
-                                        /*validateSelfRebindings:*/ false)) {
+                                        /*validatingNestedClosure:*/ true)) {
           return outerClosure;
         }
 
@@ -2082,7 +2079,7 @@ class ImplicitSelfUsageChecker : public BaseDiagnosticWalker {
       if (validateIntermediateParents) {
         if (!selfDeclAllowsImplicitSelf(selfDecl, captureType, outerClosure,
                                         /*validateParentClosures*/ false,
-                                        /*validateSelfRebindings*/ false)) {
+                                        /*validatingNestedClosure*/ 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
+          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 {{capture 'self' was never used}}
+      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 {{capture 'self' was never used}}
+      _ = {
+        foo()
+      }
+    }
+
+    _ = { [weak self] in
+      _ = { [self] in // expected-warning {{capture 'self' was never used}}
+        foo()
+      }
+    }
+  }
+}

test/expr/closure/closures_swift6.swift

@@ -608,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
           }
         }
@@ -647,6 +647,12 @@ class TestGithubIssue69911 {
           }
         }
       }
+
+      doVoidStuff { [weak self] in
+        doVoidStuff { [self] in // expected-error {{value of optional type 'TestGithubIssue69911?' must be unwrapped to a value of type 'TestGithubIssue69911'}} expected-note{{coalesce using '??' to provide a default when the optional value contains 'nil'}} expected-note{{force-unwrap using '!' to abort execution if the optional value contains 'nil'}}
+          x += 1
+        }
+      }
     }
 }
 
@@ -923,3 +929,65 @@ 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}}
+      self.foo()
+      self?.bar()
+    }
+
+    _ = {
+      foo()
+      self.foo()
+      self?.bar()
+    }
+
+    _ = { [weak self] in
+      _ = {
+        foo()
+        self.foo()
+        self?.bar()
+      }
+    }
+
+    _ = { [weak self] in
+      _ = { [self] in
+        foo()
+        self.foo()
+        self?.bar()
+      }
+    }
+  }
+}
+
+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}}
+      }
+    }
+  }
+}