Merge pull request #10950 from rudkx/revert-recursive-protocol-constraints-changes

Revert recursive protocol constraints changes

Author: rudkx

include/swift/AST/DiagnosticsSema.def

@@ -1666,6 +1666,8 @@ ERROR(requires_generic_param_made_equal_to_concrete,none,
       (Type))
 ERROR(recursive_type_reference,none,
       "%0 %1 references itself", (DescriptiveDeclKind, Identifier))
+ERROR(recursive_requirement_reference,none,
+      "type may not reference itself as a requirement",())
 ERROR(recursive_same_type_constraint,none,
       "same-type constraint %0 == %1 is recursive", (Type, Type))
 ERROR(recursive_superclass_constraint,none,

include/swift/AST/GenericSignatureBuilder.h

@@ -417,6 +417,10 @@ class GenericSignatureBuilder {
   template<typename F>
   void visitPotentialArchetypes(F f);
 
+  void markPotentialArchetypeRecursive(PotentialArchetype *pa,
+                                       ProtocolDecl *proto,
+                                       const RequirementSource *source);
+
 public:
   /// Construct a new generic signature builder.
   ///
@@ -1328,28 +1332,32 @@ class GenericSignatureBuilder::PotentialArchetype {
   /// that share a name.
   llvm::MapVector<Identifier, StoredNestedType> NestedTypes;
 
+  /// \brief Recursively conforms to itself.
+  unsigned IsRecursive : 1;
+
   /// \brief Construct a new potential archetype for an unresolved
   /// associated type.
   PotentialArchetype(PotentialArchetype *parent, Identifier name);
 
   /// \brief Construct a new potential archetype for an associated type.
   PotentialArchetype(PotentialArchetype *parent, AssociatedTypeDecl *assocType)
-    : parentOrBuilder(parent), identifier(assocType)
+    : parentOrBuilder(parent), identifier(assocType), IsRecursive(false)
   {
     assert(parent != nullptr && "Not an associated type?");
   }
 
   /// \brief Construct a new potential archetype for a concrete declaration.
   PotentialArchetype(PotentialArchetype *parent, TypeDecl *concreteDecl)
-    : parentOrBuilder(parent), identifier(concreteDecl)
+    : parentOrBuilder(parent), identifier(concreteDecl), IsRecursive(false)
   {
     assert(parent != nullptr && "Not an associated type?");
   }
 
   /// \brief Construct a new potential archetype for a generic parameter.
   PotentialArchetype(GenericSignatureBuilder *builder,
                      GenericParamKey genericParam)
-    : parentOrBuilder(builder), identifier(genericParam)
+    : parentOrBuilder(builder), identifier(genericParam),
+      IsRecursive(false)
   {
   }
 
@@ -1592,6 +1600,9 @@ class GenericSignatureBuilder::PotentialArchetype {
     return Type();
   }
 
+  void setIsRecursive() { IsRecursive = true; }
+  bool isRecursive() const { return IsRecursive; }
+
   LLVM_ATTRIBUTE_DEPRECATED(
       void dump() const,
       "only for use within the debugger");

include/swift/Basic/LangOptions.h

@@ -206,6 +206,9 @@ namespace swift {
     /// Should we use \c ASTScope-based resolution for unqualified name lookup?
     bool EnableASTScopeLookup = false;
 
+    /// Enable SE-0157: Recursive Protocol Constraints.
+    bool EnableRecursiveConstraints = false;
+
     /// Whether to use the import as member inference system
     ///
     /// When importing a global, try to infer whether we can import it as a

include/swift/Option/FrontendOptions.td

@@ -109,6 +109,9 @@ def disable_target_os_checking :
 def enable_astscope_lookup : Flag<["-"], "enable-astscope-lookup">,
   HelpText<"Enable ASTScope-based unqualified name lookup">;
 
+def enable_recursive_constraints : Flag<["-"], "enable-recursive-constraints">,
+  HelpText<"Enable SE-0157: Recursive Protocol Constraints">;
+
 def print_clang_stats : Flag<["-"], "print-clang-stats">,
   HelpText<"Print Clang importer statistics">;
 

lib/AST/GenericSignatureBuilder.cpp

@@ -90,13 +90,6 @@ struct GenericSignatureBuilder::Implementation {
   /// The set of requirements that have been delayed for some reason.
   SmallVector<DelayedRequirement, 4> DelayedRequirements;
 
-  /// The generation number, which is incremented whenever we successfully
-  /// introduce a new constraint.
-  unsigned Generation = 0;
-
-  /// The generation at which we last processed all of the delayed requirements.
-  unsigned LastProcessedGeneration = 0;
-
 #ifndef NDEBUG
   /// Whether we've already finalized the builder.
   bool finalized = false;
@@ -1406,9 +1399,6 @@ bool PotentialArchetype::addConformance(ProtocolDecl *proto,
     return false;
   }
 
-  // Bump the generation.
-  ++builder.Impl->Generation;
-
   // Add the conformance along with this constraint.
   equivClass->conformsTo[proto].push_back({this, proto, source});
   ++NumConformanceConstraints;
@@ -1876,12 +1866,6 @@ PotentialArchetype *PotentialArchetype::updateNestedTypeForConformance(
   if (!assocType && !concreteDecl)
     return nullptr;
 
-  // If we were asked for a complete, well-formed archetype, make sure we
-  // process delayed requirements if anything changed.
-  SWIFT_DEFER {
-    getBuilder()->processDelayedRequirements();
-  };
-
   Identifier name = assocType ? assocType->getName() : concreteDecl->getName();
   ProtocolDecl *proto =
     assocType ? assocType->getProtocol()
@@ -1916,9 +1900,6 @@ PotentialArchetype *PotentialArchetype::updateNestedTypeForConformance(
     switch (kind) {
     case ArchetypeResolutionKind::CompleteWellFormed:
     case ArchetypeResolutionKind::WellFormed: {
-      // Bump the generation count, because we created a new archetype.
-      ++getBuilder()->Impl->Generation;
-
       if (assocType)
         resultPA = new PotentialArchetype(this, assocType);
       else
@@ -2837,9 +2818,6 @@ ConstraintResult GenericSignatureBuilder::addLayoutRequirement(
     return ConstraintResult::Resolved;
   }
 
-  // Bump the generation.
-  ++Impl->Generation;
-
   auto pa = resolvedSubject->getPotentialArchetype();
   return addLayoutRequirementDirect(pa, layout, source.getSource(pa));
 }
@@ -2924,9 +2902,6 @@ ConstraintResult GenericSignatureBuilder::addSuperclassRequirementDirect(
     .push_back(ConcreteConstraint{T, superclass, source});
   ++NumSuperclassConstraints;
 
-  // Bump the generation.
-  ++Impl->Generation;
-
   // Update the equivalence class with the constraint.
   updateSuperclass(T, superclass, source);
   return ConstraintResult::Resolved;
@@ -3086,9 +3061,6 @@ GenericSignatureBuilder::addSameTypeRequirementBetweenArchetypes(
   if (T1 == T2)
     return ConstraintResult::Resolved;
 
-  // Bump the generation.
-  ++Impl->Generation;
-
   unsigned nestingDepth1 = T1->getNestingDepth();
   unsigned nestingDepth2 = T2->getNestingDepth();
 
@@ -3224,9 +3196,6 @@ ConstraintResult GenericSignatureBuilder::addSameTypeRequirementToConcrete(
                                       ConcreteConstraint{T, Concrete, Source});
   ++NumConcreteTypeConstraints;
 
-  // Bump the generation.
-  ++Impl->Generation;
-
   // If we've already been bound to a type, match that type.
   if (equivClass->concreteType) {
     return addSameTypeRequirement(equivClass->concreteType, Concrete, Source,
@@ -3357,6 +3326,28 @@ ConstraintResult GenericSignatureBuilder::addSameTypeRequirementDirect(
   }
 }
 
+// Local function to mark the given associated type as recursive,
+// diagnosing it if this is the first such occurrence.
+void GenericSignatureBuilder::markPotentialArchetypeRecursive(
+    PotentialArchetype *pa, ProtocolDecl *proto, const RequirementSource *source) {
+  if (pa->isRecursive())
+    return;
+  pa->setIsRecursive();
+
+  pa->addConformance(proto, source, *this);
+  if (!pa->getParent())
+    return;
+
+  auto assocType = pa->getResolvedAssociatedType();
+  if (!assocType || assocType->isInvalid())
+    return;
+
+  Diags.diagnose(assocType->getLoc(), diag::recursive_requirement_reference);
+
+  // Silence downstream errors referencing this associated type.
+  assocType->setInvalid();
+}
+
 ConstraintResult GenericSignatureBuilder::addInheritedRequirements(
                              TypeDecl *decl,
                              UnresolvedType type,
@@ -3407,6 +3398,26 @@ ConstraintResult GenericSignatureBuilder::addInheritedRequirements(
                         getFloatingSource(typeRepr, /*forInferred=*/true));
     }
 
+    if (!decl->getASTContext().LangOpts.EnableRecursiveConstraints) {
+      // Check for direct recursion.
+      if (auto assocType = dyn_cast<AssociatedTypeDecl>(decl)) {
+        auto proto = assocType->getProtocol();
+        if (auto inheritedProto = inheritedType->getAs<ProtocolType>()) {
+          if (inheritedProto->getDecl() == proto ||
+              inheritedProto->getDecl()->inheritsFrom(proto)) {
+            auto source = getFloatingSource(typeRepr, /*forInferred=*/false);
+            if (auto resolved = resolve(type, source)) {
+              if (auto pa = resolved->getPotentialArchetype()) {
+                markPotentialArchetypeRecursive(pa, proto,
+                                                source.getSource(pa));
+                return ConstraintResult::Conflicting;
+              }
+            }
+          }
+        }
+      }
+    }
+
     return addTypeRequirement(type, inheritedType,
                               getFloatingSource(typeRepr,
                                                 /*forInferred=*/false),
@@ -3967,13 +3978,6 @@ static GenericSignatureBuilder::UnresolvedType asUnresolvedType(
 }
 
 void GenericSignatureBuilder::processDelayedRequirements() {
-  // If we're already up-to-date, do nothing.
-  if (Impl->Generation == Impl->LastProcessedGeneration) return;
-
-  SWIFT_DEFER {
-    Impl->LastProcessedGeneration = Impl->Generation;
-  };
-
   bool anySolved = !Impl->DelayedRequirements.empty();
   while (anySolved) {
     // Steal the delayed requirements so we can reprocess them.

lib/Frontend/CompilerInvocation.cpp

@@ -1007,6 +1007,8 @@ static bool ParseLangArgs(LangOptions &Opts, ArgList &Args,
   }
 
   Opts.EnableASTScopeLookup |= Args.hasArg(OPT_enable_astscope_lookup);
+  Opts.EnableRecursiveConstraints |=
+    Args.hasArg(OPT_enable_recursive_constraints);
   Opts.DebugConstraintSolver |= Args.hasArg(OPT_debug_constraints);
   Opts.EnableConstraintPropagation |= Args.hasArg(OPT_propagate_constraints);
   Opts.IterativeTypeChecker |= Args.hasArg(OPT_iterative_type_checker);

test/Generics/same_type_constraints.swift

@@ -154,7 +154,7 @@ extension Dictionary {
 
 // rdar://problem/19245317
 protocol P {
-	associatedtype T: P
+	associatedtype T: P // expected-error{{type may not reference itself as a requirement}}
 }
 
 struct S<A: P> {

test/decl/protocol/recursive_requirement.swift

@@ -3,7 +3,7 @@
 // -----
 
 protocol Foo {
-  associatedtype Bar : Foo
+  associatedtype Bar : Foo // expected-error{{type may not reference itself as a requirement}}
 }
 
 struct Oroborous : Foo {
@@ -13,7 +13,7 @@ struct Oroborous : Foo {
 // -----
 
 protocol P {
- associatedtype A : P
+ associatedtype A : P // expected-error{{type may not reference itself as a requirement}}
 }
 
 struct X<T: P> {
@@ -26,7 +26,7 @@ func f<T : P>(_ z: T) {
 // -----
 
 protocol PP2 {
-  associatedtype A : P2 = Self
+  associatedtype A : P2 = Self // expected-error{{type may not reference itself as a requirement}}
 }
 
 protocol P2 : PP2 {
@@ -41,13 +41,13 @@ struct Y2 : P2 {
 }
 
 func f<T : P2>(_ z: T) {
- _ = X2<T.A>()
+ _ = X2<T.A>() // expected-error{{type 'T.A' does not conform to protocol 'P2'}}
 }
 
 // -----
 
 protocol P3 {
- associatedtype A: P4 = Self
+ associatedtype A: P4 = Self // expected-error{{type may not reference itself as a requirement}}
 }
 
 protocol P4 : P3 {}

test/decl/protocol/recursive_requirement_ok.swift

@@ -1,4 +1,4 @@
-// RUN: %target-typecheck-verify-swift
+// RUN: %target-typecheck-verify-swift -enable-recursive-constraints
 
 protocol P {
   associatedtype Assoc : P
@@ -9,6 +9,5 @@ protocol P {
 func testP<T: P>(_ t: T) {
   testP(t.assoc)
   testP(t.assoc.assoc)
-  testP(t.assoc.assoc.assoc)
-  testP(t.assoc.assoc.assoc.assoc.assoc.assoc.assoc)
+  testP(t.assoc.assoc.assoc) // FIXME: expected-error{{argument type 'T.Assoc.Assoc.Assoc' does not conform to expected type 'P'}}
 }