improve type promotion in strong mode

[jmesserly]

[Edit Sep 17 2018, eernstg: This seems to be the most generic issue on the topic of improving promotion, so I'd like to position it explicitly as "the place to go" for this discussion; I've added a comment listing other issues related to this topic here. I'll adjust the labels accordingly.]

---

There are some cases where type promotion won't work. Found while investigating the state of dart-archive/dev_compiler#31.

It roughly falls into a few buckets:

• if the original type of T of v was dynamic, the test v is SomeType won't promote. This leads to some suboptimal codegen in SDK (see strong mode should allow type promotion from dynamic #25486)
• if the tested type S is not a subtype of T, the test won't promote. You'd have to track a union type.
• the type promotion is only valid in the body/else clause of that if statement. There's no analysis of the rest of the method, meaning patterns like if (v is! S) return; don't affect the type of v in the rest of the method body.
• from Sky, we've heard they would like to do assert(v is S); and have v be treated as S for the rest of the method. Sort of like an unchecked (in production mode) cast, similar to S v2 = v but not requiring a second variable name to be introduced.
• from package:yaml, assignment to the variable in a closure will disable all promotion, this is true even if that closure hasn't been defined yet (and therefore, cannot possibly mutate the variable).
• Constrained generic parameter like T extends Event, not promoting to a subtype of Event like InputEvent, see Stack trace from editor evaluating code in CompileTimeConst visitor #327
• If the variable is mutated, it won't promote. This is the case even if the mutation doesn't affect the type.
• If the type test wants to allow null, like if (v == null || v is T), it should promote to nullable-T

[jmesserly]

Moved from dart-archive/dev_compiler#274.

[nex3]

I'd like to propose that v == null promote V. I don't know the type theory well enough to say what— maybe dynamic?—but this would support the following pattern:

void fn(T v) {
  // ...
}

void fnTakesObject(Object v) {
  if (v != null && v is! T) return;
  fn(v);
}

This comes up when implementing collections, since they have a number of methods that take Objects but need more specialized behavior for values of the collection's type.

[jmesserly]

ah, good example. So right now that won't promote because of flow analysis (issue 3 above) but it'd be equivalent to this:

void fnTakesObject(Object v) {
  if (v == null || v is T) {
    fn(v);
  }
}

Doing || generally would take union types, but we could definitely special case null, since that works for any T.

[srawlins]

To add to the list (unless this is being split into smaller issues?):

• Promote in ternary: x is SomeType ? x.someField : 'something' should have an understanding that x has a someField field after the ?.

[jmesserly]

@srawlins ternary operators already work:

class C {
  var greeting = 'hello';
}
main() {
  Object c = new C();
  print(c is C ? c.greeting : 'world');
}

$ dartanalyzer --strong test.dart
Analyzing [test.dart]...
No issues found

What you are likely seeing is one of the other problems above. With more info I can help you figure it out.

[matanlurey]

Is this still happening? Will the analyzer and front_end both do the right thing?

[jmesserly]

Maybe @leafpetersen knows where the language is heading on this?

[leafpetersen]

@bwilkerson Had a very nice proposal that covered a fair bit of this. It didn't make the cut for Dart 2 just because of time, but I hope we can find a good way to roll it out post 2.

[natebosch]

if the tested type S is not a subtype of T, the test won't promote. You'd have to track a union type.

I think this is #33932

Some of the others are covered by #29624

[eernstg]

It looks like this is the canonical request for improved promotion, which is a topic that has been discussed for a long time (no proposal made it into Dart 2, but surely we'll get there at some point).

I think it would be nice to use this issue as a focal point for that proposal. So here's a list of issues on this topic which contains the issues that I know about (you're welcome to extend the list if you know about a relevant issue that I overlooked):

• Allow promotion using the raw type #17281: 'Allow promotion using the raw type'.
• Make type rules more flow sensitive. #18921: 'Make type rules more flow sensitive'.
• "is" operator inconsistent with type coercion for member variables #21530: '"is" operator inconsistent with type coercion for member variables'.
• "this is Foo ? this : null" => "Type check failed: ... is not of type Foo" #25260: '"this is Foo ? this : null" => "Type check failed: ... is not of type Foo"' (closed, but refers to some properties that may call for enhancements).
• type promotion of upper bound does not work inside closures #27725: 'type promotion of upper bound does not work inside closures'.
• Analyzer does not infer type of dynamic class fields after typecheck #27813: 'Analyzer does not infer type of dynamic class fields after typecheck'.
• analyzer doesn't narrow the type after an is-check when you're reassigning #28483: 'analyzer doesn't narrow the type after an is-check when you're reassigning' (closed, but raises the issue of allowing some assignments).
• analyzer doesn't trust the "is" operator enough #28586: 'analyzer doesn't trust the "is" operator enough' (closed, but essentially requests intersection types, which has been part of the promotion discussion for a long time).
• promote locals without an upcast first #33932: 'promote locals without an upcast first' (raises the same concern that usually gives rise to a request for intersection types ;-).
• Analyzer won't promote type of member variables #34480: 'Analyzer won't promote type of member variables' (closed, but touches on a topic that we discussed: promoting other entities than local variables).
• Type promotion could be even better if it analyzed more of the control flow #34483: 'Type promotion could be even better if it analyzed more of the control flow' (this one brings up a more subtle idea: an if that doesn't throw/return may provide a guarantee by assigning to the scrutinee variable).

Moreover, here's a PR that includes an overview of the ideas on the table at the given time:

• "Enhanced Type Promotion" informal proposal. #29624: "Enhanced Type Promotion" informal proposal.

[eernstg]

@jmesserly, I've added a comment at the top of the original text of this issue and adjusted the labels to make this issue "the place to go" for discussions on promotion. I believe it does not fit so well as an 'area-analyzer' issue because the language design needs to be settled first, so I made it an 'area-language' issue; similarly, I removed 'analyzer-strong-mode' because it's now a Dart 2 effort (or 2.x, for some finite x), and the priority (because that priority was presumably concerned with the analyzer implementation).

[jmesserly]

@eernstg -- that sounds perfect. Yeah feel free to edit my old description to whatever is most is helpful. It's great to have a canonical issue for type promotion work!

[eernstg]

Putting check marks on a couple of cases in the initial comment:

• if the original type of T of v was dynamic, the test v is SomeType won't promote. This leads to some suboptimal codegen in SDK (see strong mode should allow type promotion from dynamic #25486)

In current Dart, it is possible to promote a local variable whose type is dynamic.

• if the tested type S is not a subtype of T, the test won't promote. You'd have to track a union type.

Also true today.

• the type promotion is only valid in the body/else clause of that if statement. There's no analysis of the rest of the method, meaning patterns like if (v is! S) return; don't affect the type of v in the rest of the method body.

Today we have a rather complex flow analysis, and there are many ways in which the type of v can be affected by the promotion machinery outside the true-continuation of that test.

• from Sky, we've heard they would like to do assert(v is S); and have v be treated as S for the rest of the method. Sort of like an unchecked (in production mode) cast, similar to S v2 = v but not requiring a second variable name to be introduced.

We don't do that (because production code cannot be affected by assert statements since they aren't executed), but it can be done using v as S. Flow analysis will then know that this type cast must have succeeded at every location in the code which is reached after having executed the type cast (modulo try/cast and other complex stuff -- but it works! ;-).

• from package:yaml, assignment to the variable in a closure will disable all promotion, this is true even if that closure hasn't been defined yet (and therefore, cannot possibly mutate the variable).

Kept this one active, the flow analysis basically doesn't try to keep track of which locations in the code are such that the function object definitely hasn't been invoked. So we're still disabling promotion a bit more than we'd like.

• Constrained generic parameter like T extends Event, not promoting to a subtype of Event like InputEvent, see Stack trace from editor evaluating code in CompileTimeConst visitor #327

In current Dart we do this: There is a kind of type which is known as an intersection type (it's not a general intersection type, just a special case, and only a compile-time phenomenon). This kind of type supports a few special cases, presumably including the case about Event and InputEvent which is mentioned here. So I added the check mark here, too.

• If the variable is mutated, it won't promote. This is the case even if the mutation doesn't affect the type.

Today there are lots of mutations of local variables which are compatible with promotion. We also have a notion of 'demotion' in the case where a promoted variable v gets assigned the value of an expression whose static type isn't assignable to the promoted type of v, but it is assignable to the declared type of v.

• If the type test wants to allow null, like if (v == null || v is T), it should promote to nullable-T

I put a check mark here, too, because we can use if (v is T?) and get that promotion.

This seems to indicate that only the part about function objects with potential side effects on local variables isn't yet covered.