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

Rework how diagnostic lints are stored. #119922

Merged
merged 2 commits into from
Jan 17, 2024

Conversation

nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Jan 13, 2024

Diagnostic::code has the type DiagnosticId, which has Error and
Lint variants. Plus Diagnostic::is_lint is a bool, which should be
redundant w.r.t. Diagnostic::code.

Seems simple. Except it's possible for a lint to have an error code, in
which case its code field is recorded as Error, and is_lint is
required to indicate that it's a lint. This is what happens with
derive(LintDiagnostic) lints. Which means those lints don't have a
lint name or a has_future_breakage field because those are stored in
the DiagnosticId::Lint.

It's all a bit messy and confused and seems unintentional.

This commit:

  • removes DiagnosticId;
  • changes Diagnostic::code to Option<String>, which means both
    errors and lints can straightforwardly have an error code;
  • changes Diagnostic::is_lint to Option<IsLint>, where IsLint is a
    new type containing a lint name and a has_future_breakage bool, so
    all lints can have those, error code or not.

r? @oli-obk

@rustbot rustbot added A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 13, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jan 13, 2024

rustc_macros::diagnostics was changed

cc @davidtwco, @compiler-errors, @TaKO8Ki

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in need_type_info.rs

cc @lcnr

@nnethercote nnethercote force-pushed the fix-Diag-code-is_lint branch 3 times, most recently from 04c9d37 to 6748d95 Compare January 14, 2024 02:55
`Diagnostic::code` has the type `DiagnosticId`, which has `Error` and
`Lint` variants. Plus `Diagnostic::is_lint` is a bool, which should be
redundant w.r.t. `Diagnostic::code`.

Seems simple. Except it's possible for a lint to have an error code, in
which case its `code` field is recorded as `Error`, and `is_lint` is
required to indicate that it's a lint. This is what happens with
`derive(LintDiagnostic)` lints. Which means those lints don't have a
lint name or a `has_future_breakage` field because those are stored in
the `DiagnosticId::Lint`.

It's all a bit messy and confused and seems unintentional.

This commit:
- removes `DiagnosticId`;
- changes `Diagnostic::code` to `Option<String>`, which means both
  errors and lints can straightforwardly have an error code;
- changes `Diagnostic::is_lint` to `Option<IsLint>`, where `IsLint` is a
  new type containing a lint name and a `has_future_breakage` bool, so
  all lints can have those, error code or not.
@nnethercote
Copy link
Contributor Author

FWIW, there are only three derive(LintDiagnostic) diagnostics with a code:

  • UnknownLintFromCommandLine (E0602)
  • BindingsWithVariantName (E0170)
  • ReprConflictingLint (E0566)

That's few enough that I'm wondering if they're necessary and/or legitimate.

@@ -104,7 +104,7 @@ pub struct Diagnostic {
pub(crate) level: Level,

pub messages: Vec<(DiagnosticMessage, Style)>,
pub code: Option<DiagnosticId>,
pub code: Option<String>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to store an integer instead of a string here? An ErrorCode type?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! You're going to love #119972, which I just finished :)

@@ -1382,7 +1382,7 @@ fn compare_number_of_generics<'tcx>(
kind = kind,
),
);
err.code(DiagnosticId::Error("E0049".into()));
err.code("E0049".into());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drive-by: this should use the error_code! macro.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed that in #119972.

},
pub struct IsLint {
/// The lint name.
pub(crate) name: String,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this use the &Lint? Or is there an issue with crate dependency order?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting idea. Crate dependencies shouldn't be a problem. This struct has two fields:

