Merge pull request #17385 from huonw/lazier-conditional-requirements-4.2

[4.2] More errors instead of crashes for conditional conformances that are invalid or involve hard-to-resolve recursion

Author: huonw

include/swift/AST/DiagnosticsSema.def

@@ -647,6 +647,9 @@ ERROR(ambiguous_module_type,none,
 ERROR(use_nonmatching_operator,none,
       "%0 is not a %select{binary|prefix unary|postfix unary}1 operator",
       (DeclName, unsigned))
+ERROR(unsupported_recursion_in_associated_type_reference,none,
+      "unsupported recursion for reference to associated type %0 of type %1",
+      (DeclName, Type))
 ERROR(broken_associated_type_witness,none,
       "reference to invalid associated type %0 of type %1", (DeclName, Type))
 
@@ -1800,6 +1803,9 @@ NOTE(redundant_conformance_here,none,
      "inferred from type here}0",
      (unsigned, Type, ProtocolDecl *))
 
+ERROR(unsupported_recursive_requirements, none,
+      "requirement involves recursion that is not currently supported", ())
+
 ERROR(same_type_conflict,none,
       "%select{generic parameter |protocol |}0%1 cannot be equal to both "
       "%2 and %3", (unsigned, Type, Type, Type))

include/swift/AST/GenericSignatureBuilder.h

@@ -337,6 +337,12 @@ class GenericSignatureBuilder {
                                    EquivalenceClass *unresolvedEquivClass,
                                    UnresolvedHandlingKind unresolvedHandling);
 
+  /// Add any conditional requirements from the given conformance.
+  ///
+  /// \returns \c true if an error occurred, \c false if not.
+  bool addConditionalRequirements(ProtocolConformanceRef conformance,
+                                  ModuleDecl *inferForModule, SourceLoc loc);
+
   /// Resolve the conformance of the given type to the given protocol when the
   /// potential archetype is known to be equivalent to a concrete type.
   ///

include/swift/AST/ProtocolConformance.h

@@ -306,6 +306,10 @@ class alignas(1 << DeclAlignInBits) ProtocolConformance {
   /// Get the property declaration for a behavior conformance, if this is one.
   AbstractStorageDecl *getBehaviorDecl() const;
 
+  /// Get any additional requirements that are required for this conformance to
+  /// be satisfied, if it is possible for them to be computed.
+  Optional<ArrayRef<Requirement>> getConditionalRequirementsIfAvailable() const;
+
   /// Get any additional requirements that are required for this conformance to
   /// be satisfied.
   ArrayRef<Requirement> getConditionalRequirements() const;
@@ -381,7 +385,16 @@ class NormalProtocolConformance : public ProtocolConformance,
 
   /// Any additional requirements that are required for this conformance to
   /// apply, e.g. 'Something: Baz' in 'extension Foo: Bar where Something: Baz'.
-  ArrayRef<Requirement> ConditionalRequirements;
+  mutable ArrayRef<Requirement> ConditionalRequirements;
+  enum class ConditionalRequirementsState {
+    Uncomputed,
+    Computing,
+    Complete,
+  };
+  /// The state of the ConditionalRequirements field: whether it has been
+  /// computed or not.
+  mutable ConditionalRequirementsState CRState =
+      ConditionalRequirementsState::Uncomputed;
 
   /// The lazy member loader provides callbacks for populating imported and
   /// deserialized conformances.
@@ -418,7 +431,7 @@ class NormalProtocolConformance : public ProtocolConformance,
 
   void resolveLazyInfo() const;
 
-  void differenceAndStoreConditionalRequirements();
+  void differenceAndStoreConditionalRequirements() const;
 
 public:
   /// Get the protocol being conformed to.
@@ -438,11 +451,38 @@ class NormalProtocolConformance : public ProtocolConformance,
     }
   }
 
