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 diagnostics for duplicate enum discriminants #97533

Closed
clarfonthey opened this issue May 29, 2022 · 6 comments · Fixed by #100238
Closed

Improve diagnostics for duplicate enum discriminants #97533

clarfonthey opened this issue May 29, 2022 · 6 comments · Fixed by #100238
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints D-papercut Diagnostics: An error or lint that needs small tweaks. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. D-verbose Diagnostics: Too much output caused by a single piece of incorrect code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@clarfonthey
Copy link
Contributor

clarfonthey commented May 29, 2022

This is split out from the discussion in #97319.

First, the existing error for duplicate enum discriminants could be improved. The best summary of what we have now can be seen in the test added for #97319, which fixes part of it:

enum Enum {
    P = 3,
    X = 3,
    Y = 5
}

#[repr(u8)]
enum EnumOverflowRepr {
    P = 257,
    X = 513,
}

#[repr(i8)]
enum NegDisEnum {
    First = -1,
    Second = -2,
    Last,
}

Which outputs:

error[E0081]: discriminant value `3` assigned more than once
  --> $DIR/E0081.rs:1:1
   |
LL | / enum Enum {
LL | |
LL | |     P = 3,
   | |         - first assignment of `3`
LL | |
LL | |     X = 3,
   | |         - second assignment of `3`
LL | |
LL | |     Y = 5
LL | | }
   | |_^

error[E0081]: discriminant value `1` assigned more than once
  --> $DIR/E0081.rs:11:1
   |
LL | / enum EnumOverflowRepr {
LL | |
LL | |     P = 257,
   | |         --- first assignment of `1` (overflowed from `257`)
LL | |
LL | |     X = 513,
   | |         --- second assignment of `1` (overflowed from `513`)
LL | |
LL | | }
   | |_^

error[E0081]: discriminant value `-1` assigned more than once
  --> $DIR/E0081.rs:20:1
   |
LL | / enum NegDisEnum {
LL | |
LL | |     First = -1,
   | |             -- first assignment of `-1`
LL | |
LL | |     Second = -2,
   | |     ----------- assigned discriminant for `Last` was incremented from this discriminant
LL | |
LL | |     Last,
   | |     ---- second assignment of `-1`
LL | |
LL | | }
   | |_^

error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0081`.

I suggested that the last version of the error should suggest explicitly stating that the discriminant for Last is computed as (Second = -2) + (1 variant later) = -1 = First, or somehow better wording that. Essentially, make it clear to the user that things are incremented positively, regardless of the order of existing discriminants.

Secondly, @Bryysen suggested that the code that does this (check_enum) could definitely use some additional refactoring as well in this comment.

@clarfonthey clarfonthey added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 29, 2022
@Bryysen
Copy link
Contributor

Bryysen commented May 29, 2022

Another thing we might want to do is report all the duplicates in one error, instead of generating a new error for every collision. So currently for:

#[repr(u8)]   
enum Derp {    
    First = 1, 
    Second = 1,
    Third = 1, 
    Fourth = 1,
}

We get the errors:

error[E0081]: discriminant value `1` assigned more than once
  --> tst.rs:5:1
   |
5  | / enum Derp {
6  | |     First = 1,
   | |             - first assignment of `1`
7  | |     Second = 1,
   | |              - second assignment of `1`
8  | |     Third = 1,
9  | |     Fourth = 1,
10 | | }
   | |_^

error[E0081]: discriminant value `1` assigned more than once
  --> tst.rs:5:1
   |
5  | / enum Derp {
6  | |     First = 1,
   | |             - first assignment of `1`
7  | |     Second = 1,
8  | |     Third = 1,
   | |             - second assignment of `1`
9  | |     Fourth = 1,
10 | | }
   | |_^

error[E0081]: discriminant value `1` assigned more than once
  --> tst.rs:5:1
   |
5  | / enum Derp {
6  | |     First = 1,
   | |             - first assignment of `1`
7  | |     Second = 1,
8  | |     Third = 1,
9  | |     Fourth = 1,
   | |              - second assignment of `1`
10 | | }
   | |_^

error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0081`.

It would be nice if we instead got:

error[E0081]: discriminant value `1` assigned more than once
  --> tst.rs:5:1
   |
5  | / enum Derp {
6  | |     First = 1,
   | |             - 1st assignment of `1`
7  | |     Second = 1,
   | |              - 2nd assignment of `1`
8  | |     Third = 1,
   | |              - 3rd assignment of `1`
9  | |     Fourth = 1,
   | |              - 4th assignment of `1`
10 | | }
   | |_^

@estebank estebank added D-papercut Diagnostics: An error or lint that needs small tweaks. D-verbose Diagnostics: Too much output caused by a single piece of incorrect code. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. labels Jun 6, 2022
@rdzhaafar
Copy link
Contributor

@rustbot claim

@rdzhaafar
Copy link
Contributor

I don't have time to work on this, unfortunately. I did get all the errors to show up in the same error message, will paste the code here when I get to my computer.

@rustbot release-assignment

@Bryysen
Copy link
Contributor

Bryysen commented Jul 5, 2022

Thanks for the effort. I have a bunch of spare time going forwards so I can pick up where you left off

@rdzhaafar
Copy link
Contributor

@Bryysen

Perfect! My changes are in this fork

Basically I've factored out the duplicate-checking code into a separate function in rustc_typeck/check/check.rs. I've also added a couple of derive macros to make sure that Discr<>'s can be used in a hashmap.

The code now displays all duplicates in one message. What I haven't figured out, however, is how to check that a Variant<>'s predecessor is negative, since the value is stored as a u128.

Best of luck!

@Bryysen
Copy link
Contributor

Bryysen commented Jul 6, 2022

@rustbot claim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints D-papercut Diagnostics: An error or lint that needs small tweaks. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. D-verbose Diagnostics: Too much output caused by a single piece of incorrect code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants