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

Consider this example:

    protocol P : C {}
    class C : P {}

    <T where T : P>

The GenericSignatureBuilder thinks the minimized signature is
<T where T : P>. The RequirementMachine would minimize it down to
<T where T : C>. The latter is more correct, since the conformance
here is concrete and no witness table needs to be passed in at
runtime, however for strict binary compatibility we need to produce
the same signature as the GenericSignatureBuilder.

Accomplish this by changing the minimal conformances algorithm to
detect "circular concrete conformance rules", which take the form

    [P].[concrete: C : Q]

Where Q : P. These rules are given special handling. Ordinarily a
protocol conformance rule is eliminated before a concrete conformance
rule; however concrete conformances derived from circular
conformances are considered to be redundant from the get-go,
preventing protocol conformances that can be written in terms of
such concrete conformances from themselves becoming redundant.

Fixes rdar://problem/89633532.

Author: slavapestov

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