Don't crash on error codes passed to --explain which exceed our internal limit of 9999 by Kivooeo · Pull Request #140700 · rust-lang/rust (original) (raw)
removed panic in case where we do --explain > 9999 and added check for it
now error looks like this instead of ICE
$ rustc.exe --explain E10000
error: E10000 is not a valid error code
fmease is on vacation.
Please choose another assignee.
r? @davidtwco
rustbot has assigned @davidtwco.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.
Use r? to explicitly pick a reviewer
Status: Awaiting review from the assignee but also interested parties.
Relevant to the compiler team, which will review and decide on the PR/issue.
labels
This comment has been minimized.
Status: This is awaiting some action (such as code changes or more information) from the author.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
I feel like this is very arbitrary. The 9999 limit is just an internal implementation detail (an internal "this is obviously wrong" check and would be bumped if we ever get to that many error codes) and shouldn't be exposed to the user. I would've just expected
$ rustc --explain E12345
error: E12345 is not a valid error code
to behave the same as
$ rustc --explain E4444
error: E4444 is not a valid error code
and
$ rustc --explain E12345678901234567890 # exceeds u32::MAX
error: E12345678901234567890 is not a valid error code
to gracefully fail with the same error message.
@jieyouxu any actual idea why tidy start fails here?
tidy check
tidy error: Error code `E9999` is used in the compiler but not defined and documented in `compiler/rustc_error_codes/src/lib.rs`.
like... should I just add E9999 there..?
and about your suggestion, you asking to merge both errors into one error_code is not a valid error code right?
oh no, looks like adding E9999 was a miskate here
but it actually makes tidy pass, so idk, correct me here pls
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any actual idea why tidy start fails here?
I'm pretty sure it's simply due to tidy (src/tools/tidy/src/error_codes.rs:361) grepping through ~all Rust compiler source files looking for \bE\d{4}\b and finding the E9999 in your previous error message that you've since removed.
This comment has been minimized.
@ShE3py oh, yes i want to add tests for it, didnt knew that there is some for --explain, should i create it separate tests or can do it in one file?
yeah, i guess i cant do this in one fileerror: Option 'explain' given more than once
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two suggestions then we should be good to go!
@fmease seems done, but check pls if i got you and made all right
i also may should add some tests for other cases like valid error code for error that not exists or just something weird like word instead of number etc
i also may should add some tests for other cases like valid error code for error that not exists or just something weird like word instead of number etc
If you'd like to, go right ahead, sure sounds like a good idea.
@fmease should be good to go, added few more test cases
@bors r=davidtwco,fmease rollup
📌 Commit 3c1c072 has been approved by davidtwco,fmease
It is now in the queue for this repository.
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
ICE: added error handle for values greater than 9999 in Don't crash on error codes passed to --explain which exceed our internal limit of 9999
bors added a commit to rust-lang-ci/rust that referenced this pull request
…llaumeGomez
Rollup of 8 pull requests
Successful merges:
- rust-lang#140234 (Separate dataflow analysis and results)
- rust-lang#140614 (Correct warning message in restricted visibility)
- rust-lang#140671 (Parser: Recover error from named params while parse_path)
- rust-lang#140700 (Don't crash on error codes passed to
--explainwhich exceed our internal limit of 9999 ) - rust-lang#140706 ([rustdoc] Ensure that temporary doctest folder is correctly removed even if doctests failed)
- rust-lang#140734 (Fix regression from rust-lang#140393 for espidf / horizon / nuttx / vita)
- rust-lang#140741 (add armv5te-unknown-linux-gnueabi target maintainer)
- rust-lang#140745 (run-make-support: set rustc dylib path for cargo wrapper)
r? @ghost
@rustbot modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request
…llaumeGomez
Rollup of 8 pull requests
Successful merges:
- rust-lang#140234 (Separate dataflow analysis and results)
- rust-lang#140614 (Correct warning message in restricted visibility)
- rust-lang#140671 (Parser: Recover error from named params while parse_path)
- rust-lang#140700 (Don't crash on error codes passed to
--explainwhich exceed our internal limit of 9999 ) - rust-lang#140706 ([rustdoc] Ensure that temporary doctest folder is correctly removed even if doctests failed)
- rust-lang#140734 (Fix regression from rust-lang#140393 for espidf / horizon / nuttx / vita)
- rust-lang#140741 (add armv5te-unknown-linux-gnueabi target maintainer)
- rust-lang#140745 (run-make-support: set rustc dylib path for cargo wrapper)
r? @ghost
@rustbot modify labels: rollup
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request
Rollup merge of rust-lang#140700 - Kivooeo:new-fix-six, r=davidtwco,fmease
Don't crash on error codes passed to --explain which exceed our internal limit of 9999
removed panic in case where we do --explain > 9999 and added check for it
now error looks like this instead of ICE
$ rustc.exe --explain E10000
error: E10000 is not a valid error codefixes rust-lang#140647
r? @fmease