Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Overhaul Diagnostic and DiagnosticBuilder #722

Closed
3 tasks done
nnethercote opened this issue Feb 7, 2024 · 3 comments
Closed
3 tasks done

Overhaul Diagnostic and DiagnosticBuilder #722

nnethercote opened this issue Feb 7, 2024 · 3 comments
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@nnethercote
Copy link

nnethercote commented Feb 7, 2024

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 while size_of(DiagnosticBuilder) is small because it uses a Box.

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 DiagnosticBuilders 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 DiagInner1 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.
  • 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

  1. Update: @WaffleLapkin suggested DiagData instead of DiagInner, which I think is fine.

@nnethercote nnethercote added T-compiler Add this label so rfcbot knows to poll the compiler team major-change A proposal to make a major change to rustc labels Feb 7, 2024
@rustbot
Copy link
Collaborator

rustbot commented Feb 7, 2024

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.

Concerns or objections to the proposal should be discussed on Zulip and formally registered here by adding a comment with the following syntax:

@rustbot concern reason-for-concern 
<description of the concern> 

Concerns can be lifted with:

@rustbot resolve reason-for-concern 

See documentation at https://forge.rust-lang.org

cc @rust-lang/compiler @rust-lang/compiler-contributors

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Feb 7, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 8, 2024
…ticBuilder, r=<try>

Overhaul `Diagnostic` and `DiagnosticBuilder`

Implements the first part of rust-lang/compiler-team#722, which moves functionality and use away from `Diagnostic`, onto `DiagnosticBuilder`.

Likely follow-ups:
- Move things around, because this PR was written to minimize diff size, so some things end up in sub-optimal places. E.g. `DiagnosticBuilder` has impls in both `diagnostic.rs` and `diagnostic_builder.rs`.
- Rename `Diagnostic` as `DiagInner` and `DiagnosticBuilder` as `Diag`.

r? `@ghost`
@davidtwco
Copy link
Member

@rustbot second

@rustbot rustbot added the final-comment-period The FCP has started, most (if not all) team members are in agreement label Feb 8, 2024
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Feb 8, 2024
@nnethercote
Copy link
Author

@rustbot label -final-comment-period +major-change-accepted

@rustbot rustbot added major-change-accepted A major change proposal that was accepted to-announce Announce this issue on triage meeting and removed final-comment-period The FCP has started, most (if not all) team members are in agreement labels Feb 20, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 20, 2024
…ticBuilder, r=davidtwco

Overhaul `Diagnostic` and `DiagnosticBuilder`

Implements the first part of rust-lang/compiler-team#722, which moves functionality and use away from `Diagnostic`, onto `DiagnosticBuilder`.

Likely follow-ups:
- Move things around, because this PR was written to minimize diff size, so some things end up in sub-optimal places. E.g. `DiagnosticBuilder` has impls in both `diagnostic.rs` and `diagnostic_builder.rs`.
- Rename `Diagnostic` as `DiagInner` and `DiagnosticBuilder` as `Diag`.

r? `@davidtwco`
fmease added a commit to fmease/rust that referenced this issue Feb 21, 2024
…rs, r=compiler-errors

Remove `diagnostic_builder.rs`

rust-lang#120576 moved a big chunk of `DiagnosticBuilder`'s functionality out of `diagnostic_builder.rs` into `diagnostic.rs`, which left `DiagnosticBuilder` spread across the two files.

This PR fixes that messiness by merging what remains of `diagnostic_builder.rs` into `diagnostic.rs`.

This is part of rust-lang/compiler-team#722.

r? `@davidtwco`
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Feb 21, 2024
Rollup merge of rust-lang#121366 - nnethercote:rm-diagnostic_builder.rs, r=compiler-errors

Remove `diagnostic_builder.rs`

rust-lang#120576 moved a big chunk of `DiagnosticBuilder`'s functionality out of `diagnostic_builder.rs` into `diagnostic.rs`, which left `DiagnosticBuilder` spread across the two files.

This PR fixes that messiness by merging what remains of `diagnostic_builder.rs` into `diagnostic.rs`.

This is part of rust-lang/compiler-team#722.

r? `@davidtwco`
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Feb 22, 2024
…r, r=davidtwco

Overhaul `Diagnostic` and `DiagnosticBuilder`

Implements the first part of rust-lang/compiler-team#722, which moves functionality and use away from `Diagnostic`, onto `DiagnosticBuilder`.

Likely follow-ups:
- Move things around, because this PR was written to minimize diff size, so some things end up in sub-optimal places. E.g. `DiagnosticBuilder` has impls in both `diagnostic.rs` and `diagnostic_builder.rs`.
- Rename `Diagnostic` as `DiagInner` and `DiagnosticBuilder` as `Diag`.

r? `@davidtwco`
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue Feb 22, 2024
…r, r=davidtwco

Overhaul `Diagnostic` and `DiagnosticBuilder`

Implements the first part of rust-lang/compiler-team#722, which moves functionality and use away from `Diagnostic`, onto `DiagnosticBuilder`.

Likely follow-ups:
- Move things around, because this PR was written to minimize diff size, so some things end up in sub-optimal places. E.g. `DiagnosticBuilder` has impls in both `diagnostic.rs` and `diagnostic_builder.rs`.
- Rename `Diagnostic` as `DiagInner` and `DiagnosticBuilder` as `Diag`.

r? `@davidtwco`
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Feb 22, 2024
bors added a commit to rust-lang-ci/rust that referenced this issue Feb 28, 2024
Diagnostic renaming

Renaming various diagnostic types from `Diagnostic*` to `Diag*`. Part of rust-lang/compiler-team#722. There are more to do but this is enough for one PR.

r? `@davidtwco`
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Feb 29, 2024
Diagnostic renaming

Renaming various diagnostic types from `Diagnostic*` to `Diag*`. Part of rust-lang/compiler-team#722. There are more to do but this is enough for one PR.

r? `@davidtwco`
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue Mar 7, 2024
Diagnostic renaming

Renaming various diagnostic types from `Diagnostic*` to `Diag*`. Part of rust-lang/compiler-team#722. There are more to do but this is enough for one PR.

r? `@davidtwco`
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
…r, r=davidtwco

Overhaul `Diagnostic` and `DiagnosticBuilder`

Implements the first part of rust-lang/compiler-team#722, which moves functionality and use away from `Diagnostic`, onto `DiagnosticBuilder`.

Likely follow-ups:
- Move things around, because this PR was written to minimize diff size, so some things end up in sub-optimal places. E.g. `DiagnosticBuilder` has impls in both `diagnostic.rs` and `diagnostic_builder.rs`.
- Rename `Diagnostic` as `DiagInner` and `DiagnosticBuilder` as `Diag`.

r? `@davidtwco`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
…r, r=davidtwco

Overhaul `Diagnostic` and `DiagnosticBuilder`

Implements the first part of rust-lang/compiler-team#722, which moves functionality and use away from `Diagnostic`, onto `DiagnosticBuilder`.

Likely follow-ups:
- Move things around, because this PR was written to minimize diff size, so some things end up in sub-optimal places. E.g. `DiagnosticBuilder` has impls in both `diagnostic.rs` and `diagnostic_builder.rs`.
- Rename `Diagnostic` as `DiagInner` and `DiagnosticBuilder` as `Diag`.

r? `@davidtwco`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc major-change-accepted A major change proposal that was accepted T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

4 participants