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.
Differential D151575
[clang][diagnostics] Always show include stacks on top-level diagnostics SlaterLatiao on May 26 2023, 11:18 AM. Authored by
Details
Always show include stacks on top-level diagnostics
Diff Detail
Event TimelineComment Actions Thanks for working on this. Would you mind adding more context to the commit description please? Comment Actions 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. 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(); ^ Comment Actions 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.
Comment Actions 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?) Comment Actions 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? Comment Actions 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? Comment Actions
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. |
I'd probably just implement this as
if (Level != DiagnosticsEngine::Error && LastIncludeLoc == IncludeLoc), rather than the 2 + an assignment.