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

Fix match_single_binding suggestion for assign expressions #8726

Merged
merged 1 commit into from
May 11, 2022
Merged

Fix match_single_binding suggestion for assign expressions #8726

merged 1 commit into from
May 11, 2022

Conversation

Serial-ATA
Copy link
Contributor

changelog: Fix suggestion for assign expressions in [match_single_binding]
closes: #8723

@rust-highfive
Copy link

r? @xFrednet

(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 Apr 20, 2022
@xFrednet
Copy link
Member

r? @dswij

dswij
dswij approved these changes Apr 24, 2022
View reviewed changes
Copy link
Member

@dswij dswij left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the changes!

@xFrednet

xFrednet
xFrednet reviewed Apr 26, 2022
View reviewed changes
Copy link
Member

@xFrednet xFrednet left a comment

Choose a reason for hiding this comment

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

The refactoring already looks good to me. However, I believe the suggestion should be changed a bit, to keep the same semantics 🙃

tests/ui/match_single_binding.stderr
Comment on lines +192 to +196
LL ~ let (pre, suf) = val.split_at(idx);
LL + val = {
LL + println!("{}", pre);
LL + suf
LL ~ };
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this suggest the following instead:

    val = {
        let (pre, suf) = val.split_at(idx);
        println!("{}", pre);
        suf
    };

Having the let <pattern> before the linted assignment can potentially shadow values. It also extends the lifetime of the pattern values. Now we can access pre and suf after the assignment. Which we couldn't previously.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should it? I know that's what the suggestion was in the issue, but that's how it behaved before I started this.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I was under the impression that that's how the suggestion behaved before. But the suggestion will introduce a change in behavior, and that's certainly not what we want.

I think @xFrednet's suggestion makes sense

Copy link
Member

Choose a reason for hiding this comment

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

Okay, then we should change the suggestion. @Serial-ATA Could you make this change? It should be fairly simple, from what I'm guessing. If not, we can also create a new issue and merge this as it is. Your changes are already a solid improvement 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I'll try it out 🙂

@bors
Copy link
Contributor

bors commented May 9, 2022

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

@xFrednet
Copy link
Member

xFrednet commented May 10, 2022 •
edited
Loading

Hey @Serial-ATA, what's the status on this PR? The last unresolved comment was an optional improvement. Do we want to get this merged and created an issue for this possible enhancement?

If so, you would just need to rebase, as all changes looked good to us 🙃

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels May 10, 2022
@Serial-ATA
Copy link
Contributor Author

Completely forgot about this 😄. I tried for some time to make the change, but I couldn't get it to work reliably, unless I'm missing something obvious. I'll make an issue and come back to it soon enough if no one claims it.

@Serial-ATA Serial-ATA force-pushed the issue-8723 branch 2 times, most recently from 79db0f7 to df82b55 Compare May 10, 2022 22:35
@Serial-ATA
Copy link
Contributor Author

@rustbot ready

@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 from the author. (Use `@rustbot ready` to update this status) labels May 10, 2022
@xFrednet
Copy link
Member

LGTM, thank you for also creating the issue regarding the suggestion 🙃

Thank you @dswij for the initial review 👍

@bors r=dswij,xFrednet

@bors
Copy link
Contributor

bors commented May 11, 2022

📌 Commit 554dc41 has been approved by dswij,xFrednet

@bors
Copy link
Contributor

bors commented May 11, 2022

⌛ Testing commit 554dc41 with merge 6889d09...

@bors
Copy link
Contributor

bors commented May 11, 2022

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: dswij,xFrednet
Pushing 6889d09 to master...

@bors bors merged commit 6889d09 into rust-lang:master May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dswij dswij dswij approved these changes

@xFrednet xFrednet xFrednet approved these changes

Assignees

@xFrednet xFrednet

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

match_single_binding suggest wrong code
6 participants
@Serial-ATA @rust-highfive @xFrednet @bors @dswij @rustbot