RequirementMachine: Mark conflicting rules

This isn't quite right, because we really want the longer (more
specific) of the two rules to be conflicting, not the most-recently
added rule.

Author: slavapestov

lib/AST/RequirementMachine/HomotopyReduction.cpp

@@ -523,16 +523,19 @@ void RewriteSystem::minimizeRewriteSystem() {
 
 /// In a conformance-valid rewrite system, any rule with unresolved symbols on
 /// the left or right hand side should have been simplified by another rule.
-bool RewriteSystem::hasNonRedundantUnresolvedRules() const {
+bool RewriteSystem::hadError() const {
   assert(Complete);
   assert(Minimized);
 
   for (const auto &rule : Rules) {
-    if (!rule.isRedundant() &&
-        !rule.isPermanent() &&
-        rule.containsUnresolvedSymbols()) {
+    if (rule.isPermanent())
+      continue;
+
+    if (rule.isConflicting())
+      return true;
+
+    if (!rule.isRedundant() && rule.containsUnresolvedSymbols())
       return true;
-    }
   }
 
   return false;
@@ -555,6 +558,7 @@ RewriteSystem::getMinimizedProtocolRules(
 
     if (rule.isPermanent() ||
         rule.isRedundant() ||
+        rule.isConflicting() ||
         rule.containsUnresolvedSymbols()) {
       continue;
     }
@@ -584,6 +588,7 @@ RewriteSystem::getMinimizedGenericSignatureRules() const {
 
     if (rule.isPermanent() ||
         rule.isRedundant() ||
+        rule.isConflicting() ||
         rule.containsUnresolvedSymbols()) {
       continue;
     }

lib/AST/RequirementMachine/PropertyMap.cpp

@@ -309,14 +309,15 @@ void PropertyMap::clear() {
 
 /// Record a protocol conformance, layout or superclass constraint on the given
 /// key. Must be called in monotonically non-decreasing key order.
-void PropertyMap::addProperty(
+bool PropertyMap::addProperty(
     Term key, Symbol property, unsigned ruleID,
     SmallVectorImpl<InducedRule> &inducedRules) {
   assert(property.isProperty());
   assert(*System.getRule(ruleID).isPropertyRule() == property);
   auto *props = getOrCreateProperties(key);
-  props->addProperty(property, ruleID, Context,
-                     inducedRules, Debug.contains(DebugFlags::ConcreteUnification));
+  bool debug = Debug.contains(DebugFlags::ConcreteUnification);
+  return props->addProperty(property, ruleID, Context,
+                            inducedRules, debug);
 }
 
 /// Build the property map from all rules of the form T.[p] => T, where
@@ -375,7 +376,10 @@ PropertyMap::buildPropertyMap(unsigned maxIterations,
 
   for (const auto &bucket : properties) {
     for (auto property : bucket) {
-      addProperty(property.key, property.symbol, property.ruleID, inducedRules);
+      bool conflict = addProperty(property.key, property.symbol,
+                                  property.ruleID, inducedRules);
+      if (conflict)
+        System.getRule(property.ruleID).markConflicting();
     }
   }
 

lib/AST/RequirementMachine/PropertyMap.h

@@ -100,7 +100,7 @@ class PropertyBag {
 
   explicit PropertyBag(Term key) : Key(key) {}
 
-  void addProperty(Symbol property,
+  bool addProperty(Symbol property,
                    unsigned ruleID,
                    RewriteContext &ctx,
                    SmallVectorImpl<InducedRule> &inducedRules,
@@ -202,7 +202,7 @@ class PropertyMap {
 
 private:
   void clear();
-  void addProperty(Term key, Symbol property, unsigned ruleID,
+  bool addProperty(Term key, Symbol property, unsigned ruleID,
                    SmallVectorImpl<InducedRule> &inducedRules);
 
   void computeConcreteTypeInDomainMap();

lib/AST/RequirementMachine/PropertyUnification.cpp

@@ -250,7 +250,7 @@ static bool unifyConcreteTypes(
 ///
 /// Returns the most derived superclass, which becomes the new superclass
 /// that gets recorded in the property map.
-static Symbol unifySuperclasses(
+static std::pair<Symbol, bool> unifySuperclasses(
     Symbol lhs, Symbol rhs, RewriteContext &ctx,
     SmallVectorImpl<InducedRule> &inducedRules,
     bool debug) {
@@ -282,7 +282,7 @@ static Symbol unifySuperclasses(
       llvm::dbgs() << "%% Unrelated superclass types\n";
     }
 
-    return lhs;
+    return std::make_pair(lhs, true);
   }
 
   if (lhsClass != rhsClass) {
@@ -300,14 +300,15 @@ static Symbol unifySuperclasses(
     if (debug) {
       llvm::dbgs() << "%% Superclass conflict\n";
     }
-    return rhs;
+    return std::make_pair(rhs, true);
   }
 
   // Record the more specific class.
-  return rhs;
+  return std::make_pair(rhs, false);
 }
 
-void PropertyBag::addProperty(
+/// Returns true if there was a conflict.
+bool PropertyBag::addProperty(
     Symbol property, unsigned ruleID, RewriteContext &ctx,
     SmallVectorImpl<InducedRule> &inducedRules,
     bool debug) {
@@ -316,46 +317,55 @@ void PropertyBag::addProperty(
   case Symbol::Kind::Protocol:
     ConformsTo.push_back(property.getProtocol());
     ConformsToRules.push_back(ruleID);
-    return;
+    return false;
 
   case Symbol::Kind::Layout:
     if (!Layout) {
       Layout = property.getLayoutConstraint();
       LayoutRule = ruleID;
-    } else
+    } else {
       Layout = Layout.merge(property.getLayoutConstraint());
+      if (!Layout->isKnownLayout())
+        return true;
+    }
 
-    return;
+    return false;
 
   case Symbol::Kind::Superclass: {
     // FIXME: Also handle superclass vs concrete
 
-    if (Superclass) {
-      Superclass = unifySuperclasses(*Superclass, property,
-                                     ctx, inducedRules, debug);
-    } else {
+    if (!Superclass) {
       Superclass = property;
       SuperclassRule = ruleID;
+    } else {
+      auto pair = unifySuperclasses(*Superclass, property,
+                                    ctx, inducedRules, debug);
+      Superclass = pair.first;
+      bool conflict = pair.second;
+      if (conflict)
+        return true;
     }
 
-    return;
+    return false;
   }
 
   case Symbol::Kind::ConcreteType: {
-    if (ConcreteType) {
-      (void) unifyConcreteTypes(*ConcreteType, property,
-                                ctx, inducedRules, debug);
-    } else {
+    if (!ConcreteType) {
       ConcreteType = property;
       ConcreteTypeRule = ruleID;
+    } else {
+      bool conflict = unifyConcreteTypes(*ConcreteType, property,
+                                         ctx, inducedRules, debug);
+      if (conflict)
+        return true;
     }
 
-    return;
+    return false;
   }
 
   case Symbol::Kind::ConcreteConformance:
     // FIXME
-    return;
+    return false;
 
   case Symbol::Kind::Name:
   case Symbol::Kind::GenericParam:

lib/AST/RequirementMachine/RequirementMachine.cpp

@@ -279,7 +279,7 @@ bool RequirementMachine::isComplete() const {
 bool RequirementMachine::hadError() const {
   // FIXME: Implement other checks here
   // FIXME: Assert if hadError() is true but we didn't emit any diagnostics?
-  return System.hasNonRedundantUnresolvedRules();
+  return System.hadError();
 }
 
 void RequirementMachine::dump(llvm::raw_ostream &out) const {

lib/AST/RequirementMachine/RewriteSystem.cpp

@@ -150,6 +150,8 @@ void Rule::dump(llvm::raw_ostream &out) const {
     out << " [simplified]";
   if (Redundant)
     out << " [redundant]";
+  if (Conflicting)
+    out << "[conflicting]";
 }
 
 RewriteSystem::RewriteSystem(RewriteContext &ctx)

lib/AST/RequirementMachine/RewriteSystem.h

@@ -66,13 +66,25 @@ class Rule final {
   /// set of requirements in a generic signature.
   unsigned Redundant : 1;
 
+  /// A 'conflicting' rule is a property rule which cannot be satisfied by any
+  /// concrete type because it is mutually exclusive with some other rule.
+  /// An example would be a pair of concrete type rules:
+  ///
+  ///    T.[concrete: Int] => T
+  ///    T.[concrete: String] => T
+  ///
+  /// Conflicting rules are detected in property map construction, and are
+  /// dropped from the minimal set of requirements.
+  unsigned Conflicting : 1;
+
 public:
   Rule(Term lhs, Term rhs)
       : LHS(lhs), RHS(rhs) {
     Permanent = false;
     Explicit = false;
     Simplified = false;
     Redundant = false;
+    Conflicting = false;
   }
 
   const Term &getLHS() const { return LHS; }
@@ -105,6 +117,10 @@ class Rule final {
     return Redundant;
   }
 
+  bool isConflicting() const {
+    return Conflicting;
+  }
+
   bool containsUnresolvedSymbols() const {
     return (LHS.containsUnresolvedSymbols() ||
             RHS.containsUnresolvedSymbols());
@@ -132,6 +148,13 @@ class Rule final {
     Redundant = true;
   }
 
+  void markConflicting() {
+    // It's okay to mark a rule as conflicting multiple times, but it must not
+    // be a permanent rule.
+    assert(!Permanent && "Permanent rule should not conflict with anything");
+    Conflicting = true;
+  }
+
   unsigned getDepth() const;
 
   int compare(const Rule &other, RewriteContext &ctx) const;
@@ -356,7 +379,7 @@ class RewriteSystem final {
 public:
   void minimizeRewriteSystem();
 
-  bool hasNonRedundantUnresolvedRules() const;
+  bool hadError() const;
 
   llvm::DenseMap<const ProtocolDecl *, std::vector<unsigned>>
   getMinimizedProtocolRules(ArrayRef<const ProtocolDecl *> protos) const;