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

Add ErrCode #119972

Merged
merged 3 commits into from
Jan 29, 2024
Merged

Add ErrCode #119972

merged 3 commits into from
Jan 29, 2024

Conversation

nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Jan 14, 2024

Error codes are integers, but String is used everywhere to represent them. Gross!

This commit introduces ErrCode, an integral newtype for error codes, replacing String. The commit also introduces constants like E0123 for all the used error codes.

r? @estebank

@rustbot rustbot added A-meta Area: Issues & PRs about the rust-lang/rust repository itself A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-testsuite Area: The testsuite used to check the correctness of rustc 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-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jan 14, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jan 14, 2024

Some changes might have occurred in exhaustiveness checking

cc @Nadrieril

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

Some changes occurred in need_type_info.rs

cc @lcnr

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

rustc_macros::diagnostics was changed

cc @davidtwco, @compiler-errors, @TaKO8Ki

@nnethercote
Copy link
Contributor Author

The first two commits (before the dummy ---- commit) are from #119922, which is this PR builds on.

Best reviewed one commit at a time.

cc @cjgillot @compiler-errors @oli-obk

Copy link
Member

@GuillaumeGomez GuillaumeGomez left a comment

Choose a reason for hiding this comment

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

Nice catch! :)

@nnethercote
Copy link
Contributor Author

nnethercote commented Jan 16, 2024

I have updated the code with the new approach: defining a constant such as E0123 for every error code. This gives consistency and visual niceness and greppability.

@rust-log-analyzer

This comment has been minimized.

@nnethercote
Copy link
Contributor Author

BTW, defining all the error code constants in rustc_error_codes and the re-exporting them from rustc_errors seems a bit complicated. We could change register_diagnostics into a higher-order macro that is defined in rustc_error_codes but only called in rustc_errors, which means the constants would be defined in rustc_errors.

@nnethercote
Copy link
Contributor Author

We could change register_diagnostics into a higher-order macro that is defined in rustc_error_codes but only called in rustc_errors, which means the constants would be defined in rustc_errors.

I have done this, I think it makes things nicer.

Copy link
Contributor

@oli-obk oli-obk left a comment

Choose a reason for hiding this comment

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

r=me with empty commit removed

README.md Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

this commit can be removed now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The first two commits (before the dummy ---- commit) are from #119922, which is this PR builds on.

Did you review those two commits from #119922 as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes I did, didn't realize they were from a separate PR. Let's land the other one first then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 thanks

@bors
Copy link
Contributor

bors commented Jan 17, 2024

