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

Improve error message when someone triggers E0507 because of an invisible borrow due to implicit use of Deref trait #45515

Closed
coriolinus opened this issue Oct 25, 2017 · 6 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. D-confusing Diagnostics: Confusing error or lint that should be reworked. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-diagnostics Working group: Diagnostics

Comments

@coriolinus
Copy link

I recently hit a bug in which I wrote a function containing only owned data, but which produced a "cannot move out of borrowed content" compiler error. I only figured it out after building a minimal example.

I'd like to improve the error message, at least to the point where if an E0507 is triggered and there's a use of the Deref trait in the immediate context, that will be pointed out as a possible cause. I'm willing to put in the work to implement this myself, but I have zero compiler experience; mentorship would be extremely helpful.

Is this considered a "feature introduction" which would need an RFC, or should I just start working on a PR? If the latter, where in the codebase should I start looking in order to handle this?

@estebank estebank added A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. E-needs-mentor WG-diagnostics Working group: Diagnostics labels Oct 25, 2017
@estebank
Copy link
Contributor

The error for E0507 is in a method in src/librustc_mir/util/borrowck_errors.rs which seems to be used in:

  • src/librustc_borrowck/borrowck/gather_loans/move_error.rs
  • src/librustc_mir/borrow_check.rs
  • src/librustc_mir/dataflow/move_paths/builder.rs

Feel free to start working as this is a feature request that wouldn't need an RFC, those are for when an implied or explicit "contract" would change, adding more diagnostics wouldn't count. You can ask for help here, in https://gitter.im/rust-impl-period/WG-compiler-errors and IRC.

@coriolinus
Copy link
Author

Ok, thanks! I'll get started on this.

@coriolinus
Copy link
Author

First stumbling block: I've ensured my system has all the dependencies, cloned the repo, and built rust using x.py. However, without having yet changed anything at all, the tests don't pass:

$ $ ./x.py test --stage 1
<output trimmed>
test [run-pass] run-pass-fulldeps/proc-macro/span-api-tests.rs ... FAILED

failures:

---- [run-pass] run-pass-fulldeps/proc-macro/span-api-tests.rs stdout ----

error: compilation failed!
status: exit code: 101
command: "/mnt/d/Users/coriolinus/Documents/Projects/rust-lang/build/x86_64-unknown-linux-gnu/stage1/bin/rustc" "/mnt/d/Users/coriolinus/Documents/Projects/rust-lang/src/test/run-pass-fulldeps/proc-macro/span-api-tests.rs" "-L" "/mnt/d/Users/coriolinus/Documents/Projects/rust-lang/build/x86_64-unknown-linux-gnu/test/run-pass-fulldeps" "--target=x86_64-unknown-linux-gnu" "-C" "prefer-dynamic" "-o" "/mnt/d/Users/coriolinus/Documents/Projects/rust-lang/build/x86_64-unknown-linux-gnu/test/run-pass-fulldeps/proc-macro/span-api-tests.stage1-x86_64-unknown-linux-gnu" "-Crpath" "-O" "-Lnative=/mnt/d/Users/coriolinus/Documents/Projects/rust-lang/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/mnt/d/Users/coriolinus/Documents/Projects/rust-lang/build/x86_64-unknown-linux-gnu/test/run-pass-fulldeps/proc-macro/span-api-tests.stage1-x86_64-unknown-linux-gnu.run-pass.libaux"
stdout:
------------------------------------------

------------------------------------------
stderr:
------------------------------------------
thread '<unnamed>' panicked at 'proc_macro::__internal::with_sess() called before set_parse_sess()!', src/libproc_macro/lib.rs:819:8
note: Run with `RUST_BACKTRACE=1` for a backtrace.
error: proc macro panicked
  --> /mnt/d/Users/coriolinus/Documents/Projects/rust-lang/src/test/run-pass-fulldeps/proc-macro/span-api-tests.rs:29:1
   |
29 | assert_source_file! { "Hello, world!" }
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^


------------------------------------------

thread '[run-pass] run-pass-fulldeps/proc-macro/span-api-tests.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:2485:8
note: Run with `RUST_BACKTRACE=1` for a backtrace.


failures:
    [run-pass] run-pass-fulldeps/proc-macro/span-api-tests.rs

