support the JS builtin

support the JS builtin

* fix a few cases where we regressed js_ast parser interpolations
* infer correct static type for JS call expressions

R=vsm@google.com

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

[Jennifer Messerly, 2015-02-27 03:10:21]

Patchset #1 (id:1) has been deleted

[Jennifer Messerly, 2015-02-27 03:10:56]

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

[Jennifer Messerly, 2015-02-27 03:10:57]

baby steps :)

[Jennifer Messerly, 2015-02-27 14:59:23]

https://codereview.chromium.org/962083002/diff/40001/test/codegen/expect/coll...
File test/codegen/expect/collection/collection.js (right):

https://codereview.chromium.org/962083002/diff/40001/test/codegen/expect/coll...
test/codegen/expect/collection/collection.js:218: return
dart.notNull(dart.is(key, core.num)) && dart.notNull((key & 0x3ffffff) === key);
filed https://github.com/dart-lang/dev_compiler/issues/68 ... the result of `is`
can't be null

https://codereview.chromium.org/962083002/diff/40001/test/codegen/expect/conv...
File test/codegen/expect/convert/convert.js (left):

https://codereview.chromium.org/962083002/diff/40001/test/codegen/expect/conv...
test/codegen/expect/convert/convert.js:9: if
(dart.dbinary(_foreign_helper.JS('bool', '# == null', e), '||',
_foreign_helper.JS('bool', 'typeof # != "object"', e))) {
it'd be nice to understand this is not null, too.

[vsm, 2015-02-27 17:20:05]

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

https://codereview.chromium.org/962083002/diff/40001/lib/src/checker/resolver...
lib/src/checker/resolver.dart:375: node.staticType = node.propagatedType;
We haven't been using propagatedType so far - not clear when we can rely on it
soundly.  In this case, it looks like it boils down to:

library.getType(elementName)

where elementName is the string.  Would that just work here?

https://codereview.chromium.org/962083002/diff/40001/test/codegen/expect/_iso...
File test/codegen/expect/_isolate_helper/_isolate_helper.js (right):

https://codereview.chromium.org/962083002/diff/40001/test/codegen/expect/_iso...
test/codegen/expect/_isolate_helper/_isolate_helper.js:29: args = args;
?

https://codereview.chromium.org/962083002/diff/40001/test/codegen/expect/_iso...
test/codegen/expect/_isolate_helper/_isolate_helper.js:97: let function =
function(f, a) {
note the var named "function"

https://codereview.chromium.org/962083002/diff/40001/test/codegen/expect/coll...
File test/codegen/expect/collection/collection.js (right):

https://codereview.chromium.org/962083002/diff/40001/test/codegen/expect/coll...
test/codegen/expect/collection/collection.js:218: return
dart.notNull(dart.is(key, core.num)) && dart.notNull((key & 0x3ffffff) === key);
On 2015/02/27 14:59:23, John Messerly wrote:
> filed https://github.com/dart-lang/dev_compiler/issues/68 ... the result of
`is`
> can't be null

Commented there.  I think the cleanest solution is some form of non-nullability
at the type level.

[Jennifer Messerly, 2015-02-27 18:27:07]

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

https://codereview.chromium.org/962083002/diff/40001/lib/src/checker/resolver...
lib/src/checker/resolver.dart:375: node.staticType = node.propagatedType;
On 2015/02/27 17:20:05, vsm wrote:
> We haven't been using propagatedType so far - not clear when we can rely on it
> soundly.  In this case, it looks like it boils down to:
> 
> library.getType(elementName)
> 
> where elementName is the string.  Would that just work here?
> 

Analyzer is doing exactly the right logic:
https://github.com/dart-lang/bleeding_edge/blob/master/dart/pkg/analyzer/lib/...

I'm literally just copying the value they computed into the right field.

Our check for 'JS' is even more restrictive than theirs, too.

https://codereview.chromium.org/962083002/diff/40001/test/codegen/expect/_iso...
File test/codegen/expect/_isolate_helper/_isolate_helper.js (right):

https://codereview.chromium.org/962083002/diff/40001/test/codegen/expect/_iso...
test/codegen/expect/_isolate_helper/_isolate_helper.js:29: args = args;
On 2015/02/27 17:20:05, vsm wrote:
> ?

that's what their code is doing. I don't claim to understand it :)

https://codereview.chromium.org/962083002/diff/40001/test/codegen/expect/_iso...
test/codegen/expect/_isolate_helper/_isolate_helper.js:97: let function =
function(f, a) {
On 2015/02/27 17:20:05, vsm wrote:
> note the var named "function"

yup. This issue already exists, it's covered by
https://github.com/dart-lang/dev_compiler/issues/60

[vsm, 2015-02-27 18:39:31]

lgtm

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

https://codereview.chromium.org/962083002/diff/40001/lib/src/checker/resolver...
lib/src/checker/resolver.dart:375: node.staticType = node.propagatedType;
On 2015/02/27 18:27:07, John Messerly wrote:
> On 2015/02/27 17:20:05, vsm wrote:
> > We haven't been using propagatedType so far - not clear when we can rely on
it
> > soundly.  In this case, it looks like it boils down to:
> > 
> > library.getType(elementName)
> > 
> > where elementName is the string.  Would that just work here?
> > 
> 
> Analyzer is doing exactly the right logic:
>
https://github.com/dart-lang/bleeding_edge/blob/master/dart/pkg/analyzer/lib/...
> 
> I'm literally just copying the value they computed into the right field.
> 
> Our check for 'JS' is even more restrictive than theirs, too.

I agree, it's doing the right logic right now.  I'm not generally comfortable
assuming soundness of propagatedType going forward though.  I'd prefer we just
call the underlying function - refactoring where needed.  But that could be a
TODO.

[Jennifer Messerly, 2015-02-27 18:48:09]

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

https://codereview.chromium.org/962083002/diff/40001/lib/src/checker/resolver...
lib/src/checker/resolver.dart:375: node.staticType = node.propagatedType;
On 2015/02/27 18:39:31, vsm wrote:
> On 2015/02/27 18:27:07, John Messerly wrote:
> > On 2015/02/27 17:20:05, vsm wrote:
> > > We haven't been using propagatedType so far - not clear when we can rely
on
> it
> > > soundly.  In this case, it looks like it boils down to:
> > > 
> > > library.getType(elementName)
> > > 
> > > where elementName is the string.  Would that just work here?
> > > 
> > 
> > Analyzer is doing exactly the right logic:
> >
>
https://github.com/dart-lang/bleeding_edge/blob/master/dart/pkg/analyzer/lib/...
> > 
> > I'm literally just copying the value they computed into the right field.
> > 
> > Our check for 'JS' is even more restrictive than theirs, too.
> 
> I agree, it's doing the right logic right now.  I'm not generally comfortable
> assuming soundness of propagatedType going forward though.  I'd prefer we just
> call the underlying function - refactoring where needed.  But that could be a
> TODO.

I'm super confused :|

soundness -- what does that mean for JS(...) builtin? Isn't that up to whoever
writes each JS(...) call?

Assuming soundness of propegatedType ... right, this change does not. This is
*super* restricted to JS builtins *only*. The only sensible type there is the
one the author of the JS builtin wrote.

Apologies if I'm super confused here... am just not seeing what could possibly
be controversial about this.

(I mean, there's the much deeper issue of so much JS(...) in the code ... but
this change surely doesn't make that any worse ... )

[vsm, 2015-02-27 18:58:02]

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

https://codereview.chromium.org/962083002/diff/40001/lib/src/checker/resolver...
lib/src/checker/resolver.dart:375: node.staticType = node.propagatedType;
On 2015/02/27 18:48:09, John Messerly wrote:
> On 2015/02/27 18:39:31, vsm wrote:
> > On 2015/02/27 18:27:07, John Messerly wrote:
> > > On 2015/02/27 17:20:05, vsm wrote:
> > > > We haven't been using propagatedType so far - not clear when we can rely
> on
> > it
> > > > soundly.  In this case, it looks like it boils down to:
> > > > 
> > > > library.getType(elementName)
> > > > 
> > > > where elementName is the string.  Would that just work here?
> > > > 
> > > 
> > > Analyzer is doing exactly the right logic:
> > >
> >
>
https://github.com/dart-lang/bleeding_edge/blob/master/dart/pkg/analyzer/lib/...
> > > 
> > > I'm literally just copying the value they computed into the right field.
> > > 
> > > Our check for 'JS' is even more restrictive than theirs, too.
> > 
> > I agree, it's doing the right logic right now.  I'm not generally
comfortable
> > assuming soundness of propagatedType going forward though.  I'd prefer we
just
> > call the underlying function - refactoring where needed.  But that could be
a
> > TODO.
> 
> I'm super confused :|
> 
> soundness -- what does that mean for JS(...) builtin? Isn't that up to whoever
> writes each JS(...) call?
> 
> Assuming soundness of propegatedType ... right, this change does not. This is
> *super* restricted to JS builtins *only*. The only sensible type there is the
> one the author of the JS builtin wrote.

Sorry for belaboring the point.  :-)  Here's another go at what I'm trying to
say:
- I think what your doing here is more or less fine (hence the lgtm :-)).
- I think a comment in the code along the lines of what you just wrote above
would be helpful - lest someone else get the temptation to use propagatedType
elsewhere.
- Longer term, I'd prefer we just call something like getTypeForJS(...) or
something like that.

> 
> Apologies if I'm super confused here... am just not seeing what could possibly
> be controversial about this.
> 
> (I mean, there's the much deeper issue of so much JS(...) in the code ... but
> this change surely doesn't make that any worse ... )

[Jennifer Messerly, 2015-02-27 19:24:49]

PTAL

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

https://codereview.chromium.org/962083002/diff/40001/lib/src/checker/resolver...
lib/src/checker/resolver.dart:375: node.staticType = node.propagatedType;
On 2015/02/27 18:58:02, vsm wrote:
> On 2015/02/27 18:48:09, John Messerly wrote:
> > On 2015/02/27 18:39:31, vsm wrote:
> > > On 2015/02/27 18:27:07, John Messerly wrote:
> > > > On 2015/02/27 17:20:05, vsm wrote:
> > > > > We haven't been using propagatedType so far - not clear when we can
rely
> > on
> > > it
> > > > > soundly.  In this case, it looks like it boils down to:
> > > > > 
> > > > > library.getType(elementName)
> > > > > 
> > > > > where elementName is the string.  Would that just work here?
> > > > > 
> > > > 
> > > > Analyzer is doing exactly the right logic:
> > > >
> > >
> >
>
https://github.com/dart-lang/bleeding_edge/blob/master/dart/pkg/analyzer/lib/...
> > > > 
> > > > I'm literally just copying the value they computed into the right field.
> > > > 
> > > > Our check for 'JS' is even more restrictive than theirs, too.
> > > 
> > > I agree, it's doing the right logic right now.  I'm not generally
> comfortable
> > > assuming soundness of propagatedType going forward though.  I'd prefer we
> just
> > > call the underlying function - refactoring where needed.  But that could
be
> a
> > > TODO.
> > 
> > I'm super confused :|
> > 
> > soundness -- what does that mean for JS(...) builtin? Isn't that up to
whoever
> > writes each JS(...) call?
> > 
> > Assuming soundness of propegatedType ... right, this change does not. This
is
> > *super* restricted to JS builtins *only*. The only sensible type there is
the
> > one the author of the JS builtin wrote.
> 
> Sorry for belaboring the point.  :-)  Here's another go at what I'm trying to
> say:
> - I think what your doing here is more or less fine (hence the lgtm :-)).
> - I think a comment in the code along the lines of what you just wrote above
> would be helpful - lest someone else get the temptation to use propagatedType
> elsewhere.
> - Longer term, I'd prefer we just call something like getTypeForJS(...) or
> something like that.
> 
> > 
> > Apologies if I'm super confused here... am just not seeing what could
possibly
> > be controversial about this.
> > 
> > (I mean, there's the much deeper issue of so much JS(...) in the code ...
but
> > this change surely doesn't make that any worse ... )
>  
> 

ah, gotcha. yeah I see how someone could misunderstand that.
so I looked at the base class again, and it seems it's only 4 lines after
inline+refactor+tree shaking. WDYT?

[vsm, 2015-02-27 19:27:08]

lgtm!

[Jennifer Messerly, 2015-02-27 19:28:46]

Committed patchset #3 (id:60001) manually as
9d61df70bd406df6c04cf8e2f4268e8284e6523c (presubmit successful).