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

fix: Don't skip closure captures after let-else #15625

Merged
merged 4 commits into from Sep 17, 2023

Conversation

jDomantas
Copy link
Contributor

@jDomantas jDomantas commented Sep 17, 2023

As I understand that return was left there by accident. It caused capture analysis to skip the rest of the block after a let-else, and then missed captures caused incorrect results in borrowck, closure hints, layout calculation, etc.

Fixes #15623

I didn't understand why I using the example from #15623 as-is doesn't work - I don't get the warnings unless I remove the call_me() call, even on the same commit as my own RA version which does show those warnings.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 17, 2023
@HKalbasi
Copy link
Member

Thanks! Can you also add a test for captures themselves? Something like this:

let a = 2;
let b = 3;
let c = 5;
|| {
  let x = a else { return b; };
  let y = c;
};

The closure should capture all variables. You can (ab)use hover tests as IIRC there is no infrastructure for testing closure captures directly.

I didn't understand why I using the example from #15623 as-is doesn't work - I don't get the warnings unless I remove the call_me() call, even on the same commit as my own RA version which does show those warnings.

You probably need to add minicore flags needed for Fn traits.

@jDomantas
Copy link
Contributor Author

I added an indirect test for captures via layout test, if that's not ok I can look into redoing it with a hover test a bit later.

@HKalbasi
Copy link
Member

That works as well. We probably need dedicated capture tests in future.
@bors r+

@bors
Copy link
Collaborator

bors commented Sep 17, 2023

📌 Commit a961068 has been approved by HKalbasi

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Sep 17, 2023

⌛ Testing commit a961068 with merge 0566644...

@bors
Copy link
Collaborator

bors commented Sep 17, 2023

☀️ Test successful - checks-actions
Approved by: HKalbasi
Pushing 0566644 to master...

@bors bors merged commit 0566644 into rust-lang:master Sep 17, 2023
10 checks passed
@jDomantas jDomantas deleted the domantas/fix-15623 branch September 18, 2023 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False positive "variable does not need to be mutable"
4 participants