MIR-borrowck: moves of prefixes invalidate uses too #45025

pnkfelix · 2017-10-04T16:01:51Z

I overlooked the fact that when we check if a path is moved, we need to check for interference between the (shallow) prefixes and the use in question.

Long term, we may want to revise how this computation is done. For example, it might be better to represent the set of invalidated prefixes in the dataflow computation (the maybe_uninitialized dataflow), and thus avoid one of the loops in the code here.

Update: I was wrong in my original recollection of the dataflow code, which actually does the right thing, in terms of precisely tracking substructure initialization and movement.

Fix #44833

Update: The initial version of this PR's description (and the code as well) erroneously focused on supporting prefixes. ~~But the two main cases of interest are: 1. the shallow prefixes, and 2. the deref-free prefix built off a local (if the lvalue is indeed built off a local)~~

Update 2: The main cases of interest are in fact: 1. the nearest prefix with a MovePath, and 2. the suffixes.

rust-highfive · 2017-10-04T16:01:55Z

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

pnkfelix · 2017-10-04T16:04:08Z

src/test/compile-fail/borrowck/borrowck-uninit-field-access.rs

+    let mut line1 = Line::default();
+    let _moved = line1.origin;
+    let _ = line1.origin.x + 1; //[ast]~ ERROR use of collaterally moved value: `line1.origin.x`
+                                //[mir]~^       [E0382]


(yes, I do see that the error code here don't match. I also know that we're missing the "(Ast)" in the output from the ast-borrowcker when running under -Z borrowck-mir. I want to fix both of those things, but it need not be in this PR.)

(BTW I think this will be fixed by #45167)

pnkfelix · 2017-10-04T16:05:23Z

Muchas gracias to @mikhail-m1 for narrowing this down to such a small example! That really helped me quickly identify the cause.

eddyb · 2017-10-04T16:39:40Z

@bors r+

bors · 2017-10-04T16:39:41Z

📌 Commit 64cf5a6 has been approved by eddyb

arielb1 · 2017-10-04T19:33:48Z

I'm not sure this is correct:

        // A move of any *supporting prefix* of `lvalue` interferes
        // with an attempt to use `lvalue`. This is scenarios 1 and 2
        // above; and we are looking only at the supporting prefixes
        // because of scenario 5 above.

You shouldn't be able to write:

let x: &&_;
let y= &**x;

@bors r-

I think the reason this patch is needed is because the move-path code does not try to handle things behind (non-Box) references. IIRC the right way to handle this is to use the mpi (possibly in a LookupResult::Parent) returned by MovePathLookup::find and to check all of its children using on_all_children_bits.

Of course, I should have added comments there when I first rewrote that code, but I wasn't sure how close MIR borrowck was then.

eddyb · 2017-10-04T21:56:13Z

r? @arielb1

pnkfelix · 2017-10-04T22:12:05Z

@arielb1 wrote:

I'm not sure this is correct:

        // A move of any *supporting prefix* of `lvalue` interferes
        // with an attempt to use `lvalue`. This is scenarios 1 and 2
        // above; and we are looking only at the supporting prefixes
        // because of scenario 5 above.

Just to be clear: Am I correct that the part of that paragraph that you object to is the "we are looking only at the supporting prefixes because of scenario 5 above", right?

I.e., that you think there are reasons that we need to consider even the prefixes that are non-supporting, and that is why you have given your example of

let x: &&_;
let y= &**x;

Does all that sound correct?

(I'm not arguing with the example. I'm just wrapping my head around where that case was meant to fall in my mental model of the analysis.)

arielb1 · 2017-10-05T11:10:29Z

Just to be clear: Am I correct that the part of that paragraph that you object to is the "we are looking only at the supporting prefixes because of scenario 5 above", right?

Right

pnkfelix · 2017-10-05T15:33:37Z

After discussion with @arielb1, I am going to try to handle a generalization of the scenario he presented above. Namely, partial initialization like:

let x: S; x.a = 0; use(&x.a);

(where in general S does not implement Drop but may have fields beyond a)

In parallel with this, @arielb1 is looking into amending the NLL RFC to include this.

pnkfelix · 2017-10-09T13:41:40Z

src/librustc_mir/borrow_check.rs

+        // A move of any shallow suffix of `lvalue` also interferes
+        // with an attempt to use `lvalue`. This is scenario 3 above.
+        //
+        // (Distinct from previous loop because `lvalue` does not


nit: "previous loop" is confusing here. (The code has been internally refactored so that there is no longer a loop immediately present above, because its been factored into a helper method.)

nikomatsakis · 2017-10-09T14:36:02Z

src/librustc_mir/borrow_check.rs

            }
        }
    }

