Merge pull request #82147 from hamishknight/fix-nested-arenas

[CS] Avoid solver-allocated inputs with `typesSatisfyConstraint`

Author: hamishknight

include/swift/AST/Types.h

@@ -720,6 +720,12 @@ class alignas(1 << TypeAlignInBits) TypeBase
     return getRecursiveProperties().hasTypeVariable();
   }
 
+  // Convenience for checking whether the given type either has a type
+  // variable or placeholder.
+  bool hasTypeVariableOrPlaceholder() const {
+    return hasTypeVariable() || hasPlaceholder();
+  }
+
   /// Determine where this type is a type variable or a dependent
   /// member root in a type variable.
   bool isTypeVariableOrMember();

include/swift/Sema/ConstraintSystem.h

@@ -102,9 +102,7 @@ enum class FreeTypeVariableBinding {
   /// Disallow any binding of such free type variables.
   Disallow,
   /// Allow the free type variables to persist in the solution.
-  Allow,
-  /// Bind the type variables to UnresolvedType to represent the ambiguity.
-  UnresolvedType
+  Allow
 };
 
 /// Describes whether or not a result builder method is supported.

lib/AST/ASTContext.cpp

@@ -3577,7 +3577,13 @@ void LocatableType::Profile(llvm::FoldingSetNodeID &id, SourceLoc loc,
 Type ErrorType::get(const ASTContext &C) { return C.TheErrorType; }
 
 Type ErrorType::get(Type originalType) {
-  assert(originalType);
+  ASSERT(originalType);
+  // We don't currently support solver-allocated error types, if we want
+  // this in the future we'll need to adjust `Solution::simplifyType` to fold
+  // them into regular ErrorTypes. Additionally, any clients of
+  // `typesSatisfyConstraint` will need to be taught not to pass such types.
+  ASSERT(!originalType->getRecursiveProperties().isSolverAllocated() &&
+         "Solver-allocated error types not supported");
 
   auto originalProperties = originalType->getRecursiveProperties();
   auto arena = getArena(originalProperties);
@@ -3588,14 +3594,8 @@ Type ErrorType::get(Type originalType) {
 
   void *mem = ctx.Allocate(sizeof(ErrorType) + sizeof(Type),
                            alignof(ErrorType), arena);
-  RecursiveTypeProperties properties = RecursiveTypeProperties::HasError;
-
-  // We need to preserve the solver allocated bit, to ensure any wrapping
-  // types are solver allocated too.
-  if (originalProperties.isSolverAllocated())
-    properties |= RecursiveTypeProperties::SolverAllocated;
-
-  return entry = new (mem) ErrorType(ctx, originalType, properties);
+  return entry = new (mem) ErrorType(ctx, originalType,
+                                     RecursiveTypeProperties::HasError);
 }
 
 void ErrorUnionType::Profile(llvm::FoldingSetNodeID &id, ArrayRef<Type> terms) {

lib/AST/TypeSubstitution.cpp

@@ -446,8 +446,13 @@ Type TypeSubstituter::transformDependentMemberType(DependentMemberType *dependen
 
   auto result = conformance.getTypeWitness(assocType, IFS.getOptions());
   if (result->is<ErrorType>()) {
+    // Substitute the base type for the original ErrorType for type printing.
+    // Avoid doing this if the substitutions introduce type variables or
+    // placeholders since ErrorTypes can't be solver-allocated currently (and
+    // type variables aren't helpful when printing anyway).
     auto substBase = origBase.subst(IFS);
-    return DependentMemberType::get(ErrorType::get(substBase), assocType);
+    if (!substBase->hasTypeVariableOrPlaceholder())
+      return DependentMemberType::get(ErrorType::get(substBase), assocType);
   }
   return result;
 }
@@ -1242,4 +1247,4 @@ ProtocolConformanceRef ReplaceExistentialArchetypesWithConcreteTypes::operator()
 
   return subs.lookupConformance(
       getInterfaceType(existentialArchetype)->getCanonicalType(), proto);
-}
+}

lib/Sema/CSFix.cpp

@@ -185,9 +185,12 @@ CoerceToCheckedCast *CoerceToCheckedCast::attempt(ConstraintSystem &cs,
                                                   Type fromType, Type toType,
                                                   bool useConditionalCast,
                                                   ConstraintLocator *locator) {
-  // If any of the types has a type variable, don't add the fix.
-  if (fromType->hasTypeVariable() || toType->hasTypeVariable())
+  // If any of the types have type variables or placeholders, don't add the fix.
+  // `typeCheckCheckedCast` doesn't support checking such types.
+  if (fromType->hasTypeVariableOrPlaceholder() ||
+      toType->hasTypeVariableOrPlaceholder()) {
     return nullptr;
+  }
 
   auto anchor = locator->getAnchor();
   if (auto *assignExpr = getAsExpr<AssignExpr>(anchor))

lib/Sema/CSGen.cpp

@@ -4872,29 +4872,16 @@ bool ConstraintSystem::generateConstraints(
         return convertTypeLocator;
       };
 
-      // Substitute type variables in for placeholder types (and unresolved
-      // types, if allowed).
-      if (allowFreeTypeVariables == FreeTypeVariableBinding::UnresolvedType) {
-        convertType = convertType.transformRec([&](Type type) -> std::optional<Type> {
-          if (type->is<UnresolvedType>() || type->is<PlaceholderType>()) {
-            return Type(createTypeVariable(getLocator(type),
-                                           TVO_CanBindToNoEscape |
-                                               TVO_PrefersSubtypeBinding |
-                                               TVO_CanBindToHole));
-          }
-          return std::nullopt;
-        });
-      } else {
-        convertType = convertType.transformRec([&](Type type) -> std::optional<Type> {
-          if (type->is<PlaceholderType>()) {
-            return Type(createTypeVariable(getLocator(type),
-                                           TVO_CanBindToNoEscape |
-                                               TVO_PrefersSubtypeBinding |
-                                               TVO_CanBindToHole));
-          }
-          return std::nullopt;
-        });
-      }
+      // Substitute type variables in for placeholder types.
+      convertType = convertType.transformRec([&](Type type) -> std::optional<Type> {
+        if (type->is<PlaceholderType>()) {
+          return Type(createTypeVariable(getLocator(type),
+                                         TVO_CanBindToNoEscape |
+                                             TVO_PrefersSubtypeBinding |
+                                             TVO_CanBindToHole));
+        }
+        return std::nullopt;
+      });
 
       addContextualConversionConstraint(expr, convertType, ctp,
                                         convertTypeLocator);

lib/Sema/CSRanking.cpp

@@ -1413,26 +1413,10 @@ SolutionCompareResult ConstraintSystem::compareSolutions(
     auto type1 = types.Type1;
     auto type2 = types.Type2;
 
-    // If either of the types still contains type variables, we can't
-    // compare them.
-    // FIXME: This is really unfortunate. More type variable sharing
-    // (when it's sound) would help us do much better here.
-    if (type1->hasTypeVariable() || type2->hasTypeVariable()) {
-      identical = false;
-      continue;
-    }
-
-    // With introduction of holes it's currently possible to form solutions
-    // with UnresolvedType bindings, we need to account for that in
-    // ranking. If one solution has a hole for a given type variable
-    // it's always worse than any non-hole type other solution might have.
-    if (type1->is<UnresolvedType>() || type2->is<UnresolvedType>()) {
-      if (type1->is<UnresolvedType>()) {
-        ++score2;
-      } else {
-        ++score1;
-      }
-
+    // If either of the types have holes or unresolved type variables, we can't
+    // compare them. `isSubtypeOf` cannot be used with solver-allocated types.
+    if (type1->hasTypeVariableOrPlaceholder() ||
+        type2->hasTypeVariableOrPlaceholder()) {
       identical = false;
       continue;
     }
@@ -1469,15 +1453,12 @@ SolutionCompareResult ConstraintSystem::compareSolutions(
     // The systems are not considered equivalent.
     identical = false;
 
-    // Archetypes are worse than concrete types (i.e. non-placeholder and
-    // non-archetype)
+    // Archetypes are worse than concrete types
     // FIXME: Total hack.
-    if (type1->is<ArchetypeType>() && !type2->is<ArchetypeType>() &&
-        !type2->is<PlaceholderType>()) {
+    if (type1->is<ArchetypeType>() && !type2->is<ArchetypeType>()) {
       ++score2;
       continue;
-    } else if (type2->is<ArchetypeType>() && !type1->is<ArchetypeType>() &&
-               !type1->is<PlaceholderType>()) {
+    } else if (type2->is<ArchetypeType>() && !type1->is<ArchetypeType>()) {
       ++score1;
       continue;
     }

lib/Sema/CSSimplify.cpp

@@ -4909,8 +4909,12 @@ static bool
 repairViaBridgingCast(ConstraintSystem &cs, Type fromType, Type toType,
                       SmallVectorImpl<RestrictionOrFix> &conversionsOrFixes,
                       ConstraintLocatorBuilder locator) {
-  if (fromType->hasTypeVariable() || toType->hasTypeVariable())
+  // Don't check if any of the types have type variables or placeholders,
+  // `typeCheckCheckedCast` doesn't support checking solver-allocated types.
+  if (fromType->hasTypeVariableOrPlaceholder() ||
+      toType->hasTypeVariableOrPlaceholder()) {
     return false;
+  }
 
   auto objectType1 = fromType->getOptionalObjectType();
   auto objectType2 = toType->getOptionalObjectType();
@@ -9375,10 +9379,12 @@ static ConstraintFix *maybeWarnAboutExtraneousCast(
   if (locator.endsWith<LocatorPathElt::GenericArgument>())
     return nullptr;
 
-  // Both types have to be fixed.
-  if (fromType->hasTypeVariable() || toType->hasTypeVariable() ||
-      fromType->hasPlaceholder() || toType->hasPlaceholder())
+  // Both types have to be resolved, `typeCheckCheckedCast` doesn't support
+  // checking solver-allocated types.
+  if (fromType->hasTypeVariableOrPlaceholder() ||
+      toType->hasTypeVariableOrPlaceholder()) {
     return nullptr;
+  }
 
   SmallVector<LocatorPathElt, 4> path;
   auto anchor = locator.getLocatorParts(path);

lib/Sema/CSSolver.cpp

@@ -91,10 +91,6 @@ Solution ConstraintSystem::finalize() {
 
     case FreeTypeVariableBinding::Allow:
       break;
-
-    case FreeTypeVariableBinding::UnresolvedType:
-      assignFixedType(tv, ctx.TheUnresolvedType);
-      break;
     }
   }
 

lib/Sema/TypeCheckConstraints.cpp

@@ -1080,8 +1080,14 @@ bool TypeChecker::typesSatisfyConstraint(Type type1, Type type2,
                                          bool openArchetypes,
                                          ConstraintKind kind, DeclContext *dc,
                                          bool *unwrappedIUO) {
-  assert(!type1->hasTypeVariable() && !type2->hasTypeVariable() &&
-         "Unexpected type variable in constraint satisfaction testing");
+  // Don't allow any type variables to leak into the nested ConstraintSystem
+  // (including as originator types for placeholders). This also ensure that we
+  // avoid lifetime issues for e.g cases where we lazily populate the
+  // `ContextSubMap` on `NominalOrBoundGenericNominalType` in the nested arena,
+  // since it will be destroyed on leaving.
+  ASSERT(!type1->getRecursiveProperties().isSolverAllocated() &&
+         !type2->getRecursiveProperties().isSolverAllocated() &&
+         "Cannot escape solver-allocated types into a nested ConstraintSystem");
 
   ConstraintSystem cs(dc, ConstraintSystemOptions());
   if (openArchetypes) {