Merge pull request swiftlang#40646 from slavapestov/rqm-more-validation-test-fixes

RequirementMachine: More fixes for validation tests with -requirement-machine-protocol-signatures=verify

Author: slavapestov

lib/AST/RequirementMachine/MinimalConformances.cpp

@@ -655,21 +655,21 @@ bool MinimalConformances::isValidConformancePath(
 
       if (!foundValidConformancePath)
         return false;
-    }
-
-    auto found = ParentPaths.find(ruleID);
-    if (found != ParentPaths.end()) {
-      SWIFT_DEFER {
-        visited.erase(ruleID);
-      };
-      visited.insert(ruleID);
-
-      // If 'req' is based on some other conformance requirement
-      // `T.[P.]A : Q', we want to make sure that we have a
-      // non-redundant derivation for 'T : P'.
-      if (!isValidConformancePath(visited, found->second,
-                                  /*allowConcrete=*/false)) {
-        return false;
+    } else {
+      auto found = ParentPaths.find(ruleID);
+      if (found != ParentPaths.end()) {
+        SWIFT_DEFER {
+          visited.erase(ruleID);
+        };
+        visited.insert(ruleID);
+
+        // If 'req' is based on some other conformance requirement
+        // `T.[P.]A : Q', we want to make sure that we have a
+        // non-redundant derivation for 'T : P'.
+        if (!isValidConformancePath(visited, found->second,
+                                    /*allowConcrete=*/false)) {
+          return false;
+        }
       }
     }
   }
@@ -868,6 +868,9 @@ void MinimalConformances::computeMinimalConformances(bool firstPass) {
           llvm::dbgs() << " pass: ";
           llvm::dbgs() << System.getRule(ruleID).getLHS();
           llvm::dbgs() << "\n";
+          llvm::dbgs() << "-- via valid path: ";
+          dumpConformancePath(llvm::errs(), path);
+          llvm::dbgs() << "\n";
         }
 
         RedundantConformances.insert(ruleID);
@@ -897,6 +900,7 @@ void MinimalConformances::verifyMinimalConformances() const {
         llvm::errs() << "Redundant conformance is not recoverable:\n";
         llvm::errs() << rule << "\n\n";
         dumpMinimalConformanceEquations(llvm::errs());
+        dumpMinimalConformances(llvm::errs());
         abort();
       }
 
@@ -907,6 +911,7 @@ void MinimalConformances::verifyMinimalConformances() const {
       llvm::errs() << "Minimal conformance contains unresolved symbols: ";
       llvm::errs() << rule << "\n\n";
       dumpMinimalConformanceEquations(llvm::errs());
+      dumpMinimalConformances(llvm::errs());
       abort();
     }
   }

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)) {

lib/AST/RequirementMachine/RequirementLowering.cpp

@@ -505,9 +505,9 @@ TypeAliasRequirementsRequest::evaluate(Evaluator &evaluator,
   for (auto *inheritedProto : ctx.getRewriteContext().getInheritedProtocols(proto)) {
     for (auto req : inheritedProto->getMembers()) {
       if (auto *typeReq = dyn_cast<TypeDecl>(req)) {
-        // Ignore generic typealiases.
-        if (auto typeAliasReq = dyn_cast<TypeAliasDecl>(typeReq))
-          if (typeAliasReq->isGeneric())
+        // Ignore generic types.
+        if (auto genReq = dyn_cast<GenericTypeDecl>(req))
+          if (genReq->getGenericParams())
             continue;
 
         inheritedTypeDecls[typeReq->getName()].push_back(typeReq);

lib/AST/RequirementMachine/RewriteSystem.cpp

@@ -98,7 +98,8 @@ bool Rule::isProtocolRefinementRule() const {
       RHS.size() == 1 &&
       LHS[0] == RHS[0] &&
       LHS[0].getKind() == Symbol::Kind::Protocol &&
-      LHS[1].getKind() == Symbol::Kind::Protocol &&
+      (LHS[1].getKind() == Symbol::Kind::Protocol ||
+       LHS[1].getKind() == Symbol::Kind::ConcreteConformance) &&
       LHS[0] != LHS[1]) {
 
     // A protocol refinement rule must be from a directly-stated

test/Generics/redundant_parent_path_in_protocol.swift

@@ -0,0 +1,50 @@
+// RUN: %target-swift-frontend -typecheck %s -debug-generic-signatures -requirement-machine-protocol-signatures=on 2>&1 | %FileCheck %s
+
+// CHECK-LABEL: redundant_parent_path_in_protocol.(file).P1@
+// CHECK-NEXT: Requirement signature: <Self>
+
+protocol P1 {}
+
+// CHECK-LABEL: redundant_parent_path_in_protocol.(file).P2@
+// CHECK-NEXT: Requirement signature: <Self where Self.A : P1, Self.B : P2>
+
+protocol P2 {
+  associatedtype A: P1
+  associatedtype B: P2
+}
+
+struct Concrete: P1, P2 {
+  typealias A = Concrete
+  typealias B = Concrete
+}
+
+// CHECK-LABEL: redundant_parent_path_in_protocol.(file).P3a@
+// CHECK-NEXT: Requirement signature: <Self where Self.T : P2>
+
+protocol P3a {
+  associatedtype T : P2
+}
+
+// CHECK-LABEL: redundant_parent_path_in_protocol.(file).P3b@
+// CHECK-NEXT: Requirement signature: <Self where Self.T : P2, Self.T.A == Self.T.B>
+
+protocol P3b {
+  associatedtype T : P2 where T.A == T.B
+}
+
+// CHECK-LABEL: redundant_parent_path_in_protocol.(file).P4a@
+// CHECK-NEXT: Requirement signature: <Self where Self : P3a, Self.T == Concrete>
+
+protocol P4a : P3a where T.A == T.B, T == Concrete {}
+
+// CHECK-LABEL: redundant_parent_path_in_protocol.(file).P4b@
+// CHECK-NEXT: Requirement signature: <Self where Self : P3b, Self.T == Concrete>
+
+protocol P4b : P3b where T == Concrete {}
+
+// CHECK-LABEL: redundant_parent_path_in_protocol.(file).P5@
+// CHECK-NEXT: Requirement signature: <Self where Self.T == Concrete>
+
+protocol P5 {
+  associatedtype T : P2 where T.A == T.B, T == Concrete
+}

test/Generics/redundant_protocol_inheritance_via_concrete.swift

@@ -0,0 +1,8 @@
+// RUN: %target-swift-frontend -typecheck %s -debug-generic-signatures -requirement-machine-protocol-signatures=on 2>&1 | %FileCheck %s
+
+protocol P {}
+class C : P {}
+
+// CHECK-LABEL: redundant_protocol_inheritance_via_concrete.(file).Q@
+// CHECK-NEXT: Requirement signature: <Self where Self : C>
+protocol Q : P, C {}