Flow analysis doesn't promote on implicit type cast

[fishythefish]

Flow analysis specifies that an explicit as will promote the LHS to the RHS type. However, it does not similarly specify promotion due to an implicit cast. Such situations are rare because most downcasts must be explicit, but there are benefits to promoting implicit downcasts from dynamic.

Can we specify promotion on implicit casts as well?

@leafpetersen @stereotype441
/cc @johnniwinther

---

Some context: dart2js has had its own static type computation since the pre-CFE era. This was historically more precise than the CFE, but the gap narrowed considerably when null-safety/flow analysis was added. We'd like to switch to simply using the CFE's static types (mainly for VariableGets) so that we don't have to also implement support for new language features, but we've encountered some slight code size/quality regressions, most of which seem to be due to this issue. They're small and uncommon enough to be non-blocking, but it would be great if we could avoid them.

Consider the following code:

class Foo {
  int get x => 42;
}

class Bar {
  int get x => -1;
}

@pragma('dart2js:noInline')
void unchecked(foo) => print(foo);

@pragma('dart2js:noInline')
void checked(Foo foo) => print(foo);

@pragma('dart2js:noInline')
void process(foo, bool isFoo) {
  if (isFoo) {
    unchecked(foo);
    print(foo.x); // #1
    checked(foo); // #2
    print(foo.x); // #3
  }
}

void main() {
  process(Foo(), true);
  process(Bar(), false);
}

Both dart2js and the CFE agree that the type of foo at #1 is dynamic, so we compile the getter invocation to foo.get$x(). At #2, there's an implicit parameter type check. dart2js uses this to promote the static type of foo to Foo, but the CFE doesn't. This means that when we use dart2js' static types, the invocation at #3 is monomorphic and we can just inline 42. When we switch to the CFE's static types, we still emit foo.get$x(). We could also trigger implicit checks with assignments, with the same result.

If we delete #1, the regression is even worse - we would previously have emitted a definition of .get$x() only on Foo, but we now have to emit one for Bar as well. In actual apps, we observe such definitions being added to a large number of protobuf classes unnecessarily.

Users can work around this by adding explicit casts (and it should probably be unsurprising that this is better for optimization than relying on dynamic), but just one bad caller can cause us to emit extra code for the entire app.

[fishythefish]

The consensus in #1362 is that we shouldn't do this because it would make code more surprising, so maybe it isn't feasible to specify this at a language level. (@munificent also mentioned that we could start requiring explicit downcasts on dynamic as well. I agree we should do that and it would fix this problem, but we can't just flip a switch for that immediately.)

Would it be reasonable for the CFE to expose a per-target flag to enable promotion on implicit casts?

[eernstg]

Interestingly, #2846 was created because downcasts from dynamic as part of pattern matching did give rise to promotions (and still does in Dart SDK 3.0.0-322.0.dev, just checked).

[eernstg]

I'm recommending in #2846 that we should adopt the non-promoting semantics: This is the near-consensus in #1362, so it's a surprising change that pattern declarations would give rise to promotions in essentially identical code:

void main() {
  dynamic pi = 3.14;
  // final (double v1) = pi; // If we include this line: Promote `pi`.
  // final double v1 = pi; // If we include this line: Do not promote `pi`.
  int v2 = pi; // Error iff `pi` was promoted.
}

[fishythefish]

Bumping in case it got missed earlier:

Would it be reasonable for the CFE to expose a per-target flag to enable promotion on implicit casts? We could e.g. enable it in dart2js for better codegen but leave it disabled in DDC for easier debugging, and there wouldn't be any soundness issues, right?

[eernstg]

I don't think we have the freedom to enable promotion on implicit casts in some configurations and not in others: We would immediately have cases where there is a compile-time error in one configuration, and no such error in another configuration (as in the example here).

In other situations we would have different semantics: An invocation of a member that happens to exist in the run-time type could succeed when the receiver has type dynamic, but when the static type is not dynamic the same call site could give rise to an extension method invocation:

class A {}

class B implements A {
  void foo() => print('B.foo');
}

extension E on A {
  void foo() => print('E.foo');
}

void main() {
  dynamic d = B();
  A a = d; // Promote `d`?
  d.foo(); // Invokes `B.foo` if `d` is `dynamic`, `E.foo` if `A`.
}

[fishythefish]

FWIW, we already have code that causes errors on one configuration but not another, both for compile-time and runtime errors. The potential change in extension method resolution is a good point, though.

Since this isn't a blocker for dart2js (we've already landed the change, actually), I'll close this issue in favor of #1362.