toString() ignores optional parameters

[Dana-Ferguson]

• 2.0.0-dev.68.0
• linux_x64
• DDC on Chrome

class Foo extends BaseFoo {
  // Good!
  static staticConstructor() => new Foo();
  Foo.redirectingConstructor() : this();
  Foo();

  // Bad! `x` will always be null for toString([String x]);
  // Behavior is always correct in VM\Flutter and Dart2JS
  factory Foo.factoryConstructor() => new Foo();

  @override String toString([String x]) => 'str: $x';

  // these always work
  String toStringFoo([String x]) => 'foo: $x';
  @override String tooFoo([String x]) => 'coo: $x';
}

// Test to see if it applies to any override
class BaseFoo {
  String tooFoo() => 'base!';
}

Expectation: This code will not throw.

  var foo = test.Foo.factoryConstructor();
  assert(foo.toString('foo') == 'str: foo');

Reality: This code will throw.

Any ideas?

I suppose for Dart 2, all factory constructors could be replaced with static constructors, but I don't like that.

[matanlurey]

I'm guessing this is an invalid optimization for .toString() in DDC, of some sorts.

/cc @jmesserly

[jmesserly]

DDC uses a helper for toString() whenever the target can be null. That's why the factory behaves differently (factories can return null).

For toString with arguments, we can dispatch directly because null.toString('foo') should throw nSM. Same rule should apply to calling obj.noSuchMethod(...) with any number of arguments other than 1.

[matanlurey]

Thanks for jumping on this! Just curious, why does the static method work in this case?

(Curious how the instriscs for factory methods are different here)

[jmesserly]

Thanks for jumping on this! Just curious, why does the static method work in this case?
(Curious how the instriscs for factory methods are different here)

the static method case did not work, I presume that is an error in the repro. (I know the feeling, it's easy to lose track of which test case you ran when you're running lots of tests to reproduce compiler bugs. :) )

edit: to be clear, there is no difference between static methods or factory constructors, for the purpose of this bug. They both are treated as nullable expressions.

[jmesserly]

https://dart-review.googlesource.com/c/sdk/+/65282

[Dana-Ferguson]

Hmm... that's weird. My machine differs. So... I looked into it, and found something new.

This code gives the below result.

class Foo {
  static staticConstructor1() => new Foo();
  static Foo staticConstructor2() => new Foo();
  Foo.redirectingConstructor() : this();
  factory Foo.factoryConstructor() => new Foo();

  Foo();

  @override String toString([String x]) => '$x';
}

void main() {
  var text = 'not null';
  print('default: ' + Foo().toString(text));
  print('redirectingConstructor: ' + Foo.redirectingConstructor().toString(text));
  print('staticConstructor1: ' + Foo.staticConstructor1().toString(text));
  print('staticConstructor2: ' + Foo.staticConstructor2().toString(text));
  print('factoryConstructor: ' + Foo.factoryConstructor().toString(text));
}

default: not null
redirectingConstructor: not null
staticConstructor1: not null
staticConstructor2: null
factoryConstructor: null

There is a change in code behavior that depends on whether the type annotation is given for the static constructor or not.

[jmesserly]

There is a change in code behavior that depends on whether the type annotation is given for the static constructor or not.

I will double check that my fix addresses it, but yes that makes sense. Dynamic dispatch is handled via a different code path that avoids the bug.

[jmesserly]

I confirmed that all of those are fixed in https://dart-review.googlesource.com/c/sdk/+/65282

[Dana-Ferguson]

Thank you for being awesome.

[jmesserly]

happy to help! :)

mind if I leave this bug open? My change above hasn't been checked in yet, waiting for a code review from a team member. I think github will automatically close it once my change is merged.

[jmesserly]

p.s. thanks for reporting this! that was a sneaky bug, none of our tests caught it.

[Dana-Ferguson]

Cool, yeap. Sorry about that. I never know what to do at the end of an issue.

[matanlurey]

@jmesserly I'm guessing this did not make it into 2.0, right?

[jmesserly]

Probably not, IIRC, we were only merging critical fixes at that point into the 2.0 branch.