Skip to content

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

Merged
bors merged 1 commit intorust-lang:masterfrom
de-vri-es:show-discriminant-before-overflow
Aug 22, 2021
Merged

Show discriminant before overflow in diagnostic for duplicate values.#87166
bors merged 1 commit intorust-lang:masterfrom
de-vri-es:show-discriminant-before-overflow

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
Contributor

rust-highfive commented Jul 15, 2021

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
Copy link
Contributor Author

@de-vri-es de-vri-es Jul 15, 2021

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

jackh726 commented Aug 20, 2021

Sorry for the wait here. @bors r+ rollup

@bors
Copy link
Collaborator

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

de-vri-es commented Aug 20, 2021

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