partially implement instance of checks and some codegen fixes

partially implement instance of checks and some codegen fixes

R=vsm@google.com

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

[Jennifer Messerly, 2015-04-01 00:32:35]

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

[Jennifer Messerly, 2015-04-01 00:32:36]

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

https://codereview.chromium.org/1050723002/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:446: dart.originalDeclaration =
Symbol('originalDeclaration');
better name suggestions?

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

https://codereview.chromium.org/1050723002/diff/1/lib/src/codegen/js_codegen....
lib/src/codegen/js_codegen.dart:1877: // TODO(jmesserly): this doesn't seem like
it will work for JS primtive
this comment is a bit outdated (dart.is should work for primitives). But still
we should reconcile, because visitIsExpression generates a fast path

https://codereview.chromium.org/1050723002/diff/1/lib/src/js/printer.dart
File lib/src/js/printer.dart (right):

https://codereview.chromium.org/1050723002/diff/1/lib/src/js/printer.dart#new...
lib/src/js/printer.dart:619: visitNestedExpression(node.target, ACCESS,
this is important in ES6 at least:

`new foo()()` won't work
`new (foo())()` will

https://codereview.chromium.org/1050723002/diff/1/test/browser/runtime_tests.js
File test/browser/runtime_tests.js (left):

https://codereview.chromium.org/1050723002/diff/1/test/browser/runtime_tests....
test/browser/runtime_tests.js:7: suite('dart_runtime generic', function() {
this test had bit rotted

[Jennifer Messerly, 2015-04-01 00:33:16]

fyi Jacob, if you want to patch this, it has a few codegen fixes that should
help. _internal can now be imported (though I didn't add to dependency_graph)

[Jennifer Messerly, 2015-04-01 00:34:35]

also Vijay: this doesn't track your latest refactorings. I think those mostly
covered function types, but let me know if I need to fix anything here.

(I still want to purge reified dynamic from JS codegen, but that's an
outstanding TODO)

[vsm, 2015-04-01 15:38:00]

lgtm!

Some comments, but feel free to land - we can keep iterating.

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

https://codereview.chromium.org/1050723002/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:99: if (obj == null) return obj;
Can you add a TODO(vsm) here about handling non-nullable types?

https://codereview.chromium.org/1050723002/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:132: function instanceOf(obj, type) {
This needs a test for null:

if (obj == null) {
  return (type == core.Object) || (type == core.Null) || (type == dart.dynamic);
}

(Note: null is T and null as T are inconsistently defined in the spec.)

https://codereview.chromium.org/1050723002/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:178: // Here, we treat dynamic as top - the base
type of everything.
We'll need to update as per recent CL.  Both dynamic and Object should be
recognized as top.

https://codereview.chromium.org/1050723002/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:235: if (isClassSubType(t1.__proto__, t2)) return
true;
This won't cover mixins, right?  The application of the mixin on our proto chain
won't match the original mixin chain.  E.g., x is MapMixin.

https://codereview.chromium.org/1050723002/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:255: // TODO(jmesserly): do we need this function?
We may want to keep this around if we want to reject at runtime.  We'll need to
update to handle functions.

[Jennifer Messerly, 2015-04-01 15:57:32]

thanks! I uploaded a new version but need to fix mixins still + add tests

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

https://codereview.chromium.org/1050723002/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:99: if (obj == null) return obj;
On 2015/04/01 15:38:00, vsm wrote:
> Can you add a TODO(vsm) here about handling non-nullable types?

sure thing :)

https://codereview.chromium.org/1050723002/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:132: function instanceOf(obj, type) {
On 2015/04/01 15:38:00, vsm wrote:
> This needs a test for null:
> 
> if (obj == null) {
>   return (type == core.Object) || (type == core.Null) || (type ==
dart.dynamic);
> }
> 
> (Note: null is T and null as T are inconsistently defined in the spec.)

yeah, I saw that code, but it seems like null is already covered by the normal
logic?

getRuntimeType(null) -> core.Null

isSubtype(core.Null, core.Object) -> true
isSubtype(core.Null, dart.dynamic) -> true
isSubtype(core.Null, core.Null) -> true

the tests already covered Object and dynamic, I added the core.Null test as
well. It all seems to work but maybe I'm missing something?

https://codereview.chromium.org/1050723002/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:178: // Here, we treat dynamic as top - the base
type of everything.
On 2015/04/01 15:38:00, vsm wrote:
> We'll need to update as per recent CL.  Both dynamic and Object should be
> recognized as top.

sounds good. happy to update either now or after your CL lands

https://codereview.chromium.org/1050723002/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:235: if (isClassSubType(t1.__proto__, t2)) return
true;
On 2015/04/01 15:37:59, vsm wrote:
> This won't cover mixins, right?  The application of the mixin on our proto
chain
> won't match the original mixin chain.  E.g., x is MapMixin. 

doh! Good catch. It seems like the Dart code might have a similar issue, I'll
investigate.

https://codereview.chromium.org/1050723002/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:255: // TODO(jmesserly): do we need this function?
On 2015/04/01 15:38:00, vsm wrote:
> We may want to keep this around if we want to reject at runtime.  We'll need
to
> update to handle functions.

sounds good. Updated the comment

[vsm, 2015-04-01 16:29:40]

still lgtm

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

https://codereview.chromium.org/1050723002/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:132: function instanceOf(obj, type) {
On 2015/04/01 15:57:32, John Messerly wrote:
> On 2015/04/01 15:38:00, vsm wrote:
> > This needs a test for null:
> > 
> > if (obj == null) {
> >   return (type == core.Object) || (type == core.Null) || (type ==
> dart.dynamic);
> > }
> > 
> > (Note: null is T and null as T are inconsistently defined in the spec.)
> 
> yeah, I saw that code, but it seems like null is already covered by the normal
> logic?
> 
> getRuntimeType(null) -> core.Null
> 
> isSubtype(core.Null, core.Object) -> true
> isSubtype(core.Null, dart.dynamic) -> true
> isSubtype(core.Null, core.Null) -> true
> 
> the tests already covered Object and dynamic, I added the core.Null test as
> well. It all seems to work but maybe I'm missing something?

The problem is the opposite case:
  isSubtype(core.Null, core.String) -> true
but
  null is String -> false

https://codereview.chromium.org/1050723002/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:235: if (isClassSubType(t1.__proto__, t2)) return
true;
On 2015/04/01 15:57:32, John Messerly wrote:
> On 2015/04/01 15:37:59, vsm wrote:
> > This won't cover mixins, right?  The application of the mixin on our proto
> chain
> > won't match the original mixin chain.  E.g., x is MapMixin. 
> 
> doh! Good catch. It seems like the Dart code might have a similar issue, I'll
> investigate.

Yeah, updating my CL too.  :-)

[Jennifer Messerly, 2015-04-01 16:41:22]

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

https://codereview.chromium.org/1050723002/diff/1/lib/runtime/dart_runtime.js...
lib/runtime/dart_runtime.js:132: function instanceOf(obj, type) {
On 2015/04/01 16:29:40, vsm wrote:
> On 2015/04/01 15:57:32, John Messerly wrote:
> > On 2015/04/01 15:38:00, vsm wrote:
> > > This needs a test for null:
> > > 
> > > if (obj == null) {
> > >   return (type == core.Object) || (type == core.Null) || (type ==
> > dart.dynamic);
> > > }
> > > 
> > > (Note: null is T and null as T are inconsistently defined in the spec.)
> > 
> > yeah, I saw that code, but it seems like null is already covered by the
normal
> > logic?
> > 
> > getRuntimeType(null) -> core.Null
> > 
> > isSubtype(core.Null, core.Object) -> true
> > isSubtype(core.Null, dart.dynamic) -> true
> > isSubtype(core.Null, core.Null) -> true
> > 
> > the tests already covered Object and dynamic, I added the core.Null test as
> > well. It all seems to work but maybe I'm missing something?
> 
> The problem is the opposite case:
>   isSubtype(core.Null, core.String) -> true
> but
>   null is String -> false

chatted about this & added a test case. I think we're okay since our "sane"
isSubtype doesn't special case Null, and Null only extends Object.

[Jennifer Messerly, 2015-04-01 16:44:42]

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