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: Fix value moved diagnose messages #46231

Merged
merged 1 commit into from Nov 28, 2017

Conversation

Projects
None yet
5 participants
@ritiek
Member

ritiek commented Nov 24, 2017

#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:

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

error[E0382]: used 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

(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() tries to handle both these messages by taking in only one form ofdesired_action.

These messages rise from: "{noun} of moved value" and "value {verb} here after move".

This PR fixes "value {verb} here after move" type messages by passing a corresponding verb (desired_action) instead of the original noun.

@rust-highfive

This comment has been minimized.

Show comment
Hide comment
@rust-highfive

rust-highfive Nov 24, 2017

Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @arielb1 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

Collaborator

rust-highfive commented Nov 24, 2017

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @arielb1 (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@ritiek

This comment has been minimized.

Show comment
Hide comment
@ritiek
Member

ritiek commented Nov 24, 2017

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Nov 24, 2017

Member

cc @pnkfelix.

Personally I'd create an enum so that the unreachable branch won't appear at all, but that will introduce a lot of changes just for a grammatical fix.

#[derive(Copy, Clone)]
enum DesiredAction {
    Update,
    Borrow,
    Use,
    Assignment,
}
impl DesiredAction {
    fn as_noun(self) -> &'static str {
        match self {
            DesiredAction::Update => "update",
            ...
        }
    }
    fn as_verb_in_past_tense(self) -> &'static str {
        match self {
            DesiredAction::Update => "updated",
            ...
        }
    }
}
Member

kennytm commented Nov 24, 2017

cc @pnkfelix.

Personally I'd create an enum so that the unreachable branch won't appear at all, but that will introduce a lot of changes just for a grammatical fix.

#[derive(Copy, Clone)]
enum DesiredAction {
    Update,
    Borrow,
    Use,
    Assignment,
}
impl DesiredAction {
    fn as_noun(self) -> &'static str {
        match self {
            DesiredAction::Update => "update",
            ...
        }
    }
    fn as_verb_in_past_tense(self) -> &'static str {
        match self {
            DesiredAction::Update => "updated",
            ...
        }
    }
}
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 24, 2017

Contributor

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

Contributor

bors commented Nov 24, 2017

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

@ritiek

This comment has been minimized.

Show comment
Hide comment
@ritiek

ritiek Nov 25, 2017

Member

I've replaced the branch with span_bug!, not sure if it is correct though. An enum would introduce a lot of changes indeed but I'll wait for the final call.

Member

ritiek commented Nov 25, 2017

I've replaced the branch with span_bug!, not sure if it is correct though. An enum would introduce a lot of changes indeed but I'll wait for the final call.

@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Nov 25, 2017

Contributor

@ritiek

Could you create a new enum enum InitializationRequiringAction and use it instead of passing strings for desired_action and matching on them? For one thing, this will avoid weird problems if we update the error messages.

r=me modulo that

Contributor

arielb1 commented Nov 25, 2017

@ritiek

Could you create a new enum enum InitializationRequiringAction and use it instead of passing strings for desired_action and matching on them? For one thing, this will avoid weird problems if we update the error messages.

r=me modulo that

@kennytm

This comment has been minimized.

Show comment
Hide comment
@kennytm

kennytm Nov 25, 2017

Member

Please also leave no merge commits (those Merge branch 'master' into verbs). Rebase and squash after you're done.

Member

kennytm commented Nov 25, 2017

Please also leave no merge commits (those Merge branch 'master' into verbs). Rebase and squash after you're done.

@ritiek

This comment has been minimized.

Show comment
Hide comment
@ritiek

ritiek Nov 26, 2017

Member

@arielb1 Sure, I'll report back as soon as I make progress.

@kennytm Okie, I'll keep that in mind.

Member

ritiek commented Nov 26, 2017

@arielb1 Sure, I'll report back as soon as I make progress.

@kennytm Okie, I'll keep that in mind.

MIR: Fix value moved diagnose messages
MIR: adopt borrowck test

Fix trailing whitespace

span_bug! on unexpected action

Make RegionVid use newtype_index!

Closes #45843

Check rvalue aggregates during check_stmt in tycheck, add initial, (not passing) test

Fix failing test

Remove attributes and test comments accidentally left behind, add in span_mirbugs

Normalize LvalueTy for ops and format code to satisfy tidy check

only normalize operand types when in an ADT constructor

avoid early return

handle the active field index in unions

normalize types in ADT constructor

Fixes #45940

Fix borrowck compiler errors for upvars contain "spurious" dereferences

Fixes #46003

added associated function Box::leak

Box::leak - improve documentation

Box::leak - fixed bug in documentation

Box::leak - relaxed constraints wrt. lifetimes

Box::leak - updated documentation

Box::leak - made an oops, fixed now =)

Box::leak: update unstable issue number (46179).

Add test for #44953

Add missing Debug impls to std_unicode

