Invalid optional parameter passing with factory constructor tearoffs

[jellynoone]

Dart SDK version: 2.16.1 (stable) (Tue Feb 8 12:02:33 2022 +0100) on "macos_x64"
and
Dart SDK version: 2.17.0-182.0.dev (dev) (Sun Mar 6 19:03:46 2022 -0800) on "macos_x64"

Consider the following example:

abstract class A {
  int get value;
  factory A({int value}) = _AImpl;
}

class _AImpl implements A {
  final int value;
  _AImpl({this.value = 0});
}

const _new = A.new;
void main(List<String> args) {
  print(A().value);
  print(A.new().value);
  print(_new().value);
  print((A.new)().value);
}

the expected output would be:

0
0
0
0

but the actual result is:

0
0
null
null

similarly if we replace the optional positional parameters with named ones the same invalid behavior can be observed.

abstract class A {
  int get value;
  factory A({int value}) = _AImpl;
}

class _AImpl implements A {
  final int value;
  _AImpl({this.value = 0});
}

const _new = A.new;
void main(List<String> args) {
  print(A().value);
  print(A.new().value);
  print(_new().value);
  print((A.new)().value);
}

[jellynoone]

Using the _AImpl directly yields the expected results:

class _AImpl {
  final int value;
  _AImpl({this.value = 0});
}

const _new = _AImpl.new;
void main(List<String> args) {
  print(_AImpl().value);
  print(_AImpl.new().value);
  print(_new().value);
  print((_AImpl.new)().value);
}

[mraleph]

/cc @alexmarkov

[alexmarkov]

Currently VM relies on desugaring of redirecting factories (but not their tear-offs). It looks like we're not copying default values of optional parameters properly.

Kernel for this example:

  abstract class A extends core::Object {
    static final field dynamic _redirecting# = <dynamic>[#C1]/*isLegacy*/;
    abstract get value() → core::int;
    static factory •({core::int value = #C2}) → tes::A
      return new tes::_AImpl::•(value: value);
  }
  class _AImpl extends core::Object implements tes::A {
    final field core::int value;
    constructor •({core::int value = #C3}) → tes::_AImpl
      : tes::_AImpl::value = value, super core::Object::•()
      ;
  }
  static const field ({value: core::int}) → tes::A _new = #C4;
  static method main(core::List<core::String> args) → void {
    core::print(new tes::_AImpl::•().{tes::A::value}{core::int});
    core::print(new tes::_AImpl::•().{tes::A::value}{core::int});
    core::print(#C4(){({value: core::int}) → tes::A}.{tes::A::value}{core::int});
    core::print(#C4(){({value: core::int}) → tes::A}.{tes::A::value}{core::int});
  }

...

  #C1 = constructor-tearoff tes::A::•
  #C2 = null
  #C3 = 0
  #C4 = redirecting-factory-tearoff tes::A::•

We should probably copy default values from the target to the redirecting factory when adding a synthetic body for the redirecting factory. But this copying should not interfere with error Can't have a default value here because any default values of '_AImpl' would be used instead. which is issued if default value is specified explicitly.

@johnniwinther Could you please help with that? I'm not sure where exactly we should do that copying.

[johnniwinther]

I'll take a look at it.