-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
NLL lacks various special case handling of closures #54976
NLL lacks various special case handling of closures #54976
Conversation
--> $DIR/issue-12127.rs:18:39 | ||
| | ||
LL | let f = to_fn_once(move|| do_it(&*x)); | ||
| ^ |
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.
This test doesn't have this note in the AST borrow checker. A brief glance made me think it was right but I'm not too sure.
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.
The closure only implements FnOnce
because of to_fn_once
, otherwise it would be a Fn
closure (it doesn't drop/move x
when called).
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.
Looks good but the labeling of freevars is too eager.
freevar={:?} upvar_ty={:?}", | ||
freevar, upvar_ty, | ||
); | ||
if !upvar_ty.is_region_ptr() { |
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.
This is too broad -- the code is only looking at the type of the value that was captured (and crudely at that), but it needs to take into consideration how the value is used (e.g., the &*x
in the issue-12127.rs
does not require moving x
).
There are a few ways to handle this.
One way would be to go through the point where the closure is created and look at whether it "moves" its arguments. That seems a bit complex though.
Another way would be to consult the closure_kind_origins
map in the typeck-tables. This map indicates why a given closure has the kind it does (and in particular it will have an entry if this is due to a user of some freevar).
See the code in librustc_borrowck
for an example:
rust/src/librustc_borrowck/borrowck/mod.rs
Lines 704 to 713 in 5ea8eb5
if let Some((span, name)) = self.tables.closure_kind_origins().get(hir_id) { | |
err.span_note(*span, &format!( | |
"closure cannot be invoked more than once because \ | |
it moves the variable `{}` out of its environment", | |
name | |
)); | |
false | |
} else { | |
true | |
} |
(I think that in the case of issue-12127, there will be no entry in the map at all.)
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.
Pushed a fix for this.
@bors r+ |
This comment has been minimized.
This comment has been minimized.
@bors p=26 rollup fairness |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This commit extends existing special-casing of closures to highlight the use of variables within generators that are causing the generator to borrow them.
This commit adds a note that was present in the AST borrow checker when closures are invoked more than once and have captured variables by-value.
270f802
to
d088edc
Compare
@bors r=nikomatsakis |
📌 Commit d088edc has been approved by |
…=nikomatsakis NLL lacks various special case handling of closures Part of #52663. Firstly, this PR extends existing handling of closures to also support generators. Second, this PR adds the note found in the AST when a closure is invoked twice and captures a variable by-value: ```text note: closure cannot be invoked more than once because it moves the variable `dict` out of its environment --> $DIR/issue-42065.rs:16:29 | LL | for (key, value) in dict { | ^^^^ ``` r? @nikomatsakis cc @pnkfelix
☀️ Test successful - status-appveyor, status-travis |
Part of #52663.
Firstly, this PR extends existing handling of closures to also support generators.
Second, this PR adds the note found in the AST when a closure is invoked twice and captures a variable by-value:
r? @nikomatsakis
cc @pnkfelix