Merge pull request #84745 from hamishknight/fishmonger

[Evaluator] Enforce consistent results for cyclic requests

Author: hamishknight

include/swift/AST/Evaluator.h

@@ -164,6 +164,9 @@ class Evaluator {
   /// is treated as a stack and is used to detect cycles.
   llvm::SetVector<ActiveRequest> activeRequests;
 
+  /// A set of active requests that have been diagnosed for a cycle.
+  llvm::DenseSet<ActiveRequest> diagnosedActiveCycles;
+
   /// A cache that stores the results of requests.
   evaluator::RequestCache cache;
 
@@ -344,7 +347,17 @@ class Evaluator {
 
     recorder.beginRequest<Request>();
 
-    auto result = getRequestFunction<Request>()(request, *this);
+    auto result = [&]() -> typename Request::OutputType {
+      auto reqResult = getRequestFunction<Request>()(request, *this);
+
+      // If we diagnosed a cycle for this request, we want to only use the
+      // default value to ensure we return a consistent result.
+      if (!diagnosedActiveCycles.empty() &&
+          diagnosedActiveCycles.erase(activeReq)) {
+        return defaultValueFn();
+      }
+      return reqResult;
+    }();
 
     recorder.endRequest<Request>(request);
 

include/swift/AST/GenericSignature.h

@@ -120,6 +120,10 @@ class GenericSignature {
                               ArrayRef<Requirement> requirements,
                               bool isKnownCanonical = false);
 
+  /// Create a new placeholder generic signature from a set of generic
+  /// parameters. This is necessary for recovery in invalid cases.
+  static GenericSignature forInvalid(ArrayRef<GenericTypeParamType *> params);
+
   /// Produce a new generic signature which drops all of the marker
   /// protocol conformance requirements associated with this one.
   GenericSignature withoutMarkerProtocols() const;

lib/AST/ASTContext.cpp

@@ -3618,15 +3618,11 @@ static Type replacingTypeVariablesAndPlaceholders(Type ty) {
 Type ErrorType::get(Type originalType) {
   // The original type is only used for printing/debugging, and we don't support
   // solver-allocated ErrorTypes. As such, fold any type variables and
-  // placeholders into bare ErrorTypes, which print as placeholders.
-  //
-  // FIXME: If the originalType is itself an ErrorType we ought to be flattening
-  // it, but that's currently load-bearing as it avoids crashing for recursive
-  // generic signatures such as in `0120-rdar34184392.swift`. To fix this we
-  // ought to change the evaluator to ensure the outer step of a request cycle
-  // returns the same default value as the inner step such that we don't end up
-  // with conflicting generic signatures on encountering a cycle.
+  // placeholders into ErrorTypes. If we have a top-level one, we can return
+  // that directly.
   originalType = replacingTypeVariablesAndPlaceholders(originalType);
+  if (isa<ErrorType>(originalType.getPointer()))
+    return originalType;
 
   ASSERT(originalType);
   ASSERT(!originalType->getRecursiveProperties().isSolverAllocated() &&
@@ -6009,6 +6005,25 @@ GenericSignature::get(ArrayRef<GenericTypeParamType *> params,
   return newSig;
 }
 
+GenericSignature
+GenericSignature::forInvalid(ArrayRef<GenericTypeParamType *> params) {
+  ASSERT(!params.empty());
+  auto &ctx = params.front()->getASTContext();
+
+  // Add same type requirements that make each of the generic parameters
+  // concrete error types. This helps avoid downstream diagnostics and is
+  // handled the same as if the user wrote e.g `<T where T == Undefined>`.
+  SmallVector<Requirement, 2> requirements;
+  for (auto *param : params) {
+    if (param->isValue())
+      continue;
+
+    requirements.emplace_back(RequirementKind::SameType, param,
+                              ErrorType::get(ctx));
+  }
+  return GenericSignature::get(params, requirements);
+}
+
 GenericEnvironment *GenericEnvironment::forPrimary(GenericSignature signature) {
   auto &ctx = signature->getASTContext();
 

lib/AST/Decl.cpp

@@ -1643,8 +1643,7 @@ bool GenericContext::isComputingGenericSignature() const {
 
 /// If we hit a cycle while building the generic signature, we can't return
 /// nullptr, since this breaks invariants elsewhere. Instead, build a dummy
-/// signature where everything is Copyable and Escapable, to avoid spurious
-/// downstream diagnostics concerning move-only types.
+/// invalid signature to avoid spurious downstream diagnostics.
 static GenericSignature getPlaceholderGenericSignature(
     ASTContext &ctx, const DeclContext *DC) {
   SmallVector<GenericParamList *, 2> gpLists;
@@ -1660,22 +1659,13 @@ static GenericSignature getPlaceholderGenericSignature(
     gpLists[i]->setDepth(i);
 
   SmallVector<GenericTypeParamType *, 2> genericParams;
-  SmallVector<Requirement, 2> requirements;
-
   for (auto *gpList : gpLists) {
     for (auto *genericParam : *gpList) {
       auto type = genericParam->getDeclaredInterfaceType();
       genericParams.push_back(type->castTo<GenericTypeParamType>());
-
-      for (auto ip : InvertibleProtocolSet::allKnown()) {
-        auto proto = ctx.getProtocol(getKnownProtocolKind(ip));
-        requirements.emplace_back(RequirementKind::Conformance, type,
-                                  proto->getDeclaredInterfaceType());
-      }
     }
   }
-
-  return GenericSignature::get(genericParams, requirements);
+  return GenericSignature::forInvalid(genericParams);
 }
 
 GenericSignature GenericContext::getGenericSignature() const {
@@ -2700,10 +2690,16 @@ bool PatternBindingDecl::hasStorage() const {
 
 const PatternBindingEntry *
 PatternBindingDecl::getCheckedPatternBindingEntry(unsigned i) const {
-  return evaluateOrDefault(
+  auto result = evaluateOrDefault(
       getASTContext().evaluator,
       PatternBindingEntryRequest{const_cast<PatternBindingDecl *>(this), i},
       nullptr);
+
+  // If we ran into a cycle, we can still return the entry.
+  if (!result)
+    return &getPatternList()[i];
+
+  return result;
 }
 
 void PatternBindingDecl::setPattern(unsigned i, Pattern *P,
@@ -7392,8 +7388,7 @@ bool ProtocolDecl::existentialConformsToSelf() const {
 }
 
 bool ProtocolDecl::hasSelfOrAssociatedTypeRequirements() const {
-  // Because we will have considered all the protocols in a cyclic hierarchy by
-  // the time the cycle is hit.
+  // Be conservative and avoid diagnosing if we hit a cycle.
   const bool resultForCycle = false;
 
   return evaluateOrDefault(getASTContext().evaluator,

lib/AST/Evaluator.cpp

@@ -155,8 +155,12 @@ void Evaluator::diagnoseCycle(const ActiveRequest &request) {
   request.diagnoseCycle(cycleDiags.getDiags());
 
   for (const auto &step : llvm::reverse(activeRequests)) {
-    if (step == request)
+    if (step == request) {
+      // Note that we diagnosed a cycle for the outermost step to ensure it
+      // also returns a cyclic result.
+      diagnosedActiveCycles.insert(step);
       return;
+    }
     step.noteCycleStep(cycleDiags.getDiags());
   }
 

lib/AST/RequirementMachine/RequirementMachineRequests.cpp

@@ -750,26 +750,6 @@ AbstractGenericSignatureRequest::evaluate(
   }
 }
 
-/// If completion fails, build a dummy generic signature where everything is
-/// Copyable and Escapable, to avoid spurious downstream diagnostics
-/// concerning move-only types.
-static GenericSignature getPlaceholderGenericSignature(
-    ASTContext &ctx, ArrayRef<GenericTypeParamType *> genericParams) {
-  SmallVector<Requirement, 2> requirements;
-  for (auto param : genericParams) {
-    if (param->isValue())
-      continue;
-
-    for (auto ip : InvertibleProtocolSet::allKnown()) {
-      auto proto = ctx.getProtocol(getKnownProtocolKind(ip));
-      requirements.emplace_back(RequirementKind::Conformance, param,
-                                proto->getDeclaredInterfaceType());
-    }
-  }
-
-  return GenericSignature::get(genericParams, requirements);
-}
-
 GenericSignatureWithError
 InferredGenericSignatureRequest::evaluate(
         Evaluator &evaluator,
@@ -996,7 +976,7 @@ InferredGenericSignatureRequest::evaluate(
                          diag::requirement_machine_completion_rule,
                          rule);
 
-      auto result = getPlaceholderGenericSignature(ctx, genericParams);
+      auto result = GenericSignature::forInvalid(genericParams);
 
       if (rewriteCtx.getDebugOptions().contains(DebugFlags::Timers)) {
         rewriteCtx.endTimer("InferredGenericSignatureRequest");

lib/Sema/ConstraintSystem.cpp

@@ -1917,9 +1917,6 @@ static Type replacePlaceholderType(PlaceholderType *placeholder,
 
     return Type(gp);
   });
-  if (isa<TypeVariableType>(replacement.getPointer()))
-    return ErrorType::get(ctx);
-
   // For completion, we want to produce an archetype instead of an ErrorType
   // for a top-level generic parameter.
   // FIXME: This is pretty weird, we're producing a contextual type outside of
@@ -1930,8 +1927,8 @@ static Type replacePlaceholderType(PlaceholderType *placeholder,
       return GP->getDecl()->getInnermostDeclContext()->mapTypeIntoContext(GP);
   }
   // Return an ErrorType with the replacement as the original type. Note that
-  // if we failed to replace a type variable with a generic parameter in a
-  // dependent member, `ErrorType::get` will fold it away.
+  // if we failed to replace a type variable with a generic parameter,
+  // `ErrorType::get` will fold it away.
   return ErrorType::get(replacement);
 }
 

lib/Sema/TypeCheckConstraints.cpp

@@ -893,7 +893,8 @@ bool TypeChecker::typeCheckPatternBinding(PatternBindingDecl *PBD,
   return hadError;
 }
 
-bool TypeChecker::typeCheckForEachPreamble(DeclContext *dc, ForEachStmt *stmt) {
+bool TypeChecker::typeCheckForEachPreamble(DeclContext *dc, ForEachStmt *stmt,
+                                           bool skipWhere) {
   auto &Context = dc->getASTContext();
   FrontendStatsTracer statsTracer(Context.Stats, "typecheck-for-each", stmt);
   PrettyStackTraceStmt stackTrace(Context, "type-checking-for-each", stmt);
@@ -902,18 +903,20 @@ bool TypeChecker::typeCheckForEachPreamble(DeclContext *dc, ForEachStmt *stmt) {
   if (!typeCheckTarget(target))
     return true;
 
-  if (auto *where = stmt->getWhere()) {
-    auto boolType = dc->getASTContext().getBoolType();
-    if (!boolType)
-      return true;
+  if (!skipWhere) {
+    if (auto *where = stmt->getWhere()) {
+      auto boolType = dc->getASTContext().getBoolType();
+      if (!boolType)
+        return true;
 
-    SyntacticElementTarget whereClause(where, dc, {boolType, CTP_Condition},
-                                       /*isDiscarded=*/false);
-    auto result = typeCheckTarget(whereClause);
-    if (!result)
-      return true;
+      SyntacticElementTarget whereClause(where, dc, {boolType, CTP_Condition},
+                                         /*isDiscarded=*/false);
+      auto result = typeCheckTarget(whereClause);
+      if (!result)
+        return true;
 
-    stmt->setWhere(result->getAsExpr());
+      stmt->setWhere(result->getAsExpr());
+    }
   }
 
   // Check to see if the sequence expr is throwing (in async context),

lib/Sema/TypeCheckProtocol.cpp

@@ -1156,17 +1156,22 @@ swift::matchWitness(WitnessChecker::RequirementEnvironmentCache &reqEnvCache,
       getOrCreateRequirementEnvironment(reqEnvCache, dc, reqSig, proto,
                                         covariantSelf, conformance);
 
+  auto reqSubMap = reqEnvironment.getRequirementToWitnessThunkSubs();
+
+  Type selfTy = proto->getSelfInterfaceType().subst(reqSubMap);
+  if (selfTy->hasError()) {
+    return RequirementMatch(witness, MatchKind::WitnessInvalid,
+                            witnessType, reqEnvironment,
+                            /*optionalAdjustments*/ {});
+  }
+
   // Set up the constraint system for matching.
   auto setup =
       [&]() -> std::tuple<std::optional<RequirementMatch>, Type, Type, Type, Type> {
     // Construct a constraint system to use to solve the equality between
     // the required type and the witness type.
     cs.emplace(dc, ConstraintSystemFlags::AllowFixes);
 
-    auto reqSubMap = reqEnvironment.getRequirementToWitnessThunkSubs();
-
-    Type selfTy = proto->getSelfInterfaceType().subst(reqSubMap);
-
     // Open up the type of the requirement.
     reqLocator =
         cs->getConstraintLocator(req, ConstraintLocator::ProtocolRequirement);

lib/Sema/TypeCheckStmt.cpp

@@ -1481,7 +1481,15 @@ class StmtChecker : public StmtVisitor<StmtChecker, Stmt*> {
   }
 
   Stmt *visitForEachStmt(ForEachStmt *S) {
-    TypeChecker::typeCheckForEachPreamble(DC, S);
+    // If we're performing IDE inspection, we also want to skip the where
+    // clause if we're leaving the body unchecked.
+    // FIXME: This is a hack to avoid cycles through NamingPatternRequest when
+    // doing lazy type-checking, we ought to fix the request to be granular in
+    // the type-checking work it kicks.
+    bool skipWhere = LeaveBraceStmtBodyUnchecked &&
+      Ctx.SourceMgr.hasIDEInspectionTargetBuffer();
+
+    TypeChecker::typeCheckForEachPreamble(DC, S, skipWhere);
 
     // Type-check the body of the loop.
     auto sourceFile = DC->getParentSourceFile();