GSB: Use redundant requirements graph to diagnose redundant conformance requirements

This almost completely guts checkConformanceConstraints(), and
removes a usage of checkConstraintList().

Yet another one of those refactorings that leaves the GSB in an
intermediate state, with some new logic that's cleaner, while
leaving behind old code for other use cases that haven't been
refactored yet.

Author: slavapestov

include/swift/AST/GenericSignatureBuilder.h

@@ -646,6 +646,8 @@ class GenericSignatureBuilder {
 private:
   void computeRedundantRequirements();
 
+  void diagnoseRedundantRequirements() const;
+
   bool hasExplicitConformancesImpliedByConcrete() const;
 
   /// Describes the relationship between a given constraint and

lib/AST/GenericSignatureBuilder.cpp

@@ -5594,6 +5594,29 @@ static void expandSameTypeConstraints(GenericSignatureBuilder &builder,
   }
 }
 
+static bool compareSourceLocs(SourceManager &SM, SourceLoc lhsLoc, SourceLoc rhsLoc) {
+  if (lhsLoc.isValid() != rhsLoc.isValid())
+    return lhsLoc.isValid();
+
+  if (lhsLoc.isValid() && rhsLoc.isValid()) {
+    unsigned lhsBuffer = SM.findBufferContainingLoc(lhsLoc);
+    unsigned rhsBuffer = SM.findBufferContainingLoc(rhsLoc);
+
+    // If the buffers are the same, use source location ordering.
+    if (lhsBuffer == rhsBuffer) {
+      if (SM.isBeforeInBuffer(lhsLoc, rhsLoc))
+        return true;
+    } else {
+      // Otherwise, order by buffer identifier.
+      if (SM.getIdentifierForBuffer(lhsBuffer)
+               .compare(SM.getIdentifierForBuffer(lhsBuffer)))
+        return true;
+    }
+  }
+
+  return false;
+}
+
 /// A directed graph where the vertices are explicit requirement sources and
 /// an edge (v, w) records that the explicit source v implies the explicit
 /// source w.
@@ -5992,29 +6015,10 @@ class RedundantRequirementGraph {
     if (cmpTypes != 0)
       return cmpTypes < 0;
 
-    // Prefer source-location-based over abstract.
     auto existingLoc = existingSource->getLoc();
     auto currentLoc = currentSource->getLoc();
-    if (existingLoc.isValid() != currentLoc.isValid())
-      return currentLoc.isValid();
-
-    if (existingLoc.isValid() && currentLoc.isValid()) {
-      unsigned existingBuffer = SM.findBufferContainingLoc(existingLoc);
-      unsigned currentBuffer = SM.findBufferContainingLoc(currentLoc);
 
-      // If the buffers are the same, use source location ordering.
-      if (existingBuffer == currentBuffer) {
-        if (SM.isBeforeInBuffer(currentLoc, existingLoc))
-          return true;
-      } else {
-        // Otherwise, order by buffer identifier.
-        if (SM.getIdentifierForBuffer(currentBuffer)
-                 .compare(SM.getIdentifierForBuffer(existingBuffer)))
-          return true;
-      }
-    }
-
-    return true;
+    return compareSourceLocs(SM, currentLoc, existingLoc);
   }
 
 public:
@@ -6226,6 +6230,7 @@ GenericSignatureBuilder::finalize(TypeArrayView<GenericTypeParamType> genericPar
   processDelayedRequirements();
 
   computeRedundantRequirements();
+  diagnoseRedundantRequirements();
 
   assert(!Impl->finalized && "Already finalized builder");
 #ifndef NDEBUG
@@ -6864,41 +6869,95 @@ void GenericSignatureBuilder::checkConformanceConstraints(
 
     // Remove any self-derived constraints.
     removeSelfDerived(*this, entry.second, entry.first);
+  }
+}
 
