Merge pull request swiftlang#36582 from ahoppen/pr/rdar75200217

[CodeComplete] Fix cyclic dependency error in request evaluator during code completion

Author: ahoppen

lib/Sema/TypeCheckDecl.cpp

@@ -2503,13 +2503,38 @@ NamingPatternRequest::evaluate(Evaluator &evaluator, VarDecl *VD) const {
   }
 
   if (!namingPattern) {
-    // Try type checking parent control statement.
     if (auto parentStmt = VD->getParentPatternStmt()) {
-      if (auto CS = dyn_cast<CaseStmt>(parentStmt))
-        parentStmt = CS->getParentStmt();
-      ASTNode node(parentStmt);
-      TypeChecker::typeCheckASTNode(node, VD->getDeclContext(),
-                                    /*LeaveBodyUnchecked=*/true);
+      // Try type checking parent control statement.
+      if (auto condStmt = dyn_cast<LabeledConditionalStmt>(parentStmt)) {
+        // The VarDecl is defined inside a condition of a `if` or `while` stmt.
+        // Only type check the condition we care about: the one with the VarDecl
+        bool foundVarDecl = false;
+        for (auto &condElt : condStmt->getCond()) {
+          if (auto pat = condElt.getPatternOrNull()) {
+            if (!pat->containsVarDecl(VD)) {
+              continue;
+            }
+            // We found the condition that declares the variable. Type check it
+            // and stop the loop. The variable can only be declared once.
+
+            // We don't care about isFalsable
+            bool isFalsable = false;
+            TypeChecker::typeCheckStmtConditionElement(condElt, isFalsable,
+                                                       VD->getDeclContext());
+
+            foundVarDecl = true;
+            break;
+          }
+        }
+        assert(foundVarDecl && "VarDecl not declared in its parent?");
+      } else {
+        // We have some other parent stmt. Type check it completely.
+        if (auto CS = dyn_cast<CaseStmt>(parentStmt))
+          parentStmt = CS->getParentStmt();
+        ASTNode node(parentStmt);
+        TypeChecker::typeCheckASTNode(node, VD->getDeclContext(),
+                                      /*LeaveBodyUnchecked=*/true);
+      }
       namingPattern = VD->getCanonicalVarDecl()->NamingPattern;
     }
   }

lib/Sema/TypeCheckStmt.cpp

@@ -419,6 +419,89 @@ static LabeledStmt *findBreakOrContinueStmtTarget(
   return nullptr;
 }
 
+bool TypeChecker::typeCheckStmtConditionElement(StmtConditionElement &elt,
+                                                bool &isFalsable,
+                                                DeclContext *dc) {
+  auto &Context = dc->getASTContext();
+  if (elt.getKind() == StmtConditionElement::CK_Availability) {
+    isFalsable = true;
+
+    // Reject inlinable code using availability macros.
+    PoundAvailableInfo *info = elt.getAvailability();
+    if (auto *decl = dc->getAsDecl()) {
+      if (decl->getAttrs().hasAttribute<InlinableAttr>() ||
+          decl->getAttrs().hasAttribute<AlwaysEmitIntoClientAttr>())
+        for (auto queries : info->getQueries())
+          if (auto availSpec =
+                  dyn_cast<PlatformVersionConstraintAvailabilitySpec>(queries))
+            if (availSpec->getMacroLoc().isValid()) {
+              Context.Diags.diagnose(
+                  availSpec->getMacroLoc(),
+                  swift::diag::availability_macro_in_inlinable,
+                  decl->getDescriptiveKind());
+              break;
+            }
+    }
+
+    return false;
+  }
+
+  if (auto E = elt.getBooleanOrNull()) {
+    assert(!E->getType() && "the bool condition is already type checked");
+    bool hadError = TypeChecker::typeCheckCondition(E, dc);
+    elt.setBoolean(E);
+    isFalsable = true;
+    return hadError;
+  }
+  assert(elt.getKind() != StmtConditionElement::CK_Boolean);
+
+  // This is cleanup goop run on the various paths where type checking of the
+  // pattern binding fails.
+  auto typeCheckPatternFailed = [&] {
+    elt.getPattern()->setType(ErrorType::get(Context));
+    elt.getInitializer()->setType(ErrorType::get(Context));
+
+    elt.getPattern()->forEachVariable([&](VarDecl *var) {
+      // Don't change the type of a variable that we've been able to
+      // compute a type for.
+      if (var->hasInterfaceType() && !var->isInvalid())
+        return;
+      var->setInvalid();
+    });
+  };
+
+  // Resolve the pattern.
+  assert(!elt.getPattern()->hasType() &&
+         "the pattern binding condition is already type checked");
+  auto *pattern = TypeChecker::resolvePattern(elt.getPattern(), dc,
+                                              /*isStmtCondition*/ true);
+  if (!pattern) {
+    typeCheckPatternFailed();
+    return true;
+  }
+  elt.setPattern(pattern);
+
+  TypeChecker::diagnoseDuplicateBoundVars(pattern);
+
+  // Check the pattern, it allows unspecified types because the pattern can
+  // provide type information.
+  auto contextualPattern = ContextualPattern::forRawPattern(pattern, dc);
+  Type patternType = TypeChecker::typeCheckPattern(contextualPattern);
+  if (patternType->hasError()) {
+    typeCheckPatternFailed();
+    return true;
+  }
+
+  // If the pattern didn't get a type, it's because we ran into some
+  // unknown types along the way. We'll need to check the initializer.
+  auto init = elt.getInitializer();
+  bool hadError = TypeChecker::typeCheckBinding(pattern, init, dc, patternType);
+  elt.setPattern(pattern);
+  elt.setInitializer(init);
+  isFalsable |= pattern->isRefutablePattern();
+  return hadError;
+}
+
 /// Type check the given 'if', 'while', or 'guard' statement condition.
 ///
 /// \param stmt The conditional statement to type-check, which will be modified
