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.

Sign up for GitHub

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

Jump to bottom

Show discriminant before overflow in diagnostic for duplicate values. #87166

Merged
merged 1 commit into from
Aug 22, 2021
Merged

Show discriminant before overflow in diagnostic for duplicate values. #87166

merged 1 commit into from
Aug 22, 2021

Conversation

de-vri-es
Copy link
Contributor

@de-vri-es de-vri-es commented Jul 15, 2021 •
edited
Loading

This PR adds the value before overflow for explicit discriminant values in the error for duplicate discriminant values.
I found it rather confusing to see only the overflowed value.

It only does this for literals, since overflows in const evaluated arithmetic are already a hard error.

This is my first PR to the compiler, so please let me know if the implementation can be improved :)

Before:
image

After:
image

@rustbot rustbot added A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jul 15, 2021
@rust-highfive
Copy link
Collaborator

r? @jackh726

(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 Jul 15, 2021
@de-vri-es de-vri-es force-pushed the show-discriminant-before-overflow branch from 99210f3 to 6cd6fc3 Compare July 15, 2021 21:00
de-vri-es
de-vri-es commented Jul 15, 2021
View reviewed changes
compiler/rustc_typeck/src/check/check.rs Outdated
}
}
}
format!("{}", evaluated)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note: I also removed the backticks around the number. They seemed unnecessary to me: the value is always a decimal number, which already stands out from normal text quite well. But I can revert this if desired.

jackh726
jackh726 requested changes Jul 27, 2021
View reviewed changes
Copy link
Member

@jackh726 jackh726 left a comment

Choose a reason for hiding this comment

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

So, I'd like to actually see a test where the first discriminant overflows (like in the OP). And I think that the backticks around the value should remain (backticks generally specify a value or code).

And finally (this one is a bit more opinionated): I feel like the exact number here isn't super helpful. It might be better to say some like "overflowed from usize"

@jackh726 jackh726 added S-waiting-on-author 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 Jul 27, 2021
@de-vri-es de-vri-es force-pushed the show-discriminant-before-overflow branch from 6cd6fc3 to 4c0ff4d Compare July 28, 2021 08:04
@de-vri-es
Copy link
Contributor Author

de-vri-es commented Jul 28, 2021 •
edited
Loading

So, I'd like to actually see a test where the first discriminant overflows (like in the OP). And I think that the backticks around the value should remain (backticks generally specify a value or code).

Makes sense. I added another test case in src/test/ui/error-codes/E0081.rs. Let me know if the test case should be in a new file instead, I'll move it.

I added the backticks back, and I removed the overflow value from the main error message, leaving it only in the notes. With that, it now looks like this:

image

And finally (this one is a bit more opinionated): I feel like the exact number here isn't super helpful. It might be better to say some like "overflowed from usize"

Hmm, I actually like to see the value before overflow. It lets me know that neither me nor the compiler have gone mad and we're both really talking about the same number.

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 28, 2021
@JohnCSimon JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 15, 2021
@jackh726
Copy link
Member

Sorry for the wait here. @bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 20, 2021

📌 Commit 4c0ff4d has been approved by jackh726

@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 20, 2021
@de-vri-es
Copy link
Contributor Author

Thanks for the review! :)

@GuillaumeGomez GuillaumeGomez mentioned this pull request Aug 22, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 22, 2021
@bors
Auto merge of rust-lang#88240 - GuillaumeGomez:rollup-wdom91m, r=Guil…
af14075 
…laumeGomez

Rollup of 7 pull requests

Successful merges:

 - rust-lang#86747 (Improve wording of the `drop_bounds` lint)
 - rust-lang#87166 (Show discriminant before overflow in diagnostic for duplicate values.)
 - rust-lang#88077 (Generate an iOS LLVM target with a specific version)
 - rust-lang#88164 (PassWrapper: adapt for LLVM 14 changes)
 - rust-lang#88211 (cleanup: `Span::new` -> `Span::with_lo`)
 - rust-lang#88229 (Suggest importing the right kind of macro.)
 - rust-lang#88238 (Stop tracking namespace in used_imports.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 3b1e7b1 into rust-lang:master Aug 22, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jackh726 jackh726 jackh726 requested changes

Assignees

@jackh726 jackh726

Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. 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
Milestone
1.56.0
Development

Successfully merging this pull request may close these issues.
6 participants
@de-vri-es @rust-highfive @jackh726 @bors @JohnCSimon @rustbot