Introduction
Our tooling emits diagnostics on-the-fly, which makes it extremely difficult (if not impossible) to move the toolchain towards the proposed new diagnostic model. In the context of Clang, this means that it’s: difficult for users to see where one diagnostic (or part thereof) stops and another begins; impossible for SARIF diagnostics to engage in progressive disclosure; and occasionally interleaves completely separate diagnostics by accident. This proposal explores a solution resolving all three problems.
Acknowledgements
A huge thanks to @denik, @dblaikie, @AaronBallman, and @zygoloid for reviewing this proposal ahead of time.
Motivation
There are two kinds of diagnostics: top-level diagnostics that identify a problem in the source (errors, warnings, and remarks), and nested diagnostics that augment top-level diagnostics (presently just notes and fix-it hints). Clang currently does not acknowledge this hierarchy, and treats everything as a top-level diagnostic, which is really simple: the compiler gathers the relevant information and then passes it on to the user. Clang has largely gotten away with this, but a truly robust diagnostics model needs to structure the representation so that it is clear where one diagnostic stops and the next one starts. This will impact both the unstructured text diagnostics that we emit today, and the resulting diagnostics from the structured model that we are implementing over time.
Improve the structure without changing it
Consider the following program that’s ill-formed because it doesn’t have an appropriate selection for std::ranges::begin.
#include <ranges>
struct S {
void begin();
void end();
};
int main()
{
S s;
std::ranges::begin(s);
}
Clang currently produces the diagnostics as a simple listing, without regard for context. It is difficult to see how notes correspond to one another: the second note is a property of the candidate template ignored above it, but it can easily be mistaken as a property of the error that it sits under. Diagnostics with recursive contexts can improve the readability of the diagnostic, making it easier to identify bits of the context that matter to the reader. For example, the notes nested under the second tend to become noise as one becomes familiar with ranges, and it will be easier to mentally filter out anything nested under __member_begin over time, than it is when they’re all aligned together.
The text presented doesn’t necessarily mean that we would output this for the unstructured text model: indenting text that’s intended for the console is likely to make matters worse than present, although we could emit something fixed that at least tells the reader what “level” they’re reading. The more interesting case is the support for SARIF. The current SARIF specification doesn’t allow us to nest notes and fix-it hints inside one another, and so diagnostics are rendered similarly to the above by a diagnostic consumer (although they are collapsible in VS Code). There’s currently a SARIF work item to permit diagnostics to form a hierarchy so that it is possible for notes to only be presented as a part of progressive disclosure, instead of always being present.
Diagnostics will never appear out-of-order
Although the grouping of diagnostics mostly benefits the structuring as described above, Clang’s emit-and-forget model causes some diagnostics to appear out-of-order, and thus confuse readers. Richard Smith notes that this category of bugs tends to be fixed fairly quickly after discovery, but that there are a few unsolvable ones lingering. One such example is that the AST deserialisation mechanism can emit errors, and can be invoked at any point, including between a diagnostic and its notes. This can be somewhat frustrating for users, who will probably (and understandably) treat the second note as a bogus part of the second error’s context. Solving this problem simultaneously gives users more context in similar cases (since all the relevant notes fall into the correct position) and eliminates notes that are seemingly unrelated.
Design
Since the current design assumes that all diagnostics have the same hierarchy, we currently have a single type for all diagnostic identifiers, and doesn’t impact how we process anything: we just do something along the lines of Diag(Location, diag::Code) and get back the canonical error type. This won’t suffice moving forward, since Diag(Location, ErrorCode) needs to distinguish between top-level diagnostics and nested diagnostics at compile-time. To achieve this, we need to change what TableGen emits: there needs to be at least two categories of diagnostics: top_level_diag::Code for errors, warnings, remarks; and nested_diag::Code for everything else (currently just notes). Splitting these further, into error::Code, warn::Code, etc. may have other benefits, and we should evaluate that before settling on a dichotomy. If possible, we may like to consider privately deriving NestedDiag and TopLevelDiag from PartialDiagnostic, to reduce the amount of repeated design.
From here, we should be able to adapt existing diagnostics without impacting terribly much. For example, consider this snippet from Sema::BuildCXXNestedNameSpecifier:
Current
Diag(IdInfo.IdentifierLoc, diag::err_expected_class_or_namespace)
<< IdInfo.Identifier << getLangOpts().CPlusPlus;
if (NamedDecl *ND = Found.getAsSingle<NamedDecl>())
Diag(ND->getLocation(), diag::note_entity_declared_at)
<< IdInfo.Identifier;
After implementing this proposal’s changes
TopLevelDiag Error =
Diag(IdInfo.IdentifierLoc, top_level_diag::err_expected_class_or_namespace)
<< IdInfo.Identifier << getLangOpts().CPlusPlus;
if (NamedDecl *ND = Found.getAsSingle<NamedDecl>())
Error.AddNote(ND->getLocation(), nested_diag::entity_declared_at)
<< IdInfo.Identifier;
In the second snippet, we create a TopLevelDiag object to represent the error, instead of a SemaDiagBuilder. The two types are largely the same, except that TopLevelDiag stores NestedDiag objects. NestedDiag is a logically identical type, but doesn’t have name equivalence with TopLevelDiag, so that we aren’t able to nest top-level diagnostics.
The difference between DiagBuilder and SemaDiagBuilder is that SemaDiagBuilder takes semantic information such as SFINAE into account, and uses this to produce context notes (or similar). Since this information only needs to be stored once, we can refactor the compiler so that this can be held by TopLevelDiag; NestedDiag should hold a pointer to this information and access it as necessary. D141451 identifies that some Sema diagnostics may need to cross the boundary into the optimiser in order to provide the richest experience. This may mean that the emission model needs to change so that we buffer all diagnostics until compilation completes, so that additional information can be added as we go (here’s an example of how the transition may look). Assert handlers ought to be aware of this so that they dump everything before crashing.
Deployment
The design put forward makes it possible for us to progressively add context to all of Clang’s diagnostics, meaning that any contextless diagnostics are unaffected by this change. In simple situations like the example presented in this paper, where the context is immediately available, we intend to write a Clang Tidy pass that will automate the deployment of this design. Aaron Ballman estimates that around three-quarters of the diagnostics will be cleaned up this way, and the remainder will need to be done by hand. Identifying which ones are left will be easy enough, as we can lean on making NestedDiag nodiscard. Once partially deployed, Clang will need to adopt a policy requiring contributors to pass notes to error objects, so that we don’t end up with an ever-growing backlog of things to triage. We also consider it reasonable for reviewers to ask contributors to adapt diagnostics as they make contributions to those diagnostics (e.g. someone is correcting a typo), which helps to speed up the transition process; however, reviewers shouldn’t be blocking on this, as the conversions might be more involved than adapting Diag(Loc, EC) to Error.AddNote(Loc, NC).