RequirementMachine: New linear order on associated type symbols

slavapestov · slavapestov · commit 941438d6c812 · 2021-10-21T19:00:10.000-04:00
Previously we said that if P1 inherits from P2, then [P1:T] &lt; [P2:T].

However, this didn't generalize to merged associated type symbols;
we always had [P1&amp;P2:T] &lt; [P3:T] even if P3 inherited from both P1
and P2.

This meant that the 'merge' operation did not preserve order, which
fired an assertion in the completion logic.

Fix this by generalizing the linear order to compare the 'support'
of protocols rather than the 'depth', where the 'support' is
defined as the size of the transitive closure of the set under
protocol inheritance.

Then, if you have something like

  protocol P1 {}
  protocol P2 {}
  protocol P3 : P1, P2 {}

The support of 'P1 &amp; P2' is 2, and the support of 'P3' is 3,
therefore [P3:T] &lt; [P1&amp;P2:T] in this example.

Fixes &lt;rdar://problem/83768458&gt;.
diff --git a/lib/AST/RequirementMachine/ProtocolGraph.cpp b/lib/AST/RequirementMachine/ProtocolGraph.cpp
@@ -49,6 +49,50 @@ const ProtocolInfo &ProtocolGraph::getProtocolInfo(
   return found->second;
 }
 
+/// The "support" of a protocol P is the size of the transitive closure of
+/// the singleton set {P} under protocol inheritance.
+unsigned ProtocolGraph::getProtocolSupport(
+    const ProtocolDecl *proto) const {
+  return getProtocolInfo(proto).AllInherited.size() + 1;
+}
+
+/// The "support" of a set S of protocols is the size of the transitive
+/// closure of S under protocol inheritance. For example, if you start
+/// with
+///
+///   protocol P1 : P3 {}
+///   protocol P2 : P3 {}
+///   protocol P3 {}
+///
+/// Then the "support" of P1 & P2 is 3 because |P1 & P2 & P3| = 3.
+///
+/// The \p protos array must be sorted in canonical order and
+/// permanently-allocated; one safe choice is to use the return value of
+/// Symbol::getProtocols().
+unsigned ProtocolGraph::getProtocolSupport(
+    ArrayRef<const ProtocolDecl *> protos) const {
+  auto found = Support.find(protos);
+  if (found != Support.end())
+    return found->second;
+
+  unsigned result;
+  if (protos.size() == 1) {
+    result = getProtocolSupport(protos[0]);
+  } else {
+    llvm::DenseSet<const ProtocolDecl *> visited;
+    for (const auto *proto : protos) {
+      visited.insert(proto);
+      for (const auto *inheritedProto : getProtocolInfo(proto).AllInherited)
+        visited.insert(inheritedProto);
+    }
+
+    result = visited.size();
+  }
+
+  const_cast<ProtocolGraph *>(this)->Support[protos] = result;
+  return result;
+}
+
 /// Record information about a protocol if we have no seen it yet.
 void ProtocolGraph::addProtocol(const ProtocolDecl *proto,
                                 bool initialComponent) {
@@ -199,17 +243,20 @@ void ProtocolGraph::compute() {
 /// from another protocol Q, then P < Q. (The converse cannot be true, since
 /// this is a linear order.)
 ///
-/// We first compare the 'depth' of a protocol, which is defined in
-/// ProtocolGraph::computeProtocolDepth() above.
+/// We first compare the 'support' of a protocol, which is defined in
+/// ProtocolGraph::getProtocolSupport() above.
 ///
-/// If two protocols have the same depth, the tie is broken by the standard
+/// If two protocols have the same support, the tie is broken by the standard
 /// TypeDecl::compare().
 int ProtocolGraph::compareProtocols(const ProtocolDecl *lhs,
                                     const ProtocolDecl *rhs) const {
-  const auto &infoLHS = getProtocolInfo(lhs);
-  const auto &infoRHS = getProtocolInfo(rhs);
+  unsigned lhsSupport = getProtocolSupport(lhs);
+  unsigned rhsSupport = getProtocolSupport(rhs);
+
+  if (lhsSupport != rhsSupport)
+    return rhsSupport - lhsSupport;
 
-  return infoLHS.Index - infoRHS.Index;
+  return TypeDecl::compare(lhs, rhs);
 }
 
 /// Returns if \p thisProto transitively inherits from \p otherProto.
diff --git a/lib/AST/RequirementMachine/ProtocolGraph.h b/lib/AST/RequirementMachine/ProtocolGraph.h
@@ -88,6 +88,7 @@ struct ProtocolInfo {
 /// Out-of-line methods are documented in ProtocolGraph.cpp.
 class ProtocolGraph {
   llvm::DenseMap<const ProtocolDecl *, ProtocolInfo> Info;
+  llvm::DenseMap<ArrayRef<const ProtocolDecl *>, unsigned> Support;
   std::vector<const ProtocolDecl *> Protocols;
   bool Debug = false;
 
@@ -107,6 +108,12 @@ class ProtocolGraph {
   const ProtocolInfo &getProtocolInfo(
       const ProtocolDecl *proto) const;
 
+  unsigned getProtocolSupport(
+      const ProtocolDecl *proto) const;
+
+  unsigned getProtocolSupport(
+      ArrayRef<const ProtocolDecl *> protos) const;
+
 private:
   void addProtocol(const ProtocolDecl *proto,
                    bool initialComponent);
diff --git a/lib/AST/RequirementMachine/RewriteSystemCompletion.cpp b/lib/AST/RequirementMachine/RewriteSystemCompletion.cpp
@@ -95,7 +95,7 @@ Symbol RewriteContext::mergeAssociatedTypes(Symbol lhs, Symbol rhs,
   auto otherProtos = rhs.getProtocols();
 
   // This must follow from lhs > rhs.
-  assert(protos.size() <= otherProtos.size());
+  assert(graph.getProtocolSupport(protos) <= graph.getProtocolSupport(otherProtos));
 
   // Compute sorted and merged list of protocols, with duplicates.
   llvm::TinyPtrVector<const ProtocolDecl *> newProtos;
diff --git a/lib/AST/RequirementMachine/Symbol.cpp b/lib/AST/RequirementMachine/Symbol.cpp
@@ -460,24 +460,35 @@ ArrayRef<const ProtocolDecl *> Symbol::getRootProtocols() const {
 ///
 /// Then we break ties when both symbols have the same kind as follows:
 ///
-/// * For associated type symbols, we first order the number of protocols,
-///   with symbols containing more protocols coming first. This ensures
-///   that the following holds:
+/// * For associated type symbols, symbols with larger support are smaller
+///   than those with smaller support.
+///
+///   This ensures that if P inherits from Q, then [P:T] < [Q:T].
+///
+///   Furthermore, if P1...Pm and Q1...Qn are two minimal sets of protocols,
+///   this ensures that the following holds, where merge() is the
+///   RewriteContext::mergeAssociatedTypes() operation:
+///
+///     [merge(P1&...&Pm, Q1&...&Qn):T] < [P1&...&Pm:T]
+///     [merge(P1&...&Pm, Q1&...&Qn):T] < [Q1&...&Qn:T]
+///
+///   For example, if P1 and P2 are unrelated protocols, and P3 inherits
+///   both P1 and P2, then
 ///
 ///     [P1&P2:T] < [P1:T]
 ///     [P1&P2:T] < [P2:T]
+///     [P3:T] < [P1&P2:T]
 ///
-///   If both symbols have the same number of protocols, we perform a
-///   lexicographic comparison on the protocols pair-wise, using the
-///   protocol order defined by \p graph (see
-///   ProtocolGraph::compareProtocols()).
+///   If two different lists of protocols have the same support, the tie is
+///   broken by a lexshort comparison on the lists.
 ///
 /// * For generic parameter symbols, we first order by depth, then index.
 ///
 /// * For unbound name symbols, we compare identifiers lexicographically.
 ///
-/// * For protocol symbols, we compare the protocols using the protocol
-///   linear order on \p graph.
+/// * For protocol symbols, protocols with larger support are ordered before
+///   those with smaller support. The type order defined in TypeDecl::compare()
+///   is used to break ties; based on the protocol name and parent module.
 ///
 /// * For layout symbols, we use LayoutConstraint::compare().
 int Symbol::compare(Symbol other, const ProtocolGraph &graph) const {
@@ -509,9 +520,16 @@ int Symbol::compare(Symbol other, const ProtocolGraph &graph) const {
     if (getName() != other.getName())
       return getName().compare(other.getName());
 
-    // Symbols with more protocols are 'smaller' than those with fewer.
+    // Symbols with larger support are *smaller* than those with
+    // smaller support.
+    unsigned support = graph.getProtocolSupport(protos);
+    unsigned otherSupport = graph.getProtocolSupport(otherProtos);
+    if (support != otherSupport)
+      return support > otherSupport ? -1 : 1;
+
+    // Otherwise, perform a shortlex comparison in the protocols.
     if (protos.size() != otherProtos.size())
-      return protos.size() > otherProtos.size() ? -1 : 1;
+      return protos.size() < otherProtos.size() ? -1 : 1;
 
     for (unsigned i : indices(protos)) {
       int result = graph.compareProtocols(protos[i], otherProtos[i]);
diff --git a/test/Generics/rdar83768458.swift b/test/Generics/rdar83768458.swift
@@ -0,0 +1,21 @@
+// RUN: %target-typecheck-verify-swift
+
+public protocol Collection {
+  associatedtype Index
+}
+
+public protocol BidirectionalCollection: Collection {}
+
+public protocol RandomAccessCollection: BidirectionalCollection {}
+
+public protocol Numeric {
+  associatedtype Magnitude
+}
+
+public protocol SignedNumeric: Numeric {}
+
+public protocol BinaryInteger: Numeric {}
+
+public protocol SignedInteger: BinaryInteger, SignedNumeric {}
+
+struct S<X> where X : RandomAccessCollection, X.Index : SignedInteger {}
