InvocationMirror implemented in dart2js.

InvocationMirror implemented in dart2js.

[Johnni Winther, 2012-10-24 08:47:55]

Tested with:

class PlusMinus {
  noSuchMethod(InvocationMirror msg) {
      print(msg.memberName);
      return msg.memberName == 'mul' ? msg.invokeOn(mul())
                              : Function.apply(plus, msg.positionalArguments,
msg.namedArguments);
  }

  int plus(int a, int b) {
    print('plus($a,$b)');
    return a+b;
  }

  Mul mul() => new Mul();
}

class Mul {
  int mul(int a, int b) {
    print('mul($a,$b)');
    return a*b;
  }
}

void main() {
  var pm = new PlusMinus();
  print('pm.plus(2,3)');
  print(pm.plus(2, 3));
  print('pm.minus(2,3)');
  print(pm.minus(2, 3));
  print('pm.mul(2,3)');
  print(pm.mul(2, 3));
  print(new Mul().mul(3,4)); // Needed circumvent tree-shaking.
}

[Johnni Winther, 2012-10-24 13:33:33]

PTAL

[bakster, 2012-10-24 13:43:04]

Drive by comments,
  Lars

https://codereview.chromium.org/11264005/diff/4001/lib/compiler/implementatio...
File lib/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/11264005/diff/4001/lib/compiler/implementatio...
lib/compiler/implementation/enqueue.dart:251: if
(selector.applies(compiler.invokeOnMethod, compiler)) {
Why two if statements when you have three expressions.
Consider using one if.

https://codereview.chromium.org/11264005/diff/4001/lib/compiler/implementatio...
File lib/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11264005/diff/4001/lib/compiler/implementatio...
lib/compiler/implementation/lib/js_helper.dart:405: //return new
JSInvocationMirrorPositionalArguments(this);
Code in comments is wrong.

https://codereview.chromium.org/11264005/diff/4001/lib/compiler/implementatio...
lib/compiler/implementation/lib/js_helper.dart:415: //return new
JSInvocationMirrorNamedArguments(this);
Same here.

https://codereview.chromium.org/11264005/diff/4001/tests/language/invocation_...
File tests/language/invocation_mirror_indirect_test.dart (right):

https://codereview.chromium.org/11264005/diff/4001/tests/language/invocation_...
tests/language/invocation_mirror_indirect_test.dart:7: 
My comments from  tests/language/invocation_mirror_test.dart are relevant here
too.

https://codereview.chromium.org/11264005/diff/4001/tests/language/invocation_...
File tests/language/invocation_mirror_test.dart (right):

https://codereview.chromium.org/11264005/diff/4001/tests/language/invocation_...
tests/language/invocation_mirror_test.dart:12: } else if (invocation.memberName
== 'div') {
Inconsistent use of else parts. I suggest:
   if (bla) return foo;
   if (bla) return bar;
   return rest;

https://codereview.chromium.org/11264005/diff/4001/tests/language/invocation_...
tests/language/invocation_mirror_test.dart:29: if (?c) {
Remove else parts

https://codereview.chromium.org/11264005/diff/4001/tests/language/invocation_...
tests/language/invocation_mirror_test.dart:38: int div({int num, int denom}) {
Use => for short hand function syntax.

[Johnni Winther, 2012-10-25 08:51:34]

PTAL

[Johnni Winther, 2012-10-25 13:34:56]

https://codereview.chromium.org/11264005/diff/4001/lib/compiler/implementatio...
File lib/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/11264005/diff/4001/lib/compiler/implementatio...
lib/compiler/implementation/enqueue.dart:251: if
(selector.applies(compiler.invokeOnMethod, compiler)) {
On 2012/10/24 13:43:04, bakster wrote:
> Why two if statements when you have three expressions.
> Consider using one if. 

Test removed.

https://codereview.chromium.org/11264005/diff/4001/lib/compiler/implementatio...
File lib/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11264005/diff/4001/lib/compiler/implementatio...
lib/compiler/implementation/lib/js_helper.dart:405: //return new
JSInvocationMirrorPositionalArguments(this);
On 2012/10/24 13:43:04, bakster wrote:
> Code in comments is wrong.

Done.

https://codereview.chromium.org/11264005/diff/4001/lib/compiler/implementatio...
lib/compiler/implementation/lib/js_helper.dart:415: //return new
JSInvocationMirrorNamedArguments(this);
On 2012/10/24 13:43:04, bakster wrote:
> Same here.

Done.

https://codereview.chromium.org/11264005/diff/4001/tests/language/invocation_...
File tests/language/invocation_mirror_indirect_test.dart (right):

https://codereview.chromium.org/11264005/diff/4001/tests/language/invocation_...
tests/language/invocation_mirror_indirect_test.dart:7: 
On 2012/10/24 13:43:04, bakster wrote:
> My comments from  tests/language/invocation_mirror_test.dart are relevant here
> too.

Done.

https://codereview.chromium.org/11264005/diff/4001/tests/language/invocation_...
File tests/language/invocation_mirror_test.dart (right):

https://codereview.chromium.org/11264005/diff/4001/tests/language/invocation_...
tests/language/invocation_mirror_test.dart:12: } else if (invocation.memberName
== 'div') {
On 2012/10/24 13:43:04, bakster wrote:
> Inconsistent use of else parts. I suggest:
>    if (bla) return foo;
>    if (bla) return bar;
>    return rest;

Done.

https://codereview.chromium.org/11264005/diff/4001/tests/language/invocation_...
tests/language/invocation_mirror_test.dart:29: if (?c) {
On 2012/10/24 13:43:04, bakster wrote:
> Remove else parts

Done.

https://codereview.chromium.org/11264005/diff/4001/tests/language/invocation_...
tests/language/invocation_mirror_test.dart:38: int div({int num, int denom}) {
On 2012/10/24 13:43:04, bakster wrote:
> Use => for short hand function syntax.

Done.

[ngeoffray, 2012-10-26 08:34:55]

LGTM!

https://codereview.chromium.org/11264005/diff/10003/lib/compiler/implementati...
File lib/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/11264005/diff/10003/lib/compiler/implementati...
lib/compiler/implementation/enqueue.dart:252: /*if (!compiler.enabledInvokeOn &&
name == Compiler.INVOKE_ON) {
Remove comment.

https://codereview.chromium.org/11264005/diff/10003/lib/compiler/implementati...
File lib/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11264005/diff/10003/lib/compiler/implementati...
lib/compiler/implementation/lib/js_helper.dart:365: final int _type;
Call it kind? Type is overloaded.

https://codereview.chromium.org/11264005/diff/10003/lib/compiler/implementati...
lib/compiler/implementation/lib/js_helper.dart:369: Map<String,Dynamic>
_namedIndices = null;
space after comma (same in other places in this file).
Can we do Dynamic -> dynamic?

https://codereview.chromium.org/11264005/diff/10003/lib/compiler/implementati...
lib/compiler/implementation/lib/js_helper.dart:397: if (_type == GETTER) return
null;
check isAccessor instead?

https://codereview.chromium.org/11264005/diff/10003/lib/compiler/implementati...
File lib/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/11264005/diff/10003/lib/compiler/implementati...
lib/compiler/implementation/ssa/codegen.dart:1486: if
(node.selector.name.slowToString() == 'foo' ||
Remove

https://codereview.chromium.org/11264005/diff/10003/lib/compiler/implementati...
lib/compiler/implementation/ssa/codegen.dart:1524:
backend.compiler.enabledNoSuchMethod &&
I think you can just check enabledInvokeOn.

https://codereview.chromium.org/11264005/diff/10003/tests/language/invocation...
File tests/language/invocation_mirror_indirect_test.dart (right):

https://codereview.chromium.org/11264005/diff/10003/tests/language/invocation...
tests/language/invocation_mirror_indirect_test.dart:22: int plus(int a, int b)
=> a+b;
spaces between +, and same for other operators in this file.

https://codereview.chromium.org/11264005/diff/10003/tests/language/invocation...
File tests/language/invocation_mirror_test.dart (right):

https://codereview.chromium.org/11264005/diff/10003/tests/language/invocation...
tests/language/invocation_mirror_test.dart:17: int plus(int a, int b) => a+b;
ditto for space

[Johnni Winther, 2012-10-26 10:18:01]

https://codereview.chromium.org/11264005/diff/10003/lib/compiler/implementati...
File lib/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/11264005/diff/10003/lib/compiler/implementati...
lib/compiler/implementation/enqueue.dart:252: /*if (!compiler.enabledInvokeOn &&
name == Compiler.INVOKE_ON) {
On 2012/10/26 08:34:55, ngeoffray wrote:
> Remove comment.

Done.

https://codereview.chromium.org/11264005/diff/10003/lib/compiler/implementati...
File lib/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11264005/diff/10003/lib/compiler/implementati...
lib/compiler/implementation/lib/js_helper.dart:365: final int _type;
On 2012/10/26 08:34:55, ngeoffray wrote:
> Call it kind? Type is overloaded.

Done.

https://codereview.chromium.org/11264005/diff/10003/lib/compiler/implementati...
lib/compiler/implementation/lib/js_helper.dart:369: Map<String,Dynamic>
_namedIndices = null;
On 2012/10/26 08:34:55, ngeoffray wrote:
> space after comma (same in other places in this file).
> Can we do Dynamic -> dynamic?

Done.

https://codereview.chromium.org/11264005/diff/10003/lib/compiler/implementati...
lib/compiler/implementation/lib/js_helper.dart:397: if (_type == GETTER) return
null;
On 2012/10/26 08:34:55, ngeoffray wrote:
> check isAccessor instead?

Done.

https://codereview.chromium.org/11264005/diff/10003/lib/compiler/implementati...
File lib/compiler/implementation/ssa/codegen.dart (right):

https://codereview.chromium.org/11264005/diff/10003/lib/compiler/implementati...
lib/compiler/implementation/ssa/codegen.dart:1486: if
(node.selector.name.slowToString() == 'foo' ||
On 2012/10/26 08:34:55, ngeoffray wrote:
> Remove

Done.

https://codereview.chromium.org/11264005/diff/10003/lib/compiler/implementati...
lib/compiler/implementation/ssa/codegen.dart:1524:
backend.compiler.enabledNoSuchMethod &&
On 2012/10/26 08:34:55, ngeoffray wrote:
> I think you can just check enabledInvokeOn.

Done.

https://codereview.chromium.org/11264005/diff/10003/tests/language/invocation...
File tests/language/invocation_mirror_indirect_test.dart (right):

https://codereview.chromium.org/11264005/diff/10003/tests/language/invocation...
tests/language/invocation_mirror_indirect_test.dart:22: int plus(int a, int b)
=> a+b;
On 2012/10/26 08:34:55, ngeoffray wrote:
> spaces between +, and same for other operators in this file.

Done.

https://codereview.chromium.org/11264005/diff/10003/tests/language/invocation...
File tests/language/invocation_mirror_test.dart (right):

https://codereview.chromium.org/11264005/diff/10003/tests/language/invocation...
tests/language/invocation_mirror_test.dart:17: int plus(int a, int b) => a+b;
On 2012/10/26 08:34:55, ngeoffray wrote:
> ditto for space

Done.

[ahe, 2012-10-29 12:14:04]

https://codereview.chromium.org/11264005/diff/17001/lib/compiler/implementati...
File lib/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/11264005/diff/17001/lib/compiler/implementati...
lib/compiler/implementation/compiler.dart:180: static const SourceString MAIN =
const SourceString('main');
How about adding a TODO that these should be selectors?

https://codereview.chromium.org/11264005/diff/17001/lib/compiler/implementati...
lib/compiler/implementation/compiler.dart:335: findHelper(const
SourceString('createInvocationMirror'));
This looks specific to the JS backend.

https://codereview.chromium.org/11264005/diff/17001/lib/compiler/implementati...
lib/compiler/implementation/compiler.dart:337:
enqueuer.codegen.addToWorkList(createInvocationMirrorElement);
This should only be added if it is really needed.

https://codereview.chromium.org/11264005/diff/17001/lib/compiler/implementati...
lib/compiler/implementation/compiler.dart:390: lookupSpecialClass(const
SourceString('JSInvocationMirror'));
This looks specific to the JS backend.

https://codereview.chromium.org/11264005/diff/17001/lib/compiler/implementati...
lib/compiler/implementation/compiler.dart:425: const SourceString('invokeOn'));
This looks specific to the JS backend.

https://codereview.chromium.org/11264005/diff/17001/lib/compiler/implementati...
File lib/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/11264005/diff/17001/lib/compiler/implementati...
lib/compiler/implementation/enqueue.dart:109: compiler.enabledInvokeOn = true;
Somehow the grammar seems wrong for these variable names. Is it only me?

https://codereview.chromium.org/11264005/diff/17001/lib/compiler/implementati...
File lib/compiler/implementation/js_backend/emitter.dart (right):

https://codereview.chromium.org/11264005/diff/17001/lib/compiler/implementati...
lib/compiler/implementation/js_backend/emitter.dart:1294: CodeBuffer
generateMethod(String methodName, Selector selector) {
Could you rename this function while you're at it? This method has a general
name that hides its special purpose.

https://codereview.chromium.org/11264005/diff/17001/lib/compiler/implementati...
lib/compiler/implementation/js_backend/emitter.dart:1299: int type = METHOD;
In a compiler, type means something else. Please find a different name for this
variable, for example, invocationKind.

https://codereview.chromium.org/11264005/diff/17001/lib/compiler/implementati...
lib/compiler/implementation/js_backend/emitter.dart:1322: buffer.add('  return
this.$noSuchMethodName('
Notice that $noSuchMethodName is string interpolation. This means that it refers
to the name that is computed on line 1269.

https://codereview.chromium.org/11264005/diff/17001/lib/compiler/implementati...
lib/compiler/implementation/js_backend/emitter.dart:1323:
'\$.createInvocationMirror("$methodName", "$internalName",'
The same should be done here. The namer needs to have a say in this method name.

https://codereview.chromium.org/11264005/diff/17001/lib/compiler/implementati...
File lib/compiler/implementation/lib/js_helper.dart (right):

https://codereview.chromium.org/11264005/diff/17001/lib/compiler/implementati...
lib/compiler/implementation/lib/js_helper.dart:358: class JSInvocationMirror
implements InvocationMirror {
Please move this to its own file, like string_helper.dart and
regexp_helper.dart.

https://codereview.chromium.org/11264005/diff/17001/lib/compiler/implementati...
lib/compiler/implementation/lib/js_helper.dart:359: static const METHOD = 0;
FYI: It is possible to read these constants from the source code using the
constant evaluator. For an example of how to do this, see
ResolverTask.resolveMetadataAnnotation.

https://codereview.chromium.org/11264005/diff/17001/lib/compiler/implementati...
lib/compiler/implementation/lib/js_helper.dart:365: final int _kind;
This field could use some documentation.

https://codereview.chromium.org/11264005/diff/17001/lib/compiler/implementati...
lib/compiler/implementation/lib/js_helper.dart:377: if (_namedArgumentNames !=
null && _namedArgumentNames.length > 0) {
I think you need to compute this lazily.

https://codereview.chromium.org/11264005/diff/17001/lib/compiler/implementati...
lib/compiler/implementation/lib/js_helper.dart:398: var list = [];
You should be able to preallocate the correct size of this list.

https://codereview.chromium.org/11264005/diff/17001/lib/compiler/implementati...
lib/compiler/implementation/lib/js_helper.dart:400: _arguments.length -
_namedArgumentNames.length;
I think this fits on one line.

https://codereview.chromium.org/11264005/diff/17001/lib/compiler/implementati...
lib/compiler/implementation/lib/js_helper.dart:401: for (var index = 0 ; index <
argumentCount ; index++) {
Please use the common pattern:

for (int i = 0;

https://codereview.chromium.org/11264005/diff/17001/lib/compiler/implementati...
lib/compiler/implementation/lib/js_helper.dart:410:
_namedIndices.forEach((String name, int index) {
I'm having a really hard time seeing why you would compute _namedIndices in the
constructor when you must iterate through the values after all.