Skip to content
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

yet another "old borrowck" bug around match default bindings #51686

Conversation

nikomatsakis
Copy link
Contributor

@nikomatsakis nikomatsakis commented Jun 21, 2018

We were getting the type of the parameter from its pattern, but that didn't include adjustments. I did a ripgrep around and this seemed to be the only affected case.

The reason this didn't show up as an ICE earlier is that mem-categorization is lenient with respect to weird discrepancies. I am going to add more delay-span-bug calls shortly around that (I'll push onto the PR).

This example is an ICE, but I presume that there is a way to make a soundness example out of this -- it basically ignores borrows occuring inside match-default-bindings in a closure, though only if the implicit deref is at the top-level. It happens though that this occurs frequently in iterators, which often give a &T parameter.

Fixes #51415
Fixes #49534

r? @eddyb

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 21, 2018
@nikomatsakis
Copy link
Contributor Author

Hmm, adding in the asserts I wanted caused some failures elsewhere that I don't know if I fully want to mix into this PR. I'm going to remove the WIP.

@nikomatsakis nikomatsakis changed the title [WIP] yet another "old borrowck" bug around match default bindings yet another "old borrowck" bug around match default bindings Jun 21, 2018
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-3.9 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:start:test_ui
Check compiletest suite=ui mode=ui (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu)
[00:44:30] 
[00:44:30] running 1507 tests
[00:44:34] ...............................................F.............................................i......
[00:44:45] ....................................................................................................
[00:44:48] ....................................................................................................
[00:44:52] ....................................................................................................
[00:44:55] ....................................................................................................
---
[00:45:46] normalized stderr:
[00:45:46] error[E0507]: cannot move out of borrowed content
[00:45:46]   --> $DIR/issue-51415.rs:16:46
[00:45:46]    |
[00:45:46] LL |     let opt = a.iter().enumerate().find(|(_, &s)| {
[00:45:46]    |                                              ^-
[00:45:46]    |                                              ||
[00:45:46]    |                                              |hint: to prevent move, use `ref s` or `ref mut s`
[00:45:46]    |                                              cannot move out of borrowed content
[00:45:46] error: aborting due to previous error
[00:45:46] 
[00:45:46] For more information about this error, try `rustc --explain E0507`.
[00:45:46] 
[00:45:46] 
[00:45:46] 
[00:45:46] 
[00:45:46] The actual stderr differed from the expected stderr.
[00:45:46] Actual stderr saved to /checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/borrowck/issue-51415/issue-51415.stderr
[00:45:46] To update references, rerun the tests and pass the `--bless` flag
[00:45:46] To only update this specific test, also pass `--test-args borrowck/issue-51415.rs`
[00:45:46] error: 1 errors occurred comparing output.
[00:45:46] status: exit code: 101
[00:45:46] status: exit code: 101
[00:45:46] command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/borrowck/issue-51415.rs" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/borrowck/issue-51415/a" "-Crpath" "-O" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/borrowck/issue-51415/auxiliary" "-A" "unused"
[00:45:46] ------------------------------------------
[00:45:46] 
[00:45:46] ------------------------------------------
[00:45:46] stderr:
[00:45:46] stderr:
[00:45:46] ------------------------------------------
[00:45:46] {"message":"cannot move out of borrowed content","code":{"code":"E0507","explanation":"\nYou tried to move out of a value which was borrowed. Erroneous code example:\n\n```compile_fail,E0507\nuse std::cell::RefCell;\n\nstruct TheDarkKnight;\n\nimpl TheDarkKnight {\n    fn nothing_is_true(self) {}\n}\n\nfn main() {\n    let x = RefCell::new(TheDarkKnight);\n\n    x.borrow().nothing_is_true(); // error: cannot move out of borrowed content\n}\n```\n\nHere, the `nothing_is_true` method takes the ownership of `self`. However,\n`self` cannot be moved because `.borrow()` only provides an `&TheDarkKnight`,\nwhich is a borrow of the content owned by the `RefCell`. To fix this error,\nyou have three choices:\n\n* Try to avoid moving the variable.\n* Somehow reclaim the ownership.\n* Implement the `Copy` trait on the type.\n\nExamples:\n\n```\nuse std::cell::RefCell;\n\nstruct TheDarkKnight;\n\nimpl TheDarkKnight {\n    fn nothing_is_true(&self) {} // First case, we don't take ownership\n}\n\nfn main() {\n    let x = RefCell::new(TheDarkKnight);\n\n    x.borrow().nothing_is_true(); // ok!\n}\n```\n\nOr:\n\n```\nuse std::cell::RefCell;\n\nstruct TheDarkKnight;\n\nimpl TheDarkKnight {\n    fn nothing_is_true(self) {}\n}\n\nfn main() {\n    let x = RefCell::new(TheDarkKnight);\n    let x = x.into_inner(); // we get back ownership\n\n    x.nothing_is_true(); // ok!\n}\n```\n\nOr:\n\n```\nuse std::cell::RefCell;\n\n#[derive(Clone, Copy)] // we implement the Copy trait\nstruct TheDarkKnight;\n\nimpl TheDarkKnight {\n    fn nothing_is_true(self) {}\n}\n\nfn main() {\n    let x = RefCell::new(TheDarkKnight);\n\n    x.borrow().nothing_is_true(); // ok!\n}\n```\n\nMoving a member out of a mutably borrowed struct will also cause E0507 error:\n\n```compile_fail,E0507\nstruct TheDarkKnight;\n\nimpl TheDarkKnight {\n    fn nothing_is_true(self) {}\n}\n\nstruct Batcave {\n    knight: TheDarkKnight\n}\n\nfn main() {\n    let mut cave = Batcave {\n        knight: TheDarkKnight\n    };\n    let borrowed = &mut cave;\n\n    borrowed.knight.nothing_is_true(); // E0507\n}\n```\n\nIt is fine only if you put something back. `mem::replace` can be used for that:\n\n```\n# struct TheDarkKnight;\n# impl TheDarkKnight { fn nothing_is_true(self) {} }\n# struct Batcave { knight: TheDarkKnight }\nuse std::mem;\n\nlet mut cave = Batcave {\n    knight: TheDarkKnight\n};\nlet borrowed = &mut cave;\n\nmem::replace(&mut borrowed.knight, TheDarkKnight).nothing_is_true(); // ok!\n```\n\nYou can find more information about borrowing in the rust-book:\nhttp://doc.rust-lang.org/book/first-edition/references-and-borrowing.html\n"},"level":"error","spans":[{"file_name":"/checkout/src/test/ui/borrowck/issue-51415.rs","byte_start":677,"byte_end":679,"line_start":16,"line_end":16,"column_start":46,"column_end":48,"is_primary":true,"text":[{"text":"    let opt = a.iter().enumerate().find(|(_, &s)| {","highlight_start":46,"highlight_end":48}],"label":"cannot move out of borrowed content","suggested_replacement":null,"suggestion_applicability":null,"expansion":null},{"file_name":"/checkout/src/test/ui/borrowck/issue-51415.rs","byte_start":678,"byte_end":679,"line_start":16,"line_end":16,"column_start":47,"column_end":48,"is_primary":false,"text":[{"text":"    let opt = a.iter().enumerate().find(|(_, &s)| {","highlight_start":47,"highlight_end":48}],"label":"hint: to prevent move, use `ref s` or `ref mut s`","suggested_replacement":null,"suggestion_applicability":null,"expansion":null}],"children":[],"rendered":"error[E0507]: cannot move out of borrowed content\n  --> /checkout/src/test/ui/borrowck/issue-51415.rs:16:46\n   |\nLL |     let opt = a.iter().enumerate().find(|(_, &s)| {\n   |                                              ^-\n   |                                              ||\n   |                                              |hint: to prevent move, use `ref s` or `ref mut s`\n   |                                              cannot move out of borrowed content\n\n"}
[00:45:46] {"message":"aborting due to previous error","code":null,"level":"error","spans":[],"children":[],"rendered":"error: aborting due to previous error\n\n"}
[00:45:46] {"message":"For more information about this error, try `rustc --explain E0507`.","code":null,"level":"","spans":[],"children":[],"rendered":"For more information about this error, try `rustc --explain E0507`.\n"}
[00:45:46] ------------------------------------------
[00:45:46] 
[00:45:46] thread '[ui] ui/borrowck/issue-51415.rs' panicked at 'explicit panic', tools/compiletest/src/runtest.rs:3139:9
[00:45:46] note: Run with `RUST_BACKTRACE=1` for a backtrace.
---
[00:45:46] 
[00:45:46] thread 'main' panicked at 'Some tests failed', tools/compiletest/src/main.rs:498:22
[00:45:46] 
[00:45:46] 
[00:45:46] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-3.9/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options " "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "3.9.1\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
[00:45:46] 
[00:45:46] 
[00:45:46] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
[00:45:46] Build completed unsuccessfully in 0:02:05
[00:45:46] Build completed unsuccessfully in 0:02:05
[00:45:46] Makefile:58: recipe for target 'check' failed
[00:45:46] make: *** [check] Error 1

The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:176e6c4f
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@nikomatsakis nikomatsakis force-pushed the issue-51415-borrowck-match-default-bindings-bug branch from 0885d97 to 8289bf2 Compare June 21, 2018 18:33
@eddyb
Copy link
Member

eddyb commented Jun 21, 2018

@bors r+

@bors
Copy link
Contributor

bors commented Jun 21, 2018

📌 Commit 8289bf2 has been approved by eddyb

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 21, 2018
@kennytm
Copy link
Member

kennytm commented Jun 22, 2018

@bors p=17

@bors
Copy link
Contributor

bors commented Jun 22, 2018

⌛ Testing commit 8289bf2 with merge 7dae5c0...

bors added a commit that referenced this pull request Jun 22, 2018
…t-bindings-bug, r=eddyb

yet another "old borrowck" bug around match default bindings

We were getting the type of the parameter from its pattern, but that didn't include adjustments. I did a `ripgrep` around and this seemed to be the only affected case.

The reason this didn't show up as an ICE earlier is that mem-categorization is lenient with respect to weird discrepancies. I am going to add more delay-span-bug calls shortly around that (I'll push onto the PR).

This example is an ICE, but I presume that there is a way to make a soundness example out of this -- it basically ignores borrows occuring inside match-default-bindings in a closure, though only if the implicit deref is at the top-level. It happens though that this occurs frequently in iterators, which often give a `&T` parameter.

Fixes #51415
Fixes #49534

r? @eddyb
@bors
Copy link
Contributor

bors commented Jun 22, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: eddyb
Pushing 7dae5c0 to master...

@bors bors merged commit 8289bf2 into rust-lang:master Jun 22, 2018
@pietroalbini pietroalbini added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 22, 2018
@pietroalbini
Copy link
Member

Nominating this to be backported into beta, especially if this is going to get into 1.27.1.

@pietroalbini pietroalbini added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 25, 2018
@nikomatsakis nikomatsakis added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jun 28, 2018
@pietroalbini pietroalbini removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jun 28, 2018
bors added a commit that referenced this pull request Jun 28, 2018
[beta] Rollup backports

Merged and approved:

* #51725: Do not build LLVM tools for any of the tools
* #51852: Don't use `ParamEnv::reveal_all()` if there is a real one available
* #51686: yet another "old borrowck" bug around match default bindings
* #51868: Remove process::id from 'Stabilized APIs' in 1.27.0 release notes
*  #51335: Prohibit `global_allocator` in submodules

r? @ghost
@pietroalbini pietroalbini added the stable-nominated Nominated for backporting to the compiler in the stable channel. label Jun 30, 2018
@Mark-Simulacrum Mark-Simulacrum added the stable-accepted Accepted for backporting to the compiler in the stable channel. label Jul 3, 2018
@pietroalbini pietroalbini removed the stable-nominated Nominated for backporting to the compiler in the stable channel. label Jul 7, 2018
bors pushed a commit that referenced this pull request Jul 11, 2018
This looks like a typo introduced in #51686.

Fixes #52213.
bors added a commit that referenced this pull request Jul 11, 2018
 use the adjusted type for cat_pattern in tuple patterns

This looks like a typo introduced in #51686.

Fixes #52213.

r? @pnkfelix

beta + stable nominating because regression + unsoundness.
pietroalbini pushed a commit to pietroalbini/rust that referenced this pull request Jul 14, 2018
Mark-Simulacrum pushed a commit to Mark-Simulacrum/rust that referenced this pull request Jul 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. stable-accepted Accepted for backporting to the compiler in the stable channel. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants