RequirementMachine: Fix minimization when protocol is constrained to a class that conforms to the protocol

lib/AST/RequirementMachine/ConcreteContraction.cpp

@@ -159,6 +159,8 @@ class ConcreteContraction {
 
   llvm::SmallDenseMap<GenericParamKey, Type> ConcreteTypes;
   llvm::SmallDenseMap<GenericParamKey, Type> Superclasses;
+  llvm::SmallDenseMap<GenericParamKey,
+                      llvm::SmallVector<ProtocolDecl *, 1>> Conformances;
 
   Optional<Type> substTypeParameter(GenericTypeParamType *rootParam,
                                     DependentMemberType *memberType,
@@ -436,8 +438,8 @@ bool ConcreteContraction::performConcreteContraction(
         if (Debug) {
           llvm::dbgs() << "@ Concrete contraction cannot proceed: "
                        << "duplicate concrete type requirements\n";
-          return false;
         }
+        return false;
       }
 
       break;
@@ -454,18 +456,54 @@ bool ConcreteContraction::performConcreteContraction(
         if (Debug) {
           llvm::dbgs() << "@ Concrete contraction cannot proceed: "
                        << "duplicate superclass requirements\n";
-          return false;
         }
+        return false;
       }
 
       break;
     }
-    case RequirementKind::Conformance:
+    case RequirementKind::Conformance: {
+      auto *protoDecl = req.req.getProtocolDecl();
+      Conformances[GenericParamKey(genericParam)].push_back(protoDecl);
+
+      break;
+    }
     case RequirementKind::Layout:
       break;
     }
   }
 
+  // Block concrete contraction if a generic parameter conforms to a protocol P
+  // which has a superclass bound C which again conforms to P. This is a really
+  // silly edge case, but we go to great pains to produce the same minimized
+  // signature as the GenericSignatureBuilder in this case, <T : P>, and not the
+  // more logical <T : C>.
+  for (const auto &pair : Conformances) {
+    auto subjectType = pair.first;
+    auto found = Superclasses.find(subjectType);
+    if (found == Superclasses.end())
+      continue;
+
+    auto superclassTy = found->second;
+
+    for (const auto *proto : pair.second) {
+      if (auto otherSuperclassTy = proto->getSuperclass()) {
+        if (Debug) {
+          llvm::dbgs() << "@ Subject type of superclass requirement "
+                       << "τ_" << subjectType.Depth << "_" << subjectType.Index
+                       << " : " << superclassTy << " conforms to "
+                       << proto->getName() << " which has a superclass bound "
+                       << otherSuperclassTy << "\n";
+        }
+
+        if (superclassTy->isEqual(otherSuperclassTy)) {
+          Superclasses.erase(subjectType);
+          break;
+        }
+      }
+    }
+  }
+
   // If there's nothing to do just return.
   if (ConcreteTypes.empty() && Superclasses.empty())
     return false;

lib/AST/RequirementMachine/MinimalConformances.cpp

@@ -195,6 +195,13 @@ class MinimalConformances {
       MutableTerm term, unsigned ruleID,
       SmallVectorImpl<unsigned> &result) const;
 
+  bool isConformanceRuleRecoverable(
+    llvm::SmallDenseSet<unsigned, 4> &visited,
+    unsigned ruleID) const;
+
+  bool isDerivedViaCircularConformanceRule(
+      const std::vector<SmallVector<unsigned, 2>> &paths) const;
+
   bool isValidConformancePath(
       llvm::SmallDenseSet<unsigned, 4> &visited,
       const llvm::SmallVectorImpl<unsigned> &path) const;
@@ -601,57 +608,71 @@ void MinimalConformances::computeCandidateConformancePaths() {
   }
 }
 
-/// Determines if \p path can be expressed without any of the conformance
-/// rules appearing in \p redundantConformances, by possibly substituting
-/// any occurrences of the redundant rules with alternate definitions
-/// appearing in \p conformancePaths.
+/// If \p ruleID is redundant, determines if it can be expressed without
+/// without any of the conformance rules determined to be redundant so far.
 ///