Also adds #![deny(missing_debug_implementations)] so they don't get
missed again.

Amend RELEASES for 1.22.1

and fix the date for 1.22.0

Rename param in `[T]::swap_with_slice` from `src` to `other`.

The idea of ‘source’ and ‘destination’ aren’t very applicable for this
operation since both slices can both be considered sources and
destinations.

Clarify stdin behavior of `Command::output`.

Fixes #44929.

Add hints for the case of confusing enum with its variants

Add failing testcases

Add module population and case of enum in place of expression

Use for_each_child_stable in find_module

Use multiline text for crate conflict diagnostics

Make float::from_bits transmute (and update the documentation to reflect this).

The current implementation/documentation was made to avoid sNaN because of
potential safety issues implied by old/bad LLVM documentation. These issues
aren't real, so we can just make the implementation transmute (as permitted
by the existing documentation of this method).

Also the documentation didn't actually match the behaviour: it said we may
change sNaNs, but in fact we canonicalized *all* NaNs.

Also an example in the documentation was wrong: it said we *always* change
sNaNs, when the documentation was explicitly written to indicate it was
implementation-defined.

This makes to_bits and from_bits perfectly roundtrip cross-platform, except
for one caveat: although the 2008 edition of IEEE-754 specifies how to
interpet the signaling bit, earlier editions didn't. This lead to some platforms
picking the opposite interpretation, so all signaling NaNs on x86/ARM are quiet
on MIPS, and vice-versa.

NaN-boxing is a fairly important optimization, while we don't even guarantee
that float operations properly preserve signalingness. As such, this seems like
the more natural strategy to take (as opposed to trying to mangle the signaling
bit on a per-platform basis).

This implementation is also, of course, faster.

Simplify an Iterator::fold to Iterator::any

This method of once-diagnostics doesn't allow nesting

UI tests extract the regular output from the 'rendered' field in json

Merge cfail and ui tests into ui tests

Add a MIR pass to lower 128-bit operators to lang item calls

Runs only with `-Z lower_128bit_ops` since it's not hooked into targets yet.

Include tuple projections in MIR tests

Add type checking for the lang item

As part of doing so, add more lang items instead of passing u128 to the i128 ones where it doesn't matter in twos-complement.

Handle shifts properly

* The overflow-checking shift items need to take a full 128-bit type, since they need to be able to detect idiocy like `1i128 << (1u128 << 127)`
* The unchecked ones just take u32, like the `*_sh?` methods in core
* Because shift-by-anything is allowed, cast into a new local for every shift

incr.comp.: Make sure we don't lose unused green results from the query cache.

rustbuild: Update LLVM and enable ThinLTO

This commit updates LLVM to fix #45511 (https://reviews.llvm.org/D39981) and
also reenables ThinLTO for libtest now that we shouldn't hit #45768. This also
opportunistically enables ThinLTO for libstd which was previously blocked
(#45661) on test failures related to debuginfo with a presumed cause of #45511.

Closes #45511

std: Flag Windows TLS dtor symbol as #[used]

Turns out ThinLTO was internalizing this symbol and eliminating it. Worse yet if
you compiled with LTO turns out no TLS destructors would run on Windows! The
`#[used]` annotation should be a more bulletproof implementation (in the face of
LTO) of preserving this symbol all the way through in LLVM and ensuring it makes
it all the way to the linker which will take care of it.

Add enum InitializationRequiringAction

Fix tidy tests
@arielb1

This comment has been minimized.

Show comment
Hide comment
@arielb1

arielb1 Nov 26, 2017

Contributor

@bors r+ rollup

Contributor

arielb1 commented Nov 26, 2017

@bors r+ rollup

@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors Nov 26, 2017

Contributor

📌 Commit 1be38e0 has been approved by arielb1

Contributor

bors commented Nov 26, 2017

📌 Commit 1be38e0 has been approved by arielb1

arielb1 added a commit to arielb1/rust that referenced this pull request Nov 27, 2017

Rollup merge of rust-lang#46231 - ritiek:verbs, r=arielb1
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 pull request Nov 27, 2017

Rollup merge of rust-lang#46231 - ritiek:verbs, r=arielb1
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.

bors added a commit that referenced this pull request Nov 27, 2017

Auto merge of #46312 - kennytm:rollup, r=kennytm
Rollup of 10 pull requests

- Successful merges: #45506, #46174, #46231, #46240, #46249, #46258, #46262, #46275, #46282, #46285
- Failed merges:

bors added a commit that referenced this pull request Nov 27, 2017

Auto merge of #46312 - kennytm:rollup, r=kennytm
Rollup of 10 pull requests

- Successful merges: #45506, #46174, #46231, #46240, #46249, #46258, #46262, #46275, #46282, #46285
- Failed merges:

@bors bors merged commit 1be38e0 into rust-lang:master Nov 28, 2017

1 check passed

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

@ritiek ritiek deleted the ritiek:verbs branch Nov 28, 2017

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