Change lookup behavior in ASTScope::lookupUnqualified when self has been unwrapped

Author: calda

lib/AST/UnqualifiedLookup.cpp

@@ -388,6 +388,43 @@ UnqualifiedLookupFactory::ResultFinderForTypeContext::getBaseDeclForResult(
   return nominalDecl;
 }
 
+/// Whether or not the given self decl is defined in an optional
+/// unwrapping condition (e.g. `guard let self else { return }`).
+/// If this is true, then we know any implicit self reference in the
+/// following scope is guaranteed to be non-optional.
+bool implicitSelfReferenceIsUnwrapped(const ValueDecl *selfDecl,
+                                      const AbstractClosureExpr *inClosure) {
+  ASTContext &Ctx = selfDecl->getASTContext();
+
+  // Check if the implicit self decl refers to a var in a conditional stmt
+  LabeledConditionalStmt *conditionalStmt = nullptr;
+  if (auto var = dyn_cast<VarDecl>(selfDecl)) {
+    if (auto parentStmt = var->getParentPatternStmt()) {
+      conditionalStmt = dyn_cast<LabeledConditionalStmt>(parentStmt);
+    }
+  }
+
+  if (!conditionalStmt) {
+    return false;
+  }
+
+  // Find the condition that defined the self decl,
+  // and check that both its LHS and RHS are 'self'
+  for (auto cond : conditionalStmt->getCond()) {
+    if (auto pattern = cond.getPattern()) {
+      if (pattern->getBoundName() != Ctx.Id_self) {
+        continue;
+      }
+    }
+
+    if (auto selfDRE = dyn_cast<DeclRefExpr>(cond.getInitializer())) {
+      return (selfDRE->getDecl()->getName().isSimpleName(Ctx.Id_self));
+    }
+  }
+
+  return false;
+}
+
 ValueDecl *UnqualifiedLookupFactory::ResultFinderForTypeContext::lookupBaseDecl(
     const DeclContext *baseDC) const {
   // Perform an unqualified lookup for the base decl of this result. This
@@ -399,29 +436,59 @@ ValueDecl *UnqualifiedLookupFactory::ResultFinderForTypeContext::lookupBaseDecl(
   // self _always_ refers to the context's self `ParamDecl`, even if there
   // is another local decl with the name `self` that would be found by
   // `lookupSingleLocalDecl`.
-  bool isInWeakSelfClosure = false;
-  if (auto closureExpr = dyn_cast<ClosureExpr>(factory->DC)) {
-    if (auto decl = closureExpr->getCapturedSelfDecl()) {
-      if (auto a = decl->getAttrs().getAttribute<ReferenceOwnershipAttr>()) {
-        isInWeakSelfClosure = a->get() == ReferenceOwnership::Weak;
-      }
+  auto closureExpr = dyn_cast<ClosureExpr>(factory->DC);
+  if (!closureExpr) {
+    return nullptr;
+  }
+
+  bool capturesSelfWeakly = false;
+  if (auto decl = closureExpr->getCapturedSelfDecl()) {
+    if (auto a = decl->getAttrs().getAttribute<ReferenceOwnershipAttr>()) {
+      capturesSelfWeakly = a->get() == ReferenceOwnership::Weak;
     }
   }
 
-  // We can only change the behavior of lookup in Swift 6 and later,
-  // due to a bug in Swift 5 where implicit self is always allowed
-  // for weak self captures in non-escaping closures.
-  if (!factory->Ctx.LangOpts.isSwiftVersionAtLeast(6)) {
+  if (!capturesSelfWeakly) {
     return nullptr;
   }
 
-  if (isInWeakSelfClosure) {
-    return ASTScope::lookupSingleLocalDecl(factory->DC->getParentSourceFile(),
-                                           DeclName(factory->Ctx.Id_self),
-                                           factory->Loc);
+  auto selfDecl = ASTScope::lookupSingleLocalDecl(
+      factory->DC->getParentSourceFile(), DeclName(factory->Ctx.Id_self),
+      factory->Loc);
+
+  if (!selfDecl) {
+    return nullptr;
+  }
+
+  // In Swift 5 mode, implicit self is allowed within non-escaping
+  // closures even before self is unwrapped. For example, this is allowed:
+  //
+  //   doVoidStuffNonEscaping { [weak self] in
+  //     method() // implicitly `self.method()`
+  //   }
+  //
+  // To support this, we have to preserve the lookup behavior from
+  // Swift 5.7 and earlier where implicit self defaults to the closure's
+  // `ParamDecl`. This causes the closure to capture self strongly, however,
+  // which is not acceptable for escaping closures.
+  //
+  // Escaping closures, however, only need to permit implicit self once
+  // it has been unwrapped to be non-optional:
+  //
+  //   doVoidStuffEscaping { [weak self] in
+  //     guard let self else { return }
+  //     method()
+  //   }
+  //
+  // In these cases, using the Swift 6 lookup behavior doesn't affect
+  // how the body is type-checked, so it can be used in Swift 5 mode
+  // without breaking source compatibility for non-escaping closures.
+  if (!factory->Ctx.LangOpts.isSwiftVersionAtLeast(6) &&
+      !implicitSelfReferenceIsUnwrapped(selfDecl, closureExpr)) {
+    return nullptr;
   }
 
-  return nullptr;
+  return selfDecl;
 }
 
 // TODO (someday): Instead of adding unavailable entries to Results,

lib/Sema/MiscDiagnostics.cpp

@@ -1538,20 +1538,6 @@ static bool closureHasWeakSelfCapture(const AbstractClosureExpr *ACE) {
   return false;
 }
 
-/// Whether or not implicit self decls in this closure require manual
-/// lookup in order to perform diagnostics with the semantics described
-/// in SE-0365. This is necessary for closures that capture self weakly
-/// in Swift 5 language modes, since implicit self decls in weak self
-/// closures have different semantics in Swift 5 than in Swift 6.
-static bool
-closureRequiresManualLookupForImplicitSelf(const AbstractClosureExpr *ACE) {
-  if (ACE->getASTContext().LangOpts.isSwiftVersionAtLeast(6)) {
-    return false;
-  }
-
-  return closureHasWeakSelfCapture(ACE);
-}
-
 // Returns true if this is an implicit self expr
 static bool isImplicitSelf(const Expr *E) {
   auto *DRE = dyn_cast<DeclRefExpr>(E);
@@ -1563,27 +1549,6 @@ static bool isImplicitSelf(const Expr *E) {
   return DRE->getDecl()->getName().isSimpleName(Ctx.Id_self);
 }
 
-// When inside a closure that requires manual lookup for implicit self,
-// (any closure with a weak self capture, in Swift 5 mode),
-// we have to manually lookup what self refers to at the location
-// according to the semantics of Swift 6 in order to avoid confusing
-// 'variable self is unused' warnings in Swift 5 mode.
-static ValueDecl *lookupSelfDeclIfNecessary(const DeclRefExpr *DRE,
-                                            const AbstractClosureExpr *ACE) {
-  ASTContext &Ctx = DRE->getDecl()->getASTContext();
-
-  if (closureRequiresManualLookupForImplicitSelf(ACE) && isImplicitSelf(DRE)) {
-    auto lookupResult = ASTScope::lookupSingleLocalDecl(
-        ACE->getParentSourceFile(), DeclName(Ctx.Id_self), DRE->getLoc());
-
-    if (lookupResult) {
-      return lookupResult;
-    }
-  }
-
-  return DRE->getDecl();
-}
-
 /// Look for any property references in closures that lack a 'self.' qualifier.
 /// Within a closure, we require that the source code contain 'self.' explicitly
 /// (or that the closure explicitly capture 'self' in the capture list) because
@@ -1683,29 +1648,11 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E,
     static bool
     implicitWeakSelfReferenceIsValid(const DeclRefExpr *DRE,
                                      const AbstractClosureExpr *inClosure) {
-      // Implicit self DREs have different semantics in Swift 5 language modes
-      // than they have in Swift 6 mode. Take this example:
-      //
-      //   doVoidStuff { [weak self] in
-      //     guard let self else { return }
-      //     method()
-      //   }
-      //
-      // In Swift 6, the implicit self DRE refers to the self decl
-      // defined in the guard statement. In Swift 5 mode, the implicit
-      // self DRE instead refers to closure's self `ParamDecl`.
-      //
-      // In order to perform these diagnostics according to the semantics
-      // defined in SE-0365, we need to know the decl that the DRE refers
-      // to in Swift 6. In Swift 5 mode, that requires performing a manual
-      // lookup of what implicit self refers to at this location (according
-      // to the semantics of Swift 6).
-      auto selfDecl = lookupSelfDeclIfNecessary(DRE, inClosure);
-      ASTContext &Ctx = selfDecl->getASTContext();
+      ASTContext &Ctx = DRE->getDecl()->getASTContext();
 
       // Check if the implicit self decl refers to a var in a conditional stmt
       LabeledConditionalStmt *conditionalStmt = nullptr;
-      if (auto var = dyn_cast<VarDecl>(selfDecl)) {
+      if (auto var = dyn_cast<VarDecl>(DRE->getDecl())) {
         if (auto parentStmt = var->getParentPatternStmt()) {
           conditionalStmt = dyn_cast<LabeledConditionalStmt>(parentStmt);
         }
@@ -2656,10 +2603,6 @@ class VarDeclUsageChecker : public ASTWalker {
   /// occur in, when they are in a pattern in a StmtCondition.
   llvm::SmallDenseMap<VarDecl*, LabeledConditionalStmt*> StmtConditionForVD;
 
-  /// The stack of closure exprs for the current position in the AST.
-  /// The top item on the stack, if present, is the current closure scope.
-  SmallVector<AbstractClosureExpr *, 4> ClosureStack;
-
 #ifndef NDEBUG
   llvm::SmallPtrSet<Expr*, 32> AllExprsSeen;
 #endif
@@ -2726,44 +2669,6 @@ class VarDeclUsageChecker : public ASTWalker {
     VarDecls[vd] |= Flag;
   }
 
-  /// The `ValueDecl` to use when performing usage analysis of the given DRE.
-  /// When inside a closure that requires manual lookup for implcit self decls
-  /// (a closure that captures self weakly in Swift 5 language modes), we
-  /// look up the `ValueDecl` that implicit self refers to according to
-  /// Swift 6 semantics.
-  ValueDecl *getDeclForUsageAnalysis(DeclRefExpr *DRE) {
-    if (ClosureStack.size() == 0) {
-      return DRE->getDecl();
-    }
-
-    auto closure = ClosureStack[ClosureStack.size() - 1];
-    assert(closure);
-
-    if (isImplicitSelf(DRE)) {
-      // Implicit self DREs have different semantics in Swift 5 language modes
-      // than they have in Swift 6 mode. Take this example:
-      //
-      //   doVoidStuff { [weak self] in
-      //     guard let self else { return }
-      //     method()
-      //   }
-      //
-      // In Swift 6, the implicit self DRE refers to the self decl
-      // defined in the guard statement. In Swift 5 mode, the implicit
-      // self DRE instead refers to closure's self `ParamDecl`.
-      //
-      // If we perform usage analysis using Swift 5 semantics, we'd
-      // get a confusing "value 'self' was defined but never used;
-      // consider replacing with boolean test" warning. To suppress this
-      // warning, we instead perform usage analysis using Swift 6 semantics
-      // (by performing a manual lookup of what implicit self refers to
-      // at this location according to Swift 6 semantics).
-      return lookupSelfDeclIfNecessary(DRE, closure);
-    } else {
-      return DRE->getDecl();
-    }
-  }
-
   void markBaseOfStorageUse(Expr *E, ConcreteDeclRef decl, unsigned flags);
   void markBaseOfStorageUse(Expr *E, bool isMutating);
 
@@ -2862,7 +2767,6 @@ class VarDeclUsageChecker : public ASTWalker {
 
   /// The heavy lifting happens when visiting expressions.
   PreWalkResult<Expr *> walkToExprPre(Expr *E) override;
-  PostWalkResult<Expr *> walkToExprPost(Expr *E) override;
 
   /// handle #if directives.
   void handleIfConfig(IfConfigDecl *ICD);
@@ -3675,7 +3579,7 @@ void VarDeclUsageChecker::markStoredOrInOutExpr(Expr *E, unsigned Flags) {
 
   // If we found a decl that is being assigned to, then mark it.
   if (auto *DRE = dyn_cast<DeclRefExpr>(E)) {
-    addMark(getDeclForUsageAnalysis(DRE), Flags);
+    addMark(DRE->getDecl(), Flags);
     return;
   }
 
@@ -3760,7 +3664,7 @@ ASTWalker::PreWalkResult<Expr *> VarDeclUsageChecker::walkToExprPre(Expr *E) {
   // If this is a DeclRefExpr found in a random place, it is a load of the
   // vardecl.
   if (auto *DRE = dyn_cast<DeclRefExpr>(E)) {
-    addMark(getDeclForUsageAnalysis(DRE), RK_Read);
+    addMark(DRE->getDecl(), RK_Read);
 
     // If the Expression is a read of a getter, track for diagnostics
     if (auto VD = dyn_cast<VarDecl>(DRE->getDecl())) {
@@ -3818,20 +3722,6 @@ ASTWalker::PreWalkResult<Expr *> VarDeclUsageChecker::walkToExprPre(Expr *E) {
   if (isa<ErrorExpr>(E))
     sawError = true;
 
-  // Track the current closure in `ClosureStack`
-  if (auto closure = dyn_cast<AbstractClosureExpr>(E)) {
-    ClosureStack.push_back(closure);
-  }
-
-  return Action::Continue(E);
-}
-
-ASTWalker::PostWalkResult<Expr *> VarDeclUsageChecker::walkToExprPost(Expr *E) {
-  if (auto closure = dyn_cast<AbstractClosureExpr>(E)) {
-    assert(ClosureStack.size() > 0);
-    ClosureStack.pop_back();
-  }
-
   return Action::Continue(E);
 }
 
@@ -3851,7 +3741,7 @@ void VarDeclUsageChecker::handleIfConfig(IfConfigDecl *ICD) {
       // conservatively mark it read and written.  This will silence "variable
       // unused" and "could be marked let" warnings for it.
       if (auto *DRE = dyn_cast<DeclRefExpr>(E))
-        VDUC.addMark(VDUC.getDeclForUsageAnalysis(DRE), RK_Read | RK_Written);
+        VDUC.addMark(DRE->getDecl(), RK_Read | RK_Written);
       else if (auto *declRef = dyn_cast<UnresolvedDeclRefExpr>(E)) {
         auto name = declRef->getName();
         auto loc = declRef->getLoc();

test/expr/closure/closures.swift

@@ -261,12 +261,12 @@ class ExplicitSelfRequiredTest {
   // because its `sawError` flag is set to true. To preserve the "capture 'y' was never used" warnings
   // above, we put these cases in their own method.
   func weakSelfError() {
-    doVoidStuff({ [weak self] in x += 1 }) // expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}}
-    doVoidStuffNonEscaping({ [weak self] in x += 1 }) // expected-warning {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}}
-    doStuff({ [weak 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 _ = method() }) // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}}
-    doVoidStuffNonEscaping({ [weak self] in _ = method() }) // expected-warning {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}}
-    doStuff({ [weak self] in method() }) // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}}
+    doVoidStuff({ [weak self] in x += 1 }) // expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{variable 'self' was written to, but never read}}
+    doVoidStuffNonEscaping({ [weak self] in x += 1 }) // expected-warning {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{variable 'self' was written to, but never read}}
+    doStuff({ [weak self] in x+1 }) // expected-error {{reference to property 'x' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{variable 'self' was written to, but never read}}
+    doVoidStuff({ [weak self] in _ = method() }) // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{variable 'self' was written to, but never read}}
+    doVoidStuffNonEscaping({ [weak self] in _ = method() }) // expected-warning {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{variable 'self' was written to, but never read}}
+    doStuff({ [weak self] in method() }) // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}} expected-warning {{variable 'self' was written to, but never read}}
   }
 }
 
@@ -775,7 +775,7 @@ public class TestImplicitSelfForWeakSelfCapture {
     }
 
     doVoidStuff { [weak self] in
-      guard let self = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional else { return }
+      guard let self = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional else { return } // expected-warning {{value 'self' was defined but never used; consider replacing with boolean test}}
       method() // expected-error {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}}
     }
 
@@ -811,7 +811,7 @@ public class TestImplicitSelfForWeakSelfCapture {
     }
 
     doVoidStuffNonEscaping { [weak self] in
-      guard let self = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional else { return }
+      guard let self = self ?? TestImplicitSelfForWeakSelfCapture.staticOptional else { return } // value 'self' was defined but never used; consider replacing with boolean test
       method() // expected-warning {{call to method 'method' in closure requires explicit use of 'self' to make capture semantics explicit}}
     }
   }