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

Finalise initialisation calculations #110

Merged
merged 4 commits into from Sep 3, 2019

Conversation

@albins
Copy link
Contributor

commented Aug 6, 2019

This PR will:

  • finally remove region_live_at
  • implement enough initialisation tracking to correctly calculate drop-uses for liveness
  • extend polonius-parser to work in this new universe

Notably, it will not calculate the full move analysis, as that would a) probably take a lot of time and b) require refactoring of the entire Polonius pipeline, or at the very least extensions to handle more types of errors.

@albins albins force-pushed the albins:initialisation-phase-2 branch from c1e50a6 to 44b6796 Aug 6, 2019

@albins albins force-pushed the albins:initialisation-phase-2 branch from 8d61eaf to bdcee90 Aug 16, 2019

@albins albins force-pushed the albins:initialisation-phase-2 branch 2 times, most recently from 85a67cb to eb3443f Aug 16, 2019

@albins albins changed the title [WIP] Finalise initialisation calculations Finalise initialisation calculations Aug 19, 2019

@albins albins force-pushed the albins:initialisation-phase-2 branch from eb3443f to 4a76fc2 Aug 27, 2019

@nikomatsakis
Copy link
Contributor

left a comment

Looking good! Thoughts attached. =)

/// `var_initialized_on_exit(V, P) when the variable `V` is initialized on
/// exit from point `P` in the program flow.
pub var_initialized_on_exit: Vec<(V, P)>,
/// `child(M1, M2) when the move path `M1` is the child of `M2`.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Aug 28, 2019

Contributor

I think it'd be useful to give an example. For example, I imagine:


child(M1, M2) would be true If M1 represents a.b.c and M2 represents a.b.

This comment has been minimized.

Copy link
@albins

albins Aug 28, 2019

Author Contributor

That's true. The hand-wavyness was because I was not entirely sure if it was transitive or not. It is, though, which may or may not be what we want.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Aug 30, 2019

Contributor

Interesting. The name strongly suggests non-transitive to me. I would have said something like descendant for a transitive child relationship.

This comment has been minimized.

Copy link
@albins

albins Sep 3, 2019

Author Contributor

True, I'll make a note of fixing this at some point.

pub child: Vec<(M, M)>,

/// `path_belongs_to_var(M, V) when the move path `M` starts in variable `V`.
pub path_belongs_to_var: Vec<(M, V)>,

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Aug 28, 2019

Contributor

Why 'belongs to'? I think maybe path_starts_with_var or something.

This comment has been minimized.

Copy link
@albins

albins Aug 28, 2019

Author Contributor

