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

Further improve error message for E0081 #100238

Merged
merged 4 commits into from
Aug 9, 2022
Merged

Further improve error message for E0081 #100238

merged 4 commits into from
Aug 9, 2022

Conversation

Bryysen
Copy link
Contributor

@Bryysen Bryysen commented Aug 7, 2022

Closes #97533

Multiple duplicate assignments of the same discriminant are now reported
in the samme error. We now point out the incrementation start point for
discriminants that are not explicitly assigned that are also duplicates.
Removed old test related to E0081 that is now covered by error-codes/E0081.rs.
Also refactored parts of the `check_enum` function.
@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 7, 2022
@rust-highfive
Copy link
Collaborator

r? @cjgillot

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 7, 2022
Copy link
Contributor

@cjgillot cjgillot left a comment

Choose a reason for hiding this comment

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

Thanks @Bryysen!
Could you add a little PR message to explain how you are improving the message?
I left a few comments in code too.

compiler/rustc_typeck/src/check/check.rs Show resolved Hide resolved
compiler/rustc_typeck/src/check/check.rs Show resolved Hide resolved
compiler/rustc_typeck/src/check/check.rs Show resolved Hide resolved
compiler/rustc_typeck/src/check/check.rs Show resolved Hide resolved
compiler/rustc_typeck/src/check/check.rs Outdated Show resolved Hide resolved
src/test/ui/error-codes/E0081.stderr Show resolved Hide resolved
@Bryysen
Copy link
Contributor Author

Bryysen commented Aug 7, 2022

Sorry, was too hasty when I made the PR 😄.

So in the old algorithm we would do a lazy evaluation of the discriminant values, and store them in a vec as we go. Once we evaluate a new discriminant, we would compare its value to the previously computed discriminants and if it matched any of them, we error with a duplicate. The issue with this approach is that we can't know ahead of time all the duplicates for a given discriminant N due to the lazy evaluation, and so we would generate a seperate error for every duplicate match. This comment on the linked issue illustrates this.

My solution was to just eagerly evaluate all the discriminants, and then do a similar comparison between all discriminants as we did before. When we hit a duplicate this time, we add to the error message the span + value and shuffle that discriminant out of the vec to avoid erroring on it multiple times, then continue the cycle to round up any other duplicates for that discriminant value.

Complexity wise, this should be no worse than the old algorithm (unless I've misjugded something). Hope that clears some things up. I'll take a second to go through the review comments.

Could you add a little PR message to explain how you are improving the message?

The biggest improvement is reducing the verbosity for multiple duplicates of the same discriminant value. Another small improvement is that we point out where (and how) an implicit duplicate is incremented from, which can hopefully help clear confusion for new users:

#[repr(i8)]                                                  
enum MyEnum {                                                      
    V0 = 0,                                                  
    V1 = -2,                                                                                   
    V2,                                
    V3,               
    V4 = 0,
} 
error[E0081]: discriminant value `0` assigned more than once
  --> tst.rs:5:1
   |
5  | enum MyEnum {
   | ^^^^^^^^^^^
6  |     V0 = 0,
   |          - `0` assigned here
7  |     V1 = -2,
   |     ------- discriminant for `V3` incremented from this startpoint (`V1` + 2 variant later => `V3` = 0)
8  |     V2,
9  |     V3,
   |     -- `0` assigned here
10 |     V4 = 0,
   |          - `0` assigned here

error: aborting due to previous error

However, i'm unsure about the discriminant for `V3` incremented from this startpoint (`V1` + 2 variant later => `V3` = 0) part, maybe there is a better way to word this?

@cjgillot
Copy link
Contributor

cjgillot commented Aug 8, 2022

Thanks @Bryysen!
@bors r+

@bors
Copy link
Contributor

bors commented Aug 8, 2022

📌 Commit 74e71da has been approved by cjgillot

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-review Status: Awaiting review from the assignee but also interested parties. labels Aug 8, 2022
@Bryysen
Copy link
Contributor Author

Bryysen commented Aug 8, 2022

Would you like me to squash the commits first @cjgillot ? And thanks for taking the time to review!

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 9, 2022
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#100163 (Refactor: remove an unnecessary string search)
 - rust-lang#100212 (Remove more Clean trait implementations)
 - rust-lang#100238 (Further improve error message for E0081)
 - rust-lang#100268 (Add regression test for rust-lang#79148)
 - rust-lang#100294 (Update Duration::as_secs doc to point to as_secs_f64/32 for including fractional part)
 - rust-lang#100303 (:arrow_up: rust-analyzer)

Failed merges:

 - rust-lang#100281 (Remove more Clean trait implementations)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d63d2bd into rust-lang:master Aug 9, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve diagnostics for duplicate enum discriminants
5 participants