Description
Proposal
Basics
Diagnostic
holds all the data for a diagnostic. DiagnosticBuilder
is a wrapper for Diagnostic
, which adds the following.
- A
DiagCtxt
reference. - A
G: EmissionGuarantee
type parameter, e.g.ErrorGuaranteed
for errors and()
for warnings. - Methods to emit (producing the aforementioned
EmissionGuarantee
) and cancel the diagnostic. - A drop-time check that the diagnostic was actually emitted/cancelled.
size_of(Diagnostic)
is large whilesize_of(DiagnosticBuilder)
is small because it uses aBox
.
So we have two different levels for dealing with diagnostics: Diagnostic
(lower level) and DiagnosticBuilder
(higher level).
Operations
When you create a diagnostic, you typically use something like DiagCtxt::struct_err
, which produces a DiagnosticBuilder
. And eventually you emit or cancel it, which is also done at the DiagnosticBuilder
level. In the meantime, if you want to modify the diagnostic, you can do that at either level. There's a ton of modifier methods that are defined on both types.
For example, Diagnostic
has this:
#[rustc_lint_diagnostics]
pub fn note(&mut self, msg: impl Into<SubdiagnosticMessage>) -> &mut Self;
and DiagnosticBuilder
has these:
pub fn note(&mut self, msg: impl Into<SubdiagnosticMessage>) -> &mut Self;
pub fn with_note(mut self, msg: impl Into<SubdiagnosticMessage>) -> Self;
These latter two are defined via the forward!
macro and just call Diagnostic::note
. with_note
facilitates chaining and is very useful. But having note
functions at both levels is redundant. Furthermore, DiagnosticBuilder
impls Deref
/DerefMut
, which means that even if DiagnosticBuilder::note
wasn't present, you could still call db.note()
and get Diagnostic::note
method .
So there are three methods for adding a note to a diagnostic, or four if you count going through DerefMut
. This duplication is repeated for 30 modifier methods.
Throughout the compiler there are hundreds of functions that modify an existing diagnostic. Some of these take a &mut Diagnostic
, and some take a &mut DiagnosticBuilder
. There's no real difference, because the same-named modifier methods are available on both types. Nor is there any guidance about which is preferable.
Note also that Diagnostic::note
has a #[rustc_lint_diagnostic]
attribute, but the others don't, which means that some places that add notes to diagnostics aren't getting checked properly by the untranslatable_diagnostics
and diagnostic_outside_of_impl
lints.
Bad naming
With the builder pattern, you have types T
and TBuilder, and you can build a T
instance (or multiple instances) from a TBuilder
.
But DiagnosticBuilder
is actually a wrapper for Diagnostic
, and the builder pattern isn't used. So DiagnosticBuilder
is a misleading name and should be changed.
Also, DiagnosticBuilder
is a really long name. It appears in the code about 500 times. Diagnostic
is also not short, and appears over 700 times. Both could be made shorter.
Merge them?
My first thought was to merge them, but that's not possible. DiagnosticBuilder
has a DiagCtxt
reference, which makes it impossible to store DiagnosticBuilder
s within DiagCtxt
, which is something we need to do (e.g. for stashing and stealing diagnostics). And it's useful to have a non-generic inner type, e.g. for TRACK_DIAGNOSTICS
.
Proposed changes
I propose to move as much functionality as possible to DiagnosticBuilder
, leaving Diagnostic
as an "inner" type that is an implementation detail.
First, I want to remove all the modifier methods from Diagnostic
, and only have them on DiagnosticBuilder
. This means all the modifying functions that take a &mut Diagnostic
would be changed to &mut DiagnosticBuilder
. This would eliminate the method duplication, and the choice of which level to modify diagnostics at, and the inconsistent use of #[rustc_lint_diagnostics]
.
Second, I want to improve the names. One possibility would be to rename Diagnostic
as DiagnosticInner
, and DiagnosticBuilder
as Diagnostic
. That would reflect the fact that the latter type is now the one getting most use, and would also fix the Builder
misnomer. But those names are still long and there might be confusion caused by reusing Diagnostic
.
Instead I propose to rename Diagnostic
as DiagInner
1 and, DiagnosticBuilder
as Diag
. This follows the principal of Huffman encoding: frequently-used names should be short. Diag
is an unambiguous abbreviation, and one that's already used a lot in other ways: diag
(variable name), DiagCtxt
, DiagLevel
, VarianceDiagInfo
, ArrayIntoIterDiagSub
, etc. It fits in with Rust's long tradition of abbreviated names: fn
, u32
, Vec
, tcx: TyCtxt
, etc. This will result in many multi-line constructs being reformatted into a single line, which is nice. Less typing, too.
More renaming?
If there is an appetite for more name shortening, here are some commonly used candidates, with occurrence counts in parentheses:
derive(Diagnostic) (1303) -> derive(Diag) [or derive(IntoDiag), to match the trait name]
derive(Diagnostic) (363) -> derive(Subdiag) [or derive(AddToDiag), to match the trait name]
derive(LintDiagnostic) (224) -> derive(LintDiag) [or derive(DecorateLint), to match the trait name]
DiagnosticMessage (230) -> DiagMessage
SubDiagnosticMessage (149) -> SubdiagMessage
IntoDiagnosticArg (116) -> IntoDiagArg
IntoDiagnostic (83) -> IntoDiag
AddToDiagnostic (75) -> AddToDiag
SubDiagnostic (24) -> Subdiag
DiagnosticArgName (20) -> DiagArgName
DiagnosticArgValue (20) -> DiagArgValue
Alternatives
None beyond what was already mentioned above.
Mentors or Reviewers
I can do the work. I already have a draft implementation of the first part in rust-lang/rust#120576.
Possible reviewers include @oli-obk, @compiler-errors, @estebank.
Process
The main points of the Major Change Process are as follows:
- File an issue describing the proposal.
- A compiler team member or contributor who is knowledgeable in the area can second by writing
@rustbot second
.- Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a
-C flag
, then full team check-off is required. - Compiler team members can initiate a check-off via
@rfcbot fcp merge
on either the MCP or the PR.
- Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a
- Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.
You can read more about Major Change Proposals on forge.
Comments
This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.
Footnotes
-
Update: @WaffleLapkin suggested
DiagData
instead ofDiagInner
, which I think is fine. ↩