Don't generate forwarding hooks if all noSuchMethod implementations

Don't generate forwarding hooks if all noSuchMethod implementations
forward to Object.noSuchMethod

BUG=
R=johnniwinther@google.com

Committed: https://code.google.com/p/dart/source/detail?r=44575

[Harry Terkelsen, 2015-03-11 16:21:34]

het@google.com changed reviewers:
+ sra@google.com

[Harry Terkelsen, 2015-03-13 07:52:27]

het@google.com changed reviewers:
+ johnniwinther@google.com

[floitsch, 2015-03-13 15:51:48]

floitsch@google.com changed reviewers:
+ floitsch@google.com

[floitsch, 2015-03-13 15:51:49]

DBC (haven't actually looked at thi analysis).

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/com...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/com...
pkg/compiler/lib/src/compiler.dart:371: compiler.enabledNoSuchMethod = true;
I would have expected this to be in `enableNoSuchMethod`.

What's the semantics of "Compiler.enabledNoSuchMethod" ?
Could we keep it in the backend?

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/com...
pkg/compiler/lib/src/compiler.dart:378: void enableNoSuchMethod(Enqueuer
enqueuer) {
Can we remove this method? (or make it private)

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/no_such_method_analysis.dart (right):

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/no_such_method_analysis.dart:32: class
NoSuchMethodAnalysis {
I wouldn't call this "Analysis".
In the end it's a registry (or similar) that can answer some questions. It needs
to analyze the given nsms, but that's implicit.

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/no_such_method_analysis.dart:59:
backend.enableNoSuchMethod(enqueuer);
I would prefer if the "analysis" didn't have any side-effect.

[Johnni Winther, 2015-03-16 14:20:43]

lgtm

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/com...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/com...
pkg/compiler/lib/src/compiler.dart:367: void registerNoSuchMethod(Element
noSuchMethodElement, Enqueuer enqueuer) {
Leave the default implementation empty. Only the JS backend cares about this.

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/com...
pkg/compiler/lib/src/compiler.dart:371: compiler.enabledNoSuchMethod = true;
Add a TODO on Compiler.enabledNoSuchMethod to move it to the JS backend.

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/enq...
File pkg/compiler/lib/src/enqueue.dart (right):

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/enq...
pkg/compiler/lib/src/enqueue.dart:165:
compiler.backend.registerNoSuchMethod(function, this);
Move this to the ResolutionEnqueuer; only resolution should analyze, while
codegen should take consequences, i.e. enqueue when needed.

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/no_such_method_analysis.dart (right):

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/no_such_method_analysis.dart:57: if
(!compiler.enabledNoSuchMethod &&
Move this to the `CodegenEnqueuer.registerNoSuchMethod` where it should notify
the backend that a noSuchMethod of category A, B, or C has been enqueued.

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/no_such_method_analysis.dart:95:
_categorizeImpl(superCall);
Maybe [_categorizeImpl] should return an enum result which we can check
directly.

[Harry Terkelsen, 2015-03-17 21:43:11]

Thanks Florian and Johnni, PTAL

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/com...
File pkg/compiler/lib/src/compiler.dart (right):

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/com...
pkg/compiler/lib/src/compiler.dart:367: void registerNoSuchMethod(Element
noSuchMethodElement, Enqueuer enqueuer) {
On 2015/03/16 14:20:43, Johnni Winther wrote:
> Leave the default implementation empty. Only the JS backend cares about this.

Done.

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/com...
pkg/compiler/lib/src/compiler.dart:371: compiler.enabledNoSuchMethod = true;
On 2015/03/13 15:51:49, floitsch wrote:
> I would have expected this to be in `enableNoSuchMethod`.
> 
> What's the semantics of "Compiler.enabledNoSuchMethod" ?
> Could we keep it in the backend?

It is just a flag that says if noSuchMethod support is enabled. It is only used
by the JS backend. I moved the flag to the backend

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/com...
pkg/compiler/lib/src/compiler.dart:371: compiler.enabledNoSuchMethod = true;
On 2015/03/16 14:20:43, Johnni Winther wrote:
> Add a TODO on Compiler.enabledNoSuchMethod to move it to the JS backend.

I just moved it

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/com...
pkg/compiler/lib/src/compiler.dart:378: void enableNoSuchMethod(Enqueuer
enqueuer) {
On 2015/03/13 15:51:49, floitsch wrote:
> Can we remove this method? (or make it private)

Done.

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/enq...
File pkg/compiler/lib/src/enqueue.dart (right):

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/enq...
pkg/compiler/lib/src/enqueue.dart:165:
compiler.backend.registerNoSuchMethod(function, this);
On 2015/03/16 14:20:43, Johnni Winther wrote:
> Move this to the ResolutionEnqueuer; only resolution should analyze, while
> codegen should take consequences, i.e. enqueue when needed.

I am calling Enqueuer.registerNoSuchMethod here because to me it looks like both
the ResolutionEnqueuer and CodegenEnqueuer need to enqueue the invocation mirror
code if there is a non-trivial nsm impl. In the case of ResolutionEnqueuer, it
will store all of the nsm impls and classify them onQueueEmpty and then set
enabledNoSuchMethod. In the case of the CodegenEnqueuer, it will enqueue the
invocation mirror code if the backend has enabledNoSuchMethod

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/no_such_method_analysis.dart (right):

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/no_such_method_analysis.dart:32: class
NoSuchMethodAnalysis {
On 2015/03/13 15:51:49, floitsch wrote:
> I wouldn't call this "Analysis".
> In the end it's a registry (or similar) that can answer some questions. It
needs
> to analyze the given nsms, but that's implicit.

Done.

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/no_such_method_analysis.dart:57: if
(!compiler.enabledNoSuchMethod &&
On 2015/03/16 14:20:43, Johnni Winther wrote:
> Move this to the `CodegenEnqueuer.registerNoSuchMethod` where it should notify
> the backend that a noSuchMethod of category A, B, or C has been enqueued.

See earlier comment about how the enqueuers and backend relate wrt noSuchMethod.
Please let me know if you think this is a good approach

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/no_such_method_analysis.dart:59:
backend.enableNoSuchMethod(enqueuer);
On 2015/03/13 15:51:49, floitsch wrote:
> I would prefer if the "analysis" didn't have any side-effect.

Done.

[Harry Terkelsen, 2015-03-17 21:55:17]

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/no_such_method_analysis.dart (right):

https://codereview.chromium.org/996263002/diff/20001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/no_such_method_analysis.dart:95:
_categorizeImpl(superCall);
On 2015/03/16 14:20:43, Johnni Winther wrote:
> Maybe [_categorizeImpl] should return an enum result which we can check
> directly.

Done.

[Johnni Winther, 2015-03-18 10:47:16]

Still LGTM

https://codereview.chromium.org/996263002/diff/60001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/996263002/diff/60001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/backend.dart:2404:
(noSuchMethodRegistry.throwingImpls.isNotEmpty ||
Add getters for these, like [hasThrowingNoSuchMethod] and
[hasComplexNoSuchMethod].

https://codereview.chromium.org/996263002/diff/60001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/no_such_method_registry.dart (right):

https://codereview.chromium.org/996263002/diff/60001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/no_such_method_registry.dart:100: var category =
_categorizeImpl(superCall);
Nit: Replace 'var' with 'NsmCategory'. We only use 'var' if we cannot type the
declaration (like for unions of type).

https://codereview.chromium.org/996263002/diff/60001/tests/compiler/dart2js/n...
File tests/compiler/dart2js/no_such_method_enabled_test.dart (right):

https://codereview.chromium.org/996263002/diff/60001/tests/compiler/dart2js/n...
tests/compiler/dart2js/no_such_method_enabled_test.dart:101:
Expect.isTrue(compiler.backend.enabledNoSuchMethod)));
Maybe test that [B.noSuchMethod] is in the [throwingImpls] set and not in the
[otherImpls] set.

https://codereview.chromium.org/996263002/diff/60001/tests/compiler/dart2js/n...
tests/compiler/dart2js/no_such_method_enabled_test.dart:117:
Expect.isTrue(compiler.backend.enabledNoSuchMethod)));
Maybe test that [A.noSuchMethod] is in the [otherImpls] set.

[Harry Terkelsen, 2015-03-18 20:54:50]

I changed this so that if you have the code

class A extends B { noSuchMethod(x) => super.noSuchMethod(x); }
class B { noSuchMethod(x) => throw 'foo'; }

then both A.noSuchMethod and B.noSuchMethod are in category B. Before it would
classify A.noSuchMethod in category C.

https://codereview.chromium.org/996263002/diff/60001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/backend.dart (right):

https://codereview.chromium.org/996263002/diff/60001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/backend.dart:2404:
(noSuchMethodRegistry.throwingImpls.isNotEmpty ||
On 2015/03/18 10:47:16, Johnni Winther wrote:
> Add getters for these, like [hasThrowingNoSuchMethod] and
> [hasComplexNoSuchMethod].

Done.

https://codereview.chromium.org/996263002/diff/60001/pkg/compiler/lib/src/js_...
File pkg/compiler/lib/src/js_backend/no_such_method_registry.dart (right):

https://codereview.chromium.org/996263002/diff/60001/pkg/compiler/lib/src/js_...
pkg/compiler/lib/src/js_backend/no_such_method_registry.dart:100: var category =
_categorizeImpl(superCall);
On 2015/03/18 10:47:16, Johnni Winther wrote:
> Nit: Replace 'var' with 'NsmCategory'. We only use 'var' if we cannot type the
> declaration (like for unions of type).

Done.

https://codereview.chromium.org/996263002/diff/60001/tests/compiler/dart2js/n...
File tests/compiler/dart2js/no_such_method_enabled_test.dart (right):

https://codereview.chromium.org/996263002/diff/60001/tests/compiler/dart2js/n...
tests/compiler/dart2js/no_such_method_enabled_test.dart:101:
Expect.isTrue(compiler.backend.enabledNoSuchMethod)));
On 2015/03/18 10:47:16, Johnni Winther wrote:
> Maybe test that [B.noSuchMethod] is in the [throwingImpls] set and not in the
> [otherImpls] set.

Done.

https://codereview.chromium.org/996263002/diff/60001/tests/compiler/dart2js/n...
tests/compiler/dart2js/no_such_method_enabled_test.dart:117:
Expect.isTrue(compiler.backend.enabledNoSuchMethod)));
On 2015/03/18 10:47:16, Johnni Winther wrote:
> Maybe test that [A.noSuchMethod] is in the [otherImpls] set.

Done.

[Johnni Winther, 2015-03-19 10:21:26]

LGTM

[Harry Terkelsen, 2015-03-19 16:02:17]

Committed patchset #6 (id:100001) manually as 44575 (presubmit successful).