☔ The latest upstream changes (presumably #119922) made this pull request unmergeable. Please resolve the merge conflicts.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 17, 2024

r=me after rebase

@@ -10,9 +10,11 @@ derive_setters = "0.1.6"
rustc_ast = { path = "../rustc_ast" }
rustc_ast_pretty = { path = "../rustc_ast_pretty" }
rustc_data_structures = { path = "../rustc_data_structures" }
rustc_error_codes = { path = "../rustc_error_codes" }
Copy link
Contributor

Choose a reason for hiding this comment

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

The point of rustc_error_codes was to avoid any dependency on the error code comments from the explanations this early in the dependency tree.
OTOH, the current way to decouple messages from code is using fluent. Should we go in this direction in the future? Or is the modification rate for this crate low enough now?

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... the top-level comment in the crate says "This library is used to gather all error codes into one place, the goal being to make their maintenance easier." But nothing about the dependency graph. (I'm just noting this because it's a pet peeve of mine when that kind of important but non-obvious consideration isn't put into a comment somewhere, because it's exactly the kind of thing that should be put into a comment.)

Back to the original point: the approach we landed on of defining Ennnn constants makes the additional dependency unavoidable. Because we need one constant per error code, and those error codes are used everywhere, and they depend on the error code list. With the E!(0123) macro approach this additional dependency could have been avoided, because ErrCode and E! could have been defined in rustc_errors.

I see 93 commits to rustc_error_codes in 2023, though about a quarter of those appear to be merge commits. That doesn't seem terribly high to me.

@nnethercote
Copy link
Contributor Author

I rebased.

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Jan 21, 2024

📌 Commit 9ddc424 has been approved by oli-obk

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 21, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 22, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#118578 (core: introduce split_at{,_mut}_checked)
 - rust-lang#119369 (exclude unexported macro bindings from extern crate)
 - rust-lang#119408 (xous: misc fixes + add network support)
 - rust-lang#119943 (std::net: bind update for using backlog as `-1` too.)
 - rust-lang#119948 (Make `unsafe_op_in_unsafe_fn` migrated in edition 2024)
 - rust-lang#119999 (remote-test: use u64 to represent file size)
 - rust-lang#120152 (add help message for `exclusive_range_pattern` error)
 - rust-lang#120213 (Don't actually make bound ty/const for RTN)
 - rust-lang#120225 (Fix -Zremap-path-scope typo)

Failed merges:

 - rust-lang#119972 (Add `ErrCode`)

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

Needs rebase.
@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 27, 2024
As is already done in `rustc_span` and `rustc_data_structures`.
This makes no sense, and has no effect. I suspect it's been confused
with a `code = "{code}"` attribute on a subdiagnostic suggestion, where
it is valid (but the "code" there is suggested source code, rather than
an error code.)
Error codes are integers, but `String` is used everywhere to represent
them. Gross!

This commit introduces `ErrCode`, an integral newtype for error codes,
replacing `String`. It also introduces a constant for every error code,
e.g. `E0123`, and removes the `error_code!` macro. The constants are
imported wherever used with `use rustc_errors::codes::*`.

With the old code, we have three different ways to specify an error code
at a use point:
```
error_code!(E0123)  // macro call

struct_span_code_err!(dcx, span, E0123, "msg");  // bare ident arg to macro call

\#[diag(name, code = "E0123")]  // string
struct Diag;
```

With the new code, they all use the `E0123` constant.
```
E0123  // constant

struct_span_code_err!(dcx, span, E0123, "msg");  // constant

\#[diag(name, code = E0123)]  // constant
struct Diag;
```

The commit also changes the structure of the error code definitions:
- `rustc_error_codes` now just defines a higher-order macro listing the
  used error codes and nothing else.
- Because that's now the only thing in the `rustc_error_codes` crate, I
  moved it into the `lib.rs` file and removed the `error_codes.rs` file.
- `rustc_errors` uses that macro to define everything, e.g. the error
  code constants and the `DIAGNOSTIC_TABLES`. This is in its new
  `codes.rs` file.
@nnethercote
Copy link
Contributor Author

I rebased. Raising priority because this is conflict prone.

@bors r=oli-obk p=1

@bors
Copy link
Contributor

bors commented Jan 28, 2024

📌 Commit 5d9dfbd 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 28, 2024
@bors
Copy link
Contributor

bors commented Jan 29, 2024

⌛ Testing commit 5d9dfbd with merge 8a0b5ae...

@bors
Copy link
Contributor

bors commented Jan 29, 2024

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

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (8a0b5ae): 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)
0.8% [0.8%, 0.8%] 1
Regressions ❌
(secondary)
2.4% [2.1%, 3.4%] 5
Improvements ✅
(primary)
-2.0% [-2.6%, -1.3%] 2
Improvements ✅
(secondary)
-2.8% [-4.5%, -1.4%] 5
All ❌✅ (primary) -1.1% [-2.6%, 0.8%] 3

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)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-2.5% [-2.8%, -2.2%] 9
All ❌✅ (primary) - - 0

Binary size

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

Bootstrap: 662.476s -> 660.85s (-0.25%)
Artifact size: 308.14 MiB -> 308.02 MiB (-0.04%)

@@ -19,6 +19,6 @@ use rustc_errors::{Applicability, MultiSpan};
extern crate rustc_session;

#[derive(Diagnostic)]
#[diag(compiletest_example, code = "E0123")]
#[diag(compiletest_example, code = 0123)]
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be E0123?

Copy link
Contributor Author

@nnethercote nnethercote Jan 29, 2024

Choose a reason for hiding this comment

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

Good question! I investigated, turns out it doesn't matter. This test (deliberately) fails to compile because the slug name has the incorrect form. Apparently that error occurs earlier than the type error that we would get for the code, presumably during expansion of the derive(Diagnostic) proc macro.

I can fix it in a follow-up, but it's not urgent.

Copy link
Member

Choose a reason for hiding this comment

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

I figured it did not matter much, but since I noticed thought it's better to comment :)

nnethercote added a commit to nnethercote/rust that referenced this pull request Feb 2, 2024
In rust-lang#119972 the code should have become `E0123` rather than `0123`. This
fix doesn't affect the outcome because the proc macro errors out before
the type of the code is checked, but the fix makes the test's code
consistent with other similar code elsewhere.
hlisdero added a commit to hlisdero/cargo-check-deadlock that referenced this pull request Feb 8, 2024
Run `cargo update`.

The regression comes from this PR.
rust-lang/rust#119972
The error codes were refactored so the include changed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-meta Area: Issues & PRs about the rust-lang/rust repository itself A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-testsuite Area: The testsuite used to check the correctness of rustc 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-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.