refactor emitMemberName to be used more consistently

refactor emitMemberName to be used more consistently

other changes:
* adds _emitSend for more consistent emitting of method calls. can't be used everywhere yet, but it unifies some things
* more consistent use of the core.String.* static pattern
* fix dynamic invoke to private members
* fix implicit-this to use emitMemberName, which makes them work with extension methods
* build the global extension method table up-front and optimize lookups (method name first, then subtype test if needed)
* teach the compiler that IsExpression never returns null

R=jacobr@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/30f9e3d8d3c83839a4f8ac7d184ccd6ba1ed081e

[Jennifer Messerly, 2015-04-13 21:49:34]

Patchset #2 (id:20001) has been deleted

[Jennifer Messerly, 2015-04-13 21:49:39]

Patchset #1 (id:1) has been deleted

[Jennifer Messerly, 2015-04-13 21:58:32]

jmesserly@google.com changed reviewers:
+ jacobr@google.com

[Jacob, 2015-04-13 23:19:36]

looks good overall

https://codereview.chromium.org/1083763003/diff/40001/lib/runtime/dart/_inter...
File lib/runtime/dart/_internal.js (right):

https://codereview.chromium.org/1083763003/diff/40001/lib/runtime/dart/_inter...
lib/runtime/dart/_internal.js:26: if (this[core.$length] == 0)
nice. glad this case is fixed.

https://codereview.chromium.org/1083763003/diff/40001/lib/runtime/dart_runtim...
File lib/runtime/dart_runtime.js (left):

https://codereview.chromium.org/1083763003/diff/40001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:71: function dinvokef(f/*, ...args*/) {
dcall seems more consistent with dart. E.g
class Foo /* implements Function */ {
  call(arg1, arg2, ...);
}

https://codereview.chromium.org/1083763003/diff/40001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1083763003/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1233: code = 'dart.dsend(#, #, #)';
style idea: encode dsend, dinvokef, etc as Dart string constants 
e.g.
$SEND , $CALL, etc so that if we refactor these names again it is easy to make
sure we get all the cases.

https://codereview.chromium.org/1083763003/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1692: var type = getStaticType(expr);
P3 nit: seems like there doesn't need to be so much whitespace around this line.

https://codereview.chromium.org/1083763003/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1706: increment.staticElement =
node.staticElement;
use cascades for increment

https://codereview.chromium.org/1083763003/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1806: var dynamicHelper = const {'[]': 'dindex',
'[]=': 'dsetindex'}[name];
good example of a case where it would be nice if 'dindex' was a const, etc.

https://codereview.chromium.org/1083763003/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:2254: t = _fillDynamicTypeArgs(t);
nit: shouldn't we fill in these dynamic args in _extensionMethods data struct
instead of filling on every use?

https://codereview.chromium.org/1083763003/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:2265: var d = rules.provider.dynamicType;
nit: why is this a local variable? seems to provide 0 value.
cleaner code:

InterfaceType _fillDynamicTypeArgs(InterfaceType t) =>
  t.substitute4(new List.filled(t.typeArguments.length,
rules.provider.dynamicType));

https://codereview.chromium.org/1083763003/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:2322: names.addAll(e.methods.map((m) =>
m.name));
why not use cascades:
      var names = new HashSet<String>()
        ..addAll(e.methods.map((m) => m.name));
        ..addAll(e.accessors.map((m) => m.name));

https://codereview.chromium.org/1083763003/diff/40001/lib/src/js/nodes.dart
File lib/src/js/nodes.dart (right):

https://codereview.chromium.org/1083763003/diff/40001/lib/src/js/nodes.dart#n...
lib/src/js/nodes.dart:1082: assert(value != '"_nextCallback="');
this seems like an oddly specific assert.

[Jennifer Messerly, 2015-04-14 21:20:20]

Patchset #2 (id:60001) has been deleted

[Jennifer Messerly, 2015-04-14 21:21:01]

Thanks so much! PTAL, I think I got them.

https://codereview.chromium.org/1083763003/diff/40001/lib/runtime/dart_runtim...
File lib/runtime/dart_runtime.js (left):

https://codereview.chromium.org/1083763003/diff/40001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:71: function dinvokef(f/*, ...args*/) {
On 2015/04/13 23:19:35, Jacob wrote:
> dcall seems more consistent with dart. E.g
> class Foo /* implements Function */ {
>   call(arg1, arg2, ...);
> }

+1 ... yeah planning to fix this in follow up, but went ahead and sent
https://codereview.chromium.org/1074923003/ ... if that one lands first I can
rebase this one to simplify the diff here.

https://codereview.chromium.org/1083763003/diff/40001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1083763003/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1233: code = 'dart.dsend(#, #, #)';
On 2015/04/13 23:19:35, Jacob wrote:
> style idea: encode dsend, dinvokef, etc as Dart string constants 
> e.g.
> $SEND , $CALL, etc so that if we refactor these names again it is easy to make
> sure we get all the cases.

yeah, that could work. Although we'd still need to match up dart_runtime.js and
any comments.

One idea longer term to improve dart_runtime.js is to make it a private .dart
library, and then we can emit calls to the Dart methods. At that point we'll
definitely want constants though, so it sounds like a good idea to add them :)

https://codereview.chromium.org/1083763003/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1692: var type = getStaticType(expr);
On 2015/04/13 23:19:35, Jacob wrote:
> P3 nit: seems like there doesn't need to be so much whitespace around this
line.

Done.

https://codereview.chromium.org/1083763003/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1706: increment.staticElement =
node.staticElement;
On 2015/04/13 23:19:35, Jacob wrote:
> use cascades for increment

Done.

https://codereview.chromium.org/1083763003/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1806: var dynamicHelper = const {'[]': 'dindex',
'[]=': 'dsetindex'}[name];
On 2015/04/13 23:19:35, Jacob wrote:
> good example of a case where it would be nice if 'dindex' was a const, etc.

Done.

https://codereview.chromium.org/1083763003/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:2254: t = _fillDynamicTypeArgs(t);
On 2015/04/13 23:19:35, Jacob wrote:
> nit: shouldn't we fill in these dynamic args in _extensionMethods data struct
> instead of filling on every use?

good catch. done.

https://codereview.chromium.org/1083763003/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:2265: var d = rules.provider.dynamicType;
On 2015/04/13 23:19:35, Jacob wrote:
> nit: why is this a local variable? seems to provide 0 value.
> cleaner code:
> 
> InterfaceType _fillDynamicTypeArgs(InterfaceType t) =>
>   t.substitute4(new List.filled(t.typeArguments.length,
> rules.provider.dynamicType));

not sure, the original version was like that, just got moved around. happy to
use => form

https://codereview.chromium.org/1083763003/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:2322: names.addAll(e.methods.map((m) =>
m.name));
On 2015/04/13 23:19:35, Jacob wrote:
> why not use cascades:
>       var names = new HashSet<String>()
>         ..addAll(e.methods.map((m) => m.name));
>         ..addAll(e.accessors.map((m) => m.name));

good catch. I stopped using them when the formatter was busted, but it seems
better now, maybe because you landed a fix? or maybe this case works.

https://codereview.chromium.org/1083763003/diff/40001/lib/src/js/nodes.dart
File lib/src/js/nodes.dart (right):

https://codereview.chromium.org/1083763003/diff/40001/lib/src/js/nodes.dart#n...
lib/src/js/nodes.dart:1082: assert(value != '"_nextCallback="');
On 2015/04/13 23:19:36, Jacob wrote:
> this seems like an oddly specific assert.

eeeep. Good catch! I didn't follow my own convention for marking debug code :\

[Jacob, 2015-04-14 21:41:06]

lgtm

https://codereview.chromium.org/1083763003/diff/40001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1083763003/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1233: code = 'dart.dsend(#, #, #)';
On 2015/04/14 21:21:00, John Messerly wrote:
> On 2015/04/13 23:19:35, Jacob wrote:
> > style idea: encode dsend, dinvokef, etc as Dart string constants 
> > e.g.
> > $SEND , $CALL, etc so that if we refactor these names again it is easy to
make
> > sure we get all the cases.
> 
> yeah, that could work. Although we'd still need to match up dart_runtime.js
and
> any comments.
> 
> One idea longer term to improve dart_runtime.js is to make it a private .dart
> library, and then we can emit calls to the Dart methods. At that point we'll
> definitely want constants though, so it sounds like a good idea to add them :)

webstorm should be able to let you refactor JS code. I doubt it will let you
refactor js code in dart quoted strings. :)

https://codereview.chromium.org/1083763003/diff/80001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1083763003/diff/80001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:2294: static const DPUT = 'dput';
nit: no idea if dart has an official convention but generally I like constants
at the top of the file with a comment specifying what they are. Could be just a
single // comment for all 6 of these.