-    checkConstraintList<ProtocolDecl *, ProtocolDecl *>(
-      genericParams, entry.second, RequirementKind::Conformance,
-      [](const Constraint<ProtocolDecl *> &constraint) {
-        return true;
-      },
-      [&](const Constraint<ProtocolDecl *> &constraint) {
-        auto proto = constraint.value;
-        assert(proto == entry.first && "Mixed up protocol constraints");
+void GenericSignatureBuilder::diagnoseRedundantRequirements() const {
+  SmallVector<ExplicitRequirement, 2> redundantRequirements;
+
+  for (auto pair : Impl->RedundantRequirements) {
+    auto *source = pair.first.getSource();
+
+    // Don't diagnose anything without a source location.
+    if (source->getLoc().isInvalid())
+      continue;
+
+    // Don't diagnose redundant inferred requirements.
+    if (source->isInferredRequirement())
+      continue;
+
+    // Don't diagnose explicit requirements that are implied by
+    // inferred requirements.
+    if (llvm::all_of(pair.second,
+                     [&](const ExplicitRequirement &otherReq) {
+                       return otherReq.getSource()->isInferredRequirement();
+                     }))
+      continue;
 
-        // If this conformance requirement recursively makes a protocol
-        // conform to itself, don't complain here.
-        auto source = constraint.source;
+    redundantRequirements.push_back(pair.first);
+  }
+
+  auto &SM = Context.SourceMgr;
+
+  std::sort(redundantRequirements.begin(), redundantRequirements.end(),
+            [&](ExplicitRequirement lhs, ExplicitRequirement rhs) {
+              return compareSourceLocs(SM,
+                                       lhs.getSource()->getLoc(),
+                                       rhs.getSource()->getLoc());
+            });
+
+  for (const auto &req : redundantRequirements) {
+    auto *source = req.getSource();
+    auto loc = source->getLoc();
+    assert(loc.isValid());
+
+    auto subjectType = getSugaredDependentType(source->getStoredType(),
+                                               getGenericParams());
+
+    switch (req.getKind()) {
+    case RequirementKind::Conformance: {
+      auto *proto = req.getRHS().get<ProtocolDecl *>();
+
+      // If this conformance requirement recursively makes a protocol
+      // conform to itself, don't complain here, because we diagnose
+      // the circular inheritance elsewhere.
+      {
         auto rootSource = source->getRoot();
-        if (rootSource->kind == RequirementSource::RequirementSignatureSelf &&
-            source != rootSource &&
-            proto == rootSource->getProtocolDecl() &&
-            areInSameEquivalenceClass(rootSource->getRootType(),
-                                      source->getAffectedType())) {
-          return ConstraintRelation::Unrelated;
+        if (proto == rootSource->getProtocolDecl() &&
+            rootSource->kind == RequirementSource::RequirementSignatureSelf &&
+            rootSource->getRootType()->isEqual(source->getAffectedType())) {
+          break;
         }
+      }
 
-        // If this is a redundantly inherited Objective-C protocol, treat it
-        // as "unrelated" to silence the warning about the redundant
-        // conformance.
-        if (isRedundantlyInheritableObjCProtocol(proto, constraint.source))
-          return ConstraintRelation::Unrelated;
+      // If this is a redundantly inherited Objective-C protocol, treat it
+      // as "unrelated" to silence the warning about the redundant
+      // conformance.
+      if (isRedundantlyInheritableObjCProtocol(proto, source))
+        break;
 
-        return ConstraintRelation::Redundant;
-      },
-      None,
-      diag::redundant_conformance_constraint,
-      diag::redundant_conformance_here,
-      [](ProtocolDecl *proto) { return proto; },
-      /*removeSelfDerived=*/false);
+      Context.Diags.diagnose(loc, diag::redundant_conformance_constraint,
+                             subjectType, proto);
+
+      for (auto otherReq : Impl->RedundantRequirements[req]) {
+        auto *otherSource = otherReq.getSource();
+        auto otherLoc = otherSource->getLoc();
+        if (otherLoc.isInvalid())
+          continue;
+
+        Context.Diags.diagnose(otherLoc, diag::redundant_conformance_here,
+                               1, subjectType, proto);
+      }
+
+      break;
+    }
+
+    case RequirementKind::Superclass:
+    case RequirementKind::Layout:
+    case RequirementKind::SameType:
+      // TODO
+      break;
+    }
   }
 }
 

test/Generics/requirement_inference.swift

@@ -114,8 +114,8 @@ func inferSameType1<T, U>(_ x: Model_P3_P4_Eq<T, U>) {
 }
 
 func inferSameType2<T : P3, U : P4>(_: T, _: U) where U.P4Assoc : P2, T.P3Assoc == U.P4Assoc {}
-// expected-warning@-1{{redundant conformance constraint 'T.P3Assoc': 'P2'}}
-// expected-note@-2{{conformance constraint 'T.P3Assoc': 'P2' implied here}}
+// expected-warning@-1{{redundant conformance constraint 'U.P4Assoc': 'P2'}}
+// expected-note@-2{{conformance constraint 'U.P4Assoc': 'P2' implied here}}
 
 func inferSameType3<T : PCommonAssoc1>(_: T) where T.CommonAssoc : P1, T : PCommonAssoc2 {
 }
@@ -134,8 +134,8 @@ protocol P7 : P6 {
 
 // CHECK-LABEL: P7@
 // CHECK: Canonical generic signature: <τ_0_0 where τ_0_0 : P7, τ_0_0.AssocP6.Element : P6, τ_0_0.AssocP6.Element == τ_0_0.AssocP7.AssocP6.Element>
-extension P7 where AssocP6.Element : P6, // expected-note{{conformance constraint 'Self.AssocP6.Element': 'P6' written here}}
-        AssocP7.AssocP6.Element : P6, // expected-warning{{redundant conformance constraint 'Self.AssocP6.Element': 'P6'}}
+extension P7 where AssocP6.Element : P6, // expected-note{{conformance constraint 'Self.AssocP7.AssocP6.Element': 'P6' implied here}}
+        AssocP7.AssocP6.Element : P6, // expected-warning{{redundant conformance constraint 'Self.AssocP7.AssocP6.Element': 'P6'}}
         AssocP6.Element == AssocP7.AssocP6.Element {
   func nestedSameType1() { }
 }