+    fn move_path_closest_to(&mut self,


It would be helpful, I think, to explain what this does, and also to cover the cases in which it might return None. Not having read into this code recently, it's sort of not obvious to me -- for example -- why the code above can do

if let Some(mpi) = ... { report_error() }

i.e., why is it that a None return value means "no error" and not "definite error"?

good point. I will review (and perhaps add an error branch at the call-site when it returns None)

(one case that kept arising during development, which may or may not be relevant here, are paths that are built off of a static variable; we don't put entries in the MoveData for those. But I've been musing that it would be better to be more explicit about the scenarios: i.e., if all the cases of None end up being due to a static variable at the root, we should be able to signal that explicitly in a result here, and then the client will have better info about why its sound to not issue an error.)

nikomatsakis · 2017-10-09T14:37:57Z

src/librustc_mir/borrow_check.rs

+                    // find and report move(s) that could cause this to be uninitialized
+                    self.report_use_of_moved(context, desired_action, lvalue_span);
+                    return; // don't bother finding other problems.
+                }
            }


e.g. here, it seems weird that it's ok to not have a path -- how do we know it's initialized then?

pnkfelix · 2017-10-11T10:50:53Z

At this point I think the time has come to bite the bullet and generalize the MoveData to track even Copy-able things. But I think we can land this PR without that, and then once that generalization lands, hopefully it will lead to my being able to remove things like the fn move_path_closest_to method.

pnkfelix · 2017-10-11T13:56:57Z

I wrote:

being able to remove things like the fn move_path_closest_to method

This may have been over-ambitious. In particular, cases like structs with destructors are ones where borrowck wants to map an lvalue like tuple.has_drop.field to tuple.has_drop, and it seems natural to express that via move_path_closest_to

arielb1 · 2017-10-11T19:46:17Z

src/librustc_mir/borrow_check.rs

+        // This is scenarios 1 and 2 above.
+
+        debug!("check_if_path_is_moved part1 lvalue: {:?}", lvalue);
+        match self.move_path_closest_to(lvalue, PrefixSet::Shallow) {


Why is this considering only the shallow set of prefixes? This later does a max_deref_free_locally_rooted_prefix_of to consider deep prefixes, so I don't see why this is split into 2 parts.

arielb1 · 2017-10-11T19:46:51Z

r=me with the shallow prefix thing fixed.

Fix rust-lang#44833 (a very specific instance of a very broad bug). In `check_if_path_is_moved(L)`, check nearest prefix of L with MovePath, and suffixes of L with MovePaths. Over the course of review, ariel pointed out a number of issues that led to this version of the commit: 1. Looking solely at supporting prefixes does not suffice: it overlooks checking if the path was ever actually initialized in the first place. So you need to be willing to consider non-supporting prefixes. Once you are looking at all prefixes, you *could* just look at the local that forms the base of the projection, but to handle partial initialization (which still needs to be formally specified), this code instead looks at the nearest prefix of L that has an associated MovePath (which, in the limit, will end up being a local). 2. You also need to consider the suffixes of the given Lvalue, due to how dataflow is representing partial moves of individual fields out of struct values. 3. (There was originally a third search, but ariel pointed out that the first and third could be folded into one.) Also includes some drive-by refactorings to simplify some method signatures and prefer `for _ in _` over `loop { }` (at least when it comes semi-naturally).

Expanded to cover partial-initialization ideas.

…+mir borrowcks. (There are other tests that this PR also improves, but were not completely synchronized. I chose to wait until later to pull those into the `revisions: ast mir` testing pattern; later being either when they *are* synchronized, or in some PR where we migrate all borrowck tests, regardless of whether MIR-borrowck is "finished" for them or not.)

pnkfelix · 2017-10-11T20:59:46Z

@bors r=arielb1

bors · 2017-10-11T20:59:47Z

📌 Commit d6caf73 has been approved by arielb1

bors · 2017-10-13T04:01:01Z

⌛ Testing commit d6caf73 with merge 86e5487...

…ixes-invalidate-uses-too, r=arielb1 MIR-borrowck: moves of prefixes invalidate uses too I overlooked the fact that when we check if a path is moved, we need to check for interference between the (shallow) prefixes and the use in question. ~~Long term, we may want to revise how this computation is done. For example, it might be better to represent the set of invalidated prefixes in the dataflow computation (the `maybe_uninitialized` dataflow), and thus avoid one of the loops in the code here.~~ * Update: I was wrong in my original recollection of the dataflow code, which actually does the right thing, in terms of precisely tracking substructure initialization and movement. Fix #44833 ---- Update: The initial version of this PR's description (and the code as well) erroneously focused on supporting prefixes. ~~But the two main cases of interest are: 1. the *shallow* prefixes, and 2. the deref-free prefix built off a local (if the lvalue is indeed built off a local)~~ Update 2: The main cases of interest are in fact: 1. the nearest prefix with a MovePath, and 2. the suffixes.

bors · 2017-10-13T06:32:05Z

☀️ Test successful - status-appveyor, status-travis
Approved by: arielb1
Pushing 86e5487 to master...

rust-highfive assigned eddyb Oct 4, 2017

pnkfelix commented Oct 4, 2017

View reviewed changes

pnkfelix added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. WG-compiler-nll labels Oct 4, 2017

rust-highfive assigned arielb1 and unassigned eddyb Oct 4, 2017

pnkfelix mentioned this pull request Oct 5, 2017

MIR-borrowck: (unsound) fails to see error in borrowck-init-in-fru.rs #44833

Closed

pnkfelix force-pushed the mir-borrowck-moves-of-supporting-prefixes-invalidate-uses-too branch from ab78005 to 10dbf16 Compare October 5, 2017 15:10

pnkfelix changed the title ~~MIR-borrowck: moves of supporting prefixes invalidate uses too~~ MIR-borrowck: moves of prefixes invalidate uses too Oct 5, 2017

pnkfelix added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 5, 2017

pnkfelix changed the title ~~MIR-borrowck: moves of prefixes invalidate uses too~~ [WIP] MIR-borrowck: moves of prefixes invalidate uses too Oct 5, 2017

pnkfelix force-pushed the mir-borrowck-moves-of-supporting-prefixes-invalidate-uses-too branch from 10dbf16 to 54c6ae4 Compare October 6, 2017 16:33

pnkfelix added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 6, 2017

pnkfelix changed the title ~~[WIP] MIR-borrowck: moves of prefixes invalidate uses too~~ MIR-borrowck: moves of prefixes invalidate uses too Oct 6, 2017

pnkfelix commented Oct 9, 2017

View reviewed changes

nikomatsakis reviewed Oct 9, 2017

View reviewed changes

pnkfelix force-pushed the mir-borrowck-moves-of-supporting-prefixes-invalidate-uses-too branch from 54c6ae4 to 32a381c Compare October 11, 2017 10:17

arielb1 reviewed Oct 11, 2017

View reviewed changes

pnkfelix added 4 commits October 11, 2017 22:35

Test against accesses to uninitialized fields.

ae5fc76

Test case illustrating some variants of the issue pointed out by ariel.

b62b67a

Expanded to cover partial-initialization ideas.

pnkfelix force-pushed the mir-borrowck-moves-of-supporting-prefixes-invalidate-uses-too branch from f328f93 to d6caf73 Compare October 11, 2017 20:44

pnkfelix 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 Oct 12, 2017

bors merged commit d6caf73 into rust-lang:master Oct 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MIR-borrowck: moves of prefixes invalidate uses too #45025

MIR-borrowck: moves of prefixes invalidate uses too #45025

pnkfelix commented Oct 4, 2017 •

edited

Loading

rust-highfive commented Oct 4, 2017

pnkfelix Oct 4, 2017

pnkfelix Oct 13, 2017

pnkfelix commented Oct 4, 2017 •

edited

Loading

eddyb commented Oct 4, 2017

bors commented Oct 4, 2017

arielb1 commented Oct 4, 2017

eddyb commented Oct 4, 2017

pnkfelix commented Oct 4, 2017 •

edited

Loading

arielb1 commented Oct 5, 2017

pnkfelix commented Oct 5, 2017 •

edited

Loading

pnkfelix Oct 9, 2017

nikomatsakis Oct 9, 2017

pnkfelix Oct 10, 2017

pnkfelix Oct 10, 2017 •

edited

Loading

nikomatsakis Oct 9, 2017

pnkfelix commented Oct 11, 2017

pnkfelix commented Oct 11, 2017

arielb1 Oct 11, 2017

arielb1 commented Oct 11, 2017

pnkfelix commented Oct 11, 2017

bors commented Oct 11, 2017

bors commented Oct 13, 2017

bors commented Oct 13, 2017

MIR-borrowck: moves of prefixes invalidate uses too #45025

MIR-borrowck: moves of prefixes invalidate uses too #45025

Conversation

pnkfelix commented Oct 4, 2017 • edited Loading

rust-highfive commented Oct 4, 2017

pnkfelix Oct 4, 2017

Choose a reason for hiding this comment

pnkfelix Oct 13, 2017

Choose a reason for hiding this comment

pnkfelix commented Oct 4, 2017 • edited Loading

eddyb commented Oct 4, 2017

bors commented Oct 4, 2017

arielb1 commented Oct 4, 2017

eddyb commented Oct 4, 2017

pnkfelix commented Oct 4, 2017 • edited Loading

arielb1 commented Oct 5, 2017

pnkfelix commented Oct 5, 2017 • edited Loading

pnkfelix Oct 9, 2017

Choose a reason for hiding this comment

nikomatsakis Oct 9, 2017

Choose a reason for hiding this comment

pnkfelix Oct 10, 2017

Choose a reason for hiding this comment

pnkfelix Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

nikomatsakis Oct 9, 2017

Choose a reason for hiding this comment

pnkfelix commented Oct 11, 2017

pnkfelix commented Oct 11, 2017

arielb1 Oct 11, 2017

Choose a reason for hiding this comment

arielb1 commented Oct 11, 2017

pnkfelix commented Oct 11, 2017

bors commented Oct 11, 2017

bors commented Oct 13, 2017

bors commented Oct 13, 2017

pnkfelix commented Oct 4, 2017 •

edited

Loading

pnkfelix commented Oct 4, 2017 •

edited

Loading

pnkfelix commented Oct 4, 2017 •

edited

Loading

pnkfelix commented Oct 5, 2017 •

edited

Loading

pnkfelix Oct 10, 2017 •

edited

Loading