+  /// Get any additional requirements that are required for this conformance to
+  /// be satisfied if they can be computed.
+  ///
+  /// If \c computeIfPossible is false, this will not do the lazy computation of
+  /// the conditional requirements and will just query the current state. This
+  /// should almost certainly only be used for debugging purposes, prefer \c
+  /// getConditionalRequirementsIfAvailable (these are separate because
+  /// CONFORMANCE_SUBCLASS_DISPATCH does some type checks and a defaulted
+  /// parameter gets in the way of that).
+  Optional<ArrayRef<Requirement>>
+  getConditionalRequirementsIfAvailableOrCached(bool computeIfPossible) const {
+    if (computeIfPossible)
+      differenceAndStoreConditionalRequirements();
+
+    if (CRState == ConditionalRequirementsState::Complete)
+      return ConditionalRequirements;
+
+    return None;
+  }
+  /// Get any additional requirements that are required for this conformance to
+  /// be satisfied if they can be computed.
+  Optional<ArrayRef<Requirement>>
+  getConditionalRequirementsIfAvailable() const {
+    return getConditionalRequirementsIfAvailableOrCached(
+        /*computeIfPossible=*/true);
+  }
+
   /// Get any additional requirements that are required for this conformance to
   /// be satisfied, e.g. for Array<T>: Equatable, T: Equatable also needs
   /// to be satisfied.
   ArrayRef<Requirement> getConditionalRequirements() const {
-    return ConditionalRequirements;
+    return *getConditionalRequirementsIfAvailable();
   }
 
   /// Retrieve the state of this conformance.
@@ -661,14 +701,16 @@ class SpecializedProtocolConformance : public ProtocolConformance,
 
   /// Any conditional requirements, in substituted form. (E.g. given Foo<T>: Bar
   /// where T: Bar, Foo<Baz<U>> will include Baz<U>: Bar.)
-  ArrayRef<Requirement> ConditionalRequirements;
+  mutable Optional<ArrayRef<Requirement>> ConditionalRequirements;
 
   friend class ASTContext;
 
   SpecializedProtocolConformance(Type conformingType,
                                  ProtocolConformance *genericConformance,
                                  SubstitutionList substitutions);
 
+  void computeConditionalRequirements() const;
+
 public:
   /// Get the generic conformance from which this conformance was derived,
   /// if there is one.
@@ -687,9 +729,29 @@ class SpecializedProtocolConformance : public ProtocolConformance,
   SubstitutionMap getSubstitutionMap() const;
 
   /// Get any requirements that must be satisfied for this conformance to apply.
