Merge pull request #65310 from calda/cal--SE-3065-bad-pattern-check

[SE-0365] Fix issue where boolean condition before `let self` condition would prevent using implicit self

Author: xedin

include/swift/AST/Stmt.h

@@ -556,11 +556,17 @@ class alignas(1 << PatternAlignInBits) StmtConditionElement {
     assert(getKind() == CK_PatternBinding && "Not a pattern binding condition");
     ThePattern = P;
   }
+  
+  /// Pattern Binding Accessors.
+  Expr *getInitializerOrNull() const {
+    return getKind() == CK_PatternBinding ? Condition.get<Expr *>() : nullptr;
+  }
 
   Expr *getInitializer() const {
     assert(getKind() == CK_PatternBinding && "Not a pattern binding condition");
     return Condition.get<Expr *>();
   }
+  
   void setInitializer(Expr *E) {
     assert(getKind() == CK_PatternBinding && "Not a pattern binding condition");
     Condition = E;
@@ -588,6 +594,11 @@ class alignas(1 << PatternAlignInBits) StmtConditionElement {
     Condition = Info;
   }
 
+  /// Whether or not this conditional stmt rebinds self with a `let self`
+  /// or `let self = self` condition. If `requireLoadExpr` is `true`,
+  /// additionally requires that the RHS of the self condition is a `LoadExpr`.
+  bool rebindsSelf(ASTContext &Ctx, bool requireLoadExpr = false) const;
+
   SourceLoc getStartLoc() const;
   SourceLoc getEndLoc() const;
   SourceRange getSourceRange() const;
@@ -691,6 +702,11 @@ class LabeledConditionalStmt : public LabeledStmt {
   ///        stored in \c ASTNode.
   StmtCondition *getCondPointer() { return &Cond; }
 
+  /// Whether or not this conditional stmt rebinds self with a `let self`
+  /// or `let self = self` condition. If `requireLoadExpr` is `true`,
+  /// additionally requires that the RHS of the self condition is a `LoadExpr`.
+  bool rebindsSelf(ASTContext &Ctx, bool requireLoadExpr = false) const;
+
   static bool classof(const Stmt *S) {
     return S->getKind() >= StmtKind::First_LabeledConditionalStmt &&
            S->getKind() <= StmtKind::Last_LabeledConditionalStmt;

lib/AST/Stmt.cpp

@@ -466,6 +466,66 @@ void LabeledConditionalStmt::setCond(StmtCondition e) {
   Cond = e;
 }
 
+/// Whether or not this conditional stmt rebinds self with a `let self`
+/// or `let self = self` condition. If `requireLoadExpr` is `true`,
+/// additionally requires that the RHS of the self condition is a `LoadExpr`.
+bool LabeledConditionalStmt::rebindsSelf(ASTContext &Ctx,
+                                         bool requireLoadExpr) const {
+  return llvm::any_of(getCond(), [&Ctx, requireLoadExpr](const auto &cond) {
+    return cond.rebindsSelf(Ctx, requireLoadExpr);
+  });
+}
+
+/// Whether or not this conditional stmt rebinds self with a `let self`
+/// or `let self = self` condition. If `requireLoadExpr` is `true`,
+/// additionally requires that the RHS of the self condition is a `LoadExpr`.
+bool StmtConditionElement::rebindsSelf(ASTContext &Ctx,
+                                       bool requireLoadExpr) const {
+  auto pattern = getPatternOrNull();
+  if (!pattern) {
+    return false;
+  }
+
+  // Check whether or not this pattern defines a new `self` decl
+  bool isSelfRebinding = false;
+  if (pattern->getBoundName() == Ctx.Id_self) {
+    isSelfRebinding = true;
+  }
+
+  else if (auto OSP = dyn_cast<OptionalSomePattern>(pattern)) {
+    if (auto subPattern = OSP->getSubPattern()) {
+      isSelfRebinding = subPattern->getBoundName() == Ctx.Id_self;
+    }
+  }
+
+  if (!isSelfRebinding) {
+    return false;
+  }
+
+  // Check that the RHS expr is exactly `self` and not something else
+  Expr *exprToCheckForDRE = getInitializerOrNull();
+  if (!exprToCheckForDRE) {
+    return false;
+  }
+
+  if (requireLoadExpr && !isa<LoadExpr>(exprToCheckForDRE)) {
+    return false;
+  }
+
+  if (auto *load = dyn_cast<LoadExpr>(exprToCheckForDRE)) {
+    exprToCheckForDRE = load->getSubExpr();
+  }
+
+  if (auto *DRE = dyn_cast<DeclRefExpr>(
+          exprToCheckForDRE->getSemanticsProvidingExpr())) {
+    auto *decl = DRE->getDecl();
+    return decl && decl->hasName() ? decl->getName().isSimpleName(Ctx.Id_self)
+                                   : false;
+  }
+
+  return false;
+}
+
 PoundAvailableInfo *
 PoundAvailableInfo::create(ASTContext &ctx, SourceLoc PoundLoc,
                            SourceLoc LParenLoc,

lib/AST/UnqualifiedLookup.cpp

@@ -397,45 +397,7 @@ bool implicitSelfReferenceIsUnwrapped(const ValueDecl *selfDecl,
     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()) {
-      bool isSelfRebinding = false;
-
-      if (pattern->getBoundName() == Ctx.Id_self) {
-        isSelfRebinding = true;
-      }
-
-      else if (auto OSP = dyn_cast<OptionalSomePattern>(pattern)) {
-        if (auto subPattern = OSP->getSubPattern()) {
-          isSelfRebinding = subPattern->getBoundName() == Ctx.Id_self;
-        }
-      }
-
-      if (!isSelfRebinding) {
-        continue;
-      }
-    }
-
-    Expr *exprToCheckForDRE = cond.getInitializer();
-    if (auto LE = dyn_cast<LoadExpr>(exprToCheckForDRE)) {
-      if (auto subexpr = LE->getSubExpr()) {
-        exprToCheckForDRE = subexpr;
-      }
-    }
-
-    exprToCheckForDRE = exprToCheckForDRE->getSemanticsProvidingExpr();
-
-    DeclRefExpr *condDRE = dyn_cast<DeclRefExpr>(exprToCheckForDRE);
-    if (!condDRE || !condDRE->getDecl()->hasName()) {
-      return false;
-    }
-
-    return condDRE->getDecl()->getName().isSimpleName(Ctx.Id_self);
-  }
-
-  return false;
+  return conditionalStmt->rebindsSelf(Ctx);
 }
 
 // Finds the nearest parent closure, which would define the

lib/Sema/MiscDiagnostics.cpp

@@ -1689,23 +1689,14 @@ static void diagnoseImplicitSelfUseInClosure(const Expr *E,
         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 OSP = dyn_cast<OptionalSomePattern>(cond.getPattern())) {
-          if (OSP->getSubPattern()->getBoundName() != Ctx.Id_self) {
-            continue;
-          }
-
-          if (auto LE = dyn_cast<LoadExpr>(cond.getInitializer())) {
-            if (auto selfDRE = dyn_cast<DeclRefExpr>(LE->getSubExpr())) {
-              return (selfDRE->getDecl()->getName().isSimpleName(Ctx.Id_self));
-            }
-          }
-        }
-      }
-
-      return false;
+      // Require `LoadExpr`s when validating the self binding.
+      // This lets us reject invalid examples like:
+      //
+      //   let `self` = self ?? .somethingElse
+      //   guard let self = self else { return }
+      //   method() // <- implicit self is not allowed
+      //
+      return conditionalStmt->rebindsSelf(Ctx, /*requireLoadExpr*/ true);
     }
 
     /// Return true if this is a closure expression that will require explicit

