ICE: added error handle for values greater than 9999 in

[Kivooeo]

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

fixes #140647
r? @fmease

[rustbot]

fmease is on vacation.

Please choose another assignee.

[rustbot]

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

[jieyouxu]

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.

[Kivooeo]

@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?

[rustbot]

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

[Kivooeo]

oh no, looks like adding E9999 was a miskate here
but it actually makes tidy pass, so idk, correct me here pls

[fmease]

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.

[fmease]

No, this shouldn't be necessary now that you've removed the specialized error message.

[Kivooeo]

yeah i removed this custom message error for anything greater than 9999 and removed E9999 from error_codes, ran tidy locally and tidy said it's fine, let's see

[fmease]

While we obviously shouldn't crash when the user enters error codes greater than E9999, the original purpose of this internal limit is to prevent rustc devs from introducing five-digit error codes without changing all other relevant parts of the compiler, docs and tools (like tidy) to now pad all four-digit codes with an additional 0 to become five-digit codes, etc.

[Kivooeo]

i was wondering how can i fix this to make not panicking here, and without changing NewType entirely, so this is only solution that worked fine that i came up with

[ShE3py]

Running the ICE with debuginfo gives this line:

rust/compiler/rustc_driver_impl/src/lib.rs

Line 466 in 651e9cf

&& let Ok(description) = registry.try_find_description(ErrCode::from_u32(code))

You probably want to manually add an impl TryFrom<u32> for ErrCode.

[Kivooeo]

doesnt it will still panic inside NewType because of max limitations?

[Kivooeo]

i believe that the panic occurs here

rust/compiler/rustc_index_macros/src/newtype.rs

Line 183 in 651e9cf

assert!(value <= (#max as usize));

because ICE was adressing to this line

rust/compiler/rustc_errors/src/codes.rs

Line 9 in 651e9cf

rustc_index::newtype_index! {

thread 'main' panicked at /rustc/d6a325d93a84077580c677f89affbe45682606c3\compiler\rustc_errors\src\codes.rs:9:1:
assertion failed: value <= 9999

[Kivooeo]

so, as far as i concerned, we cannot really prevent this panic when having #[max = 9999] and pass something bigger than it, we should not let this error code (that is bigger than max value) get to this assertations because it'll panic is such cases

but i may missing something for sure, if so please let me know we'll brainstorm it together

[ShE3py]

The idea is to return Err(_)/None, i.e. not constructing the newtype if code > 9999 (= not calling from_u32, so the assert! is not reached).

Alternativement, adding impl Borrow<u32> for ErrCode so this fn can directly being called with an u32 (try_find_description<C>(&self, code: C) where ErrCode: Borrow<C>);

rust/compiler/rustc_errors/src/registry.rs

Lines 18 to 22 in 651e9cf

	/// Returns `InvalidErrorCode` if the code requested does not exist in the
	/// registry.
	pub fn try_find_description(&self, code: ErrCode) -> Result<&'static str, InvalidErrorCode> {
	self.long_descriptions.get(&code).copied().ok_or(InvalidErrorCode)
	}

Alternatively, just adding && code <= ErrCode::MAX_AS_U32 before this:

rust/compiler/rustc_driver_impl/src/lib.rs

Line 466 in 651e9cf

&& let Ok(description) = registry.try_find_description(ErrCode::from_u32(code))

[Kivooeo]

yep, currently playing around this ideas, this would take some time because of recompilling, thanks for ideas

[Kivooeo]

ive tried this thing

  pub fn try_find_description(&self, code: ErrCode) -> Result<&'static str, InvalidErrorCode> {
      if code > ErrCode::from_u32(9999) {
          return Err(InvalidErrorCode);
      }
      self.long_descriptions.get(&code).copied().ok_or(InvalidErrorCode)
  }

and its not worked for me
but this one && code <= ErrCode::MAX_AS_U32 runs like perfect, so we can stick with it

[ShE3py]

You may want to add a regression test based on tests/ui/explain.rs

[Kivooeo]

@ShE3py oh, yes i want to add tests for it, didnt knew that there is some for --explain, i should create it separate tests or can do it in one file?

yeah, i guess i cant do this in one file
error: Option 'explain' given more than once