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

Improve error message #681

Merged
merged 4 commits into from
Jan 25, 2025

Conversation

gui1117
Copy link
Contributor

@gui1117 gui1117 commented Jan 16, 2025

@pkhry I tried to improve the error message, I just print all variant and there index, if there is a formula it will just be the formula expression without the evaluation but I still think it can be enough information for debugging, WDYT?

Copy link
Contributor

@pkhry pkhry left a comment

Choose a reason for hiding this comment

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

Lgtm

derive/src/utils.rs Outdated Show resolved Hide resolved
derive/src/utils.rs Outdated Show resolved Hide resolved
derive/src/utils.rs Outdated Show resolved Hide resolved
@@ -52,7 +67,7 @@ pub fn const_eval_check_variant_indexes(
let mut j = i + 1;
while j < len {
if indices[i] == indices[j] {
::core::panic!("Found Variants that have duplicate indexes. Use different indexes for each variant");
::core::panic!(#error_message);
Copy link
Member

Choose a reason for hiding this comment

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

And then we can generate an even better error message here :P

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 thought it was impossible, but it is. Done in 876a14a

Also are we ok adding a dependency to const_format, if not I can always write the concatenation of strings inlined.
(convertion to array, then calculate the new length in const, then instantiate the new array and copy content + plus special code to format u8).

Comment on lines +80 to +90
let msg = #crate_path::__private::concatcp!(
"Found variants that have duplicate indexes. Both `",
indices[DUP_INFO.1].1,
"` and `",
indices[DUP_INFO.2].1,
"` have the index `",
indices[DUP_INFO.1].0,
"`. Use different indexes for each variant."
);

::core::panic!("{}", msg);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let msg = #crate_path::__private::concatcp!(
"Found variants that have duplicate indexes. Both `",
indices[DUP_INFO.1].1,
"` and `",
indices[DUP_INFO.2].1,
"` have the index `",
indices[DUP_INFO.1].0,
"`. Use different indexes for each variant."
);
::core::panic!("{}", msg);
::core::panic!("Found variants that have duplicate indexes. Both `{}` and `{}` have index `{}`. Each variant is required to have an unique index.",
indices[DUP_INFO.1].1,
indices[DUP_INFO.2].1,
indices[DUP_INFO.1].0,
);

DOesn't that work? :D

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 wish constant evaluation gets a bit more love from rustc developers.

But formatting macro doesn't actually work at compile-time:

		const _: () = {
		    const FOO: &str = "a";
		    const BAR: &str = "b";
			panic!("{}{}", FOO, BAR);
		};
error[E0015]: cannot call non-const formatting macro in constants
 --> src/lib.rs:4:12
  |
4 |             panic!("{}{}", FOO, BAR);
  |                     ^^
  |
  = note: calls in constants are limited to constant functions, tuple structs and tuple variants
  = note: this error originates in the macro `$crate::const_format_args` which comes from the expansion of the macro `panic` (in Nightly builds, run with -Z macro-backtrace for more info)

@gui1117 gui1117 merged commit 65fae2f into pkhry/duplicate-indexes-const-eval Jan 25, 2025
17 checks passed
bkchr added a commit that referenced this pull request Jan 25, 2025
* init

* Improve error message (#681)

* improve error message

* Apply suggestions from code review

Co-authored-by: Bastian Köcher <[email protected]>

* finally understanding rust const environment

* fmt

---------

Co-authored-by: Bastian Köcher <[email protected]>

---------

Co-authored-by: Guillaume Thiolliere <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants