RequirementMachine: Mark rules as conflicting more aggressively

Author: slavapestov

lib/AST/RequirementMachine/PropertyMap.cpp

@@ -309,7 +309,10 @@ void PropertyMap::clear() {
 
 /// Record a protocol conformance, layout or superclass constraint on the given
 /// key. Must be called in monotonically non-decreasing key order.
-bool PropertyMap::addProperty(
+///
+/// If there was a conflict, returns the conflicting rule ID; otherwise
+/// returns None.
+Optional<unsigned> PropertyMap::addProperty(
     Term key, Symbol property, unsigned ruleID,
     SmallVectorImpl<InducedRule> &inducedRules) {
   assert(property.isProperty());
@@ -376,10 +379,22 @@ PropertyMap::buildPropertyMap(unsigned maxIterations,
 
   for (const auto &bucket : properties) {
     for (auto property : bucket) {
-      bool conflict = addProperty(property.key, property.symbol,
-                                  property.ruleID, inducedRules);
-      if (conflict)
-        System.getRule(property.ruleID).markConflicting();
+      auto existingRuleID = addProperty(property.key, property.symbol,
+                                        property.ruleID, inducedRules);
+      if (existingRuleID) {
+        // The GSB only dropped the new rule in the case of a conflicting
+        // superclass requirement, so maintain that behavior here.
+        auto &existingRule = System.getRule(*existingRuleID);
+        if (existingRule.isPropertyRule()->getKind() !=
+            Symbol::Kind::Superclass) {
+          if (existingRule.getRHS().size() == property.key.size())
+            existingRule.markConflicting();
+        }
+
+        auto &newRule = System.getRule(property.ruleID);
+        assert(newRule.getRHS().size() == property.key.size());
+        newRule.markConflicting();
+      }
     }
   }
 

lib/AST/RequirementMachine/PropertyMap.h

@@ -101,11 +101,11 @@ class PropertyBag {
 
   explicit PropertyBag(Term key) : Key(key) {}
 
-  bool addProperty(Symbol property,
-                   unsigned ruleID,
-                   RewriteContext &ctx,
-                   SmallVectorImpl<InducedRule> &inducedRules,
-                   bool debug);
+  Optional<unsigned> addProperty(Symbol property,
+                                 unsigned ruleID,
+                                 RewriteContext &ctx,
+                                 SmallVectorImpl<InducedRule> &inducedRules,
+                                 bool debug);
   void copyPropertiesFrom(const PropertyBag *next,
                           RewriteContext &ctx);
 
@@ -204,8 +204,9 @@ class PropertyMap {
 
 private:
   void clear();
-  bool addProperty(Term key, Symbol property, unsigned ruleID,
-                   SmallVectorImpl<InducedRule> &inducedRules);
+  Optional<unsigned>
+  addProperty(Term key, Symbol property, unsigned ruleID,
+              SmallVectorImpl<InducedRule> &inducedRules);
 
   void computeConcreteTypeInDomainMap();
   void concretizeNestedTypesFromConcreteParents(

lib/AST/RequirementMachine/PropertyUnification.cpp

@@ -307,8 +307,9 @@ static std::pair<Symbol, bool> unifySuperclasses(
   return std::make_pair(rhs, false);
 }
 
-/// Returns true if there was a conflict.
-bool PropertyBag::addProperty(
+/// Returns the old conflicting rule ID if there was a conflict,
+/// otherwise returns None.
+Optional<unsigned> PropertyBag::addProperty(
     Symbol property, unsigned ruleID, RewriteContext &ctx,
     SmallVectorImpl<InducedRule> &inducedRules,
     bool debug) {
@@ -317,19 +318,20 @@ bool PropertyBag::addProperty(
   case Symbol::Kind::Protocol:
     ConformsTo.push_back(property.getProtocol());
     ConformsToRules.push_back(ruleID);
-    return false;
+    return None;
 
   case Symbol::Kind::Layout:
     if (!Layout) {
       Layout = property.getLayoutConstraint();
       LayoutRule = ruleID;
     } else {
+      assert(LayoutRule.hasValue());
       Layout = Layout.merge(property.getLayoutConstraint());
       if (!Layout->isKnownLayout())
-        return true;
+        return LayoutRule;
     }
 
-    return false;
+    return None;
 
   case Symbol::Kind::Superclass: {
     // FIXME: Also handle superclass vs concrete
@@ -338,34 +340,36 @@ bool PropertyBag::addProperty(
       Superclass = property;
       SuperclassRule = ruleID;
     } else {
+      assert(SuperclassRule.hasValue());
       auto pair = unifySuperclasses(*Superclass, property,
                                     ctx, inducedRules, debug);
       Superclass = pair.first;
       bool conflict = pair.second;
       if (conflict)
-        return true;
+        return SuperclassRule;
     }
 
-    return false;
+    return None;
   }
 
   case Symbol::Kind::ConcreteType: {
     if (!ConcreteType) {
       ConcreteType = property;
       ConcreteTypeRule = ruleID;
     } else {
+      assert(ConcreteTypeRule.hasValue());
       bool conflict = unifyConcreteTypes(*ConcreteType, property,
                                          ctx, inducedRules, debug);
       if (conflict)
-        return true;
+        return ConcreteTypeRule;
     }
 
-    return false;
+    return None;
   }
 
   case Symbol::Kind::ConcreteConformance:
     // FIXME
-    return false;
+    return None;
 
   case Symbol::Kind::Name:
   case Symbol::Kind::GenericParam:
@@ -533,10 +537,13 @@ void PropertyMap::concretizeNestedTypesFromConcreteParent(
       // With concrete types, a missing conformance is a conflict.
       if (requirementKind == RequirementKind::SameType) {
         // FIXME: Diagnose conflict
-        //
-        // FIXME: We should mark the more specific rule of the two
-        // as conflicting.
-        System.getRule(conformanceRuleID).markConflicting();
+        auto &concreteRule = System.getRule(concreteRuleID);
+        if (concreteRule.getRHS().size() == key.size())
+          concreteRule.markConflicting();
+
+        auto &conformanceRule = System.getRule(conformanceRuleID);
+        if (conformanceRule.getRHS().size() == key.size())
+          conformanceRule.markConflicting();
       }
 
       if (Debug.contains(DebugFlags::ConcretizeNestedTypes)) {