test/expr/closure/closures.swift

@@ -983,6 +983,82 @@ class NoImplicitSelfInInnerClass {
     }
 
   }
+  
+  func foo(condition: Bool) {
+    doVoidStuff { [weak self] in
+      guard condition, let self else { return }
+      method()
+    }
+    
+    doVoidStuff { [weak self] in
+      guard let self, condition else { return }
+      method()
+    }
+    
+    doVoidStuffNonEscaping { [weak self] in
+      guard condition, let self else { return }
+      method()
+    }
+    
+    doVoidStuffNonEscaping { [weak self] in
+      guard let self, condition else { return }
+      method()
+    }
+  }
+
+  func foo(optionalCondition: Bool?) {
+    doVoidStuff { [weak self] in
+      guard let optionalCondition, optionalCondition, let self else { return }
+      method()
+    }
+    
+    doVoidStuff { [weak self] in
+      guard let self, let optionalCondition, optionalCondition else { return }
+      method()
+    }
+    
+    doVoidStuff { [weak self] in
+      guard let optionalCondition, let self, optionalCondition else { return }
+      method()
+    }
+    
+    doVoidStuffNonEscaping { [weak self] in
+      guard let optionalCondition, optionalCondition, let self else { return }
+      method()
+    }
+    
+    doVoidStuffNonEscaping { [weak self] in
+      guard let self, let optionalCondition, optionalCondition else { return }
+      method()
+    }
+    
+    doVoidStuffNonEscaping { [weak self] in
+      guard let optionalCondition, let self, optionalCondition else { return }
+      method()
+    }
+  }
+  
+  func foo() {
+    doVoidStuff { [weak self] in
+      guard #available(SwiftStdlib 5.8, *), let self else { return }
+      method()
+    }
+    
+    doVoidStuff { [weak self] in
+      guard let self, #available(SwiftStdlib 5.8, *) else { return }
+      method()
+    }
+    
+    doVoidStuffNonEscaping { [weak self] in
+      guard #available(SwiftStdlib 5.8, *), let self else { return }
+      method()
+    }
+    
+    doVoidStuffNonEscaping { [weak self] in
+      guard let self, #available(SwiftStdlib 5.8, *) else { return }
+      method()
+    }
+  }
 }
 
 public class TestRebindingSelfIsDisallowed {

test/expr/closure/closures_swift6.swift

@@ -210,6 +210,82 @@ class NoImplicitSelfInInnerClass {
       }
     }
   }
