Skip to content

Maybe initialized liveness #12

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

Conversation

Nashenas88
Copy link

Original PR conversation is here: rust-lang#45794.

@nikomatsakis
Copy link
Owner

I'd rather wait until we see the PR for rust-lang#45889 first, I think. It'd be nice if we could get get this test passing. =)

@Nashenas88 Nashenas88 force-pushed the maybe-initialized-liveness-for-nll-master branch from 07c6dbb to 95d95e6 Compare November 12, 2017 15:12
47 | let wrap = Wrap { p: &mut x };
| ------ borrow of `x` occurs here
...
52 | x += 1; // OK, drops are inert
Copy link
Author

Choose a reason for hiding this comment

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

I still need to look into why this is an error.

@nikomatsakis nikomatsakis force-pushed the nll-master branch 2 times, most recently from ef2d660 to c93dd18 Compare November 12, 2017 18:21
@nikomatsakis nikomatsakis force-pushed the maybe-initialized-liveness-for-nll-master branch from 95d95e6 to 305a883 Compare November 12, 2017 19:39
@nikomatsakis
Copy link
Owner

Incidentally, although I prefer ui tests in general, I think this test might be better written as a compile-fail test, at least until such time as we have the ability to add //~ ERROR annotations into ui tests. It's pretty annoying to try and match up the errors from the stderr output with the lines of the test; this is one place where compile-fail style tests shine (particularly as we are just interested in ensuring we get the right errors at the right places).

@nikomatsakis
Copy link
Owner

This has a lot of conflicts due to me having merged: #14

@nikomatsakis
Copy link
Owner

OK, per recent conversation on gitter, we need to:

  1. Rebase. =)
  2. Rewrite the code so that rather than just check for any maybe-init child of X, we instead walk the tree of children and identity the leaves that are maybe-init. For each leaf L of type T, ensure the regions are live in its type T as we do now.

@Nashenas88 Nashenas88 force-pushed the maybe-initialized-liveness-for-nll-master branch from 0a0256d to 99ed9e2 Compare November 14, 2017 04:39
self.add_drop_live_constraint(initialized_child_ty, location);
}

if any {
Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure if this was still necessary or not. Also, the test is still not working as expected. For one, it's still in /ui. I thought I had moved it to /compile-fail, but I might have lost that work when rebasing.

@nikomatsakis
Copy link
Owner

So, the problem is that initialized_children_of is not returning the right thing, I think. Let me explain.

In the case of the example, we have the local variable foo and then there is a move from foo.a and a move from foo.b. The move analysis thus constructs a little tree of paths:

  • foo, with child link to:
    • foo.a, with sibling link to:
    • foo.b

The statement foo = Foo { .. } will cause all three paths to be considered initialized (assigning to lvalue initializes it and all of its transitive children, more generally). The subsequent moves from foo.a and foo.b will make those paths be considered deinitialized; but the path foo is still in the "initialized" set. (This can be a bit confusing: even though foo is in the set of maybe-initialized things, it could not be considered initialized, because it contains children that are not in the set. There are good reasons to track foo separately from foo.a -- but it's kind of a distraction here.)

In any case, for our purposes, if we see that foo is drop-live, what we want to do is to enumerate the leaf nodes reachable from foo -- that is, [foo.a, foo.b] -- and then check if they are maybe-initialized. If so, we want to do the drop testing on them. This is not quite what your code is doing. It is enumerating all nodes reachable from foo -- that is, [foo, foo.a, foo.b]. This is why it doesn't work.

That said, I tried to tweak the routine in [this diff]((https://gist.github.com/nikomatsakis/a19d21a651a6e56b25ad03111666765a), but it doesn't seem quite right. It makes the test case in question pass, but it also suppresses a lot of other errors we should be reporting. The diff only adds a node to the final result if it has no child -- that is, it is a leaf node in the tree. But the name is perhaps a bit misleadling, and should maybe be called leaf_nodes_present_in_set or something.

@nikomatsakis
Copy link
Owner

Oh, I see the problem with my check. The problem is that we only create move paths for the things that are individually touched, but in this case foo.b is not touched. So we would need to "splinter" foo into foo.a and foo.b a bit more eagerly. Hmm. Well, that's annoying! That will be a bit difficult to do, I expect. Maybe if we combine with the maybe-uninit? I have to think about this. =)

@nikomatsakis
Copy link
Owner

nikomatsakis commented Nov 14, 2017

@arielb1 @pnkfelix

I have to run in a bit, but I have (maybe) a quick question. We're hitting an obstacle in this PR. We have a struct with two fields, a and b, and we want to know at any given point which fields may still be dropped. The existing data structures tell us that, but not in the most convenient way. For example, in this code:

{
  let foo = Foo { a, b };
  drop(foo.b);
  // here: `foo` is still in the initialized set, `foo.b` is not, 
  // and there is no move-path for `foo.a`
}

whereas:

{
  let foo = Foo { a, b };
  drop(foo.a);
  drop(foo.b);
  // here: `foo` is still in the initialized set, but `foo.a` and `foo.b` are not
}

It seems to me that, to find the set of maybe-init lvalues that start from Foo, we would need to enumerate the "potential children" of foo (even those where no move-path yet exists) and check, for each one, if a move-path exists, and if so, if it is in the set. Is there a subroutine for doing this sort of thing already?

(I thought we could just look at the leaves, which handles the second example, but not the first one.)

@arielb1
Copy link

arielb1 commented Nov 14, 2017

Is there a subroutine for doing this sort of thing already?

There's no subroutine for that in librustc - that handling is done intertwined by the code in drop elaboration, and this is necessary because it needs to handle arrays and enums.

@nikomatsakis
Copy link
Owner

I suppose we could also leave this as a fixme for now -- we can handle the simple case of drop(foo) but not the more complex cases of drop(foo.b) etc.

@arielb1
Copy link

arielb1 commented Nov 14, 2017

@nikomatsakis

It wouldn't be too hard to add such a function, but I agree that a FIXME would be ok at first.

@nikomatsakis
Copy link
Owner

sure, just seems like it ought to be a PR of its own

@nikomatsakis
Copy link
Owner

ok, really going afk now =)

@nikomatsakis nikomatsakis force-pushed the nll-master branch 2 times, most recently from b3f3390 to 55e195b Compare November 17, 2017 01:48
@nikomatsakis nikomatsakis force-pushed the maybe-initialized-liveness-for-nll-master branch from f2cf20d to b4ed07f Compare November 17, 2017 09:45
let mdpe = MoveDataParamEnv {
move_data: move_data,
param_env: param_env,
};
Copy link
Owner

Choose a reason for hiding this comment

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

Oh damn, I applied rustfmt accidentally. Let me separate that into its own commit at least.

nikomatsakis and others added 2 commits November 17, 2017 04:47
In particular, if we see a variable is DROP-LIVE, but it is not
MAYBE-INIT, then we can ignore the drop. This leavess attempt to use
more complex refinements of the idea (e.g., for subpaths or subfields)
to future work.
@nikomatsakis nikomatsakis force-pushed the maybe-initialized-liveness-for-nll-master branch from b4ed07f to 0a7b7dd Compare November 17, 2017 09:48
@nikomatsakis nikomatsakis merged commit 0a7b7dd into nikomatsakis:nll-master Nov 17, 2017
@Nashenas88 Nashenas88 deleted the maybe-initialized-liveness-for-nll-master branch November 20, 2017 15:43
nikomatsakis pushed a commit that referenced this pull request Sep 12, 2019
sync with rust-lang/rust branch master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants