Skip to content

Refine when invalid prefix case error arises #105161

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
Merged
merged 1 commit into from
Dec 14, 2022
Merged

Refine when invalid prefix case error arises #105161

merged 1 commit into from
Dec 14, 2022

Conversation

cassaundra
Copy link
Contributor

Fix cases where the "invalid base prefix for number literal" error arises with suffixes that look erroneously capitalized but which are actually invalid.

@rustbot
Copy link
Collaborator

rustbot commented Dec 2, 2022

r? @fee1-dead

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 2, 2022
@cassaundra
Copy link
Contributor Author

r? @nnethercote

@rustbot rustbot assigned nnethercote and unassigned fee1-dead Dec 2, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@cassaundra cassaundra force-pushed the numeric-literal-error branch from 6a894ca to 33d1f80 Compare December 2, 2022 03:26
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@cassaundra
Copy link
Contributor Author

Ah, I apparently lost some progress in an Emacs crash. Will fix these errors.

nnethercote
nnethercote reviewed Dec 2, 2022
View reviewed changes
compiler/rustc_session/src/errors.rs Outdated
@@ -324,7 +341,8 @@ pub fn report_lit_error(sess: &ParseSess, err: LitError, lit: token::Lit, span:
if looks_like_width_suffix(&['i', 'u'], &suf) {
// If it looks like a width, try to be helpful.
sess.emit_err(InvalidIntLiteralWidth { span, width: suf[1..].into() });
} else if let Some(fixed) = fix_base_capitalisation(suf) {
} else if let Some(fixed) = fix_base_capitalisation(suf)
&& looks_like_base_prefixed_number(symbol.as_str(), fix_base_capitalisation(suf)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The repeated call to fix_base_capitalisation is a bit clumsy. I think you can change looks_like_base_prefix_number to look for an upper-case X/O/B, and then call just it in the condition, and then call fix_base_capitalisation in the body?

nnethercote
nnethercote reviewed Dec 2, 2022
View reviewed changes
compiler/rustc_session/src/errors.rs Outdated
@@ -291,6 +291,23 @@ pub fn report_lit_error(sess: &ParseSess, err: LitError, lit: token::Lit, span:
s.len() > 1 && s.starts_with(first_chars) && s[1..].chars().all(|c| c.is_ascii_digit())
}

// Check if a prefix and suffix look correct irrespective of base.
fn looks_like_base_prefixed_number(prefix: &str, suffix: &str) -> bool {
let base = match suffix.chars().next().unwrap() {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can avoid the repeated suffix.chars() in this function. Do let chars = suffix.chars(), then do chars.next().unwrap() for the base check, then do chars().filter(...)... in the second part (thus avoiding the skip(1)).

@cassaundra cassaundra force-pushed the numeric-literal-error branch from 33d1f80 to 0f548f6 Compare December 6, 2022 04:11
@cassaundra
Copy link
Contributor Author

Issues mentioned have been fixed! Thanks for the review so far.

@nnethercote
Copy link
Contributor

Apologies: due to the combination of communication in this PR and via Zulip, and some initial incorrect suggestions I made, you've ended up making changes that I didn't intend you to make.

Let me try to clarify:

  • You should bring looks_like_base_prefixed_number back, it's needed. Because in the 0XYZ case, it should not suggest 0xYZ as an alternative, because Y and Z aren't valid hex digits. Likewise for 0OPQ and 0BCD.
  • Once you do that, as per my comment above, it should be possible to structure the code such that fix_base_capitalisation is only called once, not twice as was done in the original version.
  • Also, my comment above about the repeated suffix.chars() will apply then, too.

Sorry! I hope that's clearer.

@cassaundra cassaundra force-pushed the numeric-literal-error branch from 0f548f6 to e6dd291 Compare December 7, 2022 22:33
@cassaundra
Copy link
Contributor Author

Apologies for the confusion! I went ahead and merged the behavior of fix_base_capitalisation and looks_like_base_prefixed_number to clean up the code considerably.

@cassaundra cassaundra force-pushed the numeric-literal-error branch 3 times, most recently from c12d230 to f59655f Compare December 7, 2022 23:17
nnethercote
nnethercote reviewed Dec 8, 2022
View reviewed changes
Copy link
Contributor

@nnethercote nnethercote left a comment

Choose a reason for hiding this comment

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

Almost there! Just two minor suggestions.

@cassaundra
Refine when invalid prefix case error arises
52a9280 
Fix cases where the "invalid base prefix for number literal" error arises with
suffixes that look erroneously capitalized but which are in fact invalid.
@cassaundra cassaundra force-pushed the numeric-literal-error branch from f59655f to 52a9280 Compare December 13, 2022 03:33
@cassaundra
Copy link
Contributor Author

Okay, those changes have been applied.

@nnethercote
Copy link
Contributor

Looks good, thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented Dec 13, 2022

📌 Commit 52a9280 has been approved by nnethercote

It is now in the queue for this repository.

@bors bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 13, 2022
@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 13, 2022
@nnethercote
Copy link
Contributor

This is small enough to be easily included in a rollup.

@bors rollup

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Dec 13, 2022
@Dylan-DPC
Rollup merge of rust-lang#105161 - cassaundra:numeric-literal-error, …
e3f6464 
…r=nnethercote

Refine when invalid prefix case error arises

Fix cases where the "invalid base prefix for number literal" error arises with suffixes that look erroneously capitalized but which are actually invalid.
@Dylan-DPC Dylan-DPC mentioned this pull request Dec 13, 2022
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Dec 14, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 14, 2022
@bors
Auto merge of rust-lang#105686 - matthiaskrgr:rollup-bedfk3j, r=matth…
7bdda8f 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#103644 (Add docs for question mark operator for Option)
 - rust-lang#105161 (Refine when invalid prefix case error arises)
 - rust-lang#105491 (Illegal sized bounds: only suggest mutability change if needed)
 - rust-lang#105502 (Suggest impl in the scenario of typo with fn)
 - rust-lang#105523 (Suggest `collect`ing into `Vec<_>`)
 - rust-lang#105595 (Suggest dereferencing receiver arguments properly)
 - rust-lang#105611 (fold instead of obliterating args)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8ed0384 into rust-lang:master Dec 14, 2022
@rustbot rustbot added this to the 1.68.0 milestone Dec 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nnethercote nnethercote nnethercote left review comments

Assignees

@nnethercote nnethercote

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.68.0
Development

Successfully merging this pull request may close these issues.
6 participants
@cassaundra @rustbot @rust-log-analyzer @nnethercote @bors @fee1-dead