I called it that originally, but I got confused and was unsure if it held transitively (it doesn't). I'll change it to something better.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Aug 30, 2019

Contributor

Hmm. I'm not sure what "transitive" means in this case?

This comment has been minimized.

Copy link
@albins

albins Sep 3, 2019

Author Contributor

"Includes descendants"

pub path_belongs_to_var: Vec<(M, V)>,

/// `initialized_at(M, P) when the move path `M` was initialized at point
/// `P`.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Aug 28, 2019

Contributor

Again, I think an example would be good --


If we have some Rust code like:

x.y = 3 // point P

where M1 represents x.y, then we would have initialized_at(M1, P).

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Aug 28, 2019

Contributor

Reading over the code I had a few questions. Are you assuming that:

a = (22, 44);

would generate:

initialized_at(a);
initialized_at(a.0);
initialized_at(a.1);

?

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Aug 28, 2019

Contributor

I think my preference would NOT to assume that -- that is, I think the compiler should give us just initialized_at(a), and if we care to elaborate the closure with respect to child paths, we do it ourselves.

This comment has been minimized.

Copy link
@albins

albins Aug 28, 2019

Author Contributor

Yes, I had to investigate what the current fact generation actually does, and it only generates the fact for the path being initialized directly. I'll clarify this in the comments and add an example.

/// `P`.
pub initialized_at: Vec<(M, P)>,

/// `moved_out_at(M, P) when the move path `M` was moved at point `P`.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Aug 28, 2019

Contributor

As above.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Aug 28, 2019

Contributor

Similarly, are we assuming here that drop(a); would generate moved_out(a.0) and moved_out(a.1) facts?

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Aug 28, 2019

Contributor

Same as above -- I think we should do the elaboration ourselves.

polonius-engine/src/facts.rs Show resolved Hide resolved

use datafrog::{Iteration, Relation, RelationLeaper};

pub(super) fn init_var_maybe_initialized_on_exit<Region, Loan, Point, Variable, MovePath>(

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Aug 28, 2019

Contributor

This function definitely wants a comment with some kind of Rust example showing where it's true and not true.

polonius-engine/src/output/initialization.rs Outdated Show resolved Hide resolved

// path_maybe_initialized_on_exit(Mother, P) :-
// path_maybe_initialized_on_exit(Daughter, P),
// child(Daughter, Mother).

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Aug 28, 2019

Contributor

I'm debating the purpose of this rule. It feels sort of incomplete on its own. It also suggests that the name "maybe initialized" could be refined -- in particular I guess it includes partial initialization? That is, this rule says that if a.b is "maybe initialized", then a is "maybe initialized". There is no corresponding rule saying (for example) that a move from a is also a move from a.b, so I have to assume that moved_out facts are the "transitive closure" over children (and that suggests to me that initialization facts should be the same).

Well, I guess the point of this "maybe partially initialized" relation is precisely to inform the region-live-at computation in NLL, which is probably already imprecise in this manner? I guess I better double check, but I remember us making some decisions like that.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Aug 28, 2019

Contributor

Still, if the goal is to ultimately compute this relation, then why do we need the previous rule?

Well, it doesn't seem harmful -- but I guess we can either do the elaboration at the initial point -- i.e., if some path M is initialized at a point P, then we could make the initial value include that all paths which are either parents or children of M are "maybe initialized" at the point P. And then we don't need this rule to be part of the iteration -- rather, we compute this transition closure before hand.

I think this is what Lark did, but I suppose I should go back and try to write up the rules I used there. Maybe a good blog post.

This comment has been minimized.

Copy link
@albins

albins Aug 28, 2019

Author Contributor

I think this connects with your next comment, so I'll reply to it there.

// var_maybe_initialized_on_exit(V, P) :-
// path_belongs_to_var(M, V),
// path_maybe_initialized_at(M, P).
var_maybe_initialized_on_exit.from_leapjoin(

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Aug 28, 2019

Contributor

Something is tickling me here. If the goal ultimately is to compute which variables are maybe initialized, why do we have to extend to the parents? That is, it feels like we should track initialization precisely.

Something like this:

  • When you assign to a path M, that also counts as an assignment to each child path Mc (where Mc is a transitive child of M).
  • When you move from M, that also counts as a move from from child path Mc (as above).
  • If M is initialized at P, then it is initialized at each successor Q, unless Q moves from M (as above).

then compute var_maybe_init as we are doing here.

I guess the example where this differs is something like:

a.1 = foo; // initializes a.1 in my version, but also initializes a in your version
drop(a.1); // moves out from a.1 only
// is "a" considered maybe init here? in your version it is, in mine it isn't.

The existing NLL is somewhat approximate here, so maybe this is intentional.

Am I missing something?

This comment has been minimized.

Copy link
@albins

albins Aug 28, 2019

Author Contributor

I think what you are detecting is that I am computing sort of two different transitive closures. What I want to do is to trace back partial initialisation to a variable for use in the next step to determine if a drop would happen.

Probably, the cleanest design would be to compute the transitive closure over paths downwards, and separately extend the "variable-rootedness" across another transitive closure (tracing all paths back to their variable), and then finally perform the join, but I am also not sure if this is an over-approximation. The more I think about this, the less I feel like I understand it.

I am fairly certain what we did in the original fact generation was this, though: we emitted var_maybe_initialized_at whenever some sub-path of var was (maybe) initialized at that point.

@albins albins force-pushed the albins:initialisation-phase-2 branch from 4a76fc2 to 98168c0 Aug 28, 2019

@albins albins force-pushed the albins:initialisation-phase-2 branch from 98168c0 to d720ed9 Sep 3, 2019

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

Per our discussion today, we decided to land this as is

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2019

And consider any follow-ups for later

@nikomatsakis nikomatsakis merged commit 235745d into rust-lang:master Sep 3, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.