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

MIR-borrowck: immutable unique closure upvars can be mutated #46236

Merged
merged 4 commits into from Dec 1, 2017

Conversation

Projects
None yet
8 participants
@davidtwco
Member

davidtwco commented Nov 24, 2017

Fixes #46023 and #46160 (see this comment).

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Nov 24, 2017

Collaborator

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

Collaborator

rust-highfive commented Nov 24, 2017

r? @pnkfelix

(rust_highfive has picked a reviewer for you, use r? to override)

@davidtwco

This comment has been minimized.

Show comment
Hide comment
@davidtwco

davidtwco Nov 24, 2017

Member

This isn't quite working yet and I'm not quite sure why. I think I've followed the instructions left on the issue correctly.

r? @nikomatsakis @arielb1

Member

davidtwco commented Nov 24, 2017

This isn't quite working yet and I'm not quite sure why. I think I've followed the instructions left on the issue correctly.

r? @nikomatsakis @arielb1

Show outdated Hide outdated src/librustc/mir/mod.rs Outdated
@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Nov 25, 2017

Contributor

I think you also need to change check_if_reassignment_to_immutable_state to use the new is_mutable logic - it handles error reporting for unique-immutable assignments. Otherwise assignments will not quite work. I think that's why the test is broken.

Contributor

arielb1 commented Nov 25, 2017

I think you also need to change check_if_reassignment_to_immutable_state to use the new is_mutable logic - it handles error reporting for unique-immutable assignments. Otherwise assignments will not quite work. I think that's why the test is broken.

@davidtwco

This comment has been minimized.

Show comment
Hide comment
@davidtwco

davidtwco Nov 26, 2017

Member

Pushed an alternate approach after speaking with @arielb1 on Gitter - it isn't working yet but I think I've implemented the mutability field in UpvarDecl as per our conversation.

Member

davidtwco commented Nov 26, 2017

Pushed an alternate approach after speaking with @arielb1 on Gitter - it isn't working yet but I think I've implemented the mutability field in UpvarDecl as per our conversation.

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Nov 27, 2017

Contributor

@davidtwco

BTW, if you're already at this, you could also fix #46160 by marking the closure argument (argument _1 of a closure) as mutable

Contributor

arielb1 commented Nov 27, 2017

@davidtwco

BTW, if you're already at this, you could also fix #46160 by marking the closure argument (argument _1 of a closure) as mutable

@davidtwco

This comment has been minimized.

Show comment
Hide comment
@davidtwco

davidtwco Nov 27, 2017

Member

I've not done much testing of what I just pushed, but I've added work from all the instruction provided over the past few days. It includes a rebase onto current master which included #46022 which @arielb1 said I might need.

Member

davidtwco commented Nov 27, 2017

I've not done much testing of what I just pushed, but I've added work from all the instruction provided over the past few days. It includes a rebase onto current master which included #46022 which @arielb1 said I might need.

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 30, 2017

Contributor

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

Contributor

bors commented Nov 30, 2017

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

@davidtwco

This comment has been minimized.

Show comment
Hide comment
@davidtwco

davidtwco Nov 30, 2017

Member

Pushed a compiling version of the last changes and rebased ontop of the current master. Wasn't sure what to use for the new parameter to is_mutable.

Member

davidtwco commented Nov 30, 2017

Pushed a compiling version of the last changes and rebased ontop of the current master. Wasn't sure what to use for the new parameter to is_mutable.

davidtwco added some commits Nov 30, 2017

@davidtwco davidtwco changed the title from WIP: MIR-borrowck: immutable unique closure upvars can be mutated to MIR-borrowck: immutable unique closure upvars can be mutated Nov 30, 2017

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Nov 30, 2017

Contributor

@bors r+

Contributor

arielb1 commented Nov 30, 2017

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 30, 2017

Contributor

📌 Commit c6b1ba5 has been approved by arielb1

Contributor

bors commented Nov 30, 2017

📌 Commit c6b1ba5 has been approved by arielb1

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 1, 2017

Contributor

⌛️ Testing commit c6b1ba5 with merge e3ed212...

Contributor

bors commented Dec 1, 2017

⌛️ Testing commit c6b1ba5 with merge e3ed212...

bors added a commit that referenced this pull request Dec 1, 2017

Auto merge of #46236 - davidtwco:issue-46023, r=arielb1
MIR-borrowck: immutable unique closure upvars can be mutated

Fixes #46023 and #46160 (see [this comment](#46236 (comment))).
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 1, 2017

Contributor

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing e3ed212 to master...

Contributor

bors commented Dec 1, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing e3ed212 to master...

@bors bors merged commit c6b1ba5 into rust-lang:master Dec 1, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@davidtwco davidtwco deleted the davidtwco:issue-46023 branch Dec 1, 2017

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb May 16, 2018

Member

This should have been merged like this, UpvarDecl was "supposed" to be debuginfo-only 😞.
But I'm not sure where to put this information - presumably it belongs attached to the closure type.

Member

eddyb commented May 16, 2018

This should have been merged like this, UpvarDecl was "supposed" to be debuginfo-only 😞.
But I'm not sure where to put this information - presumably it belongs attached to the closure type.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis May 16, 2018

Contributor

@eddyb

But I'm not sure where to put this information - presumably it belongs attached to the closure type

Hmm. Maybe, but I'm not sure. I guess it depends on how you view the mut information. I don't think of it as part of the type system, but rather a kind of lint that sits on top. But I suppose we do issue hard errors and so forth, so it's reasonable to view it as information that belongs in the closure type. It's sort of nice though that the "closure subst" is just types right now, and I'm not sure I want to change that. Another option would be to add some closure_upvar_info query that takes a closure def-id and gives further information, though — sort of like the existing free_vars calculation?

(On the other hand, maybe it's nice to have all the upvar information collected in one place...)

( <grumpy> I still think we should just drop the notion of declaring local variables as mut altogether. =) </grumpy> )

Contributor

nikomatsakis commented May 16, 2018

@eddyb

But I'm not sure where to put this information - presumably it belongs attached to the closure type

Hmm. Maybe, but I'm not sure. I guess it depends on how you view the mut information. I don't think of it as part of the type system, but rather a kind of lint that sits on top. But I suppose we do issue hard errors and so forth, so it's reasonable to view it as information that belongs in the closure type. It's sort of nice though that the "closure subst" is just types right now, and I'm not sure I want to change that. Another option would be to add some closure_upvar_info query that takes a closure def-id and gives further information, though — sort of like the existing free_vars calculation?

(On the other hand, maybe it's nice to have all the upvar information collected in one place...)

( <grumpy> I still think we should just drop the notion of declaring local variables as mut altogether. =) </grumpy> )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment