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.

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

Pattern Migration 2024: suggest nicer patterns #136496

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

dianne
Copy link
Contributor

@dianne dianne commented Feb 3, 2025

This is based on #136817. The new commits start with removing a comment on the tests. The goal of the refactors in the following commits is to eventually [produce suggestions which remove reference patterns], fixing #136047 in a more thorough way.

Here's a simplified version of how it works, for reference (it's not actually split into discrete phases, but these are the conceptual steps):

  1. Try removing all reference patterns and binding modifiers; if this doesn't change any bindings, we can suggest the resultant pattern.
  2. If a binding would be changed by this, we give it an explicit binding modifier. This requires adding explicit reference patterns for any dereferences above it (which may have previously been inserted implicitly by match ergonomics). This may change other bindings, in which case we'll need to suggest binding modifiers for them, which may require suggesting more reference patterns, etc.. We suggest whatever we end up with once we've fixed all such inconsistencies.
  3. Simplify &ref x to x when it's convenient. This isn't quite complete; full simplification would require potentially rewriting &ref x @ ... to x @ &..., which would add a bit of complexity, though I can implement that too if desired.

Unfortunately, it ended up being more code than I'd hoped, so I understand if it's unappealing from a maintenance perspective. If nothing else, finalizing match ergonomics 2024 should open up significant simplifications, and I'm happy to help maintain it until then. There's a chance some common bookkeeping could be shared between other diagnostics/suggestions too; I have a couple in mind, but I haven't checked whether it'd be less complexity overall.

Relevant tracking issue: #131414

This doesn't aim to provide viable suggestions under the feature gates ref_pat_eat_one_layer_2024 or ref_pat_eat_one_layer_2024_structural; some of the suggestions in the test output for those features are wrong and/or not as pretty as they could be. I've included FIXME comments providing more detail. Producing suggestions targeting the match ergonomics implemented by those features would actually be significantly simpler than what this PR does, but since it would require different1 logic, I'm inclined to save that for later. cc @Nadrieril, do any other feature gates come to mind that might require tweaks to this?

@rustbot label A-diagnostics A-patterns A-edition-2024

Footnotes

  1. It would only require a small tweak to correct the suggestions in the ref_pat_eat_one_layer_2024 and ref_pat_eat_one_layer_2024_structural tests. However, a different approach to the one in this PR would let us use simpler logic to give more local suggestions. I figure if we're adding logic specific to new match ergonomics, we may as well not (slightly) complicate this version of it further when we could be doing something better.

@rustbot
Copy link
Collaborator

rustbot commented Feb 3, 2025

r? @wesleywiser

rustbot has assigned @wesleywiser.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@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 Feb 3, 2025
@rustbot
Copy link
Collaborator

rustbot commented Feb 3, 2025

Some changes occurred in match checking

cc @Nadrieril

@rustbot rustbot added A-diagnostics Area: Messages for errors, warnings, and lints A-edition-2024 Area: The 2024 edition A-patterns Relating to patterns and pattern matching labels Feb 3, 2025
@Nadrieril
Copy link
Member

r? @Nadrieril

Hmm, would it be possible to first implement a very basic fix which is: if we find ref x under a ref binding mode (or ref mut/ref mut), instead of suggesting the desugaring we just suggest to remove it? Or does that require the kind of logic you have here? This too I was hoping to backport before edition 2024 makes it to stable.

@rustbot rustbot assigned Nadrieril and unassigned wesleywiser Feb 4, 2025
@dianne
Copy link
Contributor Author

dianne commented Feb 5, 2025

That should be possible, but it would require some logic. The nice thing is that tracking the default binding mode in the pattern the user wrote is already done by 203d310, so I think it could be built on top of that. For minimal added logic, we could give up on removing binding modifiers if we suggest adding any reference patterns; we'd only suggest adding reference patterns if a &/&mut eats an inherited reference or we encounter a ref x binding under a ref mut dbm (or ref mut x under a ref, but that's a borrowck error anyway). For more precision beyond that, I think we'd have to start tracking ancestors and maybe also descendants, like this PR does: if we suggest adding a shared & pattern, the dbm in its descendants could change from ref to ref mut, so we'd at least need to know to bail when that happens. Accounting for that alone is maybe most of the complexity of this PR.

Edit: Thought about this some more. I think it doesn't even need to be based on 203d310. I duplicated the default binding mode tracking logic in that since I didn't want to maintain a hashmap of default binding mode changes in HIR typeck on the happy path. But that's irrelevant here; this doesn't need those spans. I'll open a separate PR.

Edit 2: It ended up being more convenient to base that off of #136475 anyway. Here's the new PR: #136577.

@bors
Copy link
Contributor

bors commented Feb 5, 2025

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

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 7, 2025
…fication, r=Nadrieril

Pattern Migration 2024: try to suggest eliding redundant binding modifiers

This is based on rust-lang#136475. Only the last commit is new.

This is a simpler, more restrictive alternative to rust-lang#136496, meant to partially address rust-lang#136047. If a pattern can be migrated to Rust 2024 solely by removing redundant binding modifiers, this will make that suggestion; otherwise, it uses the old suggestion of making the pattern fully explicit.

Relevant tracking issue: rust-lang#131414

`@rustbot` label A-diagnostics A-patterns A-edition-2024

r? `@Nadrieril`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 7, 2025
…fication, r=Nadrieril

Pattern Migration 2024: try to suggest eliding redundant binding modifiers

This is based on rust-lang#136475. Only the last commit is new.

This is a simpler, more restrictive alternative to rust-lang#136496, meant to partially address rust-lang#136047. If a pattern can be migrated to Rust 2024 solely by removing redundant binding modifiers, this will make that suggestion; otherwise, it uses the old suggestion of making the pattern fully explicit.

Relevant tracking issue: rust-lang#131414

``@rustbot`` label A-diagnostics A-patterns A-edition-2024

r? ``@Nadrieril``
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 7, 2025
Rollup merge of rust-lang#136577 - dianne:simple-pat-migration-simplification, r=Nadrieril

Pattern Migration 2024: try to suggest eliding redundant binding modifiers

This is based on rust-lang#136475. Only the last commit is new.

This is a simpler, more restrictive alternative to rust-lang#136496, meant to partially address rust-lang#136047. If a pattern can be migrated to Rust 2024 solely by removing redundant binding modifiers, this will make that suggestion; otherwise, it uses the old suggestion of making the pattern fully explicit.

Relevant tracking issue: rust-lang#131414

``@rustbot`` label A-diagnostics A-patterns A-edition-2024

r? ``@Nadrieril``
…vements

The following commits are the possible future improvements.
This will let us propagate changes to the default binding mode when
suggesting adding `&` patterns.
This lets us revise the suggestion on-the-fly as we discover which
reference patterns and binding modifiers are necessary vs. which may be
omitted.
A slight refactor: we'll always want to pass the mutability in to
`push_deref` to determine the default binding mode when a pattern is
removed, so this eliminates the redundancy of having both that and the
user's dbm as separate arguments.
@dianne
Copy link
Contributor Author

dianne commented Feb 10, 2025

I've rebased on top of #136817 (which puts pattern migration in its own file) and pulled the commit structure apart a bit. Rebasing onto the other diagnostic improvements has made this a bit bigger in total than it was before, but hopefully each of the refactors should at least be a bit more digestible than they were initially.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-edition-2024 Area: The 2024 edition A-patterns Relating to patterns and pattern matching 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants