Skip to content

MIR-borrowck: diagnostic says "use" (as noun?) rather than "used" (verb, past tense) #45960

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
pnkfelix opened this issue Nov 13, 2017 · 5 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-NLL Area: Non-lexical lifetimes (NLL) C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@pnkfelix
Copy link
Member

#45877 added a ui test that shows that the AST-borrowck and MIR-borrowck are very close in their diagnostic... so close that its easy to overlook the following change to the note output:

Namely, the file src/test/ui/borrowck/borrowck-reinit.stderr says,
for AST:

   |                ^ value used here after move 

but for MIR:

   |                ^ value use here after move 

Nota bene, fixing this is not a trivial one liner, because the function that constructs the error diagnostic is parameterized over a string describing the action (i.e. a verb like "use" or "borrow"). Still, it should be a relatively easy thing to resolve.

@pnkfelix pnkfelix added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-diagnostics Area: Messages for errors, warnings, and lints WG-compiler-nll A-NLL Area: Non-lexical lifetimes (NLL) labels Nov 13, 2017
@kennytm
Copy link
Member

kennytm commented Nov 13, 2017

Mentoring instructions

The "verb" (desired_action) is passed in when calling check_if_path_is_moved in https://github.com/rust-lang/rust/blob/master/src/librustc_mir/borrow_check.rs, which currently includes:

  • "update"
  • "borrow"
  • "use"
  • "assignment" (?!)

The only code consuming the desired_action in an error message is:

  • format!("value {} here after move", desired_action)

So I think this is really just a trivial 5-liner to change the tense of all of these.

@TimNN TimNN added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Nov 14, 2017
@nikomatsakis nikomatsakis added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Nov 15, 2017
@nikomatsakis
Copy link
Contributor

Some context about what we are trying to do in the NLL / Borrow Checker etc is available in this paper document. The other thing is that I wrote this gist that talks about how to update a test to work for both AST and MIR borrowck, though in this case it may not be needed.

@austinbes
Copy link

I'd like to give this one a shot

arielb1 pushed a commit to arielb1/rust that referenced this issue Nov 27, 2017
MIR: Fix value moved diagnose messages

rust-lang#45960. I believe this will take a different approach. Simply replacing all nouns to verbs (`desired_action`) messes up the message `use of moved value` (although fixes the message in original issue). Here is what happens:

<pre>
$ rustc -Zborrowck-mir src/test/ui/borrowck/borrowck-reinit.rs

error[E0382]: <b>used</b> of moved value: `x` (Mir)
  --> src/test/ui/borrowck/borrowck-reinit.rs:18:16
   |
17 |     drop(x);
   |          - value moved here
18 |     let _ = (1,x);
   |                ^ value used here after move

error: aborting due to 2 previous errors
</pre>
(Notice: *"**used** of moved value: `x`"* instead of *"**use**"*)

Which does not seem to be okay.

After experimenting a bit, it looks like [`report_use_of_moved_value()`](https://github.com/rust-lang/rust/blob/1dc0b573e7ce4314eb196b21b7e0ea4a1bf1f673/src/librustc_mir/borrow_check.rs#L1319) tries to handle both these messages by taking in only one form of`desired_action`.

These messages rise from: *"[{noun} of moved value](https://github.com/rust-lang/rust/blob/1dc0b573e7ce4314eb196b21b7e0ea4a1bf1f673/src/librustc_mir/borrow_check.rs#L1338-L1342)"* and *"[value {verb} here after move](https://github.com/rust-lang/rust/blob/1dc0b573e7ce4314eb196b21b7e0ea4a1bf1f673/src/librustc_mir/borrow_check.rs#L1343)"*.

This PR fixes *"value {verb} here after move"* type messages by passing a corresponding verb (`desired_action`) instead of the original noun.
kennytm added a commit to kennytm/rust that referenced this issue Nov 27, 2017
MIR: Fix value moved diagnose messages

rust-lang#45960. I believe this will take a different approach. Simply replacing all nouns to verbs (`desired_action`) messes up the message `use of moved value` (although fixes the message in original issue). Here is what happens:

<pre>
$ rustc -Zborrowck-mir src/test/ui/borrowck/borrowck-reinit.rs

error[E0382]: <b>used</b> of moved value: `x` (Mir)
  --> src/test/ui/borrowck/borrowck-reinit.rs:18:16
   |
17 |     drop(x);
   |          - value moved here
18 |     let _ = (1,x);
   |                ^ value used here after move

error: aborting due to 2 previous errors
</pre>
(Notice: *"**used** of moved value: `x`"* instead of *"**use**"*)

Which does not seem to be okay.

After experimenting a bit, it looks like [`report_use_of_moved_value()`](https://github.com/rust-lang/rust/blob/1dc0b573e7ce4314eb196b21b7e0ea4a1bf1f673/src/librustc_mir/borrow_check.rs#L1319) tries to handle both these messages by taking in only one form of`desired_action`.

These messages rise from: *"[{noun} of moved value](https://github.com/rust-lang/rust/blob/1dc0b573e7ce4314eb196b21b7e0ea4a1bf1f673/src/librustc_mir/borrow_check.rs#L1338-L1342)"* and *"[value {verb} here after move](https://github.com/rust-lang/rust/blob/1dc0b573e7ce4314eb196b21b7e0ea4a1bf1f673/src/librustc_mir/borrow_check.rs#L1343)"*.

This PR fixes *"value {verb} here after move"* type messages by passing a corresponding verb (`desired_action`) instead of the original noun.
@ritiek
Copy link
Member

ritiek commented Dec 5, 2017

This issue can be closed as of #46231.

@kennytm
Copy link
Member

kennytm commented Dec 5, 2017

Closing as fixed by #46231.

@kennytm kennytm closed this as completed Dec 5, 2017
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-NLL Area: Non-lexical lifetimes (NLL) C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants