Flow analysis fails to record types of interest when an is expression is not used in a conditional context

[stereotype441]

Consider the following code (from dart-lang/co19#3126 (comment)):

class S {}

class T extends S {
  void foo() {}
}

void main() {
  var s = S();
  s is T;
  s = T();
  s.foo(); // Error, so `T` was not a type of interest, but it should be.
}

According to the flow analysis spec:

• true(s is T) should add the type T to the tested list in the VariableModel for s.
• Since there is no specific rule defining after(N) for is expressions, the general rule after(N) = join(true(N), false(N)) should be used (see the Expressions subsection).
• join is based on joinV, and the definition of joinV (see the Models subsection) says that joinV takes the union of the tested lists from the two branches.
• So after(s is T) should include T as one of the tested types for s. (In other words, T should be a type of interest for s after this is check).

What's happening instead is that instead of using the general rule after(N) = join(true(N), false(N)) for is expressions, flow analysis is failing to update its description of the _current flow state, so in effect what the implementation is doing is this (I've highlighted the difference from the spec in bold):

• instance check: If N is an expression of the form E1 is S where the static type of E1 is T then:
  • Let before(E1) = before(N)
  • If T is a bottom type, then:
    • Let true(N) = unreachable(after(E1)).
    • Let false(N) = after(E1).
  • Otherwise:
    • Let true(N) = promote(E1, S, after(E1))
    • Let false(N) = promote(E1, factor(T, S), after(E1))
    • Let after(N) = after(E1)

In practice this is pretty benign because most of the time people perform is tests, they do so in a conditional context, so the flow models that get propagated forward come from true(N) and false(N) (which are behaving correctly). The incorrect behavior only happens if the is test isn't used in a condition, so the flow model that gets propagated forward come from the buggy after(N).

Since the bug only manifests if an is test is used in an unusual way, and only causes problems with types of interest, I'm classifying this as a P3. I will still try to get to fixing it though.

[lrhn]

If the test result is not used in a condition, and not used to build a value that is stored in a variable, then it's effectively dead code. I don't care if it causes the type to be a type of interest, because the code shouldn't exist.
(Not unless we want this syntax to be a way to introduce a type of interest, and then we should make sure the analyzer sø doesn't warm about it ... but I'd prefer a warning.)

So it doesn't matter to me whether the fix is to make it introduce a type of interest, or change the spec to say it doesn't.

[eernstg]

Using v is T; to indicate that the type T is of interest for the variable v (whose type is not a bottom type) does make sense: This form is safe (as opposed to v as T;), it can be eliminated completely at run time (it is known to have no side effects), and it expresses interest as described using the normal rules (no need to introduce a special syntax for that purpose).

Subsequent code will then be able to achieve the promotion based on assignments, even in paths where there are no further type tests or checks on v for T. Makes sense to me.

[stereotype441]

If the test result is not used in a condition, and not used to build a value that is stored in a variable, then it's effectively dead code. I don't care if it causes the type to be a type of interest, because the code shouldn't exist. (Not unless we want this syntax to be a way to introduce a type of interest, and then we should make sure the analyzer sø doesn't warm about it ... but I'd prefer a warning.)

So it doesn't matter to me whether the fix is to make it introduce a type of interest, or change the spec to say it doesn't.

I have a modest preference to fix the bug by making it introduce a type of interest. Here's my reason: flow analysis allows the user to create a boolean local variable that records the result of a type test, and then use that variable later in a conditional context. For example:

var vIsT = v is T; // (1)
// other code
if (vIsT) { // Promotes `v` to `T`, provided that the values of `v` and `vIsT` haven't changed since (1)
}

The way the implementation currently works, T is added to the list of types of interest at the site where the vIsT variable is used. That seems really counterintuitive to me. If we change things so that v is T always introduces T as a type of interest for v , then the type of interest will be introduced at (1), which I think is more intuitive behavior.