  • name, which is equivalent to Lint::lower_name().
  • has_future_breakage, which is a combination of Lint::future_incompatible, Lint::default_level, and sess.opts.unstable_opts.future_incompat_test. Adding a sess argument to Diagnostic::has_future_breakage() is awkward.

So I think keeping IsLint is probably best.

@nnethercote nnethercote mentioned this pull request Jan 14, 2024
@oli-obk
Copy link
Contributor

oli-obk commented Jan 16, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Jan 16, 2024

📌 Commit d71f535 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 16, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 16, 2024
… r=oli-obk

Rework how diagnostic lints are stored.

`Diagnostic::code` has the type `DiagnosticId`, which has `Error` and
`Lint` variants. Plus `Diagnostic::is_lint` is a bool, which should be
redundant w.r.t. `Diagnostic::code`.

Seems simple. Except it's possible for a lint to have an error code, in
which case its `code` field is recorded as `Error`, and `is_lint` is
required to indicate that it's a lint. This is what happens with
`derive(LintDiagnostic)` lints. Which means those lints don't have a
lint name or a `has_future_breakage` field because those are stored in
the `DiagnosticId::Lint`.

It's all a bit messy and confused and seems unintentional.

This commit:
- removes `DiagnosticId`;
- changes `Diagnostic::code` to `Option<String>`, which means both
  errors and lints can straightforwardly have an error code;
- changes `Diagnostic::is_lint` to `Option<IsLint>`, where `IsLint` is a
  new type containing a lint name and a `has_future_breakage` bool, so
  all lints can have those, error code or not.

r? `@oli-obk`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 16, 2024
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#119062 (Deny braced macro invocations in let-else)
 - rust-lang#119922 (Rework how diagnostic lints are stored.)
 - rust-lang#119978 (Move async closure parameters into the resultant closure's future eagerly)
 - rust-lang#119984 (Change return type of unstable `Waker::noop()` from `Waker` to `&Waker`.)
 - rust-lang#120020 (Gracefully handle missing typeck information if typeck errored)
 - rust-lang#120021 (don't store const var origins for known vars)

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 16, 2024
… r=oli-obk

Rework how diagnostic lints are stored.

`Diagnostic::code` has the type `DiagnosticId`, which has `Error` and
`Lint` variants. Plus `Diagnostic::is_lint` is a bool, which should be
redundant w.r.t. `Diagnostic::code`.

Seems simple. Except it's possible for a lint to have an error code, in
which case its `code` field is recorded as `Error`, and `is_lint` is
required to indicate that it's a lint. This is what happens with
`derive(LintDiagnostic)` lints. Which means those lints don't have a
lint name or a `has_future_breakage` field because those are stored in
the `DiagnosticId::Lint`.

It's all a bit messy and confused and seems unintentional.

This commit:
- removes `DiagnosticId`;
- changes `Diagnostic::code` to `Option<String>`, which means both
  errors and lints can straightforwardly have an error code;
- changes `Diagnostic::is_lint` to `Option<IsLint>`, where `IsLint` is a
  new type containing a lint name and a `has_future_breakage` bool, so
  all lints can have those, error code or not.

r? ``@oli-obk``
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 17, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#119855 (Enable Static Builds for FreeBSD)
 - rust-lang#119922 (Rework how diagnostic lints are stored.)
 - rust-lang#119978 (Move async closure parameters into the resultant closure's future eagerly)
 - rust-lang#119984 (Change return type of unstable `Waker::noop()` from `Waker` to `&Waker`.)
 - rust-lang#120020 (Gracefully handle missing typeck information if typeck errored)
 - rust-lang#120021 (don't store const var origins for known vars)
 - rust-lang#120032 (Fix `rustc_abi` build on stable)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 17, 2024
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#119855 (Enable Static Builds for FreeBSD)
 - rust-lang#119922 (Rework how diagnostic lints are stored.)
 - rust-lang#119978 (Move async closure parameters into the resultant closure's future eagerly)
 - rust-lang#119984 (Change return type of unstable `Waker::noop()` from `Waker` to `&Waker`.)
 - rust-lang#120020 (Gracefully handle missing typeck information if typeck errored)
 - rust-lang#120021 (don't store const var origins for known vars)
 - rust-lang#120032 (Fix `rustc_abi` build on stable)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors
Copy link
Contributor

bors commented Jan 17, 2024

⌛ Testing commit d71f535 with merge 16f4b02...

@bors
Copy link
Contributor

bors commented Jan 17, 2024

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 16f4b02 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 17, 2024
@bors bors merged commit 16f4b02 into rust-lang:master Jan 17, 2024
12 checks passed
@rustbot rustbot added this to the 1.77.0 milestone Jan 17, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (16f4b02): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
2.7% [2.7%, 2.7%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-5.0% [-5.0%, -5.0%] 1
All ❌✅ (primary) 2.7% [2.7%, 2.7%] 1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
2.0% [2.0%, 2.0%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.8% [-3.6%, -2.1%] 7
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 665.423s -> 666.76s (0.20%)
Artifact size: 308.34 MiB -> 308.34 MiB (0.00%)

@nnethercote nnethercote deleted the fix-Diag-code-is_lint branch January 21, 2024 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants