Move remaining logic in LookupResultEntry::getBaseDecl() to ASTScope::lookupUnqualified impl, add more extensive tests, fix failing tests

calda · calda · commit 9dd56f9daff2 · 2022-09-22T11:04:08.000-07:00
diff --git a/include/swift/AST/NameLookup.h b/include/swift/AST/NameLookup.h
@@ -103,7 +103,8 @@ struct LookupResultEntry {
   /// extension (if it found something at that level).
   DeclContext *BaseDC;
 
-  /// The declaration that defines the base of the call to `Value`
+  /// The declaration that defines the base of the call to `Value`.
+  /// This is always available, as long as `BaseDC` is not null.
   ValueDecl *BaseDecl;
   
   /// The declaration corresponds to the given name; i.e. the decl we are
diff --git a/lib/AST/NameLookup.cpp b/lib/AST/NameLookup.cpp
@@ -56,28 +56,7 @@ ValueDecl *LookupResultEntry::getBaseDecl() const {
   if (BaseDC == nullptr)
     return nullptr;
   
-  if (BaseDecl)
-    return BaseDecl;
-
-  if (auto *AFD = dyn_cast<AbstractFunctionDecl>(BaseDC))
-    return AFD->getImplicitSelfDecl();
-
-  if (auto *PBI = dyn_cast<PatternBindingInitializer>(BaseDC)) {
-    auto *selfDecl = PBI->getImplicitSelfDecl();
-    assert(selfDecl);
-    return selfDecl;
-  }
-
-  if (auto *CE = dyn_cast<ClosureExpr>(BaseDC)) {
-    auto *selfDecl = CE->getCapturedSelfDecl();
-    assert(selfDecl);
-    assert(selfDecl->isSelfParamCapture());
-    return selfDecl;
-  }
-
-  auto *nominalDecl = BaseDC->getSelfNominalTypeDecl();
-  assert(nominalDecl);
-  return nominalDecl;
+  return BaseDecl;
 }
 
 void LookupResult::filter(
diff --git a/lib/AST/UnqualifiedLookup.cpp b/lib/AST/UnqualifiedLookup.cpp
@@ -24,6 +24,7 @@
 #include "swift/AST/NameLookupRequests.h"
 #include "swift/AST/PropertyWrappers.h"
 #include "swift/AST/SourceFile.h"
+#include "swift/AST/Initializer.h"
 #include "swift/Basic/Debug.h"
 #include "swift/Basic/STLExtras.h"
 #include "swift/Basic/SourceManager.h"
@@ -345,20 +346,59 @@ void UnqualifiedLookupFactory::ResultFinderForTypeContext::findResults(
   for (auto Result : Lookup) {
     auto baseDC = const_cast<DeclContext *>(whereValueIsMember(Result));
     
+    // Retrieve the base decl for this lookup
+    ValueDecl* baseDecl;
+    
     // Perform an unqualified lookup for the base decl of this result. This handles cases
     // where self was rebound (e.g. `guard let self = self`) earlier in the scope.
-    // Only do this in closures, since implicit self isn't allowed to be rebound
-    // in other contexts. Otherwise, we use the `ParamDecl` from `baseDC` if applicable.
+    //  - Only do this in closures that capture self weakly, since implicit self isn't
+    //    allowed to be rebound in other contexts. In other contexts, implicit 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`.
     ValueDecl* localBaseDecl = nullptr;
-    if (factory->DC->getContextKind() == DeclContextKind::AbstractClosureExpr && baseDC) {
-      auto *localDecl = ASTScope::lookupSingleLocalDecl(factory->DC->getParentSourceFile(),
-                                                       DeclName(factory->Ctx.Id_self),
-                                                       factory->Loc);
-      if (localDecl)
-        localBaseDecl = localDecl;
+    bool isInWeakSelfClosure = false;
+    if (auto closureExpr = dyn_cast<ClosureExpr>(factory->DC)) {
+      if (auto selfDecl = closureExpr->getCapturedSelfDecl()) {
+        auto *attr = selfDecl->getAttrs().getAttribute<ReferenceOwnershipAttr>();
+        isInWeakSelfClosure = attr->get() == ReferenceOwnership::Weak;
+      }
+    }
+            
+    if (isInWeakSelfClosure) {
+      localBaseDecl = ASTScope::lookupSingleLocalDecl(factory->DC->getParentSourceFile(),
+                                                      DeclName(factory->Ctx.Id_self),
+                                                      factory->Loc);
+    }
+    
+    if (baseDC == nullptr)
+      baseDecl = nullptr;
+    
+    else if (localBaseDecl)
+      baseDecl = localBaseDecl;
+
+    else if (auto *AFD = dyn_cast<AbstractFunctionDecl>(baseDC))
+      baseDecl = AFD->getImplicitSelfDecl();
+
+    else if (auto *PBI = dyn_cast<PatternBindingInitializer>(baseDC)) {
+      auto *selfDecl = PBI->getImplicitSelfDecl();
+      assert(selfDecl);
+      baseDecl = selfDecl;
+    }
+
+    else if (auto *CE = dyn_cast<ClosureExpr>(baseDC)) {
+      auto *selfDecl = CE->getCapturedSelfDecl();
+      assert(selfDecl);
+      assert(selfDecl->isSelfParamCapture());
+      baseDecl = selfDecl;
+    }
+
+    else {
+      auto *nominalDecl = baseDC->getSelfNominalTypeDecl();
+      assert(nominalDecl);
+      baseDecl = nominalDecl;
     }
     
-    results.emplace_back(baseDC, localBaseDecl, Result);
+    results.emplace_back(baseDC, baseDecl, Result);
 #ifndef NDEBUG
     factory->addedResult(results.back());
 #endif
diff --git a/lib/Sema/MiscDiagnostics.cpp b/lib/Sema/MiscDiagnostics.cpp
@@ -1565,30 +1565,6 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E,
 
       if (!DRE || !DRE->isImplicit())
         return false;
-
-      // Defensive check for type. If the expression doesn't have type here, it
-      // should have been diagnosed somewhere else.
-      Type ty = DRE->getType();
-      assert(ty && "Implicit self parameter ref without type");
-      if (!ty)
-        return false;
-
-      // Metatype self captures don't extend the lifetime of an object.
-      if (ty->is<MetatypeType>())
-        return false;
-
-      // If self does not have reference semantics, it is very unlikely that
-      // capturing it will create a reference cycle.
-      if (!ty->hasReferenceSemantics())
-        return false;
-      
-      // If this is an implicit self parameter to a `AbstractFunctionDecl`
-      // (`FuncDecl`, `ConstructorDecl`, or `DestructorDecl`,
-      //  `@autoclosure @escaping () -> String = String()` as one example)
-      // then it isn't actually capturing the closure's 'self', and is fine.
-      if (isa<AbstractFunctionDecl>(DRE->getDecl())) {
-        return false;
-      }
       
       // If this decl isn't named "self", then it isn't an implicit self capture
       // and we have no reason to reject it.
@@ -1600,73 +1576,82 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E,
       auto isExplicitWeakSelfCapture = false;
       if (auto closureExpr = dyn_cast<ClosureExpr>(inClosure)) {
         if (auto selfDecl = closureExpr->getCapturedSelfDecl()) {
-          // If `weak self` was captured explicitly, then implicit self
-          // is always allowed allowed once `self` is unwrapped.
-          //  - If `self` is still an Optional, compilation would have failed already,
-          //    so we don't need to check for that here.
           if (selfDecl->getType()->is<WeakStorageType>()) {
             isExplicitWeakSelfCapture = true;
           }
-          
-          // If this capture is using the name `self` actually referring
-          // to some other variable (e.g. with `[self = "hello"]`)
-          // then implicit self is not allowed.
-          else if (!selfDecl->isSelfParamCapture()) {
-            return true;
-          }
         }
       }
       
-      if (auto var = dyn_cast<VarDecl>(DRE->getDecl())) {
-        if (auto parentStmt = var->getParentPatternStmt()) {
-          if (isa<LabeledConditionalStmt>(parentStmt)) {
-            // If this `self` decl was not captured explicitly by this closure,
-            // but is actually from an outer `weak` capture's `if let self = self`
-            // or `guard let self = self` etc, then we don't allow implicit self.
-            if (!isExplicitWeakSelfCapture) {
-              return true;
-            }
-            
-            // If this is an explicit `weak self` capture, then we need
-            // to validate that the unwrapped `self` decl is actually
-            // referring to `self`, and not something else like in
-            // `guard let self = .someOptionalVariable else { return }`
-            auto hasCorrectSelfBindingCondition = false;
-            for (auto cond : dyn_cast<LabeledConditionalStmt>(parentStmt)->getCond()) {
-              if (cond.getKind() == StmtConditionElement::CK_PatternBinding) {
-                if (auto optionalPattern = dyn_cast<OptionalSomePattern>(cond.getPattern())) {
+      // If this is an explicit `weak self` capture, then implicit self is allowed
+      // once the closure's self param is unwrapped. We need to validate that the
+      // unwrapped `self` decl specifically refers to an unwrapped copy of the
+      // closure's `self` param, and not something else like in
+      // `guard let self = .someOptionalVariable else { return }` or
+      // `let self = someUnrelatedVariable`. If self hasn't been unwrapped yet
+      // and is still an optional, we would have already hit an error elsewhere.
+      if (isExplicitWeakSelfCapture) {
+        bool hasCorrectSelfBindingCondition = false;
+        if (auto var = dyn_cast<VarDecl>(DRE->getDecl())) {
+          // if the `self` decls was defined in a `let`, `guard`, or `while` condition...
+          if (auto parentStmt = var->getParentPatternStmt())
+            if (auto parentConditionalStmt = dyn_cast<LabeledConditionalStmt>(parentStmt))
+              for (auto cond : parentConditionalStmt->getCond())
+                if (auto optionalPattern = dyn_cast_or_null<OptionalSomePattern>(cond.getPattern()))
                   // if the lhs of the optional binding is `self`...
-                  if (optionalPattern->getSubPattern()->getBoundName() == Ctx.Id_self) {
-                    if (auto loadExpr = dyn_cast<LoadExpr>(cond.getInitializer())) {
-                      if (auto declRefExpr = dyn_cast<DeclRefExpr>(loadExpr->getSubExpr())) {
+                  if (optionalPattern->getSubPattern()->getBoundName() == Ctx.Id_self)
+                    if (auto loadExpr = dyn_cast<LoadExpr>(cond.getInitializer()))
+                      if (auto declRefExpr = dyn_cast<DeclRefExpr>(loadExpr->getSubExpr()))
                         // and the rhs of the optional binding is `self`...
-                        if (declRefExpr->getDecl()->getName().isSimpleName(Ctx.Id_self)) {
-                          // then we can permit implicit self in this closure
+                        if (declRefExpr->getDecl()->getName().isSimpleName(Ctx.Id_self))
+                          // then we can permit implicit self in this case
                           hasCorrectSelfBindingCondition = true;
-                        }
-                      }
-                    }
-                  }
-                }
-              }
-            }
-            
-            return !hasCorrectSelfBindingCondition;
-          }
         }
         
-        if (!isEnclosingSelfReference(var, inClosure)) {
-          return false;
-        }
+        return !hasCorrectSelfBindingCondition;
       }
       
+      // Defensive check for type. If the expression doesn't have type here, it
+      // should have been diagnosed somewhere else.
+      Type ty = DRE->getType();
+      assert(ty && "Implicit self parameter ref without type");
+      if (!ty)
+        return false;
+
+      // Metatype self captures don't extend the lifetime of an object.
+      if (ty->is<MetatypeType>())
+        return false;
+
+      // If self does not have reference semantics, it is very unlikely that
+      // capturing it will create a reference cycle.
+      if (!ty->hasReferenceSemantics())
+        return false;
+      
+      if (auto closureExpr = dyn_cast<ClosureExpr>(inClosure))
+        if (auto selfDecl = closureExpr->getCapturedSelfDecl())
+          // If this capture is using the name `self` actually referring
+          // to some other variable (e.g. with `[self = "hello"]`)
+          // then implicit self is not allowed.
+          if (!selfDecl->isSelfParamCapture())
+            return true;
+      
+      if (auto var = dyn_cast<VarDecl>(DRE->getDecl()))
+        if (!isEnclosingSelfReference(var, inClosure))
+          return false;
+      
       return true;
     }
 
     /// Return true if this is a closure expression that will require explicit
     /// use or capture of "self." for qualification of member references.
     static bool isClosureRequiringSelfQualification(
                   const AbstractClosureExpr *CE) {
+      // If this closure capture self weakly, then we have to validate each usage
+      // of implicit self individually, even in a nonescaping closure
+      if (auto closureExpr = dyn_cast<ClosureExpr>(CE))
+        if (auto selfDecl = closureExpr->getCapturedSelfDecl())
+          if (selfDecl->getType()->is<WeakStorageType>())
+            return true;
+            
       // If the closure's type was inferred to be noescape, then it doesn't
       // need qualification.
       if (AnyFunctionRef(const_cast<AbstractClosureExpr *>(CE))
@@ -1713,23 +1698,19 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E,
       // Until Swift 6, only emit a warning when we get this with an
       // explicit capture, since we used to not diagnose this at all.
       auto shouldOnlyWarn = [&](Expr *selfRef) {
-        if (auto declRef = dyn_cast<DeclRefExpr>(selfRef)) {
-          if (auto decl = declRef->getDecl()) {
-            if (auto varDecl = dyn_cast<VarDecl>(decl)) {
-              // If the self decl was defined in an conditional binding in an `if`/`guard`/`while`,
-              // then we know this is an inner closure of some outer closure's `weak self` capture.
-              // Since this wasn't allowed in Swift 5.5, we should just always emit an error.
-              if (auto parentStmt = varDecl->getParentPatternStmt()) {
-                if (isa<LabeledConditionalStmt>(parentStmt)) {
-                  return false;
-                }
-              }
-              
+        // If this implicit self decl is from a closure that captured self weakly,
+        // then we should always emit an error, since implicit self was only
+        // allowed starting in Swift 5.8 and later.
+        if (auto closureExpr = dyn_cast<ClosureExpr>(ACE))
+          if (auto selfDecl = closureExpr->getCapturedSelfDecl())
+            if (selfDecl->getType()->is<WeakStorageType>())
+              return false;
+        
+        if (auto declRef = dyn_cast<DeclRefExpr>(selfRef))
+          if (auto decl = declRef->getDecl())
+            if (auto varDecl = dyn_cast<VarDecl>(decl))
               return !varDecl->isSelfParameter();
-            }
-          }
-        }
-          
+        
         return false;
       };
       
diff --git a/test/expr/closure/closures.swift b/test/expr/closure/closures.swift