@@ -427,88 +510,12 @@ static LabeledStmt *findBreakOrContinueStmtTarget(
 /// \returns true if an error occurred, false otherwise.
 static bool typeCheckConditionForStatement(LabeledConditionalStmt *stmt,
                                            DeclContext *dc) {
-  auto &Context = dc->getASTContext();
   bool hadError = false;
   bool hadAnyFalsable = false;
   auto cond = stmt->getCond();
   for (auto &elt : cond) {
-    if (elt.getKind() == StmtConditionElement::CK_Availability) {
-      hadAnyFalsable = true;
-
-      // Reject inlinable code using availability macros.
-      PoundAvailableInfo *info = elt.getAvailability();
-      if (auto *decl = dc->getAsDecl()) {
-        if (decl->getAttrs().hasAttribute<InlinableAttr>() ||
-            decl->getAttrs().hasAttribute<AlwaysEmitIntoClientAttr>())
-          for (auto queries : info->getQueries())
-            if (auto availSpec =
-                  dyn_cast<PlatformVersionConstraintAvailabilitySpec>(queries))
-              if (availSpec->getMacroLoc().isValid()) {
-                Context.Diags.diagnose(
-                    availSpec->getMacroLoc(),
-                    swift::diag::availability_macro_in_inlinable,
-                    decl->getDescriptiveKind());
-                break;
-              }
-      }
-
-      continue;
-    }
-
-    if (auto E = elt.getBooleanOrNull()) {
-      assert(!E->getType() && "the bool condition is already type checked");
-      hadError |= TypeChecker::typeCheckCondition(E, dc);
-      elt.setBoolean(E);
-      hadAnyFalsable = true;
-      continue;
-    }
-    assert(elt.getKind() != StmtConditionElement::CK_Boolean);
-
-    // This is cleanup goop run on the various paths where type checking of the
-    // pattern binding fails.
-    auto typeCheckPatternFailed = [&] {
-      hadError = true;
-      elt.getPattern()->setType(ErrorType::get(Context));
-      elt.getInitializer()->setType(ErrorType::get(Context));
-
-      elt.getPattern()->forEachVariable([&](VarDecl *var) {
-        // Don't change the type of a variable that we've been able to
-        // compute a type for.
-        if (var->hasInterfaceType() && !var->isInvalid())
-          return;
-        var->setInvalid();
-      });
-    };
-
-    // Resolve the pattern.
-    assert(!elt.getPattern()->hasType() &&
-           "the pattern binding condition is already type checked");
-    auto *pattern = TypeChecker::resolvePattern(elt.getPattern(), dc,
-                                                /*isStmtCondition*/ true);
-    if (!pattern) {
-      typeCheckPatternFailed();
-      continue;
-    }
-    elt.setPattern(pattern);
-
-    TypeChecker::diagnoseDuplicateBoundVars(pattern);
-
-    // Check the pattern, it allows unspecified types because the pattern can
-    // provide type information.
-    auto contextualPattern = ContextualPattern::forRawPattern(pattern, dc);
-    Type patternType = TypeChecker::typeCheckPattern(contextualPattern);
-    if (patternType->hasError()) {
-      typeCheckPatternFailed();
-      continue;
-    }
-
-    // If the pattern didn't get a type, it's because we ran into some
-    // unknown types along the way. We'll need to check the initializer.
-    auto init = elt.getInitializer();
-    hadError |= TypeChecker::typeCheckBinding(pattern, init, dc, patternType);
-    elt.setPattern(pattern);
-    elt.setInitializer(init);
-    hadAnyFalsable |= pattern->isRefutablePattern();
+    hadError |=
+        TypeChecker::typeCheckStmtConditionElement(elt, hadAnyFalsable, dc);
   }
 
   // If the binding is not refutable, and there *is* an else, reject it as

lib/Sema/TypeChecker.h

@@ -423,6 +423,13 @@ bool typesSatisfyConstraint(Type t1, Type t2, bool openArchetypes,
 /// of the function, set the result type of the expression to that sugar type.
 Expr *substituteInputSugarTypeForResult(ApplyExpr *E);
 
+/// Type check a \c StmtConditionElement.
+/// Sets \p isFalsable to \c true if the condition might evaluate to \c false,
+/// otherwise leaves \p isFalsable untouched.
+/// \returns \c true if there was an error type checking, \c false otherwise.
+bool typeCheckStmtConditionElement(StmtConditionElement &elt, bool &isFalsable,
+                                   DeclContext *dc);
+
 void typeCheckASTNode(ASTNode &node, DeclContext *DC,
                       bool LeaveBodyUnchecked = false);
 

test/IDE/complete_rdar75200217.swift

@@ -0,0 +1,35 @@
+// RUN: %target-swift-ide-test -code-completion -source-filename %s -F %S/Inputs/mock-sdk -code-completion-token=COMPLETE | %FileCheck %s
+
+enum Encoding {
+  case utf8
+}
+
+func foo(bytes: ArraySlice<UInt16>, encoding: Encoding) {
+  fatalError()
+}
+
+extension Array {
+  func bar<R>(r: R) -> ArraySlice<Element> where R : RangeExpression, Int == R.Bound {
+    fatalError()
+  }
+}
+
+/// The issue, that caused this to fail was that type checking `start` caused
+/// the entire `if` condition to be type-checked, thus also type-checking `end`
+/// which depends on `start`, thus creating a dependency cycle in the request
+/// evaluator.
+/// Thus `end` would get assigned an error type, causing `a` to be an error
+/// type and thus the completion on `encoding` fails.
+/// We need a particular order of type check requests to hit this behaviour,
+/// that's why the test case feels over-complicated.
+func deserializeName(_ data: Array<UInt16>, flag: Bool) {
+  if flag, let start = Optional(Array<UInt16>.Index()), let end = Optional(start) {
+    let range = start..<end
+    let a = data.bar(r: range)
+    foo(bytes: a, encoding: .#^COMPLETE^#)
+  }
+}
+
+// CHECK: Begin completions
+// CHECK-DAG: Decl[EnumElement]/ExprSpecific/TypeRelation[Identical]: utf8[#Encoding#];
+// CHECK: End completions