+  
+  func foo(condition: Bool) {
+    doVoidStuff { [weak self] in
+      guard condition, let self else { return }
+      method()
+    }
+    
+    doVoidStuff { [weak self] in
+      guard let self, condition else { return }
+      method()
+    }
+    
+    doVoidStuffNonEscaping { [weak self] in
+      guard condition, let self else { return }
+      method()
+    }
+    
+    doVoidStuffNonEscaping { [weak self] in
+      guard let self, condition else { return }
+      method()
+    }
+  }
+
+  func foo(optionalCondition: Bool?) {
+    doVoidStuff { [weak self] in
+      guard let optionalCondition, optionalCondition, let self else { return }
+      method()
+    }
+    
+    doVoidStuff { [weak self] in
+      guard let self, let optionalCondition, optionalCondition else { return }
+      method()
+    }
+    
+    doVoidStuff { [weak self] in
+      guard let optionalCondition, let self, optionalCondition else { return }
+      method()
+    }
+    
+    doVoidStuffNonEscaping { [weak self] in
+      guard let optionalCondition, optionalCondition, let self else { return }
+      method()
+    }
+    
+    doVoidStuffNonEscaping { [weak self] in
+      guard let self, let optionalCondition, optionalCondition else { return }
+      method()
+    }
+    
+    doVoidStuffNonEscaping { [weak self] in
+      guard let optionalCondition, let self, optionalCondition else { return }
+      method()
+    }
+  }
+  
+  func foo() {
+    doVoidStuff { [weak self] in
+      guard #available(SwiftStdlib 5.8, *), let self else { return }
+      method()
+    }
+    
+    doVoidStuff { [weak self] in
+      guard let self, #available(SwiftStdlib 5.8, *) else { return }
+      method()
+    }
+    
+    doVoidStuffNonEscaping { [weak self] in
+      guard #available(SwiftStdlib 5.8, *), let self else { return }
+      method()
+    }
+    
+    doVoidStuffNonEscaping { [weak self] in
+      guard let self, #available(SwiftStdlib 5.8, *) else { return }
+      method()
+    }
+  }
 }
 
 public class TestRebindingSelfIsDisallowed {