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

Issue #46589 - Kill borrows on a local variable whenever we assign ov… #46752

Merged
merged 1 commit into from Dec 22, 2017

Conversation

Projects
None yet
7 participants
@Yoric
Contributor

Yoric commented Dec 15, 2017

…er this variable

This is a first patch for the issue, handling the simple case while I figure out the data structures involved in the more complex cases.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Dec 15, 2017

Collaborator

r? @pnkfelix

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

Collaborator

rust-highfive commented Dec 15, 2017

r? @pnkfelix

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

@bjorn3

This comment has been minimized.

Show comment
Hide comment
@bjorn3

bjorn3 Dec 16, 2017

Contributor

Issue #46589 (just to link it)

Could you add a test?

Contributor

bjorn3 commented Dec 16, 2017

Issue #46589 (just to link it)

Could you add a test?

@pnkfelix

This comment has been minimized.

Show comment
Hide comment
@pnkfelix

pnkfelix Dec 16, 2017

Member

The code looks fine to me; just add a test and we should be good.

Member

pnkfelix commented Dec 16, 2017

The code looks fine to me; just add a test and we should be good.

@Yoric

This comment has been minimized.

Show comment
Hide comment
@Yoric

Yoric Dec 19, 2017

Contributor

I have added the test of issue #46589 and much to my surprise, it still fails, although it looks like it should be covered by my patch. I must be missing something obvious:

error[E0506]: cannot assign to `list` because it is borrowed
  --> /Users/david/Documents/Code/rust-nll/src/test/run-pass/nll-iterating-and-updating.rs:25:13
   |
23 |         result.push(&mut list.value);
   |                     --------------- borrow of `list` occurs here
