Merge pull request #42151 from slavapestov/rqm-more-minor-diagnostics-fixes

RequirementMachine: More minor diagnostics fixes

Author: slavapestov

include/swift/AST/Requirement.h

@@ -42,10 +42,13 @@ class Requirement
   Requirement(RequirementKind kind, Type first, LayoutConstraint second)
     : RequirementBase(kind, first, second) {}
 
-  /// Whether this requirement is in its canonical form.
+  /// Whether this requirement's types contain ErrorTypes.
+  bool hasError() const;
+
+  /// Whether this requirement is written with canonical types.
   bool isCanonical() const;
 
-  /// Get the canonical form of this requirement.
+  /// Canonicalize the types in this requirement.
   Requirement getCanonical() const;
 
   /// Subst the types involved in this requirement.

lib/AST/GenericSignature.cpp

@@ -591,15 +591,26 @@ void swift::simple_display(raw_ostream &out, GenericSignature sig) {
     out << "NULL";
 }
 
+bool Requirement::hasError() const {
+  if (getFirstType()->hasError())
+    return true;
+
+  if (getKind() != RequirementKind::Layout &&
+      getSecondType()->hasError())
+    return true;
+
+  return false;
+}
+
 bool Requirement::isCanonical() const {
-  if (getFirstType() && !getFirstType()->isCanonical())
+  if (!getFirstType()->isCanonical())
     return false;
 
   switch (getKind()) {
   case RequirementKind::Conformance:
   case RequirementKind::SameType:
   case RequirementKind::Superclass:
-    if (getSecondType() && !getSecondType()->isCanonical())
+    if (!getSecondType()->isCanonical())
       return false;
     break;
 
@@ -612,17 +623,13 @@ bool Requirement::isCanonical() const {
 
 /// Get the canonical form of this requirement.
 Requirement Requirement::getCanonical() const {
-  Type firstType = getFirstType();
-  if (firstType)
-    firstType = firstType->getCanonicalType();
+  Type firstType = getFirstType()->getCanonicalType();
 
   switch (getKind()) {
   case RequirementKind::Conformance:
   case RequirementKind::SameType:
   case RequirementKind::Superclass: {
-    Type secondType = getSecondType();
-    if (secondType)
-      secondType = secondType->getCanonicalType();
+    Type secondType = getSecondType()->getCanonicalType();
     return Requirement(getKind(), firstType, secondType);
   }
 

lib/AST/RequirementMachine/ConcreteContraction.cpp

@@ -182,7 +182,8 @@ class ConcreteContraction {
 
   bool performConcreteContraction(
       ArrayRef<StructuralRequirement> requirements,
-      SmallVectorImpl<StructuralRequirement> &result);
+      SmallVectorImpl<StructuralRequirement> &result,
+      SmallVectorImpl<RequirementError> &errors);
 };
 
 }  // end namespace
@@ -380,6 +381,13 @@ ConcreteContraction::substRequirement(const Requirement &req) const {
 
   case RequirementKind::Layout: {
     auto substFirstType = substTypeParameter(firstType);
+    if (!substFirstType->isTypeParameter() &&
+        !substFirstType->satisfiesClassConstraint() &&
+        req.getLayoutConstraint()->isClass()) {
+      // If the concrete type doesn't satisfy the layout constraint,
+      // leave it unsubstituted so that we produce a better diagnostic.
+      return req;
+    }
 
     return Requirement(req.getKind(),
                        substFirstType,
@@ -464,7 +472,8 @@ bool ConcreteContraction::preserveSameTypeRequirement(
 /// original \p requirements.
 bool ConcreteContraction::performConcreteContraction(
     ArrayRef<StructuralRequirement> requirements,
-    SmallVectorImpl<StructuralRequirement> &result) {
+    SmallVectorImpl<StructuralRequirement> &result,
+    SmallVectorImpl<RequirementError> &errors) {
 
   // Phase 1 - collect concrete type and superclass requirements where the
   // subject type is a generic parameter.
@@ -577,47 +586,25 @@ bool ConcreteContraction::performConcreteContraction(
       llvm::dbgs() << "\n";
     }
 
-    if (preserveSameTypeRequirement(req.req)) {
-      if (Debug) {
-        llvm::dbgs() << "@ Preserving original requirement: ";
-        req.req.dump(llvm::dbgs());
-        llvm::dbgs() << "\n";
-      }
-
-      // Make the duplicated requirement 'inferred' so that we don't diagnose
-      // it as redundant.
-      result.push_back({req.req, SourceLoc(), /*inferred=*/true});
-    }
-
     // Substitute the requirement.
-    Optional<Requirement> substReq = substRequirement(req.req);
-
-    // If substitution failed, we have a conflict; bail out here so that we can
-    // diagnose the conflict later.
-    if (!substReq) {
-      if (Debug) {
-        llvm::dbgs() << "@ Concrete contraction cannot proceed; requirement ";
-        llvm::dbgs() << "substitution failed:\n";
-        req.req.dump(llvm::dbgs());
-        llvm::dbgs() << "\n";
-      }
-
-      continue;
-    }
+    auto substReq = substRequirement(req.req);
 
     if (Debug) {
       llvm::dbgs() << "@ Substituted requirement: ";
-      substReq->dump(llvm::dbgs());
+      substReq.dump(llvm::dbgs());
       llvm::dbgs() << "\n";
     }
 
     // Otherwise, desugar the requirement again, since we might now have a
     // requirement where the left hand side is not a type parameter.
-    //
-    // FIXME: Do we need to check for errors? Right now they're just ignored.
     SmallVector<Requirement, 4> reqs;
-    SmallVector<RequirementError, 1> errors;
-    desugarRequirement(*substReq, reqs, errors);
+    if (req.inferred) {
+      SmallVector<RequirementError, 4> discardErrors;
+      desugarRequirement(substReq, SourceLoc(), reqs, discardErrors);
+    } else {
+      desugarRequirement(substReq, req.loc, reqs, errors);
+    }
+
     for (auto desugaredReq : reqs) {
       if (Debug) {
         llvm::dbgs() << "@@ Desugared requirement: ";
@@ -626,6 +613,20 @@ bool ConcreteContraction::performConcreteContraction(
       }
       result.push_back({desugaredReq, req.loc, req.inferred});
     }
+
+    if (preserveSameTypeRequirement(req.req) &&
+        (!req.req.getFirstType()->isEqual(substReq.getFirstType()) ||
+         !req.req.getSecondType()->isEqual(substReq.getSecondType()))) {
+      if (Debug) {
+        llvm::dbgs() << "@ Preserving original requirement: ";
+        req.req.dump(llvm::dbgs());
+        llvm::dbgs() << "\n";
+      }
+
+      // Make the duplicated requirement 'inferred' so that we don't diagnose
+      // it as redundant.
+      result.push_back({req.req, SourceLoc(), /*inferred=*/true});
+    }
   }
 
   if (Debug) {
@@ -647,7 +648,9 @@ bool ConcreteContraction::performConcreteContraction(
 bool swift::rewriting::performConcreteContraction(
     ArrayRef<StructuralRequirement> requirements,
     SmallVectorImpl<StructuralRequirement> &result,
+    SmallVectorImpl<RequirementError> &errors,
     bool debug) {
   ConcreteContraction concreteContraction(debug);
-  return concreteContraction.performConcreteContraction(requirements, result);
+  return concreteContraction.performConcreteContraction(
+      requirements, result, errors);
 }

lib/AST/RequirementMachine/ConcreteTypeWitness.cpp

@@ -568,7 +568,7 @@ void PropertyMap::inferConditionalRequirements(
       llvm::dbgs() << "\n";
     }
 
-    desugarRequirement(req, desugaredRequirements, errors);
+    desugarRequirement(req, SourceLoc(), desugaredRequirements, errors);
   }
 
   // Now, convert desugared conditional requirements to rules.

lib/AST/RequirementMachine/PropertyUnification.cpp

@@ -666,51 +666,59 @@ void PropertyMap::checkConcreteTypeRequirements() {
       auto concreteType = pair.first;
       unsigned concreteTypeRule = pair.second;
 
-      // A rule (T.[concrete: C] => T) where C is a class type induces a rule
-      // (T.[superclass: C] => T).
-      if (concreteType.getConcreteType()->getClassOrBoundGenericClass()) {
-        auto superclassSymbol = Symbol::forSuperclass(
-            concreteType.getConcreteType(),
-            concreteType.getSubstitutions(),
-            Context);
-
-        recordRelation(props->getKey(), concreteTypeRule,
-                       superclassSymbol, System, debug);
-
       // If the concrete type is not a class and we have a superclass
       // requirement, we have a conflict.
-      } else if (props->hasSuperclassBound()) {
+      if (!concreteType.getConcreteType()->getClassOrBoundGenericClass() &&
+          props->hasSuperclassBound()) {
         const auto &req = props->getSuperclassRequirement();
         for (auto pair : req.SuperclassRules) {
-          System.recordConflict(concreteTypeRule, pair.second);
+          if (checkRulePairOnce(concreteTypeRule, pair.second))
+            System.recordConflict(concreteTypeRule, pair.second);
         }
       }
 
-      // A rule (T.[concrete: C] => T) where C is a class type induces a rule
-      // (T.[layout: L] => T), where L is either AnyObject or _NativeObject.
-      if (concreteType.getConcreteType()->satisfiesClassConstraint()) {
-        Type superclassType = concreteType.getConcreteType();
-        if (!superclassType->getClassOrBoundGenericClass())
-          superclassType = superclassType->getSuperclass();
-
-        auto layoutConstraint = LayoutConstraintKind::Class;
-        if (superclassType)
-          if (auto *classDecl = superclassType->getClassOrBoundGenericClass())
-            layoutConstraint = classDecl->getLayoutConstraintKind();
-
-        auto layout =
-            LayoutConstraint::getLayoutConstraint(
-              layoutConstraint, Context.getASTContext());
-        auto layoutSymbol = Symbol::forLayout(layout, Context);
+      if (checkRuleOnce(concreteTypeRule)) {
+        if (concreteType.getConcreteType()->getClassOrBoundGenericClass()) {
+          // A rule (T.[concrete: C] => T) where C is a class type induces a rule
+          // (T.[superclass: C] => T).
+          auto superclassSymbol = Symbol::forSuperclass(
+              concreteType.getConcreteType(),
+              concreteType.getSubstitutions(),
+              Context);
+
+          recordRelation(props->getKey(), concreteTypeRule,
+                         superclassSymbol, System, debug);
+        }
 
-        recordRelation(props->getKey(), concreteTypeRule,
-                       layoutSymbol, System, debug);
+        // A rule (T.[concrete: C] => T) where C is a class type induces a rule
+        // (T.[layout: L] => T), where L is either AnyObject or _NativeObject.
+        if (concreteType.getConcreteType()->satisfiesClassConstraint()) {
+          Type superclassType = concreteType.getConcreteType();
+          if (!superclassType->getClassOrBoundGenericClass())
+            superclassType = superclassType->getSuperclass();
+
+          auto layoutConstraint = LayoutConstraintKind::Class;
+          if (superclassType)
+            if (auto *classDecl = superclassType->getClassOrBoundGenericClass())
+              layoutConstraint = classDecl->getLayoutConstraintKind();
+
+          auto layout =
+              LayoutConstraint::getLayoutConstraint(
+                layoutConstraint, Context.getASTContext());
+          auto layoutSymbol = Symbol::forLayout(layout, Context);
+
+          recordRelation(props->getKey(), concreteTypeRule,
+                         layoutSymbol, System, debug);
+        }
+      }
 
       // If the concrete type does not satisfy a class layout constraint and
       // we have such a layout requirement, we have a conflict.
-      } else if (props->LayoutRule &&
-                 props->Layout->isClass()) {
-        System.recordConflict(concreteTypeRule, *props->LayoutRule);
+      if (!concreteType.getConcreteType()->satisfiesClassConstraint() &&
+          props->LayoutRule &&
+          props->Layout->isClass()) {
+        if (checkRulePairOnce(concreteTypeRule, *props->LayoutRule))
+          System.recordConflict(concreteTypeRule, *props->LayoutRule);
       }
     }
   }