[Jennifer Messerly, 2015-04-14 22:17:19]

https://codereview.chromium.org/1083763003/diff/80001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1083763003/diff/80001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:2294: static const DPUT = 'dput';
On 2015/04/14 21:41:06, Jacob wrote:
> nit: no idea if dart has an official convention but generally I like constants
> at the top of the file with a comment specifying what they are. Could be just
a
> single // comment for all 6 of these.

Hmm. I guess I absorbed a rule at some point from someone to put
implementation-y details near the end, out of the main flow for the reader. I'm
fine moving them, but did you mean top of class or top of file? If top of class,
does it go before instance fields/ctors? Right now they're scoped to the class,
since I didn't think it made sense to use outside of the class scope.

(side note: technically the camelCase convention is preferred too, but it'd
probably be bad here, as we need to ensure the argument to js.call() and
js.statement() is always a const string.)

[Jacob, 2015-04-14 22:25:01]

lgtm

https://codereview.chromium.org/1083763003/diff/80001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1083763003/diff/80001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:2294: static const DPUT = 'dput';
On 2015/04/14 22:17:19, John Messerly wrote:
> On 2015/04/14 21:41:06, Jacob wrote:
> > nit: no idea if dart has an official convention but generally I like
constants
> > at the top of the file with a comment specifying what they are. Could be
just
> a
> > single // comment for all 6 of these.
> 
> Hmm. I guess I absorbed a rule at some point from someone to put
> implementation-y details near the end, out of the main flow for the reader.
I'm
> fine moving them, but did you mean top of class or top of file? If top of
class,
> does it go before instance fields/ctors? Right now they're scoped to the
class,
> since I didn't think it made sense to use outside of the class scope.
> 
> (side note: technically the camelCase convention is preferred too, but it'd
> probably be bad here, as we need to ensure the argument to js.call() and
> js.statement() is always a const string.)

I don't care deeply about it but I'd personally make these top level to the
library instead of static as that semantically makes sense to me as  the impact
is global given how these constants change the generated code. Perhaps keeping
them at the top is cargo cult wisdom from the days of C where that would
actually matter in terms of how they were resolved..

FWIW the dart style guide examples do show top level consts before the class not
after it.
const pi = 3.14;
const defaultTimeout = 1000;
final urlScheme = new RegExp('^([a-z]+):');

class Dice {
  static final numberGenerator = new Random();
}

[Jennifer Messerly, 2015-04-14 22:45:28]

https://codereview.chromium.org/1083763003/diff/80001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1083763003/diff/80001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:2294: static const DPUT = 'dput';
On 2015/04/14 22:25:00, Jacob wrote:
> On 2015/04/14 22:17:19, John Messerly wrote:
> > On 2015/04/14 21:41:06, Jacob wrote:
> > > nit: no idea if dart has an official convention but generally I like
> constants
> > > at the top of the file with a comment specifying what they are. Could be
> just
> > a
> > > single // comment for all 6 of these.
> > 
> > Hmm. I guess I absorbed a rule at some point from someone to put
> > implementation-y details near the end, out of the main flow for the reader.
> I'm
> > fine moving them, but did you mean top of class or top of file? If top of
> class,
> > does it go before instance fields/ctors? Right now they're scoped to the
> class,
> > since I didn't think it made sense to use outside of the class scope.
> > 
> > (side note: technically the camelCase convention is preferred too, but it'd
> > probably be bad here, as we need to ensure the argument to js.call() and
> > js.statement() is always a const string.)
> 
> I don't care deeply about it but I'd personally make these top level to the
> library instead of static as that semantically makes sense to me as  the
impact
> is global given how these constants change the generated code. Perhaps keeping
> them at the top is cargo cult wisdom from the days of C where that would
> actually matter in terms of how they were resolved..
> 
> FWIW the dart style guide examples do show top level consts before the class
not
> after it.
> const pi = 3.14;
> const defaultTimeout = 1000;
> final urlScheme = new RegExp('^([a-z]+):');
> 
> class Dice {
>   static final numberGenerator = new Random();
> }

OK. Moved to top, added comment.

[Jennifer Messerly, 2015-04-14 23:07:33]

Committed patchset #3 (id:100001) manually as
30f9e3d8d3c83839a4f8ac7d184ccd6ba1ed081e (presubmit successful).