-  ArrayRef<Requirement> getConditionalRequirements() const {
+  ///
+  /// If \c computeIfPossible is false, this will not do the lazy computation of
+  /// the conditional requirements and will just query the current state. This
+  /// should almost certainly only be used for debugging purposes, prefer \c
+  /// getConditionalRequirementsIfAvailable (these are separate because
+  /// CONFORMANCE_SUBCLASS_DISPATCH does some type checks and a defaulted
+  /// parameter gets in the way of that).
+  Optional<ArrayRef<Requirement>>
+  getConditionalRequirementsIfAvailableOrCached(bool computeIfPossible) const {
+    if (computeIfPossible)
+      computeConditionalRequirements();
     return ConditionalRequirements;
   }
+  Optional<ArrayRef<Requirement>>
+  getConditionalRequirementsIfAvailable() const {
+    return getConditionalRequirementsIfAvailableOrCached(
+        /*computeIfPossible=*/true);
+  }
+
+  /// Get any requirements that must be satisfied for this conformance to apply.
+  ArrayRef<Requirement> getConditionalRequirements() const {
+    return *getConditionalRequirementsIfAvailable();
+  }
 
   /// Get the protocol being conformed to.
   ProtocolDecl *getProtocol() const {
@@ -798,6 +860,12 @@ class InheritedProtocolConformance : public ProtocolConformance,
     return InheritedConformance->getProtocol();
   }
 
+  /// Get any requirements that must be satisfied for this conformance to apply.
+  Optional<ArrayRef<Requirement>>
+  getConditionalRequirementsIfAvailable() const {
+    return InheritedConformance->getConditionalRequirementsIfAvailable();
+  }
+
   /// Get any requirements that must be satisfied for this conformance to apply.
   ArrayRef<Requirement> getConditionalRequirements() const {
     return InheritedConformance->getConditionalRequirements();

include/swift/AST/ProtocolConformanceRef.h

@@ -130,6 +130,10 @@ class ProtocolConformanceRef {
   /// Create a canonical conformance from the current one.
   ProtocolConformanceRef getCanonicalConformanceRef() const;
 
+  /// Get any additional requirements that are required for this conformance to
+  /// be satisfied, if they're possible to compute.
+  Optional<ArrayRef<Requirement>> getConditionalRequirementsIfAvailable() const;
+
   /// Get any additional requirements that are required for this conformance to
   /// be satisfied.
   ArrayRef<Requirement> getConditionalRequirements() const;

lib/AST/ASTDumper.cpp

@@ -2885,10 +2885,17 @@ void ProtocolConformance::dump(llvm::raw_ostream &out, unsigned indent) const {
       }
     }
 
-    for (auto requirement : normal->getConditionalRequirements()) {
+    if (auto condReqs = normal->getConditionalRequirementsIfAvailableOrCached(
+            /*computeIfPossible=*/false)) {
+      for (auto requirement : *condReqs) {
+        out << '\n';
+        out.indent(indent + 2);
+        requirement.dump(out);
+      }
+    } else {
       out << '\n';
       out.indent(indent + 2);
-      requirement.dump(out);
+      out << "(conditional requirements unable to be computed)";
     }
     break;
   }
@@ -2909,10 +2916,16 @@ void ProtocolConformance::dump(llvm::raw_ostream &out, unsigned indent) const {
       sub.dump(out, indent + 2);
       out << '\n';
     }
-    for (auto subReq : conf->getConditionalRequirements()) {
+    if (auto condReqs = conf->getConditionalRequirementsIfAvailableOrCached(
+            /*computeIfPossible=*/false)) {
+      for (auto subReq : *condReqs) {
+        out.indent(indent + 2);
+        subReq.dump(out);
+        out << '\n';
+      }
+    } else {
       out.indent(indent + 2);
-      subReq.dump(out);
-      out << '\n';
+      out << "(conditional requirements unable to be computed)\n";
     }
     conf->getGenericConformance()->dump(out, indent + 2);
     break;

lib/AST/GenericSignatureBuilder.cpp

@@ -2593,20 +2593,28 @@ ConstraintResult GenericSignatureBuilder::handleUnresolvedRequirement(
   }
 }
 
-// Function for feeding through any other requirements that the conformance
-// requires to be satisfied. These are things we're inferring.
-static void addConditionalRequirements(GenericSignatureBuilder &builder,
-                                       ProtocolConformanceRef conformance,
-                                       ModuleDecl *inferForModule) {
+bool GenericSignatureBuilder::addConditionalRequirements(
+    ProtocolConformanceRef conformance, ModuleDecl *inferForModule,
+    SourceLoc loc) {
   // Abstract conformances don't have associated decl-contexts/modules, but also
   // don't have conditional requirements.
   if (conformance.isConcrete()) {
-    auto source = FloatingRequirementSource::forInferred(nullptr);
-    for (auto requirement : conformance.getConditionalRequirements()) {
-      builder.addRequirement(requirement, source, inferForModule);
-      ++NumConditionalRequirementsAdded;
+    if (auto condReqs = conformance.getConditionalRequirementsIfAvailable()) {
+      auto source = FloatingRequirementSource::forInferred(nullptr);
+      for (auto requirement : *condReqs) {
+        addRequirement(requirement, source, inferForModule);
+        ++NumConditionalRequirementsAdded;
+      }
+    } else {
+      if (loc.isValid())
+        Diags.diagnose(loc, diag::unsupported_recursive_requirements);
+
+      Impl->HadAnyError = true;
+      return true;
     }
   }
+
+  return false;
 }
 
 const RequirementSource *
@@ -2647,7 +2655,10 @@ GenericSignatureBuilder::resolveConcreteConformance(ResolvedType type,
 
   concreteSource = concreteSource->viaConcrete(*this, *conformance);
   equivClass->recordConformanceConstraint(*this, type, proto, concreteSource);
-  addConditionalRequirements(*this, *conformance, /*inferForModule=*/nullptr);
+  if (addConditionalRequirements(*conformance, /*inferForModule=*/nullptr,
+                                 concreteSource->getLoc()))
+    return nullptr;
+
   return concreteSource;
 }
 const RequirementSource *GenericSignatureBuilder::resolveSuperConformance(
@@ -2678,7 +2689,10 @@ const RequirementSource *GenericSignatureBuilder::resolveSuperConformance(
   superclassSource =
     superclassSource->viaSuperclass(*this, *conformance);
   equivClass->recordConformanceConstraint(*this, type, proto, superclassSource);
-  addConditionalRequirements(*this, *conformance, /*inferForModule=*/nullptr);
+  if (addConditionalRequirements(*conformance, /*inferForModule=*/nullptr,
+                                 superclassSource->getLoc()))
+    return nullptr;
+
   return superclassSource;
 }
 
@@ -4719,7 +4733,9 @@ ConstraintResult GenericSignatureBuilder::addTypeRequirement(
 
         // FIXME: diagnose if there's no conformance.
         if (conformance) {
-          addConditionalRequirements(*this, *conformance, inferForModule);
+          if (addConditionalRequirements(*conformance, inferForModule,
+                                         source.getLoc()))
+            return ConstraintResult::Conflicting;
         }
       }
     }