Generate static calls for Object fields and methods

Generate static calls for Object fields and methods

R=jmesserly@google.com

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

[vsm, 2015-04-23 17:39:54]

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

[Jennifer Messerly, 2015-04-23 19:01:42]

Nice, really like where this is going

https://codereview.chromium.org/1100633006/diff/1/lib/runtime/dart/_intercept...
File lib/runtime/dart/_interceptors.js (right):

https://codereview.chromium.org/1100633006/diff/1/lib/runtime/dart/_intercept...
lib/runtime/dart/_interceptors.js:1023: return dart.noSuchMethod(super,
invocation);
this doesn't seem like it would work?

https://codereview.chromium.org/1100633006/diff/1/lib/runtime/dart/_internal.js
File lib/runtime/dart/_internal.js (right):

https://codereview.chromium.org/1100633006/diff/1/lib/runtime/dart/_internal....
lib/runtime/dart/_internal.js:145: return dart.toString(buffer);
if we knew the type was not null, could we generate toString() ?

(e.g. this would be a pretty easy case to infer "buffer" as non-null
StringBuffer)

https://codereview.chromium.org/1100633006/diff/1/lib/runtime/dart_runtime.js
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/1100633006/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:141: function runtimeType(obj) {
The intent of this method was "get the _real_ runtimeType", inspired by
`getRuntimeType` in _js_helper. So the Object.runtimeType getter should return
this, but in Dart, you can override the runtimeType getter. In other words, we
really need two methods:

realRuntimeType(obj) --> this method

runtimeType(obj) {
  // ... check null and primitives ...

  // delegate to user code
  return obj.runtimeType;
}

I guess they probably need some common code for the null/primitives

https://codereview.chromium.org/1100633006/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:861: return core.Null;
copy+paste error? this code can't be reached. `obj == null` above will match
`null` and `undefined`

https://codereview.chromium.org/1100633006/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:867: // TODO(vsm): Call the JSString hashCode?
+1

https://codereview.chromium.org/1100633006/diff/1/lib/src/checker/resolver.dart
File lib/src/checker/resolver.dart (right):

https://codereview.chromium.org/1100633006/diff/1/lib/src/checker/resolver.da...
lib/src/checker/resolver.dart:572: Map<String, DartType> _objectMembers;
ooh nice.

https://codereview.chromium.org/1100633006/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1100633006/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:101: _objectMembers =
getObjectMemberMap(rules.provider);
thought here, TypeRules is our type (src/checker/rules.dart). Could we put it
there?

https://codereview.chromium.org/1100633006/diff/1/tool/input_sdk/patch/core_p...
File tool/input_sdk/patch/core_patch.dart (right):

https://codereview.chromium.org/1100633006/diff/1/tool/input_sdk/patch/core_p...
tool/input_sdk/patch/core_patch.dart:53: Type get runtimeType => JS('Type',
'dart.runtimeType(#)', this); // getRuntimeType(this);
no need to keep the old code around :). Note however, this would be
dart.realRuntimeType per my earlier suggestion

[Jennifer Messerly, 2015-04-23 19:02:54]

https://codereview.chromium.org/1100633006/diff/1/lib/runtime/dart_runtime.js
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/1100633006/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:874: function toString(obj) {
oh, just noticed noSuchMethod helper is missing?

https://codereview.chromium.org/1100633006/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:878: return obj.toString();
we'll have to make sure to never call dart.toString (and friends) for "super"
calls, or this will infinite loop

[Jennifer Messerly, 2015-04-23 19:05:58]

oops, one more comment

https://codereview.chromium.org/1100633006/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1100633006/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:1291: } else if
(_objectMembers.containsKey(name)) {
Is this only if `target` is nullable**? A test case would be: does
`this.toString()` generate using the helper? it shouldn't, `this` can't be null.

** or should the check be: "could be a JS primitive"? In which case for List
etc, the answer is "yes". But for many many types, the answer is "no".

[vsm, 2015-04-23 20:35:56]

thanks!  PTAL

https://codereview.chromium.org/1100633006/diff/1/lib/runtime/dart/_intercept...
File lib/runtime/dart/_interceptors.js (right):

https://codereview.chromium.org/1100633006/diff/1/lib/runtime/dart/_intercept...
lib/runtime/dart/_interceptors.js:1023: return dart.noSuchMethod(super,
invocation);
On 2015/04/23 19:01:41, John Messerly wrote:
> this doesn't seem like it would work?

Ahh, forgot this helper.  :-)

https://codereview.chromium.org/1100633006/diff/1/lib/runtime/dart/_internal.js
File lib/runtime/dart/_internal.js (right):

https://codereview.chromium.org/1100633006/diff/1/lib/runtime/dart/_internal....
lib/runtime/dart/_internal.js:145: return dart.toString(buffer);
On 2015/04/23 19:01:41, John Messerly wrote:
> if we knew the type was not null, could we generate toString() ?
> 
> (e.g. this would be a pretty easy case to infer "buffer" as non-null
> StringBuffer)

Yeah, we should do some simple analysis to catch this ... and the notNull on i
above as well.

https://codereview.chromium.org/1100633006/diff/1/lib/runtime/dart_runtime.js
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/1100633006/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:141: function runtimeType(obj) {
On 2015/04/23 19:01:42, John Messerly wrote:
> The intent of this method was "get the _real_ runtimeType", inspired by
> `getRuntimeType` in _js_helper. So the Object.runtimeType getter should return
> this, but in Dart, you can override the runtimeType getter. In other words, we
> really need two methods:
> 
> realRuntimeType(obj) --> this method
> 
> runtimeType(obj) {
>   // ... check null and primitives ...
> 
>   // delegate to user code
>   return obj.runtimeType;
> }
> 
> I guess they probably need some common code for the null/primitives

I've split this back out for now.

The other option to consider is disallowing override of runtimeType.  I was
leaning toward this, but I guess there is no strong reason to for now ... we
don't use it for actual type checks and neither does dart2js/vm AFAIK.

https://codereview.chromium.org/1100633006/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:861: return core.Null;
On 2015/04/23 19:01:41, John Messerly wrote:
> copy+paste error? this code can't be reached. `obj == null` above will match
> `null` and `undefined`

Done.

https://codereview.chromium.org/1100633006/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:867: // TODO(vsm): Call the JSString hashCode?
On 2015/04/23 19:01:42, John Messerly wrote:
> +1

Will look at primitives next.

https://codereview.chromium.org/1100633006/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:874: function toString(obj) {
On 2015/04/23 19:02:54, John Messerly wrote:
> oh, just noticed noSuchMethod helper is missing?

Added.

https://codereview.chromium.org/1100633006/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:878: return obj.toString();
On 2015/04/23 19:02:54, John Messerly wrote:
> we'll have to make sure to never call dart.toString (and friends) for "super"
> calls, or this will infinite loop

Good point!  I added a check to the codegen that should prevent this.

https://codereview.chromium.org/1100633006/diff/1/lib/src/checker/resolver.dart
File lib/src/checker/resolver.dart (right):

https://codereview.chromium.org/1100633006/diff/1/lib/src/checker/resolver.da...
lib/src/checker/resolver.dart:572: Map<String, DartType> _objectMembers;
On 2015/04/23 19:01:42, John Messerly wrote:
> ooh nice.

Acknowledged.

https://codereview.chromium.org/1100633006/diff/1/lib/src/codegen/js_codegen....
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1100633006/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:101: _objectMembers =
getObjectMemberMap(rules.provider);
On 2015/04/23 19:01:42, John Messerly wrote:
> thought here, TypeRules is our type (src/checker/rules.dart). Could we put it
> there?

Yeah, I suppose that's better than here.

Done.

https://codereview.chromium.org/1100633006/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:1291: } else if
(_objectMembers.containsKey(name)) {
On 2015/04/23 19:05:58, John Messerly wrote:
> Is this only if `target` is nullable**? A test case would be: does
> `this.toString()` generate using the helper? it shouldn't, `this` can't be
null.
> 
> ** or should the check be: "could be a JS primitive"? In which case for List
> etc, the answer is "yes". But for many many types, the answer is "no".

I added a check in.  We need both checks as all types are nullable (for now
:-)).

https://codereview.chromium.org/1100633006/diff/1/tool/input_sdk/patch/core_p...
File tool/input_sdk/patch/core_patch.dart (right):

https://codereview.chromium.org/1100633006/diff/1/tool/input_sdk/patch/core_p...
tool/input_sdk/patch/core_patch.dart:53: Type get runtimeType => JS('Type',
'dart.runtimeType(#)', this); // getRuntimeType(this);
On 2015/04/23 19:01:42, John Messerly wrote:
> no need to keep the old code around :). Note however, this would be
> dart.realRuntimeType per my earlier suggestion

Done.

[Jennifer Messerly, 2015-04-23 21:09:30]

lgtm

https://codereview.chromium.org/1100633006/diff/60001/lib/runtime/dart/_inter...
File lib/runtime/dart/_interceptors.js (left):

https://codereview.chromium.org/1100633006/diff/60001/lib/runtime/dart/_inter...
lib/runtime/dart/_interceptors.js:743: return core.String['*'](padding, delta) +
dart.notNull(this);
nice!

https://codereview.chromium.org/1100633006/diff/60001/lib/runtime/dart/_js_he...
File lib/runtime/dart/_js_helper.js (right):

https://codereview.chromium.org/1100633006/diff/60001/lib/runtime/dart/_js_he...
lib/runtime/dart/_js_helper.js:762: return dart.toString(result);
random thought for future:

we have an issue with template strings: `hello ${name}`. Those won't use Dart
toString. The fix is to make them "tagged template":

    dart.str `hello ${name}`

where "str" is a function call:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/template_st...

https://codereview.chromium.org/1100633006/diff/60001/lib/runtime/dart_runtim...
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/1100633006/diff/60001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:897: throw new core.NoSuchMethodError(obj,
invocation.memberName,
could this be like:

    return core.Object.noSuchMethod.call(obj, invocation);

?

https://codereview.chromium.org/1100633006/diff/60001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:904: throw new core.NoSuchMethodError(obj,
invocation.memberName,
this too

https://codereview.chromium.org/1100633006/diff/60001/lib/src/checker/rules.dart
File lib/src/checker/rules.dart (right):

https://codereview.chromium.org/1100633006/diff/60001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:22: TypeRules(TypeProvider this.provider) {
minor suggestion:

final Map<String, DartType> objectMembers;

TypeRules(TypeProvider provider)
  : provider = provider,
    objectMembers = utils.getObjectMemberMap(provider);

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

https://codereview.chromium.org/1100633006/diff/60001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1914:
!rules.objectMembers.containsKey(memberId.name)) {
we chatted about this, maybe pull this out into its own path and emit
dart.#.bind(#) ?

[vsm, 2015-04-23 21:23:33]

https://codereview.chromium.org/1100633006/diff/60001/lib/runtime/dart_runtim...
File lib/runtime/dart_runtime.js (right):

https://codereview.chromium.org/1100633006/diff/60001/lib/runtime/dart_runtim...
lib/runtime/dart_runtime.js:897: throw new core.NoSuchMethodError(obj,
invocation.memberName,
On 2015/04/23 21:09:30, John Messerly wrote:
> could this be like:
> 
>     return core.Object.noSuchMethod.call(obj, invocation);
> 
> ?

This feels mildly dangerous ... depending on how Object.noSuchMethod is written
/ generated it may assume the this is non-null.

https://codereview.chromium.org/1100633006/diff/60001/lib/src/checker/rules.dart
File lib/src/checker/rules.dart (right):

https://codereview.chromium.org/1100633006/diff/60001/lib/src/checker/rules.d...
lib/src/checker/rules.dart:22: TypeRules(TypeProvider this.provider) {
On 2015/04/23 21:09:30, John Messerly wrote:
> minor suggestion:
> 
> final Map<String, DartType> objectMembers;
> 
> TypeRules(TypeProvider provider)
>   : provider = provider,
>     objectMembers = utils.getObjectMemberMap(provider);
> 

Done.

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

https://codereview.chromium.org/1100633006/diff/60001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1914:
!rules.objectMembers.containsKey(memberId.name)) {
On 2015/04/23 21:09:30, John Messerly wrote:
> we chatted about this, maybe pull this out into its own path and emit
> dart.#.bind(#) ?

Done.

[vsm, 2015-04-23 21:23:53]

Committed patchset #5 (id:80001) manually as
a5c125301d4d1e4f903ceb115ab17ec98b9a3eb4 (presubmit successful).