24 |         if let Some(n) = list.next.as_mut() {
25 |             list = n;
   |             ^^^^^^^^ assignment to borrowed `list` occurs here

error: aborting due to previous error
Contributor

Yoric commented Dec 19, 2017

I have added the test of issue #46589 and much to my surprise, it still fails, although it looks like it should be covered by my patch. I must be missing something obvious:

error[E0506]: cannot assign to `list` because it is borrowed
  --> /Users/david/Documents/Code/rust-nll/src/test/run-pass/nll-iterating-and-updating.rs:25:13
   |
23 |         result.push(&mut list.value);
   |                     --------------- borrow of `list` occurs here
24 |         if let Some(n) = list.next.as_mut() {
25 |             list = n;
   |             ^^^^^^^^ assignment to borrowed `list` occurs here

error: aborting due to previous error
@Yoric

This comment has been minimized.

Show comment
Hide comment
@Yoric

Yoric Dec 19, 2017

Contributor

Note: problem appears even if we add a

let mut list = list;

so it's probably not caused by list being an argument.

Contributor

Yoric commented Dec 19, 2017

Note: problem appears even if we add a

let mut list = list;

so it's probably not caused by list being an argument.

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Dec 19, 2017

Contributor

@Yoric

I think that's because you haven't fixed this:

StatementKind::Assign(ref lhs, ref rhs) => {
// NOTE: NLL RFC calls for *shallow* write; using Deep
// for short-term compat w/ AST-borrowck. Also, switch
// to shallow requires to dataflow: "if this is an
// assignment `place = <rvalue>`, then any loan for some
// path P of which `place` is a prefix is killed."
self.mutate_place(
ContextKind::AssignLhs.new(location),
(lhs, span),
Deep,
JustWrite,
flow_state,
);
self.consume_rvalue(
ContextKind::AssignRhs.new(location),
(rhs, span),
location,
flow_state,
);
}

Contributor

arielb1 commented Dec 19, 2017

@Yoric

I think that's because you haven't fixed this:

StatementKind::Assign(ref lhs, ref rhs) => {
// NOTE: NLL RFC calls for *shallow* write; using Deep
// for short-term compat w/ AST-borrowck. Also, switch
// to shallow requires to dataflow: "if this is an
// assignment `place = <rvalue>`, then any loan for some
// path P of which `place` is a prefix is killed."
self.mutate_place(
ContextKind::AssignLhs.new(location),
(lhs, span),
Deep,
JustWrite,
flow_state,
);
self.consume_rvalue(
ContextKind::AssignRhs.new(location),
(rhs, span),
location,
flow_state,
);
}

@Yoric

This comment has been minimized.

Show comment
Hide comment
@Yoric

Yoric Dec 19, 2017

Contributor

Ok, thanks to @arielb1, this now seems to work.

Contributor

Yoric commented Dec 19, 2017

Ok, thanks to @arielb1, this now seems to work.

@Yoric

This comment has been minimized.

Show comment
Hide comment
@Yoric

Yoric Dec 19, 2017

Contributor

(oops, forgot to remove the obsolete test, this should now be fixed)

Contributor

Yoric commented Dec 19, 2017

(oops, forgot to remove the obsolete test, this should now be fixed)

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Dec 19, 2017

Contributor

@bors r+

Contributor

arielb1 commented Dec 19, 2017

@bors r+

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 19, 2017

Contributor

📌 Commit 5e0e633 has been approved by arielb1

Contributor

bors commented Dec 19, 2017

📌 Commit 5e0e633 has been approved by arielb1

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 20, 2017

Contributor

⌛️ Testing commit 5e0e633 with merge 627022b...

Contributor

bors commented Dec 20, 2017

⌛️ Testing commit 5e0e633 with merge 627022b...

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

Auto merge of #46752 - Yoric:nll, r=arielb1
Issue #46589 - Kill borrows on a local variable whenever we assign ov…

…er this variable

This is a first patch for the issue, handling the simple case while I figure out the data structures involved in the more complex cases.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 20, 2017

Contributor

💔 Test failed - status-appveyor

Contributor

bors commented Dec 20, 2017

💔 Test failed - status-appveyor

@Yoric

This comment has been minimized.

Show comment
Hide comment
@Yoric

Yoric Dec 21, 2017

Contributor

As far as I can tell, the single failure is a timeout, which I imagine is not caused by this PR.

Contributor

Yoric commented Dec 21, 2017

As far as I can tell, the single failure is a timeout, which I imagine is not caused by this PR.

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Dec 21, 2017

Member

@bors retry — 3 hour timeout.

Build number 1.0.5591 1.0.5765
make-prepare 43.49 49.60
pytest/bootstrap 0.00 0.00
stage0-std 112.40 120.96
stage0-tidy 6.55 7.46
bootstrap 5.73 5.92
stage0-test 11.26 12.68
llvm 557.90 642.91
stage0-rustc 1080.53 1471.46
stage1-std 54.30 75.27
stage1-test 16.18 21.58
stage1-rustc 1357.72 2055.58
stage0-compiletest 54.95 103.39
test/ui 73.17 188.49
test/run-pass 1380.91 1874.05
test/compile-fail 573.45 568.06
test/parse-fail 26.47 32.96
test/run-fail 30.84 37.61
test/run-pass-valgrind 4.08 4.66
test/mir-opt 19.00 33.14
test/codegen 9.00 10.73
test/codegen-units 12.81 10.29
test/incremental 81.99 78.64
test/debuginfo 31.47 31.93
test/ui-fulldeps 17.09 32.71
test/run-pass-fulldeps 223.00 297.01
test/run-fail-fulldeps 3.08 4.81
test/compile-fail-fulldeps 69.19 91.50
stage2-rustdoc 228.74 301.32
test/run-make 144.75 171.07
test/rustdoc 121.66 175.18
stage1-test-libstd 178.30 236.88
test/libstd 790.40 914.47
stage1-test-libtest 12.46 13.91
test/libtest 13.82 18.38
stage1-test-librustc 309.59 394.63
test/librustc 314.23 400.63
stage2-test-rustdoc 45.49 54.72
test/libtools 46.40 55.56
stage0-unstable-book-gen 8.60 10.47
stage0-rustbook 177.15 181.70
doc/std 27.87 36.66
doc/compiler 1.42 3.33
stage2-error_index_generator 22.21 28.20
stage0-linkchecker 2.51 2.92
test/linkchecker 44.02 48.44
test/docs 241.15  
stage2-test-error-index 61.73  

 

Member

kennytm commented Dec 21, 2017

@bors retry — 3 hour timeout.

Build number 1.0.5591 1.0.5765
make-prepare 43.49 49.60
pytest/bootstrap 0.00 0.00
stage0-std 112.40 120.96
stage0-tidy 6.55 7.46
bootstrap 5.73 5.92
stage0-test 11.26 12.68
llvm 557.90 642.91
stage0-rustc 1080.53 1471.46
stage1-std 54.30 75.27
stage1-test 16.18 21.58
stage1-rustc 1357.72 2055.58
stage0-compiletest 54.95 103.39
test/ui 73.17 188.49
test/run-pass 1380.91 1874.05
test/compile-fail 573.45 568.06
test/parse-fail 26.47 32.96
test/run-fail 30.84 37.61
test/run-pass-valgrind 4.08 4.66
test/mir-opt 19.00 33.14
test/codegen 9.00 10.73
test/codegen-units 12.81 10.29
test/incremental 81.99 78.64
test/debuginfo 31.47 31.93
test/ui-fulldeps 17.09 32.71
test/run-pass-fulldeps 223.00 297.01
test/run-fail-fulldeps 3.08 4.81
test/compile-fail-fulldeps 69.19 91.50
stage2-rustdoc 228.74 301.32
test/run-make 144.75 171.07
test/rustdoc 121.66 175.18
stage1-test-libstd 178.30 236.88
test/libstd 790.40 914.47
stage1-test-libtest 12.46 13.91
test/libtest 13.82 18.38
stage1-test-librustc 309.59 394.63
test/librustc 314.23 400.63
stage2-test-rustdoc 45.49 54.72
test/libtools 46.40 55.56
stage0-unstable-book-gen 8.60 10.47
stage0-rustbook 177.15 181.70
doc/std 27.87 36.66
doc/compiler 1.42 3.33
stage2-error_index_generator 22.21 28.20
stage0-linkchecker 2.51 2.92
test/linkchecker 44.02 48.44
test/docs 241.15  
stage2-test-error-index 61.73  

 

@Yoric

This comment has been minimized.

Show comment
Hide comment
@Yoric

Yoric Dec 21, 2017

Contributor

Fwiw, SYS_BITS=32, RUST_CONFIGURE_ARGS=--build=i686-pc-windows-gnu, SCRIPT=python x.py test, MINGW_URL=https://s3-us-west-1.amazonaws.com/rust-lang-ci2/rust-ci-mirror, MINGW_ARCHIVE=i686-6.3.0-release-posix-dwarf-rt_v5-rev2.7z, MINGW_DIR=mingw32 timed out after 3 hr

Contributor

Yoric commented Dec 21, 2017

Fwiw, SYS_BITS=32, RUST_CONFIGURE_ARGS=--build=i686-pc-windows-gnu, SCRIPT=python x.py test, MINGW_URL=https://s3-us-west-1.amazonaws.com/rust-lang-ci2/rust-ci-mirror, MINGW_ARCHIVE=i686-6.3.0-release-posix-dwarf-rt_v5-rev2.7z, MINGW_DIR=mingw32 timed out after 3 hr

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 21, 2017

Contributor

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

Contributor

bors commented Dec 21, 2017

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

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Dec 21, 2017

Member

@bors r=arielb1

Member

kennytm commented Dec 21, 2017

@bors r=arielb1

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 21, 2017

Contributor

📌 Commit fcb1090 has been approved by arielb1

Contributor

bors commented Dec 21, 2017

📌 Commit fcb1090 has been approved by arielb1

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 22, 2017

Contributor

⌛️ Testing commit fcb1090 with merge 264af16...

Contributor

bors commented Dec 22, 2017

⌛️ Testing commit fcb1090 with merge 264af16...

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

Auto merge of #46752 - Yoric:nll, r=arielb1
Issue #46589 - Kill borrows on a local variable whenever we assign ov…

…er this variable

This is a first patch for the issue, handling the simple case while I figure out the data structures involved in the more complex cases.
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Dec 22, 2017

Contributor

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

Contributor

bors commented Dec 22, 2017

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

@bors bors merged commit fcb1090 into rust-lang:master Dec 22, 2017

2 checks passed

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

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

Auto merge of #46975 - matthewjasper:mir-moveck-asm, r=arielb1
[MIR Borrowck] Moveck inline asm statements

Closes #45695

New behavior:
* Input operands to `asm!` are moved, direct output operands are initialized.
* Direct, non-read-write outputs match the assignment changes in #46752 (Shallow writes, end borrows).

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

Auto merge of #46975 - matthewjasper:mir-moveck-asm, r=arielb1
[MIR Borrowck] Moveck inline asm statements

Closes #45695

New behavior:
* Input operands to `asm!` are moved, direct output operands are initialized.
* Direct, non-read-write outputs match the assignment changes in #46752 (Shallow writes, end borrows).

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

Auto merge of #46975 - matthewjasper:mir-moveck-asm, r=arielb1
[MIR Borrowck] Moveck inline asm statements

Closes #45695

New behavior:
* Input operands to `asm!` are moved, direct output operands are initialized.
* Direct, non-read-write outputs match the assignment changes in #46752 (Shallow writes, end borrows).

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

Auto merge of #46975 - matthewjasper:mir-moveck-asm, r=arielb1
[MIR Borrowck] Moveck inline asm statements

Closes #45695

New behavior:
* Input operands to `asm!` are moved, direct output operands are initialized.
* Direct, non-read-write outputs match the assignment changes in #46752 (Shallow writes, end borrows).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment