Skip to content

suggestion for attempted integer identifier in patterns #106591

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
Jan 17, 2023
Merged

suggestion for attempted integer identifier in patterns #106591

merged 1 commit into from
Jan 17, 2023

Conversation

@Ezrashaw
Copy link
Contributor

@Ezrashaw Ezrashaw commented Jan 8, 2023

Fixes #106552

Implemented a suggestion on E0005 that occurs when no bindings are present and the pattern is a literal integer.

@rustbot
Copy link
Collaborator

rustbot commented Jan 8, 2023

r? @wesleywiser

(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 Jan 8, 2023
Mark-Simulacrum
Mark-Simulacrum reviewed Jan 11, 2023
View reviewed changes
compiler/rustc_mir_build/src/thir/pattern/check_match.rs Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Open to this, but it seems to me that I haven't really wanted to define an integer-named variable ~ever. Maybe instead of a help: text with _int we can just note: that identifiers cannot start with digits? I think in 99% of cases they probably wanted to use if let or similar, so adding a second suggestion may be more confusing.

At least to me if I see two suggestions I think I'd think "which is better?" before thinking "oh, I wanted an if, the second one is irrelevant".

Copy link
Contributor Author

@Ezrashaw Ezrashaw Jan 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Obviously this is somewhat targeted at new developers but, personally, I've never let-matched on a integer literal before. I'd use a match statement instead which allows better handling of the default case, etc. FWIW, I think that if a user tries to do this (again, IMO very uncommon) then really meant to assign a value instead.

@Ezrashaw Ezrashaw force-pushed the attempted-integer-identifer branch from a94e510 to b4be8b8 Compare January 12, 2023 01:36
@Ezrashaw
Copy link
Contributor Author

@Mark-Simulacrum I've rebased (moved the tests), depending on your review comment, it's good to go.

@bors
Copy link
Collaborator

bors commented Jan 12, 2023

☔ The latest upstream changes (presumably #106760) made this pull request unmergeable. Please resolve the merge conflicts.

albertlarsan68
albertlarsan68 requested changes Jan 12, 2023
View reviewed changes
@Ezrashaw Ezrashaw force-pushed the attempted-integer-identifer branch 2 times, most recently from e6bec6a to 63bd68f Compare January 13, 2023 01:33
@rustbot rustbot added the A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic label Jan 13, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jan 13, 2023

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @JohnTitor, @estebank, @TaKO8Ki

estebank
estebank reviewed Jan 13, 2023
View reviewed changes
tests/ui/pattern/issue-106552.stderr Outdated
Comment on lines +19 to +31
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at this with fresh eyes, shouldn't we be suggesting if let here? 🤔

Copy link
Contributor Author

@Ezrashaw Ezrashaw Jan 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I haven't touched the if let and else suggestion code, so I haven't really looked at this but: My understanding is that this situation is a bit like an alternative if let. You can bind the value, and if it doesn't match, the else branch is called (which must diverge).

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not asking you to fix it now necessarily, just leaving a paper trail of my thinking so we don't forget to fix it in the future :)

estebank
estebank reviewed Jan 13, 2023
View reviewed changes
estebank
estebank reviewed Jan 13, 2023
View reviewed changes
@estebank
Copy link
Contributor

r? @estebank

r=me after addressing the nit picks

@rustbot rustbot assigned estebank and unassigned wesleywiser Jan 13, 2023
@Ezrashaw Ezrashaw force-pushed the attempted-integer-identifer branch from 63bd68f to 1babece Compare January 13, 2023 23:51
@Ezrashaw Ezrashaw mentioned this pull request Jan 15, 2023
Merged
@jyn514
Copy link
Member

jyn514 commented Jan 17, 2023

@bors r=Estebank

@bors
Copy link
Collaborator

bors commented Jan 17, 2023

📌 Commit 1babece has been approved by Estebank

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 Jan 17, 2023
@matthiaskrgr matthiaskrgr mentioned this pull request Jan 17, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 17, 2023
@bors
Auto merge of rust-lang#106966 - matthiaskrgr:rollup-e34pevi, r=matth…
85357e3 
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#106591 (suggestion for attempted integer identifier in patterns)
 - rust-lang#106712 (make error emitted on `impl &Trait` nicer)
 - rust-lang#106829 (Unify `Opaque`/`Projection` handling in region outlives code)
 - rust-lang#106869 (rustdoc: remove redundant item kind class from `.item-decl > pre`)
 - rust-lang#106949 (ConstBlocks are poly if their substs are poly)
 - rust-lang#106953 (Document `EarlyBinder::subst_identity` and `skip_binder`)
 - rust-lang#106958 (Don't add A-bootstrap to PRs modifying Cargo.lock)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f740442 into rust-lang:master Jan 17, 2023
@rustbot rustbot added this to the 1.68.0 milestone Jan 17, 2023
@Ezrashaw Ezrashaw deleted the attempted-integer-identifer branch January 17, 2023 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@estebank estebank estebank left review comments

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

@albertlarsan68 albertlarsan68 albertlarsan68 requested changes

Assignees

@estebank estebank

Labels

A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic 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.

Confusing error with integer identifers

8 participants

@Ezrashaw @rustbot @bors @estebank @jyn514 @Mark-Simulacrum @albertlarsan68 @wesleywiser