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.

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

dart2js: minified does not work with noSuchMethod. #9283

Closed
kevmoo opened this issue Mar 19, 2013 · 19 comments
Closed

dart2js: minified does not work with noSuchMethod. #9283

kevmoo opened this issue Mar 19, 2013 · 19 comments
Assignees
Labels
P1 A high priority bug; for example, a single project is unusable or has many test failures web-dart2js

Comments

@kevmoo
Copy link
Member

kevmoo commented Mar 19, 2013

I'll try to get a minimal repro, but for now, see the attached image.

This is running the latest commit to Pop-Pop-Win via Chrome (not Dartium)

dart-lang/sample-pop_pop_win@4589259

I'm pretty sure this relates to the call to Google Analytics.

Everything works great in Dartium and non-minimized dart2js.

Dart VM version: 0.4.2.5 r20193 (Tue Mar 19 04:23:27 2013)


Attachment:
[Screen Shot 2013-03-19 at 1.38.03 PM.png](https://storage.googleapis.com/google-code-attachments/dart/issue-9283/comment-0/Screen Shot 2013-03-19 at 1.38.03 PM.png) (99.98 KB)

@kevmoo
Copy link
Member Author

kevmoo commented Mar 19, 2013

Minimal repro: https://github.com/kevmoo/dart_sdk_9283

Last commit is with minify.
Previous commit is without minify. No issue.

This is a regression.


Attachment:
[Screen Shot 2013-03-19 at 1.54.13 PM.png](https://storage.googleapis.com/google-code-attachments/dart/issue-9283/comment-1/Screen Shot 2013-03-19 at 1.54.13 PM.png) (36.30 KB)

@sethladd
Copy link
Contributor

Added Triaged label.

@vsmenon
Copy link
Member

vsmenon commented Mar 19, 2013

I will investigate.


Set owner to @vsmenon.

@kevmoo
Copy link
Member Author

kevmoo commented Mar 19, 2013

Changed the title to: "dart2js: minified breaks JS-interop [REGRESSION]".

@vsmenon
Copy link
Member

vsmenon commented Mar 19, 2013

Dart SDK version 0.4.2.5_r20193

It's an NSM issue. If I run the code below (using the editor / wizard web app as a template), I should get:

"Called HelloWorld"

With dart2js --minify, I get:

"Called Df".

Kevin: are you saying it used to work with --minify?


import 'dart:html';

class A {
  noSuchMethod(InvocationMirror invocation) {
    String member = invocation.memberName;
    query("#sample_text_id")
    ..text = "Called $member";
  }
}

void main() {
  A a = new A();
  a.HelloWorld();
}


cc @kasperl.
cc @peter-ahe-google.
cc @rakudrama.
Removed the owner.
Changed the title to: "dart2js: minified does not work with noSuchMethod.".

@kevmoo
Copy link
Member Author

kevmoo commented Mar 19, 2013

I've used minify for a while with no issues. This seems a regression from one of the last integration builds.

@vsmenon
Copy link
Member

vsmenon commented Mar 19, 2013

Thanks, I also verified that my mini-test above works correctly on Dart SDK version 0.4.1.0_r19425.

@kasperl
Copy link

kasperl commented Mar 19, 2013

Does the code work with dart2dart minification? Did it ever? I think the answer to both questions is "no".

The new and much more compact way we deal with NSM is according to the specification and as far as I know it's perfectly valid to construct the InvocationMirror object using the minified names.

We have talked about a way of opt'ing in to carrying the mapping from minified to unminified names around. In a way, this issue is really a feature request for that.


cc @ErikCorryGoogle.
cc @gbracha.

@vsmenon
Copy link
Member

vsmenon commented Mar 19, 2013

Yes, it did indeed stop working on this CL (r20005):

    dart2js: Create noSuchMethod handlers at runtime to reduce overhead.
    
    This reduces the overhead for using noSuchMethod from over 10% to around 2%.
    Following this change the original source names are not present when
    minifying. This means you get the minified name in the methodName property
    on InvocationMirror objects.
    
    R=ngeoffray@google.com
    BUG=
    
    Review URL: https://codereview.chromium.org//12499005

@gbracha
Copy link
Contributor

gbracha commented Mar 19, 2013

FWIW, the spec does say that calling memberName on the InvocationMirror gives a string that matches the true name of the method (with an = at the end if it is a setter). Minification breaks that. You can argue that the user asked for a source transformation that has this effect, but this is highly debatable. What if the user tests to see if the names match a certain pattern (like, starting with _ to see if it was a private method).

At the very least, there needs to be an opt-out mechanism, or an API that lets you convert source names to minified names.

@vsmenon
Copy link
Member

vsmenon commented Mar 19, 2013

Gilad, can you please clarify the correct behavior?

I see no mention in the spec of minification. It does say:

"The result of looking up a method m in object o with respect to library L is.... a new instance im of the predefined class InvocationMirror is created, such that ... im.memberName evaluates to ‘m’."

which suggests to me that im.memberName should have the original method name.

@vsmenon
Copy link
Member

vsmenon commented Mar 19, 2013

Gilad, sorry for the crossed streams, and thanks for answering my question before I asked it. :-)

Kasper, how should we proceed on this?

@vsmenon
Copy link
Member

vsmenon commented Mar 19, 2013

Kevin,

If you get a chance, can you try replacing:

  js.context.pushAnalytics(...);

with this:

  js.context'pushAnalytics';

in your code? The latter should not trigger NSM.

@kevmoo
Copy link
Member Author

kevmoo commented Mar 19, 2013

No dice.

See last few commits here for changes: https://github.com/dart-lang/pop-pop-win/commits/master


Attachment:
[Screen Shot 2013-03-19 at 5.03.20 PM.png](https://storage.googleapis.com/google-code-attachments/dart/issue-9283/comment-16/Screen Shot 2013-03-19 at 5.03.20 PM.png) (102.13 KB)

@ErikCorryGoogle
Copy link
Contributor

A workaround is illustrated in https://code.google.com/p/dart/codesearch#dart/trunk/dart/tests/language/no_such_method_test.dart

If there are a few names you want to get the real name from you can use the getName technique from this test to get them.

@kevmoo
Copy link
Member Author

kevmoo commented Mar 20, 2013

Adding link to js-interop bug: https://github.com/dart-lang/js-interop/issues/57

@kasperl
Copy link

kasperl commented Apr 3, 2013

We haven't forgotten about this issue and we're trying to come up with a design that works well for both dart2dart and dart2js minified code.

@kasperl
Copy link

kasperl commented Apr 22, 2013

Added this to the Later milestone.

@kasperl
Copy link

kasperl commented May 23, 2013

Added TriageForM5 label.

@kevmoo kevmoo added Type-Defect P1 A high priority bug; for example, a single project is unusable or has many test failures web-dart2js labels May 23, 2013
@kevmoo kevmoo added this to the Later milestone May 23, 2013
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 A high priority bug; for example, a single project is unusable or has many test failures web-dart2js
Projects
None yet
Development

No branches or pull requests

7 participants