test result: FAILED. 0 passed; 1 failed; 79 ignored; 0 measured; 0 filtered out

thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:329:21


command did not execute successfully: "/mnt/d/Users/coriolinus/Documents/Projects/rust-lang/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/mnt/d/Users/coriolinus/Documents/Projects/rust-lang/build/x86_64-unknown-linux-gnu/stage1/lib" "--run-lib-path" "/mnt/d/Users/coriolinus/Documents/Projects/rust-lang/build/x86_64-unknown-linux-gnu/stage1/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/mnt/d/Users/coriolinus/Documents/Projects/rust-lang/build/x86_64-unknown-linux-gnu/stage1/bin/rustc" "--src-base" "/mnt/d/Users/coriolinus/Documents/Projects/rust-lang/src/test/run-pass-fulldeps" "--build-base" "/mnt/d/Users/coriolinus/Documents/Projects/rust-lang/build/x86_64-unknown-linux-gnu/test/run-pass-fulldeps" "--stage-id" "stage1-x86_64-unknown-linux-gnu" "--mode" "run-pass" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/mnt/d/Users/coriolinus/Documents/Projects/rust-lang/build/x86_64-unknown-linux-gnu/llvm/build/bin/FileCheck" "--nodejs" "/mnt/d/Program Files/nodejs/node" "--host-rustcflags" "-Crpath -O" "--target-rustcflags" "-Crpath -O -Lnative=/mnt/d/Users/coriolinus/Documents/Projects/rust-lang/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python" "--lldb-python" "/usr/bin/python" "--gdb" "/usr/bin/gdb" "--llvm-version" "4.0.1\n" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" ""
expected success, got: exit code: 101


failed to run: /mnt/d/Users/coriolinus/Documents/Projects/rust-lang/build/bootstrap/debug/bootstrap test --stage 1
Build completed unsuccessfully in 0:01:17

$ git status; git rev-parse --short HEAD
On branch master
Your branch is up-to-date with 'origin/master'.

nothing to commit, working directory clean
f764eaf

I don't think it would be productive for me to start coding before the tests all pass, because one way or another they'll need to pass in order to merge any commits I make.

It is possibly worth noting that my build environment is weird; it's WSL on Win 10:

$ uname -a
Linux robocomp 4.4.0-43-Microsoft #1-Microsoft Wed Dec 31 14:42:53 PST 2014 x86_64 x86_64 x86_64 GNU/Linux

Is the fast path to victory here just to set up the build from directly within Windows, or is there something else I might try? Alternately, is that test known-bad in some way?

@durka
Copy link
Contributor

durka commented Oct 27, 2017

@coriolinus I don't know about WSL, but proc macro tests don't work with stage 1 at all. Generally you only run the tests that are relevant to your PR specifically (or new ones that you added). So you might run for example ./x.py test -i --stage 1 src/test/compile-fail --test-args E0507. (This saves a ton of time as well.) Once you submit the PR, robots will automatically run all the tests.

@estebank estebank added D-confusing Diagnostics: Confusing error or lint that should be reworked. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 14, 2020
@estebank
Copy link
Contributor

Current output:

error[E0507]: cannot move out of dereference of `DerefContainer<UserData>`
  --> src/main.rs:5:28
   |
5  |     let _new_user_result = user_data.into_new_user(&conn);
   |                            ^^^^^^^^^^--------------------
   |                            |         |
   |                            |         value moved due to this method call
   |                            move occurs because value has type `UserData`, which does not implement the `Copy` trait
   |
note: `UserData::into_new_user` takes ownership of the receiver `self`, which moves value
  --> src/main.rs:67:22
   |
67 |     fn into_new_user(self, _conn: &Conn) -> Result<NewUser, Status<Json<Value>>> {
   |                      ^^^^

It could do with providing an explicit suggestion to change into_new_user to take &self instead, but I believe that we can close this ticket. What do you think @coriolinus?

@coriolinus
Copy link
Author

Oh wow, I'd forgotten all about this! Yes, the status quo is much better than it was 5 years ago, so I'm happy closing this issue now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. D-confusing Diagnostics: Confusing error or lint that should be reworked. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-diagnostics Working group: Diagnostics
Projects
None yet
Development

No branches or pull requests

3 participants