[GSB/Diagnostics] Improve redundant same-type constraint diagnostics

(cherry picked from commit ab3d1f5)

Author: xedin

lib/AST/GenericSignatureBuilder.cpp

@@ -34,6 +34,7 @@
 #include "swift/Basic/Defer.h"
 #include "swift/Basic/Statistic.h"
 #include "llvm/ADT/GraphTraits.h"
+#include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/SmallPtrSet.h"
 #include "llvm/ADT/SmallString.h"
@@ -4894,7 +4895,6 @@ GenericSignatureBuilder::addSameTypeRequirementBetweenTypeParameters(
     std::swap(T1, T2);
     std::swap(OrigT1, OrigT2);
     std::swap(equivClass, equivClass2);
-    std::swap(depType1, depType2);
   }
 
   // T1 must have an equivalence class; create one if we don't already have
@@ -4903,6 +4903,8 @@ GenericSignatureBuilder::addSameTypeRequirementBetweenTypeParameters(
     equivClass = T1->getOrCreateEquivalenceClass(*this);
 
   // Record this same-type constraint.
+  // Let's keep type order in the new constraint the same as it's written
+  // in source, which makes it much easier to diagnose later.
   equivClass->sameTypeConstraints.push_back({depType1, depType2, source});
 
   // Determine the anchor types of the two equivalence classes.
@@ -6905,34 +6907,121 @@ void GenericSignatureBuilder::checkSameTypeConstraints(
   // connect all of the components, or else we wouldn't have an equivalence
   // class.
   if (intercomponentEdges.size() > numComponents - 1) {
-    std::vector<bool> connected(numComponents, false);
-    const auto &firstEdge = intercomponentEdges.front();
-    for (const auto &edge : intercomponentEdges) {
-      // If both the source and target are already connected, this edge is
-      // not part of the spanning tree.
-      if (connected[edge.source] && connected[edge.target]) {
-        if (edge.constraint.source->shouldDiagnoseRedundancy(true) &&
-            firstEdge.constraint.source->shouldDiagnoseRedundancy(false)) {
-          Diags.diagnose(edge.constraint.source->getLoc(),
-                         diag::redundant_same_type_constraint,
-                         edge.constraint.getSubjectDependentType(
-                                                          genericParams),
-                         edge.constraint.value);
-
-          Diags.diagnose(firstEdge.constraint.source->getLoc(),
-                         diag::previous_same_type_constraint,
-                         firstEdge.constraint.source->classifyDiagKind(),
-                         firstEdge.constraint.getSubjectDependentType(
-                                                          genericParams),
-                         firstEdge.constraint.value);
+    auto &sourceMgr = getASTContext().SourceMgr;
+
+    // First let's order all of the intercomponent edges
+    // as written in source, this helps us to diagnose
+    // all of the duplicate constraints in correct order.
+    std::vector<unsigned> sourceOrderedEdges;
+    for (unsigned i : indices(intercomponentEdges))
+      sourceOrderedEdges.push_back(i);
+
+    std::sort(sourceOrderedEdges.begin(), sourceOrderedEdges.end(),
+              [&sourceMgr, &intercomponentEdges](
+                  const unsigned &indexA, const unsigned &indexB) -> bool {
+                auto &a = intercomponentEdges[indexA];
+                auto &b = intercomponentEdges[indexB];
+
+                auto locA = a.constraint.source->getLoc();
+                auto locB = b.constraint.source->getLoc();
+
+                auto bufferA = sourceMgr.findBufferContainingLoc(locA);
+                auto bufferB = sourceMgr.findBufferContainingLoc(locB);
+
+                if (bufferA != bufferB)
+                  return bufferA < bufferB;
+
+                auto offsetA = sourceMgr.getLocOffsetInBuffer(locA, bufferA);
+                auto offsetB = sourceMgr.getLocOffsetInBuffer(locB, bufferB);
+
+                return offsetA < offsetB;
+              });
+
+    auto isDiagnosable = [](const IntercomponentEdge &edge, bool isPrimary) {
+      return edge.constraint.source->shouldDiagnoseRedundancy(isPrimary);
+    };
+
+    using EquivClass = llvm::DenseMap<Type, unsigned>;
+    llvm::DenseMap<Type, EquivClass> equivalences;
+    // The idea here is to form an equivalence class per representative
+    // (picked from each edge constraint in type parameter order) and
+    // propagate all new equivalent types up the chain until duplicate
+    // entry is found, that entry is going to point to previous
+    // declaration and is going to mark current edge as a duplicate of
+    // such entry.
+    for (const unsigned edgeIdx : sourceOrderedEdges) {
+      const auto &edge = intercomponentEdges[edgeIdx];
+
+      Type lhs = edge.constraint.getSubjectDependentType(genericParams);
+      Type rhs = edge.constraint.value;
+
+      // Make sure that representative for equivalence class is picked
+      // in canonical type parameter order.
+      if (compareDependentTypes(rhs, lhs) < 0)
+        std::swap(lhs, rhs);
+
+      // Index of the previous declaration of the same-type constraint
+      // which current edge might be a duplicate of.
+      Optional<unsigned> previousIndex;
+
+      bool isDuplicate = false;
+      auto &representative = equivalences[lhs];
+      if (representative.insert({rhs, edgeIdx}).second) {
+        // Since this is a new equivalence, and right-hand side might
+        // be a representative of some other equivalence class,
+        // its existing members have to be merged up.
+        auto RHSEquivClass = equivalences.find(rhs);
+        if (RHSEquivClass != equivalences.end()) {
+          auto &equivClass = RHSEquivClass->getSecond();
+          representative.insert(equivClass.begin(), equivClass.end());
         }
 
-        continue;
+        // If left-hand side is involved in any other equivalences
+        // let's propagate new information up the chain.
+        for (auto &e : equivalences) {
+          auto &repr = e.first;
+          auto &equivClass = e.second;
+
+          if (repr->isEqual(lhs) || !equivClass.count(lhs))
+            continue;
+
+          if (!equivClass.insert({rhs, edgeIdx}).second) {
+            // Even if "previous" edge is not diagnosable we
+            // still need to produce diagnostic about main duplicate.
+            isDuplicate = true;
+
+            auto prevIdx = equivClass[rhs];
+            if (!isDiagnosable(intercomponentEdges[prevIdx],
+                               /*isPrimary=*/false))
+              continue;
+
+            // If there is a diagnosable duplicate equivalence,
+            // it means that we've found our previous declaration.
+            previousIndex = prevIdx;
+            break;
+          }
+        }
+      } else {
+        // Looks like this is a situation like T.A == T.B, ..., T.B == T.A
+        previousIndex = representative[rhs];
+        isDuplicate = true;
       }
 
-      // Put the source and target into the spanning tree.
-      connected[edge.source] = true;
-      connected[edge.target] = true;
+      if (!isDuplicate || !isDiagnosable(edge, /*isPrimary=*/true))
+        continue;
+
+      Diags.diagnose(edge.constraint.source->getLoc(),
+                     diag::redundant_same_type_constraint,
+                     edge.constraint.getSubjectDependentType(genericParams),
+                     edge.constraint.value);
+
+      if (previousIndex) {
+        auto &prevEquiv = intercomponentEdges[*previousIndex].constraint;
+        Diags.diagnose(
+            prevEquiv.source->getLoc(), diag::previous_same_type_constraint,
+            prevEquiv.source->classifyDiagKind(),
+            prevEquiv.getSubjectDependentType(genericParams), prevEquiv.value);
+      }
     }
   }
 

test/Generics/same_type_constraints.swift

@@ -351,11 +351,11 @@ func intercomponentSameComponents<T: P10>(_: T)
         T.B == T.A { } // expected-note{{previous same-type constraint 'T.B' == 'T.A' written here}}
 
 func intercomponentMoreThanSpanningTree<T: P10>(_: T)
-  where T.A == T.B, // expected-note {{previous same-type constraint 'T.A' == 'T.B' written here}}
+  where T.A == T.B,
         T.B == T.C,
-        T.D == T.E, // expected-warning {{redundant same-type constraint 'T.D' == 'T.E'}}
+        T.D == T.E, // expected-note {{previous same-type constraint 'T.D' == 'T.E' written here}}
         T.D == T.B,
-        T.E == T.B
+        T.E == T.B  // expected-warning {{redundant same-type constraint 'T.E' == 'T.B'}}
         { }
 
 func trivialRedundancy<T: P10>(_: T) where T.A == T.A { } // expected-warning{{redundant same-type constraint 'T.A' == 'T.A'}}