Wrapperless dart:html and friends

lib/src/codegen/js_codegen.dart

Index: lib/src/codegen/js_codegen.dart
diff --git a/lib/src/codegen/js_codegen.dart b/lib/src/codegen/js_codegen.dart
index 663786a19e0ae958168aef9fda664f2a612da0fa..ec7a4d39a05ade92044b7205b9ddee296ab4cc74 100644
--- a/lib/src/codegen/js_codegen.dart
+++ b/lib/src/codegen/js_codegen.dart
@@ -669,6 +669,34 @@ class JSCodegenVisitor extends GeneralizingAstVisitor
           hasIterator = true;
           jsMethods.add(_emitIterable(type));
         }
+      } else if (m is FieldDeclaration) {
+        // TODO(vsm): Refactor common logic out.

[Jennifer Messerly, 2016/02/16 17:46:38]

yeah if this was in a method like _emitRenamedNativeField, with a doc comment,
it'd be good. The logic makes sense.

BTW, in the past we've tended to care about code size & readability, so these
type of metaprogramming things are often generated via a helper:

    dart.renameField(HTMLElement, 'elementList', 'elements');

Whether that's the long term direction, I'm not sure. All of the existing
transpilers do use helpers to make the code more readable, though. Even though
it does make Closure advanced optimizations unhappy, I'd rather have
closure-advanced specific lowering.

Only other thing I'd check is if this can share any logic with existing
vitrualField code path.

[vsm, 2016/02/18 01:13:09]

Refactored and added a TODO to look into metaprogramming.  Another advantage of
your metaprogramming suggestion is code size.  These added methods are fed later
into dart.registerExtensions - perhaps it makes sense to just install there
directly?

+        if (!m.isStatic && _extensionTypes.contains(element)) {
+          for (var decl in m.fields.variables) {
+            FieldElement field = decl.element;

[Jacob, 2016/02/16 17:47:35]

nit: change these to variables to var?

[vsm, 2016/02/18 01:13:09]

Done.

+            String name = decl.name.name;
+            var annotation = findAnnotation(field, isJsName);
+            if (annotation != null) {
+              name = getConstantField(annotation, 'name', types.stringType)
+                  ?.toStringValue();
+            }
+            // Generate getter
+            var fn = new JS.Fun([], js.statement('{ return this.#; }', [name]));
+            var method = new JS.Method(_elementMemberName(field.getter), fn,
+                isGetter: true);
+            jsMethods.add(method);
+
+            // Generate setter
+            if (!decl.isFinal) {
+              var value = new JS.TemporaryId('value');
+              fn = new JS.Fun(
+                  [value], js.statement('{ this.# = #; }', [name, value]));
+              method = new JS.Method(_elementMemberName(field.setter), fn,
+                  isSetter: true);
+              jsMethods.add(method);
+            }
+          }
+        }
       }
     }
 
@@ -769,6 +797,16 @@ class JSCodegenVisitor extends GeneralizingAstVisitor
           dartxNames.add(_elementMemberName(m.element, allowExtensions: false));
         }
       }
+      for (var f in fields) {
+        if (!f.isStatic) {
+          for (var d in f.fields.variables) {
+            if (d.element.isPublic) {
+              dartxNames.add(
+                  _elementMemberName(d.element.getter, allowExtensions: false));
+            }
+          }
+        }
+      }
       if (dartxNames.isNotEmpty) {
         body.add(js.statement('dart.defineExtensionNames(#)',
             [new JS.ArrayInitializer(dartxNames, multiline: true)]));
@@ -965,8 +1003,13 @@ class JSCodegenVisitor extends GeneralizingAstVisitor
 
   JS.Statement _overrideField(FieldElement e) {
     var cls = e.enclosingElement;
-    return js.statement('dart.virtualField(#, #)',
-        [cls.name, _emitMemberName(e.name, type: cls.type)]);
+    var name;
+    if (_extensionTypes.contains(cls)) {

[Jennifer Messerly, 2016/02/16 17:46:38]

eeep. This is a little bit sketchy :). We try and make sure all extension names
are already defined by the library we're importing.

if the extension symbol isn't defined previously at the top-level, what
guarantee do we have that it's getting defined for the other code that uses it,
like method bodies?

my intuition here is that this might be papering over a problem you were seeing,
but there's still a bug that can be hit in other ways...

[vsm, 2016/02/18 01:13:09]

Hmm, seems to work now if I revert.  I had probably temporarily messed this up
somehow.  Thanks for the catch!

+      name = js.call('dart.getExtensionSymbol(#)', js.string(e.name));
+    } else {
+      name = _emitMemberName(e.name, type: cls.type);
+    }

[Jacob, 2016/02/16 17:47:35]

nit: consider

 var name = _extensionTypes.contains(cls) ?
    js.call('dart.getExtensionSymbol(#)', js.string(e.name)) :
    _emitMemberName(e.name, type: cls.type);

[vsm, 2016/02/18 01:13:09]

Removed change altogether.

+    return js.statement('dart.virtualField(#, #)', [cls.name, name]);
   }
 
   /// Generates the implicit default constructor for class C of the form
