Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

nSM Forwarding from Mixin between VM and dart2js disagree - VM loses default values #33459

Closed
srawlins opened this issue Jun 14, 2018 · 5 comments
Closed

nSM Forwarding from Mixin between VM and dart2js disagree - VM loses default values #33459

srawlins opened this issue Jun 14, 2018 · 5 comments
Assignees
@chloestefantsova
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
2.0 language
Milestone
Dart2Stable

Comments

@srawlins
Copy link
Member

Not sure what tool is in the wrong here. Here's a DartPad - dart2js is now running with --preview-dart-2.

Here's the text:

void main() {
  var a = new AImpl();
  var b = new B(a);
  print("CALLING W/O NAMED ARG");
  b.a();
  print("CALLING W/ NAMED ARG");
  b.a(followLinks: false);
}

abstract class A {
  a({followLinks: true});
}

class AImpl implements A {
  a({followLinks: true}) {
    print('AImpl: followLinks is $followLinks');
    if (followLinks) print('yay');
    if (!followLinks) print('nay');
  }
}

class BestMixinEvar {
  final methods = <Symbol, Function>{};

  noSuchMethod(Invocation i) {
    var na = i.namedArguments;
    print('BestMixinEvar.noSuchMethod: namedargs: $na');
    if (i.memberName == #a) return Function.apply(methods[#a], [], na);
  }
}

class B extends A with BestMixinEvar {
  A delegate;

  B(this.delegate) {
    methods.addAll(<Symbol, Function>{
      #a: delegate.a,
    });
  }
}

In the DartVM this blows up with --preview-dart-2 in Dart dev.61:

$ dart --preview-dart-2 a.dart 
CALLING W/O NAMED ARG
BestMixinEvar.noSuchMethod: namedargs: {Symbol("followLinks"): null}
AImpl: followLinks is null
Unhandled exception:
Failed assertion: boolean expression must not be null
#0      AImpl.a (file:///Users/srawlins/code/dart-file/packages/file/a.dart)
#1      Function._apply (dart:core/runtime/libfunction_patch.dart:10:68)
#2      Function.apply (dart:core/runtime/libfunction_patch.dart:31:12)
#3      _B&A&BestMixinEvar.noSuchMethod (file:///Users/srawlins/code/dart-file/packages/file/a.dart:28:45)
#4      _B&A&BestMixinEvar.a (file:///Users/srawlins/code/dart-file/packages/file/a.dart:11:3)
#5      main (file:///Users/srawlins/code/dart-file/packages/file/a.dart:5:5)
#6      _startIsolate.<anonymous closure> (dart:isolate/runtime/libisolate_patch.dart:279:19)
#7      _RawReceivePortImpl._handleMessage (dart:isolate/runtime/libisolate_patch.dart:165:12)

But in dart2js its OK:

CALLING W/O NAMED ARG
BestMixinEvar.noSuchMethod: namedargs: {}
Warning: 'a' is used reflectively but not in MirrorsUsed. This will break minified code.
AImpl: followLinks is true
yay
CALLING W/ NAMED ARG
BestMixinEvar.noSuchMethod: namedargs: {Symbol("followLinks"): false}
Warning: 'a' is used reflectively but not in MirrorsUsed. This will break minified code.
AImpl: followLinks is false
nay

This is causing google/file.dart#88.

This might be related to #32660 or #33380.
@srawlins
Copy link
Member Author

CC @jmesserly @keertip @lrhn @tvolkert

@srawlins srawlins changed the title nSM Forwarding between VM and dart2js disagree - VM loses default values nSM Forwarding from Mixin between VM and dart2js disagree - VM loses default values Jun 14, 2018
@srawlins srawlins mentioned this issue Jun 14, 2018
Closed
@jmesserly
Copy link

Yeah this looks like a CFE/Kernel issue. The default arg is getting dropped from the no such method forwarder:

  abstract class _B&A&BestMixinEvar = test::A with test::BestMixinEvar {
    no-such-method-forwarder method a({dynamic followLinks}) → dynamic
      return this.{test::BestMixinEvar::noSuchMethod}(new core::_Invocation::method(#a, null, <dynamic>[], <core::Symbol, dynamic>{#followLinks: followLinks})) as{TypeError} dynamic;
  }

Changing the example to use class B extends BestMixinEvar implements A instead of a mixin does not work either:

  class B extends test::BestMixinEvar implements test::A {
    // ...
    no-such-method-forwarder method a({dynamic followLinks}) → dynamic
      return this.{test::BestMixinEvar::noSuchMethod}(new core::_Invocation::method(#a, null, <dynamic>[], <core::Symbol, dynamic>{#followLinks: followLinks})) as{TypeError} dynamic;
  }

Adding an abstract method a({followLinks: true}); to B fixes it in CFE:

    no-such-method-forwarder method a({dynamic followLinks = true}) → dynamic
      return this.{test::BestMixinEvar::noSuchMethod}(new core::_Invocation::method(#a, null, <dynamic>[], <core::Symbol, dynamic>{#followLinks: followLinks})) as{TypeError} dynamic;

@jmesserly jmesserly added the area-front-end Use area-front-end for front end / CFE / kernel format related issues. label Jun 14, 2018
@srawlins
Copy link
Member Author

Thanks for digging in, @jmesserly ! The workaround might work for me for now.

@jmesserly jmesserly mentioned this issue Jun 14, 2018
Open
@leafpetersen leafpetersen added this to the Dart2Stable milestone Jun 14, 2018
@tvolkert tvolkert mentioned this issue Jun 15, 2018
Merged
@srawlins srawlins mentioned this issue Jun 15, 2018
Merged
@lrhn lrhn added the type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) label Jun 15, 2018
@JekCharlsonYu JekCharlsonYu added the P2 A bug or feature request we're likely to work on label Jun 15, 2018
@chloestefantsova
Copy link
Contributor

I'm cautiously optimistic about having a fix for the issue in CL 62808.

@chloestefantsova chloestefantsova added this to Done in 2.0 language via automation Jun 28, 2018
@chloestefantsova
Copy link
Contributor

Sorry, pressed the wrong button. I didn't mean to close the issue yet.

@chloestefantsova chloestefantsova moved this from Done to Implementation: VM/Kernel/FE in 2.0 language Jun 28, 2018
2.0 language automation moved this from Implementation: VM/Kernel/FE to Done Jul 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@chloestefantsova chloestefantsova

Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. P2 A bug or feature request we're likely to work on type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
No open projects
2.0 language
  
Done
Milestone
Dart2Stable
Development

No branches or pull requests
6 participants
@srawlins @chloestefantsova @jmesserly @lrhn @leafpetersen @JekCharlsonYu