Skip to content

Make capture-by-ref unboxed closures work - #17731

Closed
bkoropoff wants to merge 6 commits into
rust-lang:masterfrom
bkoropoff:unboxed-by-ref
Closed

bkoropoff wants to merge 6 commits into
rust-lang:masterfrom
bkoropoff:unboxed-by-ref

Conversation

@bkoropoff

Copy link
Copy Markdown
Contributor

This began as an attempt to fix an ICE in borrowck (issue #17655), but the rabbit hole went pretty deep. I ended up plumbing support for capture-by-reference unboxed closures all the way into trans.

Closes issue #17655.

…losures

This prevents a later ICE in borrowck.

Closes issue #17655
In particular, this causes mutation of an upvar to correctly mark
it as mutable during adjustment.  This makes borrowck correctly
flag conflicting borrows, etc.

We still seem to generate incorrect code in trans which copies the upvar
by value into the closure.  This remains to be fixed.
Treat upvars of capture-by-reference unboxed closures as references
with appropriate regions and mutability.
Store references to the freevars instead of copies when constructing
the environment and insert an additional load when reading them from
the environment.
This test works as a regression test for issue #17655.  It also
exercises mutation of by-ref upvars.
@huonw

huonw commented Oct 3, 2014

Copy link
Copy Markdown
Contributor

r? @pcwalton

@pcwalton

pcwalton commented Oct 3, 2014

Copy link
Copy Markdown
Contributor

This looks good. Could you add some tests for Fn and FnOnce closures as well? Thanks!

@bkoropoff

Copy link
Copy Markdown
Contributor Author

Added some more tests.

bors added a commit that referenced this pull request Oct 4, 2014
This began as an attempt to fix an ICE in borrowck (issue #17655), but the rabbit hole went pretty deep.  I ended up plumbing support for capture-by-reference unboxed closures all the way into trans.

Closes issue #17655.
@bors bors closed this Oct 4, 2014
lnicola pushed a commit to lnicola/rust that referenced this pull request Aug 13, 2024
perf: Segregate syntax and semantic diagnostics

Closes rust-lang#17731
flip1995 pushed a commit to flip1995/rust that referenced this pull request Sep 24, 2026
…r_generic_args` (rust-lang#17731)

Did a bit of poking, I think this is the kind of skip that we want.
There are a few other options where to skip, but this keeps the "least
nessary skip" I think.
Fixes rust-lang/rust-clippy#17717

changelog: [`needless_borrows_for_generic_args`]: add `const_trait_impl`
compatability (such as for example the destructor of `Vec`) to not send
the old solver into an infinite loop

Alternatively, I am also opening an PR to move this lint to `nursary`
that we can also merge and backport to make sure that stable is not
broken.

Regarding commits, I currently have not split the commits more (impl
from tests), not sure how this backporting works exactly.

Given that this affects beta (which we release in 15 days) on an
warn-by-default lint, I hope this is ok:

r? clippy
@rustbot label beta-nominated
flip1995 pushed a commit to flip1995/rust that referenced this pull request Sep 24, 2026
…r_generic_args` (rust-lang#17731)

Did a bit of poking, I think this is the kind of skip that we want.
There are a few other options where to skip, but this keeps the "least
nessary skip" I think.
Fixes rust-lang/rust-clippy#17717

changelog: [`needless_borrows_for_generic_args`]: add `const_trait_impl`
compatability (such as for example the destructor of `Vec`) to not send
the old solver into an infinite loop

Alternatively, I am also opening an PR to move this lint to `nursary`
that we can also merge and backport to make sure that stable is not
broken.

Regarding commits, I currently have not split the commits more (impl
from tests), not sure how this backporting works exactly.

Given that this affects beta (which we release in 15 days) on an
warn-by-default lint, I hope this is ok:

r? clippy
@rustbot label beta-nominated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants