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

restore move out dataflow, add report of move out errors #45877

Merged
merged 3 commits into from
Nov 11, 2017

Conversation

mikhail-m1
Copy link
Contributor

fix #45363
r? @arielb1

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 8, 2017
&self.describe_lvalue(lvalue),
Origin::Mir);
err.span_label(span, format!("value {} here after move", desired_action));
for moi in mois {
Copy link
Contributor

@arielb1 arielb1 Nov 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AST borrowck only shows 1 move here, as in:

fn main() {
    let x = Box::new(1);
    if false {
        let _y = x;
    } else {
        drop(x);
    }
    x;
}

So I think you should only show a note for the first move.

Copy link
Contributor

@arielb1 arielb1 Nov 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, AST borrowck prefers the "value might be uninitialized" error to the "value might be moved" one - you can get it if you add a "move" representing values being uninitialized at the start of a function to the move-propagation code, but I won't block this PR on not matching the AST error behavior - this is an edge case and both error messages point at real problems.

sets: &mut BlockSets<MoveOutIndex>,
location: Location)
{
let (mir, move_data) = (self.mir, self.move_data());
Copy link
Contributor

@arielb1 arielb1 Nov 8, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code doesn't account for DropAndReplace - that should be handled the same way as an assignment above, so it would be buggy in the case of e.g.

let x = Box::new(0);
let _u = x; // error shouldn't note this move
x = Box::new(1);
drop(x);
use(x);

@arielb1
Copy link
Contributor

arielb1 commented Nov 8, 2017

nice PR, r=me modulo comments (you only have to fix the 1-error and the DropAndReplace problems - I'm not sure defaulting to "maybe uninitialized" is actually better).

@arielb1
Copy link
Contributor

arielb1 commented Nov 8, 2017

Actually, could you make it so that the moving-statements pass only runs if an error occurred? This would avoid it slowing down the normal error-free path.

@arielb1
Copy link
Contributor

arielb1 commented Nov 9, 2017

r=me if this is green on travis

@arielb1
Copy link
Contributor

arielb1 commented Nov 9, 2017

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 9, 2017

📌 Commit 3acb4e9 has been approved by arielb1

@kennytm kennytm 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 Nov 10, 2017
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Nov 11, 2017
…d, r=arielb1

restore move out dataflow, add report of move out errors

fix rust-lang#45363
r? @arielb1
bors added a commit that referenced this pull request Nov 11, 2017
Rollup of 4 pull requests

- Successful merges: #45631, #45812, #45877, #45919
- Failed merges:
@bors bors merged commit 3acb4e9 into rust-lang:master Nov 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MIR-borrowck: should distinguish "uninitalized" from "moved out of"
4 participants