Flow analysis preliminaries

[leafpetersen]

This is not for submittal, just using this as a place for discussion of the setup for flow analysis, and the policy choices.

[leafpetersen]

So what's going on here? This is my attempt to incorporate Paul's extended reachability into the basic framework. I think this is roughly what the extended reachability was doing, except that instead of keeping the stack of previous reachability values on the call stack, I'm keeping it explicitly in the analysis. An implementation could do either. So far, I've found the book-keeping a little cleaner here. Note that I'm possibly applying this in more places than Paul had originally intended? For example, I think the following is a motivating example:

Object x;
if (false && x is int) {
  x.isEven
}

I'm not sure whether Paul intended this to promote in the body or not, but it seemed reasonable, and I think if I have this set up right, then we will get that.

[stereotype441]

I really like the way you've phrased this paragraph, and I'm inclined to try to adjust the implementation to keep the stack explicitly in the FlowModel just for clarity. I'll add a test case for this example and see if it works :)

[leafpetersen]

Thanks - glad that phrasing this as a stack agrees with your intuition. My text was not completely accurate though - the elements of the stack really describe the reachability of a single link in the chain of control flow splits. I've updated the text to be more precise, and hopefully still conveying the intuition.

[leafpetersen]

I'm not 100% clear whether I'm doing the right thing here. It's clear what to do if one of the branches is provisionally reachable and the other is not, and it's clear what to do if both are provisionally reachable. But what about if both are not provisionally reachable? My guess here is that the right thing to do is to treat them both as provisionally reachable - that is, keep both of their states. But I'm not 100% sure about that.

  foo(Object x, bool b) {
    throw "Unimplemented";
    if (b) {
      if (x is! int) return;
      throw "Unimplemented";
   } else {
     if (x is! int return;
     throw "Unimplemented";
  }
  x.isEven; // should work?
}

[stereotype441]

Seems reasonable to me. It's a weird corner and I doubt it will come up very frequently, but what you're proposing seems like it will be fairly well behaved.

[leafpetersen]

I've tried to capture the policy options for assignment promotion that were proposed here here. What other options should I include?

[leafpetersen]

Allow demotion to anything that we had previously promoted to? Probably also need to include the declared type here.

[leafpetersen]

cc @lrhn @johnniwinther @scheglov @eernstg @munificent @stereotype441

I'm starting to work on pulling together Paul's proposal and the various discussions that have been going on into a spec for the flow analysis based type promotion etc. This is some initial machinery and policy discussion that I thought would be useful to get some early feedback on.

[stereotype441]

This looks like a great start! Thanks Leaf!

[stereotype441]

I really like the way you've phrased this paragraph, and I'm inclined to try to adjust the implementation to keep the stack explicitly in the FlowModel just for clarity. I'll add a test case for this example and see if it works :)

[stereotype441]

I think you mean r1 = push(pop(r0), top(r0) && top(r)).

[leafpetersen]

done.

[stereotype441]

Consider defining join before drop, since drop refers to join.

[leafpetersen]

done.

[stereotype441]

I assume you mean "if top(r1) is false and top(r2) is true"

[leafpetersen]

done

[stereotype441]

Seems reasonable to me. It's a weird corner and I doubt it will come up very frequently, but what you're proposing seems like it will be fairly well behaved.

[stereotype441]

Is there a typo here? The previous bullet says the interaction between assignment based promotion and downwards inference is "probably manageable", and now we're saying that the same interaction is "a bit trickier".

[leafpetersen]

No, the first bullet says "promotion" the second says "demotion". Added bolding to emphasize the difference between the two bullets.

[stereotype441]

SGTM

[stereotype441]

Probably should be and the following rules, not and the above rules

[leafpetersen]

done.

[stereotype441]

I think this has to be notNull(N) = join(split(true(N)), split(false(N)))

[leafpetersen]

Ah, right. I think maybe I need a couple of additional combinators.

[stereotype441]

Since statements don't have values, we shouldn't need true(N), false(N), null(N), or notNull(N). Just after(N) is sufficient.

[leafpetersen]

done.

[leafpetersen]

Thanks for the comments! I've incorporated feedback so far into the main document. Sounds like we're roughly on the same page. I'll leave this open for a bit longer in case anyone else has feedback.

[leafpetersen]

done.

[leafpetersen]

done.

[leafpetersen]

done

[leafpetersen]

Added definition of terms. The problem with using a::b is that there's not really a concise notation for top and pop.

[leafpetersen]

No, the first bullet says "promotion" the second says "demotion". Added bolding to emphasize the difference between the two bullets.

[leafpetersen]

Yes, we should handle this. I think your latter definition means that the following would be valid:

Object x;
if (x is int?) {
  x = 3;  // promote to int, since int? is of interest
  x.isEven;
  x = null; // demote to int?, since int? is of interest
}

Note sure whether that's a good thing or a bad thing? I'll think about it.

[leafpetersen]

I changed the demotion criteria to demote to the first supertype of the assigned type, which I think addresses your first example. Still thinking about your second.

[leafpetersen]

Ah, right. I think maybe I need a couple of additional combinators.

[leafpetersen]

done.

[leafpetersen]

done.

[stereotype441]

I'm starting to implement the logic for writeCaptured in the prototype, and I wanted to let you know about two corner cases I've run into. First, what if a write happens in one closure and a read happens in another:

f(Object x) {
  void Function() h;
  var g = () {
    if (x is int) { // (1)
      h();
      x.isEven; // (2)
    }
  };
  h = () { x = "hello"; }; // (3)
  g();
}

Even though the if-test at (1) appears lexically before the closure that captures a write to x is created (at (3)), since (1) is itself inside a closure, there's no guarantee that it will execute prior to the write being captured. So promotion is unsafe in this case, and (2) should be a compile-time error. I'm handling this situation by saying that prior to analysis, we gather a list of all variables that are written to in all closures. Then, when entering any closure, analysis sets writeCaptured to true for any variables in that list.

Second, what if a write capturing closure happens in a loop, after a promotion attempt?

f(Object x) {
  void Function()? g;
  while (true) {
    if (x is int) { // (1)
      if (g != null) g();
      x.isEven; // (2)
    }
    g = () { x = 'hello'; };
  }
}

In this case the promotion at (1) is unsafe, because the second time through the loop, there is a write captured. So there should be a compile-time error at (2). I'm handling this situation by saying that when entering a loop, we set writeCaptured to true for any variables that are captured ahywhere within the loop.