-/// The \p conformancePaths map sends conformance rules to a list of
-/// disjunctions, where each disjunction is a product of other conformance
-/// rules.
-bool MinimalConformances::isValidConformancePath(
+/// If the rule is not redundant, determines if its parent path can
+/// also be recovered.
+bool MinimalConformances::isConformanceRuleRecoverable(
     llvm::SmallDenseSet<unsigned, 4> &visited,
-    const llvm::SmallVectorImpl<unsigned> &path) const {
+    unsigned ruleID) const {
+  if (RedundantConformances.count(ruleID)) {
+    SWIFT_DEFER {
+      visited.erase(ruleID);
+    };
+    visited.insert(ruleID);
 
-  for (unsigned ruleID : path) {
-    if (visited.count(ruleID) > 0)
+    auto found = ConformancePaths.find(ruleID);
+    if (found == ConformancePaths.end())
       return false;
 
-    if (RedundantConformances.count(ruleID)) {
+    bool foundValidConformancePath = false;
+    for (const auto &otherPath : found->second) {
+      if (isValidConformancePath(visited, otherPath)) {
+        foundValidConformancePath = true;
+        break;
+      }
+    }
+
+    if (!foundValidConformancePath)
+      return false;
+  } else {
+    auto found = ParentPaths.find(ruleID);
+    if (found != ParentPaths.end()) {
       SWIFT_DEFER {
         visited.erase(ruleID);
       };
       visited.insert(ruleID);
 
-      auto found = ConformancePaths.find(ruleID);
-      if (found == ConformancePaths.end())
+      // 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))
         return false;
+    }
+  }
 
-      bool foundValidConformancePath = false;
-      for (const auto &otherPath : found->second) {
-        if (isValidConformancePath(visited, otherPath)) {
-          foundValidConformancePath = true;
-          break;
-        }
-      }
+  return true;
+}
 
-      if (!foundValidConformancePath)
-        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))
-          return false;
-      }
-    }
+/// Determines if \p path can be expressed without any of the conformance
+/// rules determined to be redundant so far, by possibly substituting
+/// any occurrences of the redundant rules with alternate definitions
+/// appearing in the set of known conformancePaths.
+///
+/// The conformance path map sends conformance rules to a list of
+/// disjunctions, where each disjunction is a product of other conformance
+/// rules.
+bool MinimalConformances::isValidConformancePath(
+    llvm::SmallDenseSet<unsigned, 4> &visited,
+    const llvm::SmallVectorImpl<unsigned> &path) const {
+
+  for (unsigned ruleID : path) {
+    if (visited.count(ruleID) > 0)
+      return false;
+
+    if (!isConformanceRuleRecoverable(visited, ruleID))
+      return false;
   }
 
   return true;
@@ -786,12 +807,45 @@ void MinimalConformances::verifyMinimalConformanceEquations() const {
 #endif
 }
 
+bool MinimalConformances::isDerivedViaCircularConformanceRule(
+    const std::vector<SmallVector<unsigned, 2>> &paths) const {
+  for (const auto &path : paths) {
+    if (!path.empty() &&
+        System.getRule(path.back()).isCircularConformanceRule())
+      return true;
+  }
+
+  return false;
+}
+
 /// Find a set of minimal conformances by marking all non-minimal
 /// conformances redundant.
 ///
 /// In the first pass, we only consider conformance requirements that are
 /// made redundant by concrete conformances.
 void MinimalConformances::computeMinimalConformances() {
+  // First, mark any concrete conformances derived via a circular
+  // conformance as redundant upfront. See the comment at the top of
+  // Rule::isCircularConformanceRule() for an explanation of this.
+  for (unsigned ruleID : ConformanceRules) {
+    auto found = ConformancePaths.find(ruleID);
+    if (found == ConformancePaths.end())
+      continue;
+
+    const auto &paths = found->second;
+
+    if (System.getRule(ruleID).isProtocolConformanceRule())
+      continue;
+
+    if (isDerivedViaCircularConformanceRule(paths))
+      RedundantConformances.insert(ruleID);
+  }
+
+  // Now, visit each conformance rule, trying to make it redundant by
+  // deriving a path in terms of other non-redundant conformance rules.
+  //
+  // Note that the ConformanceRules vector is sorted in descending
+  // canonical term order, so less canonical rules are eliminated first.
   for (unsigned ruleID : ConformanceRules) {
     auto found = ConformancePaths.find(ruleID);
     if (found == ConformancePaths.end())
@@ -840,10 +894,7 @@ void MinimalConformances::verifyMinimalConformances() const {
       // minimal conformances.
       llvm::SmallDenseSet<unsigned, 4> visited;
 
-      llvm::SmallVector<unsigned, 1> path;
-      path.push_back(ruleID);
-
-      if (!isValidConformancePath(visited, path)) {
+      if (!isConformanceRuleRecoverable(visited, ruleID)) {
         llvm::errs() << "Redundant conformance is not recoverable:\n";
         llvm::errs() << rule << "\n\n";
         dumpMinimalConformanceEquations(llvm::errs());

lib/AST/RequirementMachine/RewriteSystem.cpp

@@ -120,6 +120,32 @@ bool Rule::isProtocolRefinementRule() const {
   return false;
 }
 
+/// If this is a rule of the form [P].[concrete: C : Q] => [P] where
+/// [P] is a protocol symbol, return true.
+///
+/// This means that P constrains 'Self' to a concrete type that conforms
+/// to some Q with P : Q. We don't consider this to be a valid conformance
+/// path element, to ensure compatibility with the GSB in an odd edge
+/// case:
+///
+///    protocol P : C {}
+///    class C : P {}
+///
+/// The GSB minimizes the signature <T where T : P> to <T where T : P>,
+/// whereas the minimal conformances algorithm would otherwise minimize
+/// it down to <T where T : C> on account of the (T.[P] => T) conformance
+/// rule being redundantly expressed via [P].[concrete: C : P].
+bool Rule::isCircularConformanceRule() const {
+  if (LHS.size() != 2 || RHS.size() != 1 || LHS[0] != RHS[0])
+    return false;
+
+  if (LHS[0].getKind() != Symbol::Kind::Protocol ||
+      LHS[1].getKind() != Symbol::Kind::ConcreteConformance)
+    return false;
+
+  return true;
+}
+
 /// A protocol typealias rule takes one of the following two forms,
 /// where T is a name symbol:
 ///

lib/AST/RequirementMachine/RewriteSystem.h

@@ -106,6 +106,8 @@ class Rule final {
 
   bool isProtocolRefinementRule() const;
 
+  bool isCircularConformanceRule() const;
+
   /// See above for an explanation of these predicates.
   bool isPermanent() const {
     return Permanent;

test/Generics/derived_via_concrete.swift

@@ -1,5 +1,7 @@
-// RUN: %target-typecheck-verify-swift -requirement-machine-inferred-signatures=verify
-// RUN: %target-swift-frontend -typecheck %s -debug-generic-signatures -requirement-machine-inferred-signatures=verify 2>&1 | %FileCheck %s
+// RUN: %target-typecheck-verify-swift -requirement-machine-inferred-signatures=off
+// RUN: not %target-swift-frontend -typecheck %s -debug-generic-signatures -requirement-machine-inferred-signatures=on 2>&1 | %FileCheck %s
+
+// FIXME: Both RUN lines should pass 'on' once diagnostics are implemented.
 
 protocol P {}
 class C {}
@@ -38,7 +40,11 @@ func derivedViaConcreteY1<A, B>(_: A, _: B)
 // expected-warning@-1 {{redundant conformance constraint 'A' : 'V'}}
 // expected-note@-2 {{conformance constraint 'A' : 'V' implied here}}
 
-// CHECK: Generic signature: <A, B where A : Y<B>, B : C>
+// CHECK: Generic signature: <A, B where A : Y<B>>
+//
+// FIXME: Should be <A, B where A : Y<B>, B : C>, but redundant
+// superclass requirements currently block concrete contraction.
+
 func derivedViaConcreteY2<A, B>(_: A, _: B)
   where A : V, B : C, A : Y<B> {}
 // expected-warning@-1 {{redundant conformance constraint 'A' : 'V'}}

test/Generics/protocol_superclass_cycle.swift

@@ -1,8 +1,54 @@
-// RUN: %target-typecheck-verify-swift
-// RUN: %target-swift-frontend -typecheck -debug-generic-signatures %s 2>&1 | %FileCheck %s
+// RUN: %target-swift-frontend -typecheck -debug-generic-signatures %s -requirement-machine-protocol-signatures=verify -requirement-machine-inferred-signatures=verify 2>&1 | %FileCheck %s
+// RUN: %target-swift-frontend -typecheck -debug-generic-signatures %s -requirement-machine-protocol-signatures=verify -requirement-machine-inferred-signatures=verify -disable-requirement-machine-concrete-contraction 2>&1 | %FileCheck %s
 
 protocol P : C {}
-final class C : P {}
+class C : P {}
+
+// CHECK-LABEL: .SubP@
+// CHECK-NEXT: Requirement signature: <Self where Self : P>
+protocol SubP : P {}
+
+protocol Q {
+  associatedtype T
+}
+
+// CHECK-LABEL: .foo1@
+// CHECK-NEXT: Generic signature: <T where T : P>
+func foo1<T : P>(_: T) {}
+
+// CHECK-LABEL: .foo1a@
+// CHECK-NEXT: Generic signature: <T where T : P>
+func foo1a<T>(_: T) where T : P, T : C {}
+
+// CHECK-LABEL: .foo1b@
+// CHECK-NEXT: Generic signature: <T where T : P>
+func foo1b<T>(_: T) where T : C, T : P {}
+
+// CHECK-LABEL: .foo2@
+// CHECK-NEXT: Generic signature: <T where T : Q, T.[Q]T : P>
+func foo2<T : Q>(_: T) where T.T : P {}
+
+// CHECK-LABEL: .foo3@
+// CHECK-NEXT: Generic signature: <T where T : SubP>
+func foo3<T : SubP>(_: T) {}
+
+// CHECK-LABEL: .foo4@
+// CHECK-NEXT: Generic signature: <T where T : Q, T.[Q]T : SubP>
+func foo4<T : Q>(_: T) where T.T : SubP {}
+
+protocol SuperP {}
+
+// CHECK-LABEL: .SubSuperP@
+// CHECK-NEXT: Requirement signature: <Self where Self : SuperC>
+protocol SubSuperP : SuperP, SuperC {}
+
+class SuperC : SubSuperP {}
+
+// CHECK-LABEL: .foo5@
+// CHECK-NEXT: Generic signature: <T where T : SubSuperP>
+func foo5<T : SubSuperP>(_: T) {}
+
+// CHECK-LABEL: .foo6@
+// CHECK-NEXT: Generic signature: <T where T : Q, T.[Q]T : SubSuperP>
+func foo6<T : Q>(_: T) where T.T : SubSuperP {}
 
-// CHECK: Generic signature: <T where T : P>
-func foo<T : P>(_: T) {}

test/SILGen/protocol_superclass_cycle.swift

@@ -1,4 +1,4 @@
-// RUN: %target-swift-frontend -emit-silgen %s
+// RUN: %target-swift-frontend -emit-silgen -requirement-machine-protocol-signatures=on -requirement-machine-inferred-signatures=on %s
 
 // When a protocol has a superclass requirement on 'Self' and the class
 // itself conforms concretely, the forwarding substitution map will have