Merge pull request #41851 from slavapestov/rqm-fix-protocol-refinement-bug

RequirementMachine: Fix terrible bug in minimization of protocol inheritance

Author: slavapestov

lib/AST/RequirementMachine/MinimalConformances.cpp

@@ -159,16 +159,18 @@ class MinimalConformances {
 
   DebugOptions Debug;
 
-  // All conformance rules, sorted by (isExplicit(), getLHS()), with non-explicit
-  // rules with longer left hand sides coming first.
+  // All conformance rules in the current minimization domain, sorted by
+  // (isExplicit(), getLHS()), with non-explicit rules with longer left hand
+  // sides coming first.
   //
   // The idea here is that we want less canonical rules to be eliminated first,
   // but we prefer to eliminate non-explicit rules, in an attempt to keep protocol
   // conformance rules in the same protocol as they were originally defined in.
   SmallVector<unsigned, 4> ConformanceRules;
 
-  // Maps a conformance rule to a conformance path deriving the subject type's
-  // base type. For example, consider the following conformance rule:
+  // Maps a conformance rule in the current minimization domain to a conformance
+  // path deriving the subject type's base type. For example, consider the
+  // following conformance rule:
   //
   //   T.[P:A].[Q:B].[R] => T.[P:A].[Q:B]
   //
@@ -177,15 +179,17 @@ class MinimalConformances {
   // path for the term T.[P:A].[Q], known as the 'parent path'.
   llvm::MapVector<unsigned, SmallVector<unsigned, 2>> ParentPaths;
 
-  // Maps a conformance rule to a list of paths. Each path in the list is a unique
-  // derivation of the conformance in terms of other conformance rules.
+  // Maps a conformance rule in the current minimization domain to a list of paths.
+  // Each path in the list is a unique derivation of the conformance in terms of
+  // other conformance rules.
   llvm::MapVector<unsigned, std::vector<SmallVector<unsigned, 2>>> ConformancePaths;
 
-  // The set of conformance rules which are protocol refinements, that is rules of
-  // the form [P].[Q] => [P].
+  // The set of conformance rules (from all minimization domains) which are protocol
+  // refinements, that is rules of the form [P].[Q] => [P].
   llvm::DenseSet<unsigned> ProtocolRefinements;
 
-  // This is the result.
+  // This is the computed result set of redundant conformance rules in the current
+  // minimization domain.
   llvm::DenseSet<unsigned> &RedundantConformances;
 
   void decomposeTermIntoConformanceRuleLeftHandSides(
@@ -391,17 +395,15 @@ void MinimalConformances::collectConformanceRules() {
     if (!rule.isAnyConformanceRule())
       continue;
 
+    // Save protocol refinement relations in a side table.
+    if (rule.isProtocolRefinementRule(Context))
+      ProtocolRefinements.insert(ruleID);
+
     if (!System.isInMinimizationDomain(rule.getLHS().getRootProtocol()))
       continue;
 
     ConformanceRules.push_back(ruleID);
 
-    // Save protocol refinement relations in a side table.
-    if (rule.isProtocolRefinementRule()) {
-      ProtocolRefinements.insert(ruleID);
-      continue;
-    }
-
     auto lhs = rule.getLHS();
 
     // Record a parent path if the subject type itself requires a non-trivial
@@ -698,7 +700,7 @@ bool MinimalConformances::isValidConformancePath(
 bool MinimalConformances::isValidRefinementPath(
     const llvm::SmallVectorImpl<unsigned> &path) const {
   for (unsigned ruleID : path) {
-    if (!System.getRule(ruleID).isProtocolRefinementRule())
+    if (ProtocolRefinements.count(ruleID) == 0)
       return false;
   }
 
@@ -867,8 +869,14 @@ void MinimalConformances::computeMinimalConformances() {
     for (const auto &path : paths) {
       // Only consider a protocol refinement rule to be redundant if it is
       // witnessed by a composition of other protocol refinement rules.
-      if (isProtocolRefinement && !isValidRefinementPath(path))
+      if (isProtocolRefinement && !isValidRefinementPath(path)) {
+        if (Debug.contains(DebugFlags::MinimalConformances)) {
+          llvm::dbgs() << "Not a refinement path: ";
+          dumpConformancePath(llvm::errs(), path);
+          llvm::dbgs() << "\n";
+        }
         continue;
+      }
 
       llvm::SmallDenseSet<unsigned, 4> visited;
       visited.insert(ruleID);

lib/AST/RequirementMachine/Rule.cpp

@@ -93,7 +93,7 @@ bool Rule::isIdentityConformanceRule() const {
 
 /// If this is a rule of the form [P].[Q] => [P] where [P] and [Q] are
 /// protocol symbols, return true, otherwise return false.
-bool Rule::isProtocolRefinementRule() const {
+bool Rule::isProtocolRefinementRule(RewriteContext &ctx) const {
   if (LHS.size() == 2 &&
       RHS.size() == 1 &&
       LHS[0] == RHS[0] &&
@@ -111,7 +111,7 @@ bool Rule::isProtocolRefinementRule() const {
     auto *proto = LHS[0].getProtocol();
     auto *otherProto = LHS[1].getProtocol();
 
-    auto inherited = proto->getInheritedProtocols();
+    auto inherited = ctx.getInheritedProtocols(proto);
     return (std::find(inherited.begin(), inherited.end(), otherProto)
             != inherited.end());
   }

lib/AST/RequirementMachine/Rule.h

@@ -108,7 +108,7 @@ class Rule final {
 
   bool isIdentityConformanceRule() const;
 
-  bool isProtocolRefinementRule() const;
+  bool isProtocolRefinementRule(RewriteContext &ctx) const;
 
   bool isCircularConformanceRule() const;
 

test/Generics/redundant_protocol_refinement.swift

@@ -1,22 +1,29 @@
 // RUN: %target-typecheck-verify-swift
 // RUN: %target-swift-frontend -typecheck -debug-generic-signatures -requirement-machine-protocol-signatures=verify %s 2>&1 | %FileCheck %s
 
-protocol Base {}
-
 // CHECK-LABEL: redundant_protocol_refinement.(file).Base@
 // CHECK-LABEL: Requirement signature: <Self>
-
-protocol Middle : Base {}
+protocol Base {}
 
 // CHECK-LABEL: redundant_protocol_refinement.(file).Middle@
 // CHECK-LABEL: Requirement signature: <Self where Self : Base>
+protocol Middle : Base {}
 
+// CHECK-LABEL: redundant_protocol_refinement.(file).Derived@
+// CHECK-LABEL: Requirement signature: <Self where Self : Middle>
 protocol Derived : Middle, Base {}
 // expected-note@-1 {{conformance constraint 'Self' : 'Base' implied here}}
 // expected-warning@-2 {{redundant conformance constraint 'Self' : 'Base'}}
 
-// CHECK-LABEL: redundant_protocol_refinement.(file).Derived@
+// CHECK-LABEL: redundant_protocol_refinement.(file).Derived2@
 // CHECK-LABEL: Requirement signature: <Self where Self : Middle>
+protocol Derived2 : Middle {}
+
+// CHECK-LABEL: redundant_protocol_refinement.(file).MoreDerived@
+// CHECK-LABEL: Requirement signature: <Self where Self : Derived2>
+protocol MoreDerived : Derived2, Base {}
+// expected-note@-1 {{conformance constraint 'Self' : 'Base' implied here}}
+// expected-warning@-2 {{redundant conformance constraint 'Self' : 'Base'}}
 
 protocol P1 {}