Implement KernelNoSuchMethodResolver.

Implement KernelNoSuchMethodResolver.

R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/f2ae4db28077fc19a4334efe4c5988187a46e942

[Johnni Winther, 2017-05-03 14:00:08]

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

[Siggi Cherem (dart-lang), 2017-05-03 17:29:49]

lgtm

https://codereview.chromium.org/2857943002/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/no_such_method_registry.dart (right):

https://codereview.chromium.org/2857943002/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/no_such_method_registry.dart:211: class
NoSuchMethodResolverImpl implements NoSuchMethodResolver {
move under resolution/* ?

https://codereview.chromium.org/2857943002/diff/1/pkg/compiler/lib/src/kernel...
File pkg/compiler/lib/src/kernel/no_such_method_resolver.dart (right):

https://codereview.chromium.org/2857943002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/no_such_method_resolver.dart:18: ir.Procedure node =
elementMap._memberList[method.memberIndex].node;
Since we are preparing to move this file as a separate library and make some
public (but restricted APIs) to fetch kernel from the KElements, maybe we can
take the first step here and add a lookup for these (and hide all the .index
uses within the file that defines the environment.)

We can still keep it private (e.g. _lookupProcedure(method))

https://codereview.chromium.org/2857943002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/no_such_method_resolver.dart:26: } else if (body is
ir.Block && body.statements.isNotEmpty) {
ha - the old implementation checked that statements.length == 1 (so a silly case
like: 
  noSuchMethod(i) {
    return super.noSuchMethod(i);
    return foo;
  }

was not recognized before, but it will be recognized now.

https://codereview.chromium.org/2857943002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/no_such_method_resolver.dart:59: } else if (body is
ir.Block && body.statements.isNotEmpty) {
nit: move this out first, so we can reuse the other two conditions:

ir.Statement body = node.function.body;
if (body is ir.Block && body.statements.isNotEmpty) {
  body = body.statements.first;
}
if (body is ir.ReturnStatement) {
  expr = body.expression;
} else if (body is ir.ExpressionStatement) {
  expr = body.expression;
}
return expr is ir.Throw;

https://codereview.chromium.org/2857943002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/no_such_method_resolver.dart:84: break;
not sure what this case does - if you have a noSuchMethod that is a getter or
has the wrong signature, you'll return Object.noSuchMethod below, was that the
intent?

https://codereview.chromium.org/2857943002/diff/1/tests/compiler/dart2js/no_s...
File tests/compiler/dart2js/no_such_method_enabled_test.dart (right):

https://codereview.chromium.org/2857943002/diff/1/tests/compiler/dart2js/no_s...
tests/compiler/dart2js/no_such_method_enabled_test.dart:271:
NoSuchMethodResolver resolver = registry.resolver;
nit: rather than making the resolver public, I'd keep it private and add a
getter like `get internalResolverForTesting => _resolver`.

https://codereview.chromium.org/2857943002/diff/1/tests/compiler/dart2js/no_s...
tests/compiler/dart2js/no_such_method_enabled_test.dart:272: FunctionEntity
noSuchMethodObject = elementEnvironment.lookupClassMember(
nit: noSuchMethodInObject or ObjectNSM or defaultNoSuchMethod?

[Johnni Winther, 2017-05-04 08:17:21]

Description was changed from

==========
Implement KernelNoSuchMethodResolver.
==========

to

==========
Implement KernelNoSuchMethodResolver.

R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/f2ae4db28077fc19a4334efe4c5988187a46e942
==========

[Johnni Winther, 2017-05-04 08:17:22]

Committed patchset #2 (id:20001) manually as
f2ae4db28077fc19a4334efe4c5988187a46e942 (presubmit successful).

[Johnni Winther, 2017-05-04 09:13:36]

https://codereview.chromium.org/2857943002/diff/1/pkg/compiler/lib/src/js_bac...
File pkg/compiler/lib/src/js_backend/no_such_method_registry.dart (right):

https://codereview.chromium.org/2857943002/diff/1/pkg/compiler/lib/src/js_bac...
pkg/compiler/lib/src/js_backend/no_such_method_registry.dart:211: class
NoSuchMethodResolverImpl implements NoSuchMethodResolver {
On 2017/05/03 17:29:49, Siggi Cherem (dart-lang) wrote:
> move under resolution/* ?

Done.

https://codereview.chromium.org/2857943002/diff/1/pkg/compiler/lib/src/kernel...
File pkg/compiler/lib/src/kernel/no_such_method_resolver.dart (right):

https://codereview.chromium.org/2857943002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/no_such_method_resolver.dart:18: ir.Procedure node =
elementMap._memberList[method.memberIndex].node;
On 2017/05/03 17:29:49, Siggi Cherem (dart-lang) wrote:
> Since we are preparing to move this file as a separate library and make some
> public (but restricted APIs) to fetch kernel from the KElements, maybe we can
> take the first step here and add a lookup for these (and hide all the .index
> uses within the file that defines the environment.)
> 
> We can still keep it private (e.g. _lookupProcedure(method))

Done.

https://codereview.chromium.org/2857943002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/no_such_method_resolver.dart:26: } else if (body is
ir.Block && body.statements.isNotEmpty) {
On 2017/05/03 17:29:49, Siggi Cherem (dart-lang) wrote:
> ha - the old implementation checked that statements.length == 1 (so a silly
case
> like: 
>   noSuchMethod(i) {
>     return super.noSuchMethod(i);
>     return foo;
>   }
> 
> was not recognized before, but it will be recognized now.

Ahh - I thought I was equally silly...

https://codereview.chromium.org/2857943002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/no_such_method_resolver.dart:59: } else if (body is
ir.Block && body.statements.isNotEmpty) {
On 2017/05/03 17:29:49, Siggi Cherem (dart-lang) wrote:
> nit: move this out first, so we can reuse the other two conditions:
> 
> ir.Statement body = node.function.body;
> if (body is ir.Block && body.statements.isNotEmpty) {
>   body = body.statements.first;
> }
> if (body is ir.ReturnStatement) {
>   expr = body.expression;
> } else if (body is ir.ExpressionStatement) {
>   expr = body.expression;
> }
> return expr is ir.Throw;

Done.

https://codereview.chromium.org/2857943002/diff/1/pkg/compiler/lib/src/kernel...
pkg/compiler/lib/src/kernel/no_such_method_resolver.dart:84: break;
On 2017/05/03 17:29:49, Siggi Cherem (dart-lang) wrote:
> not sure what this case does - if you have a noSuchMethod that is a getter or
> has the wrong signature, you'll return Object.noSuchMethod below, was that the
> intent?

Yep. That's the spec!

https://codereview.chromium.org/2857943002/diff/1/tests/compiler/dart2js/no_s...
File tests/compiler/dart2js/no_such_method_enabled_test.dart (right):

https://codereview.chromium.org/2857943002/diff/1/tests/compiler/dart2js/no_s...
tests/compiler/dart2js/no_such_method_enabled_test.dart:271:
NoSuchMethodResolver resolver = registry.resolver;
On 2017/05/03 17:29:49, Siggi Cherem (dart-lang) wrote:
> nit: rather than making the resolver public, I'd keep it private and add a
> getter like `get internalResolverForTesting => _resolver`.

Done.

https://codereview.chromium.org/2857943002/diff/1/tests/compiler/dart2js/no_s...
tests/compiler/dart2js/no_such_method_enabled_test.dart:272: FunctionEntity
noSuchMethodObject = elementEnvironment.lookupClassMember(
On 2017/05/03 17:29:49, Siggi Cherem (dart-lang) wrote:
> nit: noSuchMethodInObject or ObjectNSM or defaultNoSuchMethod?

Done.