fasta: Don't emit invalid-statement for redirecting factories

[mkustermann]

Dartk translates

class Foo {
  factory Foo() = Bar;
}
class Bar implements Foo {
  Bar() {}
}
main() {}

to

  class Foo extends core::Object {
    constructor •() → dynamic
      invalid-statement;
  }
  class Bar extends core::Object implements test::Foo {
    constructor •() → void
      : super core::Object::•() {}
  }
  static method main() → dynamic {}

It should not emit invalid-statement.

/cc @kmillikin & #28263

[asgerf]

Redirecting factories are eliminated in the front-end.

Originally they were converted to a procedure that called the target, but the analyzer's element model does not expose default parameter values, and thus we cannot generate a forwarding procedure without loading the full source code of the target.

The constructor in Foo is inserted by the sanitize_for_vm pass to ensure all classes have at least one procedure or constructor, because otherwise the VM crashes.

[mkustermann]

Some tests pass --compile-all to the VM. Which will make the VM generate code for all classes/methods. It then crashes on InvalidStatement.

I think if the input to dartk is perfectly valid and sound code, the IR should not contain any invalud-* nodes.

[asgerf]

CL for fix https://codereview.chromium.org/2638303002/

[kmillikin]

The fix, to not use invalid statement, seems good. I'll just note here that the VM doesn't crash, it hits an assertion failure in debug builds, and that assertion seems to be just a sanity-check precondition and not a correctness precondition.

Running in a release build or without the assertion in a debug build does not seem to fail.

[mkustermann]

Maybe a better way to formulate this is: The VM does not expect to ever encounter an InvalidStatement because it doesn't know how to handle it. Could we get rid of this AST node?

[asgerf]

I completely agree - we should get rid of all invalid nodes.

[mraleph]

This test still crashes with fasta and --compile-all. We need executive/design decision here how Kernel should look like for redirecting factories that were erased by the front-end (especially in the context of the modular compilation).

Also see #28421

[peter-ahe-google]

Blocked on #29841

[mkustermann]

There is btw also a test, which invokes a redirecting factory via dart:mirrors:

class A {
  factory A(
    String //  //# 01: static type warning
      x) = B;
  A._();
}

class B extends A {
  var x;
  B(int x)
      : super._(),
        this.x = x;
}

main() {
  var cm = reflectClass(A);
  // The type-annotation in A's constructor must be ignored.
  var b = cm.newInstance(const Symbol(''), [499]).reflectee;
  Expect.equals(499, b.x);
  Expect.throws(
      () => cm.newInstance(const Symbol(''), ["str"]), (e) => e is TypeError);
}

from tests/lib/mirrors/redirecting_factory_different_type_test.dart

[alexmarkov]

Fixed in 300a2ea.