support the JS builtin

lib/src/checker/resolver.dart

 // Copyright (c) 2015, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 /// Encapsulates how to invoke the analyzer resolver and overrides how it
 /// computes types on expressions to use our restricted set of types.
 library ddc.src.checker.resolver;
 
 import 'package:analyzer/analyzer.dart';
 import 'package:analyzer/src/generated/ast.dart';
@@ skipping 345 matching lines @@
     // otherLib.visibleLibraries.contains(thisLib), however because we are not
     // inferring type on any library that belongs to a cycle or that contains
     // parts, we know that this cannot be true.
     return thisLib == otherLib;
   }
 
   @override // to propagate types to identifiers
   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
   // handled by the analyzer internally.
   // TODO(vsm): in visitbinaryExpression: check computeStaticReturnType result?
   // TODO(vsm): in visitConditionalExpression: check... LUB in rules?
   // TODO(vsm): in visitFunctionDeclaration: Should we ever use the expression
   // type in a (...) => expr or just the written type?
 
 }
@@ skipping 42 matching lines @@
         element.returnType.isDynamic &&
         node.returnType == null) {
       var type = searchTypeFor(element.enclosingElement.type, element);
       if (type != null && !type.returnType.isDynamic) {
         element.returnType = type.returnType;
       }
     }
     return res;
   }
 }