This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by SlaterLatiao on May 26 2023, 11:18 AM.

Details

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.

Diff Detail

Event Timeline

SlaterLatiao created this revision.May 26 2023, 11:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2023, 11:18 AM
SlaterLatiao requested review of this revision.May 26 2023, 11:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2023, 11:18 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
SlaterLatiao edited the summary of this revision. (Show Details)May 26 2023, 11:19 AM
SlaterLatiao added reviewers: cjdb, denik.
  • Add newline to end of file.
cjdb added a comment.May 30 2023, 10:59 AM

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

  • Added release note.
SlaterLatiao edited the summary of this revision. (Show Details)May 30 2023, 11:03 AM
denik accepted this revision.May 30 2023, 12:20 PM

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();
  ^
This revision is now accepted and ready to land.May 30 2023, 12:20 PM

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.

Remove unnecessary condition and assignment in implementation.

SlaterLatiao marked an inline comment as done.Jun 15 2023, 1:10 PM

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?)

  • Emit include stack on all top-level diagnostics.
SlaterLatiao retitled this revision from [clang][diagnostics] Always show include stacks on errors to [clang][diagnostics] Always show include stacks on top-level diagnostics.Jul 7 2023, 10:35 AM
SlaterLatiao edited the summary of this revision. (Show Details)

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 added a comment.Aug 23 2023, 2:42 PM

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?

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.