Merge pull request #84698 from hamishknight/carousel

[Evaluator] Avoid emitting duplicate "through reference here" notes

Author: hamishknight

include/swift/AST/DiagnosticEngine.h

@@ -217,6 +217,9 @@ namespace swift {
     const class Decl *getDecl() const { return Decl; }
     DiagnosticBehavior getBehaviorLimit() const { return BehaviorLimit; }
 
+    /// Retrieve the stored SourceLoc, or the location of the stored Decl.
+    SourceLoc getLocOrDeclLoc() const;
+
     void setLoc(SourceLoc loc) { Loc = loc; }
     void setIsChildNote(bool isChildNote) { IsChildNote = isChildNote; }
     void setDecl(const class Decl *decl) { Decl = decl; }
@@ -1473,6 +1476,13 @@ namespace swift {
     /// Retrieve the underlying engine which will receive the diagnostics.
     DiagnosticEngine &getUnderlyingDiags() const { return UnderlyingEngine; }
 
+    /// Iterates over each captured diagnostic, running a lambda with it.
+    void forEach(llvm::function_ref<void(const Diagnostic &)> body) const;
+
+    /// Filters the queued diagnostics, dropping any where the predicate
+    /// returns \c false.
+    void filter(llvm::function_ref<bool(const Diagnostic &)> predicate);
+
     /// Clear this queue and erase all diagnostics recorded.
     void clear() {
       assert(QueueEngine.TransactionCount == 1 &&

include/swift/AST/DiagnosticsCommon.def

@@ -27,6 +27,8 @@ NOTE(kind_declname_declared_here,none,
      "%0 %1 declared here", (DescriptiveDeclKind, DeclName))
 NOTE(decl_declared_here_with_kind,none,
      "%kind0 declared here", (const Decl *))
+NOTE(through_decl_declared_here_with_kind,none,
+     "through %kind0 declared here", (const Decl *))
 
 ERROR(not_implemented,none,
       "INTERNAL ERROR: feature not implemented: %0", (StringRef))

lib/AST/DiagnosticEngine.cpp

@@ -177,6 +177,16 @@ std::optional<const DiagnosticInfo *> Diagnostic::getWrappedDiagnostic() const {
   return std::nullopt;
 }
 
+SourceLoc Diagnostic::getLocOrDeclLoc() const {
+  if (auto loc = getLoc())
+    return loc;
+
+  if (auto *D = getDecl())
+    return D->getLoc();
+
+  return SourceLoc();
+}
+
 static CharSourceRange toCharSourceRange(SourceManager &SM, SourceRange SR) {
   return CharSourceRange(SM, SR.Start, Lexer::getLocForEndOfToken(SM, SR.End));
 }
@@ -1438,24 +1448,18 @@ DiagnosticEngine::diagnosticInfoForDiagnostic(const Diagnostic &diagnostic,
     return std::nullopt;
 
   // Figure out the source location.
-  SourceLoc loc = diagnostic.getLoc();
+  SourceLoc loc = diagnostic.getLocOrDeclLoc();
   if (loc.isInvalid() && diagnostic.getDecl()) {
+    // If the location of the decl is invalid, try to pretty-print it into a
+    // buffer and capture the source location there. Make sure we don't have an
+    // active request running since printing AST can kick requests that may
+    // themselves emit diagnostics. This won't help the underlying cycle, but it
+    // at least stops us from overflowing the stack.
     const Decl *decl = diagnostic.getDecl();
-    // If a declaration was provided instead of a location, and that declaration
-    // has a location we can point to, use that location.
-    loc = decl->getLoc();
-
-    // If the location of the decl is invalid still, try to pretty-print the
-    // declaration into a buffer and capture the source location there. Make
-    // sure we don't have an active request running since printing AST can
-    // kick requests that may themselves emit diagnostics. This won't help the
-    // underlying cycle, but it at least stops us from overflowing the stack.
-    if (loc.isInvalid()) {
-      PrettyPrintDeclRequest req(decl);
-      auto &eval = decl->getASTContext().evaluator;
-      if (!eval.hasActiveRequest(req))
-        loc = evaluateOrDefault(eval, req, SourceLoc());
-    }
+    PrettyPrintDeclRequest req(decl);
+    auto &eval = decl->getASTContext().evaluator;
+    if (!eval.hasActiveRequest(req))
+      loc = evaluateOrDefault(eval, req, SourceLoc());
   }
 
   auto groupID = diagnostic.getGroupID();
@@ -1785,6 +1789,20 @@ void DiagnosticEngine::onTentativeDiagnosticFlush(Diagnostic &diagnostic) {
   }
 }
 
+void DiagnosticQueue::forEach(
+    llvm::function_ref<void(const Diagnostic &)> body) const {
+  for (auto &activeDiag : QueueEngine.TentativeDiagnostics)
+    body(activeDiag.Diag);
+}
+
+void DiagnosticQueue::filter(
+    llvm::function_ref<bool(const Diagnostic &)> predicate) {
+  llvm::erase_if(QueueEngine.TentativeDiagnostics,
+                 [&](detail::ActiveDiagnostic &activeDiag) {
+                   return !predicate(activeDiag.Diag);
+                 });
+}
+
 EncodedDiagnosticMessage::EncodedDiagnosticMessage(StringRef S)
     : Message(Lexer::getEncodedStringSegment(S, Buf, /*IsFirstSegment=*/true,
                                              /*IsLastSegment=*/true,

lib/AST/Evaluator.cpp

@@ -19,6 +19,7 @@
 #include "swift/AST/DiagnosticEngine.h"
 #include "swift/AST/TypeCheckRequests.h" // for ResolveMacroRequest
 #include "swift/Basic/Assertions.h"
+#include "swift/Basic/Defer.h"
 #include "swift/Basic/LangOptions.h"
 #include "swift/Basic/Range.h"
 #include "swift/Basic/SourceManager.h"
@@ -123,17 +124,40 @@ void Evaluator::diagnoseCycle(const ActiveRequest &request) {
     OS << "\n";
   }
 
-  request.diagnoseCycle(diags);
-  for (const auto &step : llvm::reverse(activeRequests)) {
-    if (step == request) return;
-
-    // Reporting the lifetime dependence location generates a redundant
-    // diagnostic.
-    if (step.getAs<LifetimeDependenceInfoRequest>()) {
-      continue;
-    }
+  // Perform some filtering to avoid emitting duplicate 'through reference here'
+  // notes.
+  DiagnosticQueue cycleDiags(diags, /*emitOnDestruction*/ true);
+  SWIFT_DEFER {
+    auto isDefaultStepNote = [](const Diagnostic &diag) -> bool {
+      return diag.getID() == diag::circular_reference_through.ID;
+    };
+    // First populate seen locs for everything except default step notes. If
+    // we have a diagnostic or custom note at a given location we want to prefer
+    // that over the default step note.
+    llvm::DenseSet<SourceLoc> seenLocs;
+    cycleDiags.forEach([&](const Diagnostic &diag) {
+      if (isDefaultStepNote(diag))
+        return;
+
+      if (auto loc = diag.getLocOrDeclLoc())
+        seenLocs.insert(loc);
+    });
+    // Then we can filter out unnecessary default step notes.
+    cycleDiags.filter([&](const Diagnostic &diag) -> bool {
+      if (!isDefaultStepNote(diag))
+        return true;
+
+      auto loc = diag.getLocOrDeclLoc();
+      return loc && seenLocs.insert(loc).second;
+    });
+  };
+
+  request.diagnoseCycle(cycleDiags.getDiags());
 
-    step.noteCycleStep(diags);
+  for (const auto &step : llvm::reverse(activeRequests)) {
+    if (step == request)
+      return;
+    step.noteCycleStep(cycleDiags.getDiags());
   }
 
   llvm_unreachable("Diagnosed a cycle but it wasn't represented in the stack");

lib/AST/NameLookupRequests.cpp

@@ -58,7 +58,7 @@ void SuperclassDeclRequest::diagnoseCycle(DiagnosticEngine &diags) const {
 
 void SuperclassDeclRequest::noteCycleStep(DiagnosticEngine &diags) const {
   auto decl = std::get<0>(getStorage());
-  diags.diagnose(decl, diag::decl_declared_here_with_kind, decl);
+  diags.diagnose(decl, diag::through_decl_declared_here_with_kind, decl);
 }
 
 std::optional<ClassDecl *> SuperclassDeclRequest::getCachedResult() const {

lib/AST/TypeCheckRequests.cpp

@@ -217,7 +217,7 @@ void EnumRawTypeRequest::diagnoseCycle(DiagnosticEngine &diags) const {
 
 void EnumRawTypeRequest::noteCycleStep(DiagnosticEngine &diags) const {
   auto *decl = std::get<0>(getStorage());
-  diags.diagnose(decl, diag::decl_declared_here_with_kind, decl);
+  diags.diagnose(decl, diag::through_decl_declared_here_with_kind, decl);
 }
 
 //----------------------------------------------------------------------------//
@@ -248,7 +248,7 @@ void ProtocolRequiresClassRequest::diagnoseCycle(DiagnosticEngine &diags) const
 
 void ProtocolRequiresClassRequest::noteCycleStep(DiagnosticEngine &diags) const {
   auto *proto = std::get<0>(getStorage());
-  diags.diagnose(proto, diag::decl_declared_here_with_kind, proto);
+  diags.diagnose(proto, diag::through_decl_declared_here_with_kind, proto);
 }
 
 std::optional<bool> ProtocolRequiresClassRequest::getCachedResult() const {
@@ -272,7 +272,7 @@ void ExistentialConformsToSelfRequest::diagnoseCycle(DiagnosticEngine &diags) co
 
 void ExistentialConformsToSelfRequest::noteCycleStep(DiagnosticEngine &diags) const {
   auto *proto = std::get<0>(getStorage());
-  diags.diagnose(proto, diag::decl_declared_here_with_kind, proto);
+  diags.diagnose(proto, diag::through_decl_declared_here_with_kind, proto);
 }
 
 std::optional<bool> ExistentialConformsToSelfRequest::getCachedResult() const {
@@ -298,7 +298,7 @@ void HasSelfOrAssociatedTypeRequirementsRequest::diagnoseCycle(
 void HasSelfOrAssociatedTypeRequirementsRequest::noteCycleStep(
     DiagnosticEngine &diags) const {
   auto *proto = std::get<0>(getStorage());
-  diags.diagnose(proto, diag::decl_declared_here_with_kind, proto);
+  diags.diagnose(proto, diag::through_decl_declared_here_with_kind, proto);
 }
 
 std::optional<bool>
@@ -1448,7 +1448,7 @@ void HasCircularInheritedProtocolsRequest::diagnoseCycle(
 void HasCircularInheritedProtocolsRequest::noteCycleStep(
     DiagnosticEngine &diags) const {
   auto *decl = std::get<0>(getStorage());
-  diags.diagnose(decl, diag::decl_declared_here_with_kind, decl);
+  diags.diagnose(decl, diag::through_decl_declared_here_with_kind, decl);
 }
 
 //----------------------------------------------------------------------------//
@@ -1462,7 +1462,7 @@ void HasCircularRawValueRequest::diagnoseCycle(DiagnosticEngine &diags) const {
 
 void HasCircularRawValueRequest::noteCycleStep(DiagnosticEngine &diags) const {
   auto *decl = std::get<0>(getStorage());
-  diags.diagnose(decl, diag::decl_declared_here_with_kind, decl);
+  diags.diagnose(decl, diag::through_decl_declared_here_with_kind, decl);
 }
 
 //----------------------------------------------------------------------------//

test/CircularReferences/global_typealias.swift

@@ -1,6 +1,6 @@
 // RUN: %target-typecheck-verify-swift
 
-typealias A = B // expected-error {{type alias 'A' references itself}} expected-note {{while resolving type 'B'}} expected-note {{through reference here}}
-typealias C = D // expected-note {{through reference here}} expected-note {{while resolving type 'D'}} expected-note {{through reference here}}
-typealias D = (A, Int) // expected-note {{through reference here}} expected-note {{while resolving type '(A, Int)'}} expected-note {{through reference here}}
-typealias B = C // expected-note {{through reference here}} expected-note {{while resolving type 'C'}} expected-note {{through reference here}}
+typealias A = B // expected-error {{type alias 'A' references itself}} expected-note {{while resolving type 'B'}}
+typealias C = D // expected-note {{through reference here}} expected-note {{while resolving type 'D'}}
+typealias D = (A, Int) // expected-note {{through reference here}} expected-note {{while resolving type '(A, Int)'}}
+typealias B = C // expected-note {{through reference here}} expected-note {{while resolving type 'C'}}

test/Generics/issue-69318.swift

@@ -15,6 +15,7 @@ public protocol ChildlessView: View where NodeChildren == EmptyViewGraphNodeChil
 // expected-error@-1 {{circular reference}}
 
 public struct EmptyViewGraphNodeChildren<ParentView: ChildlessView>: ViewGraphNodeChildren {}
-// expected-note@-1 3{{through reference here}}
-// expected-error@-2 {{type 'EmptyViewGraphNodeChildren<ParentView>' does not conform to protocol 'ViewGraphNodeChildren'}}
-// expected-note@-3 {{add stubs for conformance}}
+// expected-note@-1:15 {{through reference here}}
+// expected-note@-2:70 {{through reference here}}
+// expected-error@-3 {{type 'EmptyViewGraphNodeChildren<ParentView>' does not conform to protocol 'ViewGraphNodeChildren'}}
+// expected-note@-4 {{add stubs for conformance}}

test/Generics/rdar123013710.swift

@@ -8,9 +8,9 @@ public protocol P {
     static func f() -> Any?
 }
 
-protocol Q: P where B == Never {} // expected-error {{circular reference}}
+protocol Q: P where B == Never {} // expected-error@:10 {{circular reference}}
 
-extension Never: Q, P { // expected-note 2{{through reference here}}
+extension Never: Q, P { // expected-note@:1 {{through reference here}}
    public typealias A = Never
    public static func f() -> Any? { nil }
 }

test/Generics/rdar94848868.swift

@@ -3,12 +3,13 @@
 // This is too circular to work, but it shouldn't crash.
 
 protocol MyCollectionProtocol: Collection where Iterator == MyCollectionIterator<Self> {}
-// expected-error@-1 {{circular reference}}
+// expected-error@-1:10 {{circular reference}}
 
 struct MyCollectionIterator<MyCollection: MyCollectionProtocol>: IteratorProtocol {
-// expected-note@-1 3{{through reference here}}
-// expected-error@-2 {{type 'MyCollectionIterator<MyCollection>' does not conform to protocol 'IteratorProtocol'}}
-// expected-note@-3 {{add stubs for conformance}}
+  // expected-note@-1:8 {{through reference here}}
+  // expected-note@-2:66 {{through reference here}}
+  // expected-error@-3 {{type 'MyCollectionIterator<MyCollection>' does not conform to protocol 'IteratorProtocol'}}
+  // expected-note@-4 {{add stubs for conformance}}
     mutating func next() -> MyCollection.Element? {
     // expected-error@-1 {{'Element' is not a member type of type 'MyCollection'}}
         return nil