-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Manual map 7413 #7531
Manual map 7413 #7531
Conversation
r? @camsteffen (rust-highfive has picked a reviewer for you, use r? to override) |
pub enum CaptureKind { | ||
Value, | ||
Ref(Mutability), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm concerned that this is basically re-implementing ExprUseVisitor
. Have you tried using that instead? There maybe nuances there that we need to catch like fake_read
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It implements a subset of that. fake_read
doesn't need to be considered, that's needed to adjust things for the borrow checker.
I could make use of it. It would mean running two visitors, but I doubt the speed difference would be noticeable even with the extra work ExprUseVisitor
does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave it up to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After getting it to work properly with patterns and nested closures, should have just gone with ExprUseVisitor
. If capture_local_usage
is something that would be used somewhere else it's worth it, but I don't see it being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you saying that you might switch to ExprUseVisitor
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say keep it as is. It's already written. It shouldn't need to touched much until captures switch to working on fields which would require changes anyways.
99a3931
to
219a715
Compare
☔ The latest upstream changes (presumably #7558) made this pull request unmergeable. Please resolve the merge conflicts. |
Locals which can be partially moved created within the to-be-created closure shouldn't block the use of a closure
In some cases check if a borrow made in the scrutinee expression would prevent creating the closure used by `map`
* Captures by sub closures are now considered * Copy types are correctly borrowed by reference when their value is used * Fields are no longer automatically borrowed by value * Bindings in `match` and `let` patterns are now checked to determine how a local is captured
531eb6c
to
10c0460
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One minor code suggestion. Also please update the PR description to include a changelog entry for each lint changed. Then good to merge.
254974a
to
f0444d7
Compare
Thanks! @bors r+ |
📌 Commit f0444d7 has been approved by |
☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test |
fixes: #7413
This only fixes the specific problem from #7413, not the general case. The full fix requires interacting with the borrow checker to determine the lifetime of all the borrows made in the function. I'll open an issue about it later.
changelog: Don't suggest using
map
when the option is borrowed in the match, and also consumed in the arm.changelog: Locals declared within the would-be closure will not prevent the closure from being suggested in
manual_map
andmap_entry