[clang][diagnostics] Always show include stacks on top-level diagnostics

[SlaterLatiao]

Summary

Always show include stacks on top-level diagnostics

• Stops skipping the include stack on top-level diagnostics when it's a duplicate of the previous stack.
• Fixes PR#62001.

[SlaterLatiao]

• Add newline to end of file.

[cjdb]

Thanks for working on this. Would you mind adding more context to the commit description please?

[SlaterLatiao]

• Added release note.

[denik]

As long as having In file included from on each error without notes from one include file is fine, I don't see any problem with this. LGTM.
But let's check what others think about it.

For example:

// 1.h
int f() {
  b1();
  b2();
  b3();
}

// 1.c
#include "1.h"

will have

In file included from 1.c:1:
./1.h:3:3: error: use of undeclared identifier 'b1'
  b1();
  ^
In file included from 1.c:1:
./1.h:4:3: error: use of undeclared identifier 'b2'
  b2();
  ^
In file included from 1.c:1:
./1.h:5:3: error: use of undeclared identifier 'b3'
  b3();
  ^

[cjdb]

@aaron.ballman @erichkeane @shafik any reservations?

[erichkeane]

I don't have much of an opinion on the commit itself. It seems that suppressing that include stack does at least SOME work to make our error-novels less 'War and Peace', but I don't really get the bug report well enough to know whether we should be doing this.

clang/lib/Frontend/DiagnosticRenderer.cpp

170–171

I'd probably just implement this as

if (Level != DiagnosticsEngine::Error && LastIncludeLoc == IncludeLoc), rather than the 2 + an assignment.

[SlaterLatiao]

Remove unnecessary condition and assignment in implementation.

[dblaikie]

Judging by @aaron.ballman's comment on the bug, I can see the logic here.

Though perhaps this notion generalizes to all non-note types, not just errors (like perhaps we should print the include stack for each warning, too?)

[SlaterLatiao]

• Emit include stack on all top-level diagnostics.

[aaron.ballman]

I am wrestling with this one because I think the status quo is unfortunate (we silently drop relevant information in ways the user may not immediately understand) but I think always printing the include stack could be verbose and thus make the diagnostics harder to act on. If we had the ability to hyperlink to other parts of the diagnostic output, then I'd ask if we could perhaps print the full stack once and link to it from anywhere else it's being used, but I can't think of an effective way to do that purely in text. We could also add an explicit option to let the user control the behavior, but I'd prefer to avoid that if possible as the combination of modes makes testing a challenge.

Maybe we should consider adding a flag to set diagnostic verbosity level? e.g., -fdiagnostic-verbosity=terse|default|verbose where terse never prints notes or include stacks, default is what we do today, and verbose always prints include stacks?

[cjdb]

In D151575#4607047, @aaron.ballman wrote:

I am wrestling with this one because I think the status quo is unfortunate (we silently drop relevant information in ways the user may not immediately understand) but I think always printing the include stack could be verbose and thus make the diagnostics harder to act on. If we had the ability to hyperlink to other parts of the diagnostic output, then I'd ask if we could perhaps print the full stack once and link to it from anywhere else it's being used, but I can't think of an effective way to do that purely in text. We could also add an explicit option to let the user control the behavior, but I'd prefer to avoid that if possible as the combination of modes makes testing a challenge.

Maybe we should consider adding a flag to set diagnostic verbosity level? e.g., -fdiagnostic-verbosity=terse|default|verbose where terse never prints notes or include stacks, default is what we do today, and verbose always prints include stacks?

I'm not fond of terse unless each note is printed once: I think that'll get misused and lead to a lot of frustration (most notes are genuinely helpful). There's a lot of design space here, but I don't think any of it can be achieved until we have nested diagnostics, but that requires a complete rewrite of the diagnostics engine.

default and verbose might be interesting though?

[dblaikie]

Maybe we should consider adding a flag to set diagnostic verbosity level? e.g., -fdiagnostic-verbosity=terse|default|verbose where terse never prints notes or include stacks, default is what we do today, and verbose always prints include stacks?

My guess would be that not enough people would discover and use it, so we shouldn't do that.

I guess a basic question: what does GCC do about these "included from" stacks compared to clang?

Also it seemed like on the bug there was a more narrower issue being discussed - that the notes were missing from a second error because it had the same include stack as a note attached to a previous error, but not the same include stack as the previous error (which is the last thing with the include stack mentioned)? I may've missed the point where it generalized from that particular bug to "let's put the include stack always instead" rather than addressing that mismatch of looking at the last message in general, rather than the last non-note message (I guess the include stack is only printed for non-notes, by the looks of the bug example? So those are the ones that should be cached/checked against)

But equally, I suspect it's probably OK to do it unconditionally... probably.

clang/docs/ReleaseNotes.rst

- When diagnosing a constant expression where an enum without a fixed underlying
   type is set to a value outside the range of the enum's values, clang will now
   print the name of the enum in question.
 - Clang no longer diagnoses a read of an empty structure as use of an
   uninitialized variable.
   (`#26842: <https://github.com/llvm/llvm-project/issues/26842>`_)
 - The Fix-It emitted for unused labels used to expand to the next line, which caused
   visual oddities now that Clang shows more than one line of code snippet. This has
   been fixed and the Fix-It now only spans to the end of the ``:``.
+- Clang now does not skip the include stack on top-level diagnostics when it's a 
+  duplicate of the previous include stack.
+  (`#62001 <https://github.com/llvm/llvm-project/issues/62001>`_)
 
 Bug Fixes in This Version
 -------------------------
 
 - Fix segfault while running clang-rename on a non existing file.
   (`#36471 <https://github.com/llvm/llvm-project/issues/36471>`_)
 - Fix crash when diagnosing incorrect usage of ``_Nullable`` involving alias
   templates.

clang/lib/Frontend/DiagnosticRenderer.cpp

 /// \param PLoc  The presumed location of the diagnostic location.
 /// \param Level The diagnostic level of the message this stack pertains to.
 void DiagnosticRenderer::emitIncludeStack(FullSourceLoc Loc, PresumedLoc PLoc,
                                           DiagnosticsEngine::Level Level) {
   FullSourceLoc IncludeLoc =
       PLoc.isInvalid() ? FullSourceLoc()
                        : FullSourceLoc(PLoc.getIncludeLoc(), Loc.getManager());
 
-  // Skip redundant include stacks altogether.
-  if (LastIncludeLoc == IncludeLoc)
+  // Skip redundant include stacks altogether on non-top-level.
+  if (Level < DiagnosticsEngine::Warning && LastIncludeLoc == IncludeLoc)
     return;
 
   LastIncludeLoc = IncludeLoc;
 
   if (!DiagOpts->ShowNoteIncludeStack && Level == DiagnosticsEngine::Note)
     return;
 
   if (IncludeLoc.isValid())

clang/test/Misc/Inputs/include-stack-on-error-1.h

+void b1();
+template <class T> void b2(T x, T y) { x + y; }

clang/test/Misc/Inputs/include-stack-on-error-2.h

+void c() {
+    b1(0);
+    b2("0", "0");
+}

clang/test/Misc/Inputs/include-stack-on-error-3.h

+void b1();
+void c() {
+    b1(0);
+    b2("0", "0");
+}

clang/test/Misc/include-stack-on-error-1.cpp

+// Test duplicate include stacks on a new error are not skipped.
+// RUN: not %clang_cc1 -fsyntax-only -I %S/Inputs %s 2>&1 | FileCheck %s -check-prefix=STACK
+// RUN: not %clang_cc1 -fsyntax-only -fdiagnostics-show-note-include-stack -I %S/Inputs %s 2>&1 | FileCheck %s -check-prefix=NOTESTACK
+
+#include "include-stack-on-error-1.h"
+#include "include-stack-on-error-2.h"
+
+// STACK:  In file included from
+// STACK: error: no matching function for call to 'b1'
+// STACK-NOT:  In file included from
+// STACK: note: candidate function not viable
+// STACK:  In file included from
+// STACK: error: invalid operands to binary expression
+// STACK-NOT:  In file included from
+// STACK: note: in instantiation of function template specialization
+
+// NOTESTACK:  In file included from
+// NOTESTACK: error: no matching function for call to 'b1'
+// NOTESTACK:  In file included from
+// NOTESTACK: note: candidate function not viable
+// NOTESTACK:  In file included from
+// NOTESTACK: error: invalid operands to binary expression
+// NOTESTACK:  In file included from
+// NOTESTACK: note: in instantiation of function template specialization

clang/test/Misc/include-stack-on-error-2.cpp

+// Test duplicate include stacks that are not on a new error are skipped.
+// RUN: not %clang_cc1 -fsyntax-only -fdiagnostics-show-note-include-stack -I %S/Inputs %s 2>&1 | FileCheck %s -check-prefix=NOTESTACK
+
+#include "include-stack-on-error-1.h"
+#include "include-stack-on-error-3.h"
+
+// NOTESTACK:  In file included from
+// NOTESTACK: error: no matching function for call to 'b1'
+// This include should be skipped due to duplicate include location.
+// NOTESTACK-NOT:  In file included from
+// NOTESTACK: note: candidate function not viable
+// NOTESTACK:  In file included from
+// NOTESTACK: error: invalid operands to binary expression
+// NOTESTACK:  In file included from
+// NOTESTACK: note: in instantiation of function template specialization