support the JS builtin

lib/src/checker/resolver.dart

Index: lib/src/checker/resolver.dart
diff --git a/lib/src/checker/resolver.dart b/lib/src/checker/resolver.dart
index 5ca4c20dae54e941ea7a27f42d784f9624524cd3..4095a9e1136ca8db63e15bbe627f86ac4d7e103f 100644
--- a/lib/src/checker/resolver.dart
+++ b/lib/src/checker/resolver.dart
@@ -363,7 +363,17 @@ class RestrictedStaticTypeAnalyzer extends StaticTypeAnalyzer {
   visitMethodInvocation(MethodInvocation node) {
     // TODO(sigmund): follow up with analyzer team - why is this needed?
     visitSimpleIdentifier(node.methodName);
-    return super.visitMethodInvocation(node);
+    super.visitMethodInvocation(node);
+
+    // Fix types for JS builtin calls.
+    // Analyzer has already computed them in the call above, we just need to
+    // save it as the method's static return type.
+    var e = node.methodName.staticElement;
+    if (e is FunctionElement &&
+        e.library.name == '_foreign_helper' &&
+        e.name == 'JS') {
+      node.staticType = node.propagatedType;

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

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?

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

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.

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

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]

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]

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.

[Jennifer Messerly, 2015/02/27 19:24:48]

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?

+    }
   }
 
   // Review note: no longer need to override visitFunctionExpression, this is