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

[breaking change][dart2js] Stop minifying Invocation.memberName in noSuchMethod #54201

Closed
fishythefish opened this issue Nov 30, 2023 · 7 comments
Assignees
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. breaking-change-approved breaking-change-request This tracks requests for feedback on breaking changes web-dart2js

Comments

@fishythefish
Copy link
Member

Change Intent

class Foo {
  void noSuchMethod(Invocation i) => print(i.memberName);
}

void main() {
  dynamic foo = Foo();
  foo.bar;
}

Ordinarily, this program will print Symbol("bar") with dart2js, as expected. When compiled with --minify, however, it will print, e.g. Symbol("gB") (or whatever minified name is chosen).

This change will stop --minify from affecting the result.

Justification

The language spec mandates the behavior of the Invocation that is generated by noSuchMethod forwarders and passed to noSuchMethod. In particular, it specifies that memberName is the name of the noSuchMethod-forwarded method. (Note that this is the name as it appears in the source code, not the compiled output.)

Making this change improves our spec conformance and has the added benefit of causing several nSM-reliant SDK tests to start passing on minified dart2js configurations.

Impact

There's unlikely to be any impact to the semantics of programs. In order to observe the change in behavior, one would need to rely on the value of Invocation.memberName in a noSuchMethod when building with --minify.

Note that the minified name would have changed frequently already, and there's not much you can do with the resulting symbol in Dart.

@sigmundch points out that some users may be relying on --minify to obscure method names which may leak internal details. IMO, it's poor security to depend on minification for this, but in any case, only noSuchMethod-forwarded names can leak, so if developers are avoiding nSM, there's no issue.

Mitigation

Unless we add a flag for this (which seems like overkill), I don't think there is actually a way to mitigate impact if it occurs.

Change Timeline

ASAP

Associated CLs

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

@fishythefish fishythefish added the breaking-change-request This tracks requests for feedback on breaking changes label Nov 30, 2023
@kevmoo
Copy link
Member

kevmoo commented Nov 30, 2023

LGTM!

@itsjustkevin
Copy link
Contributor

@vsmenon @grouma @Hixie for a breaking change request.

@Hixie
Copy link
Contributor

Hixie commented Dec 3, 2023

fine by me

@vsmenon
Copy link
Member

vsmenon commented Dec 3, 2023

Is there any notable code size impact?

@fishythefish
Copy link
Member Author

@vsmenon: In general, each nSM forwarder will have a single minified name replaced with the original identifier. In practice, this is zero to very little code size increase because (1) that's not very much in proportion to the program and (2) people tend to avoid using nSM. Apps we looked at were either unaffected or had insignificant size increases. The Flute benchmark actually decreased in size due to an interaction with how we minify .call selectors.

@vsmenon
Copy link
Member

vsmenon commented Dec 5, 2023

thanks - lgtm

@lrhn lrhn added web-dart2js area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. labels Dec 5, 2023
@grouma
Copy link
Member

grouma commented Dec 5, 2023

Given the minimal impact to code size, LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. breaking-change-approved breaking-change-request This tracks requests for feedback on breaking changes web-dart2js
Projects
None yet
Development

No branches or pull requests

7 participants