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

If an integer is entered with an upper-case base prefix (0Xbeef, 0O755, 0B1010), suggest to make it lowercase #93019

Merged
merged 2 commits into from
Feb 1, 2022
Merged

If an integer is entered with an upper-case base prefix (0Xbeef, 0O755, 0B1010), suggest to make it lowercase #93019

merged 2 commits into from
Feb 1, 2022

Conversation

5225225
Copy link
Contributor

@5225225 5225225 commented Jan 17, 2022

The current error for this case isn't really great, it just complains about the whole thing past the 0 being an invalid suffix.

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 17, 2022
@rust-highfive
Copy link
Collaborator

r? @wesleywiser

(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 Jan 17, 2022
wesleywiser
wesleywiser reviewed Jan 18, 2022
View reviewed changes
compiler/rustc_parse/src/parser/expr.rs Outdated Show resolved Hide resolved
compiler/rustc_parse/src/parser/expr.rs Outdated Show resolved Hide resolved
@wesleywiser
Copy link
Member

This is really nice! I think the main thing we need are some UI tests to cover these cases. Let me know if you need any help with that! 🙂

@5225225
Copy link
Contributor Author

5225225 commented Jan 18, 2022

Thanks!

I mainly made this now so I can't then forget about it. I've left PRs in progress for Far Too Long a few times.

I'll make the wording changes you suggested, then add tests for it, and then it should be good to go.

I was debating if I should only make this suggestion if the resulting literal would parse, but will probably decide against that for two reasons

  1. That's hard. Or, at least, I couldn't find any way to Just Parse a &str into a Lit in a way that doesn't involve reimplementing the numeric parser but worse.
  2. Going from 0B101I to 0b101I makes the error message more useful, even if it's still an error.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@5225225
Copy link
Contributor Author

5225225 commented Jan 27, 2022

UI tests here don't work and it's too late in the night for me to work out why the note is considered expected, lol.

Will look at the error annotation syntax some more and hopefully get this done over the weekend.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@5225225 5225225 force-pushed the uppercase-suffix branch 2 times, most recently from 50e7adb to fa38c42 Compare January 30, 2022 15:55
@5225225
Copy link
Contributor Author

5225225 commented Jan 30, 2022

Okay, bikeshed the error messages with some people and came up with this.

Marking this as ready to review because I'm happy with the diagnostic as-is.

One difference between current and previous is that the previous diagnostic included the actual number in the error: text. I figured that it's not needed, since we're pointing to it in a span.

@5225225 5225225 marked this pull request as ready for review January 30, 2022 15:58
@5225225 5225225 changed the title [WIP] If an integer is entered with an upper-case base prefix (0Xbeef, 0O755, 0B1010), suggest to make it lowercase If an integer is entered with an upper-case base prefix (0Xbeef, 0O755, 0B1010), suggest to make it lowercase Jan 30, 2022
wesleywiser
wesleywiser approved these changes Jan 31, 2022
View reviewed changes
Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

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

This looks great to me! I'd just like to see the test expanded for the cases I mentioned before we merge.

src/test/ui/numeric/uppercase-base-prefix.rs Outdated Show resolved Hide resolved
@wesleywiser
Copy link
Member

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jan 31, 2022

📌 Commit ec3b711 has been approved by wesleywiser

@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 Jan 31, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 31, 2022
@matthiaskrgr
Rollup merge of rust-lang#93019 - 5225225:uppercase-suffix, r=wesleyw…
561f997 
…iser

If an integer is entered with an upper-case base prefix (0Xbeef, 0O755, 0B1010), suggest to make it lowercase

The current error for this case isn't really great, it just complains about the whole thing past the `0` being an invalid suffix.
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 31, 2022
Closed
@ehuss ehuss mentioned this pull request Feb 1, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 1, 2022
@bors
Auto merge of rust-lang#93534 - ehuss:rollup-9ecozo9, r=ehuss
93e8201 
Rollup of 9 pull requests

Successful merges:

 - rust-lang#91343 (Fix suggestion to slice if scrutinee is a `Result` or `Option`)
 - rust-lang#93019 (If an integer is entered with an upper-case base prefix (0Xbeef, 0O755, 0B1010), suggest to make it lowercase)
 - rust-lang#93090 (`impl Display for io::ErrorKind`)
 - rust-lang#93456 (Remove an unnecessary transmute from opaque::Encoder)
 - rust-lang#93492 (Hide failed command unless in verbose mode)
 - rust-lang#93504 (kmc-solid: Increase the default stack size)
 - rust-lang#93513 (Allow any pretty printed line to have at least 60 chars)
 - rust-lang#93532 (Update books)
 - rust-lang#93533 (Update cargo)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit d7c0b4f into rust-lang:master Feb 1, 2022
@rustbot rustbot added this to the 1.60.0 milestone Feb 1, 2022
@5225225 5225225 deleted the uppercase-suffix branch August 18, 2022 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wesleywiser wesleywiser wesleywiser approved these changes

Assignees

@wesleywiser wesleywiser

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
Milestone
1.60.0
Development

Successfully merging this pull request may close these issues.
6 participants
@5225225 @rust-highfive @wesleywiser @rust-log-analyzer @bors @rustbot