@@ -1328,9 +1371,8 @@ class JSCodegenVisitor extends GeneralizingAstVisitor
     }
   }
 
-  JS.Fun _emitNativeFunctionBody(
-      List<JS.Parameter> params, List<JS.Expression> paramRefs,
-      MethodDeclaration node) {
+  JS.Fun _emitNativeFunctionBody(List<JS.Parameter> params,
+      List<JS.Expression> paramRefs, MethodDeclaration node) {
     if (node.isStatic) {
       // TODO(vsm): Do we need to handle this case?
       return null;
@@ -1565,8 +1607,8 @@ class JSCodegenVisitor extends GeneralizingAstVisitor
       var body = node.body;
       if (body.isGenerator || body.isAsynchronous) {
         var paramRefs = _emitParameterReferences(node.parameters);
-        jsBody = _emitGeneratorFunctionBody(
-            params, paramRefs, body, returnType);
+        jsBody =
+            _emitGeneratorFunctionBody(params, paramRefs, body, returnType);
       } else if (body is ExpressionFunctionBody) {
         jsBody = _visit(body.expression);
       } else {
@@ -1589,9 +1631,12 @@ class JSCodegenVisitor extends GeneralizingAstVisitor
     }
   }
 
-  JS.Fun _emitFunctionBody(List<JS.Parameter> params,
-      List<JS.Expression> paramRefs, FunctionBody body,
-      List<JS.Identifier> typeParams, JS.TypeRef returnType) {
+  JS.Fun _emitFunctionBody(
+      List<JS.Parameter> params,
+      List<JS.Expression> paramRefs,
+      FunctionBody body,
+      List<JS.Identifier> typeParams,
+      JS.TypeRef returnType) {
     // sync*, async, async*
     if (body.isAsynchronous || body.isGenerator) {
       // TODO(ochafik): Refine params: we don't need default values in the
@@ -1609,9 +1654,8 @@ class JSCodegenVisitor extends GeneralizingAstVisitor
         typeParams: typeParams, returnType: returnType);
   }
 
-  JS.Expression _emitGeneratorFunctionBody(
-      List<JS.Parameter> params, List<JS.Expression> paramRefs,
-      FunctionBody body, JS.TypeRef returnType) {
+  JS.Expression _emitGeneratorFunctionBody(List<JS.Parameter> params,
+      List<JS.Expression> paramRefs, FunctionBody body, JS.TypeRef returnType) {
     var kind = body.isSynchronous ? 'sync' : 'async';
     if (body.isGenerator) kind += 'Star';
 
@@ -2180,13 +2224,12 @@ class JSCodegenVisitor extends GeneralizingAstVisitor
   // TODO(ochafik): Decouple Parameter from Identifier.
   List<JS.Expression> _emitParameterReferences(FormalParameterList node) =>
       node == null
-      ? <JS.Expression>[]
-      : _emitFormalParameterList(node, allowDestructuring: false)
-          .map((JS.Parameter p) {
-            if (p is JS.RestParameter) return new JS.Spread(p.parameter);
-            return p as JS.Identifier;
-          })
-          .toList();
+          ? <JS.Expression>[]
+          : _emitFormalParameterList(node, allowDestructuring: false)
+              .map((JS.Parameter p) {
+              if (p is JS.RestParameter) return new JS.Spread(p.parameter);
+              return p as JS.Identifier;
+            }).toList();
 
   List<JS.Parameter> _emitFormalParameterList(FormalParameterList node,
       {bool allowDestructuring: true}) {
@@ -2221,11 +2264,10 @@ class JSCodegenVisitor extends GeneralizingAstVisitor
         }
       } else {
         var jsParam = _visit(param);
-        result.add(
-            param is DefaultFormalParameter && destructure
-                ? new JS.DestructuredVariable(
-                    name: jsParam, defaultValue: _defaultParamValue(param))
-                : jsParam);
+        result.add(param is DefaultFormalParameter && destructure
+            ? new JS.DestructuredVariable(
+                name: jsParam, defaultValue: _defaultParamValue(param))
+            : jsParam);
       }
     }
 
@@ -2990,7 +3032,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor
 
   bool _requiresStaticDispatch(Expression target, String memberName) {
     var type = getStaticType(target);
-    if (!_isObjectProperty(memberName)) {
+    if (!_isObjectProperty(memberName) || target is SuperExpression) {

[Jennifer Messerly, 2016/02/16 17:46:38]

nice fix.

[Jacob, 2016/02/16 17:47:35]

Why does target is SuperExpression require static dispatch?
Naively would have thought 
super.dispatch could work dynamically by just following the JS prototype chain?

[Jennifer Messerly, 2016/02/16 18:22:00]

Yup, that's what the CL is fixing, if I understand correctly. The bug was:

// Dart
class Foo extends Object {
  toString() => super.toString();
}

we were generating this incorrectly:

// JS
class Foo extends core.Object {
  toString() { return dart.toString(super); }
}

Vijay's change makes sure that "super." never uses static dispatch. So it ends
up as:

// JS
class Foo extends core.Object {
  toString() { return super.toString(); }
}

... which is correct. BTW, the reason "Object" methods generate like this is so
we can handle null correctly. But for "super." null is not possible.

BTW, Vijay -- I'm kind of surprised !_isNullable(target) below doesn't handle
this. We should know that "super" cannot be null (same with "this") ... so maybe
there is a bug here, although not necessarily one you should try to fix unless
you're motivated. Discussed more below.

thanks Jacob for making me take another look

       return false;
     }
     if (!type.isObject &&

[Jennifer Messerly, 2016/02/16 18:22:01]

wow, this code is confusing. Can we rewrite it like?

// In these cases, the target could be `null` so we need static dispatch.
return type.isObject ||
     _isJSBuiltinType(type) ||
     _extensionTypes.contains(type.element) ||
     _isNullable(target);


... now it's clearer, but it seems totally wrong :)

1. Why check for _isJSBuiltinType and _extensionTypes? That seems redundant.
2. Why do extension types always use static dispatch for object members?
3. Why is type.isObject a special case anyway?

Anyway, for 2, I think it's related to other brokenness in
https://github.com/dart-lang/dev_compiler/issues/451...

But yeah, what a mess. Here's what the code should be:

    return _isNullable(target);

Anyway I'm fine with the "super" workaround in the meantime. I guess because
extension methods fall into the wrong path, the existing _isNullable check
doesn't help.

[vsm, 2016/02/18 01:13:09]

Excellent point - cleaned up code!  The explicit super check was just hacking
around the problem you noted.  :-)

[vsm, 2016/02/18 15:23:55]

Ugh, had to back off this a little bit.  We don't symbolize hashCode.  E.g., we
compile "hello".hashCode to "hello".hashCode with the intermediate change.  This
puts it back to dart.hashCode("hello").

@@ -3586,6 +3628,7 @@ class JSCodegenVisitor extends GeneralizingAstVisitor
       baseType = baseType.element.bound;
     }
     if (allowExtensions &&
+        baseType != null &&
         _extensionTypes.contains(baseType.element) &&
         !_isObjectProperty(name)) {
       return js.call('dartx.#', _propertyName(name));
@@ -3732,6 +3775,12 @@ class JSGenerator extends CodeGenerator {
     var finder = new _ExtensionFinder(context, _extensionTypes, _types);
     finder._addExtensionTypes('dart:_interceptors');
     finder._addExtensionTypes('dart:_native_typed_data');
+    finder._addExtensionTypes('dart:html');
+    finder._addExtensionTypes('dart:indexed_db');
+    finder._addExtensionTypes('dart:svg');
+    finder._addExtensionTypes('dart:web_audio');
+    finder._addExtensionTypes('dart:web_gl');
+    finder._addExtensionTypes('dart:web_sql');
 
     // TODO(vsm): If we're analyzing against the main SDK, those
     // types are not explicitly annotated.