Extension method support to move us closer to a valid List implementation.

Extension method support to move us closer to a valid List implementation.

BUG=
R=jmesserly@google.com

Committed: https://github.com/dart-lang/dev_compiler/commit/67519d1237191fbeac00b2243bd392d59f6e3866

[Jacob, 2015-04-02 20:07:02]

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

[Jacob, 2015-04-02 20:07:03]

The Array/List class isn't fully hooked up yet.
The codegen for call sites and for base classes looks like it is all generally
done.

https://codereview.chromium.org/1059583002/diff/1/tool/input_sdk/lib/core/ite...
File tool/input_sdk/lib/core/iterable.dart (right):

https://codereview.chromium.org/1059583002/diff/1/tool/input_sdk/lib/core/ite...
tool/input_sdk/lib/core/iterable.dart:34: @SupportJsExtensionMethods()
Similarly we should add the annotation that implementations of this interface
needs to define the api for the JS Iterable class

https://codereview.chromium.org/1059583002/diff/1/tool/input_sdk/lib/core/lis...
File tool/input_sdk/lib/core/list.dart (right):

https://codereview.chromium.org/1059583002/diff/1/tool/input_sdk/lib/core/lis...
tool/input_sdk/lib/core/list.dart:55: @JsPeerInterface(name: 'Array')
the idea for 
@JsPeerInterface
is that all classes that implement List
automatically get a JS mixin that implements the JavaScript 
Array class.
A bit magical but it makes for nice 2 way interop for these classes instead of
interop that is nice in the dart->js direction and ugly in the reverse
direction.
The mixin for Array would look something like

class JsArrayMixin implements List {
  void append(value) { add(value); }
  void slice(?????) { ????; }
  ....
}

[Jennifer Messerly, 2015-04-03 16:25:29]

At a high level, this looks amazing :)

Here are some style/factoring comments. Let me know if I can help with these.

https://codereview.chromium.org/1059583002/diff/1/tool/input_sdk/lib/core/lis...
File tool/input_sdk/lib/core/list.dart (right):

https://codereview.chromium.org/1059583002/diff/1/tool/input_sdk/lib/core/lis...
tool/input_sdk/lib/core/list.dart:55: @JsPeerInterface(name: 'Array')
On 2015/04/02 20:07:03, Jacob wrote:
> the idea for 
> @JsPeerInterface
> is that all classes that implement List
> automatically get a JS mixin that implements the JavaScript 
> Array class.
> A bit magical but it makes for nice 2 way interop for these classes instead of
> interop that is nice in the dart->js direction and ugly in the reverse
> direction.
> The mixin for Array would look something like
> 
> class JsArrayMixin implements List {
>   void append(value) { add(value); }
>   void slice(?????) { ????; }
>   ....
> }

yeah, that's really nice.

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:80: List<InterfaceType> get
_JsExtensionMethodTypes =>
nit: should be lower case _jsExtension...

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:161: 'let # = $_SYMBOL(#);', [new
JS.Identifier(name, allowRename: false), js.string(name, "'")]);
hmm, making this allowRename: false is a bit problematic. It could conflict with
top level items (classes and functions). Why do we need to prevent renaming? It
should be fine ... you just want to export it with the right name. For example:

# some bogus renamed symbol
let map$12345 = Symbol("map");

    class List {
      [map$12345](f) { ... }
    }

# export it with the right name
exports.$map = map$12345;

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:193:
body.addAll(_pendingExtensionMethodNames.map(_initExtensionMethodSymbol));
run formatter?

pub run dart_style:format -w lib/src/codegen/js_codegen.dart


(... if all files are checked in to git, ./tool/presubmit.dart will do it for
you.)

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:528: // TODO(jacobr): why doesn't the generic
i1.isSubTypeOf(i2) work?
did you try rules.isSubTypeOf(i1, i2) ? that will hit our code in
checker/rules.dart

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:557: List<InterfaceType>
getMatchingExtensionMethodTypes(InterfaceType type) {
is this method just:
    _JsExtensionMethodTypes.where((t) => _isInterfaceSubTypeOf(type, t));

(it looks like you only use the iterable API, so toList can be skipped to)

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:567: LibraryElement
lookupExtensionLibrary(Iterable<InterfaceType> extensionTypes, String name,
{bool isGetter: false, bool isSetter: false}) {
i'd probably use "get" instead of "lookup" for consistency with analyzer APIs.
also makes it consistent with getMatchingExtensionMethodTypes above.

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:570: for (var extensionType in extensionTypes) {
I'd probably just name this "type" and "extensionLibrary" -> "library" ... that
they deal with extensions is included in the context of this method

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:573: match = extensionType.getGetter(name);
I was looking at the package:analyzer implementation just now, noticed a few
things:
* these are all linear searches in analyzer. We might need to cache, as this
lookup will be done frequently.
* InterfaceType.methods is just `type.element.methods` but with generic type
substitution. That's probably just extra overhead for us. I bet we could just do
type.element.methods.
* we could simplify the call sites and just look up by name. I don't think we
need isGetter/isSetter.

with those changes it would be something like:

    /// Maps class element to its extension methods.
    final _extensionMethods = new HashMap<ClassElement, HashSet<String>>();

    _initExtensionMethods() {      
      for (var type in _jsExtensionMethodTypes) {
        var names = new HashSet<String>();
        var e = type.element;
        names.addAll(e.methods.map((m) => m.name));
        names.addAll(e.accessors.map((m) => m.name));
       _extensionMethods[e] = names;
      }
    }

    LibraryElement getExtensionLibrary(Iterable<InterfaceType> types, String
name) { 
      LibraryElement library = null;
      for (var type in types) {
        var element = type.element;
        if (_extensionMethods[element].contains(name)) {
          assert(library == null || library == element.library);
          library = element.library;
        }
      }
      return library;
    }

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:591: JS.Expression nameIfExtension(DartType
targetType, String name, {bool isGetter: false, bool isSetter: false}) {
everywhere we call this we do rules.getStaticType(expr) ... maybe just take an
expr instead?

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:594: if (extensionLibrary == null) return null;
from the places we call this, we end up with code like:
    methodName != null ? methodName: js.string('set')

can we move that logic inside this method? And just make this method handle all
member names, extension or not? It seems like this method could be merged with
_jsMemberName

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:615:
jsMethods.add(_visitMethodDeclaration(member, extensionTypes));
one thing to watch out for: _visit will associate a source span, whereas
directly calling the method won't. That said, I think it's fine in this case.
Source spans are more needed for Statements, where the user could put a
breakpoint.

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:950: JS.Method
_visitMethodDeclaration(MethodDeclaration node, List<InterfaceType>
extensionTypes) {
_emitMethodDeclaration? At some point we sort of started a convention that
"visit" was for the tree visit methods, and _emit* was other methods that return
JS ASts. That said, maybe it isn't a useful convention and we should just go
_visit.

Another random thought for after this CL, should we get rid of "_" prefixes
everywhere? I've noticed that almost all of this file is private API, and using
so much "_" starts to get ugly. We're already in lib/src/... so it would be fine
to just go public names for methods (maybe leave fields private?)

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:960: if (node.isStatic == false &&
!node.isStatic

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:961: (extensionLibrary =
lookupExtensionLibrary(extensionTypes, node.name.name, isGetter: node.isGetter,
isSetter: node.isSetter)) != null) {
style wise, I try to avoid assignment expression in "if". I'd probably refactor
this as:

if (!node.isStatic) {
  var extensionLibrary = lookupExtensionLibrary(extensionTypes, node.name.name,
i
      sGetter: node.isGetter, isSetter: node.isSetter);
  if (extensionLibrary != null) {
  ...
}

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1174: var result =
_emitDSetIfDynamic(_getTarget(lhs), lhs.propertyName, rhs);
ideas: _tryEmitDynamicSet? _maybeEmitDynamicSet? _handleDynamicSet?

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:2265: String _transformMemberNameHelper(String
name,
maybe call this one _jsMemberName, and the other one _emitMemberName? (since
_emit methods return JS.Nodes)

https://codereview.chromium.org/1059583002/diff/40001/tool/input_sdk/lib/core...
File tool/input_sdk/lib/core/annotations.dart (right):

https://codereview.chromium.org/1059583002/diff/40001/tool/input_sdk/lib/core...
tool/input_sdk/lib/core/annotations.dart:28: class SupportJsExtensionMethod {
supports?

[Jennifer Messerly, 2015-04-03 16:28:28]

oops forgot one comment

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:2283: // methods names and regular names.
I think if you change _initExtensionMethodSymbol to be `new JSTemporary(name)`
instead of `new JS.Identifier(name, allowRename: false)` then this TODO goes
away :)

JSTemporaries are for all local variables introduced into the JS code by the
compiler. They'll automatically be renamed to avoid conflicts.

[vsm, 2015-04-03 16:41:19]

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

[vsm, 2015-04-03 16:41:20]

nice!

https://codereview.chromium.org/1059583002/diff/40001/test/generated_sdk/lib/...
File test/generated_sdk/lib/core/annotations.dart (right):

https://codereview.chromium.org/1059583002/diff/40001/test/generated_sdk/lib/...
test/generated_sdk/lib/core/annotations.dart:9: class JsName {
Can you explain the use case for this annotation?

https://codereview.chromium.org/1059583002/diff/40001/tool/input_sdk/lib/core...
File tool/input_sdk/lib/core/annotations.dart (right):

https://codereview.chromium.org/1059583002/diff/40001/tool/input_sdk/lib/core...
tool/input_sdk/lib/core/annotations.dart:26: /// Whether an interface is allowed
to have JavaScript implementations that
Perhaps: A Dart interface may only be implemented by a native JavaScript object
if it is marked with this annotation.

Is that correct?

https://codereview.chromium.org/1059583002/diff/40001/tool/input_sdk/lib/core...
File tool/input_sdk/lib/core/list.dart (right):

https://codereview.chromium.org/1059583002/diff/40001/tool/input_sdk/lib/core...
tool/input_sdk/lib/core/list.dart:57: // Method that needs to be mixed in to all
Dart subclasses that implement
I don't understand this comment.

[Jacob, 2015-04-03 20:25:59]

ptal

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:80: List<InterfaceType> get
_JsExtensionMethodTypes =>
On 2015/04/03 16:25:28, John Messerly wrote:
> nit: should be lower case _jsExtension...

Done.

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:161: 'let # = $_SYMBOL(#);', [new
JS.Identifier(name, allowRename: false), js.string(name, "'")]);
On 2015/04/03 16:25:28, John Messerly wrote:
> hmm, making this allowRename: false is a bit problematic. It could conflict
with
> top level items (classes and functions). Why do we need to prevent renaming?
It
> should be fine ... you just want to export it with the right name. For
example:
> 
> # some bogus renamed symbol
> let map$12345 = Symbol("map");
> 
>     class List {
>       [map$12345](f) { ... }
>     }
> 
> # export it with the right name
> exports.$map = map$12345;

Talked offline. Added checks when we try to add the same name to export twice.
Long term the solution is we need static errors if extension methods and top
level names conflict. If dart adds extension method support to the core language
this would probably be a general restriction.

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:193:
body.addAll(_pendingExtensionMethodNames.map(_initExtensionMethodSymbol));
On 2015/04/03 16:25:28, John Messerly wrote:
> run formatter?
> 
> pub run dart_style:format -w lib/src/codegen/js_codegen.dart
> 
> 
> (... if all files are checked in to git, ./tool/presubmit.dart will do it for
> you.)

Done.

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:528: // TODO(jacobr): why doesn't the generic
i1.isSubTypeOf(i2) work?
On 2015/04/03 16:25:28, John Messerly wrote:
> did you try rules.isSubTypeOf(i1, i2) ? that will hit our code in
> checker/rules.dart

That doesn't work. The output changes dramatically and incorrectly if I change
that.
Likely because of the generic type arguments.

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:557: List<InterfaceType>
getMatchingExtensionMethodTypes(InterfaceType type) {
On 2015/04/03 16:25:28, John Messerly wrote:
> is this method just:
>     _JsExtensionMethodTypes.where((t) => _isInterfaceSubTypeOf(type, t));
> 
> (it looks like you only use the iterable API, so toList can be skipped to)

done

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:567: LibraryElement
lookupExtensionLibrary(Iterable<InterfaceType> extensionTypes, String name,
{bool isGetter: false, bool isSetter: false}) {
On 2015/04/03 16:25:28, John Messerly wrote:
> i'd probably use "get" instead of "lookup" for consistency with analyzer APIs.
> also makes it consistent with getMatchingExtensionMethodTypes above.

Done.

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:570: for (var extensionType in extensionTypes) {
On 2015/04/03 16:25:28, John Messerly wrote:
> I'd probably just name this "type" and "extensionLibrary" -> "library" ...
that
> they deal with extensions is included in the context of this method

Done.

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:573: match = extensionType.getGetter(name);
On 2015/04/03 16:25:28, John Messerly wrote:
> I was looking at the package:analyzer implementation just now, noticed a few
> things:
> * these are all linear searches in analyzer. We might need to cache, as this
> lookup will be done frequently.
> * InterfaceType.methods is just `type.element.methods` but with generic type
> substitution. That's probably just extra overhead for us. I bet we could just
do
> type.element.methods.
> * we could simplify the call sites and just look up by name. I don't think we
> need isGetter/isSetter.
> 
> with those changes it would be something like:
> 
>     /// Maps class element to its extension methods.
>     final _extensionMethods = new HashMap<ClassElement, HashSet<String>>();
> 
>     _initExtensionMethods() {      
>       for (var type in _jsExtensionMethodTypes) {
>         var names = new HashSet<String>();
>         var e = type.element;
>         names.addAll(e.methods.map((m) => m.name));
>         names.addAll(e.accessors.map((m) => m.name));
>        _extensionMethods[e] = names;
>       }
>     }
> 
>     LibraryElement getExtensionLibrary(Iterable<InterfaceType> types, String
> name) { 
>       LibraryElement library = null;
>       for (var type in types) {
>         var element = type.element;
>         if (_extensionMethods[element].contains(name)) {
>           assert(library == null || library == element.library);
>           library = element.library;
>         }
>       }
>       return library;
>     }
> 

Done.

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:591: JS.Expression nameIfExtension(DartType
targetType, String name, {bool isGetter: false, bool isSetter: false}) {
On 2015/04/03 16:25:28, John Messerly wrote:
> everywhere we call this we do rules.getStaticType(expr) ... maybe just take an
> expr instead?

Done.

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:594: if (extensionLibrary == null) return null;
On 2015/04/03 16:25:28, John Messerly wrote:
> from the places we call this, we end up with code like:
>     methodName != null ? methodName: js.string('set')
> 
> can we move that logic inside this method? And just make this method handle
all
> member names, extension or not? It seems like this method could be merged with
> _jsMemberName

There are a lot of places we call emitMemberName (new name for _jsMemberName
where extensions are not relevant. I'm not sure it would be a net negative or
positive to merge the logic together. We'd have to pass in a null target to a
lot of places 

Good point... now that we have gotten rid of distinguishing based on
getter/setter/method this is trivial

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:615:
jsMethods.add(_visitMethodDeclaration(member, extensionTypes));
On 2015/04/03 16:25:28, John Messerly wrote:
> one thing to watch out for: _visit will associate a source span, whereas
> directly calling the method won't. That said, I think it's fine in this case.
> Source spans are more needed for Statements, where the user could put a
> breakpoint.

good to know

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:950: JS.Method
_visitMethodDeclaration(MethodDeclaration node, List<InterfaceType>
extensionTypes) {
On 2015/04/03 16:25:28, John Messerly wrote:
> _emitMethodDeclaration? At some point we sort of started a convention that
> "visit" was for the tree visit methods, and _emit* was other methods that
return
> JS ASts. That said, maybe it isn't a useful convention and we should just go
> _visit.
> 
> Another random thought for after this CL, should we get rid of "_" prefixes
> everywhere? I've noticed that almost all of this file is private API, and
using
> so much "_" starts to get ugly. We're already in lib/src/... so it would be
fine
> to just go public names for methods (maybe leave fields private?)

_might _as _well _remove _the _underscores as a _followup _cl. _they _are _not
_bothering_ _me _much _ but_ don't add_ value

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:960: if (node.isStatic == false &&
On 2015/04/03 16:25:28, John Messerly wrote:
> !node.isStatic

Done.

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:961: (extensionLibrary =
lookupExtensionLibrary(extensionTypes, node.name.name, isGetter: node.isGetter,
isSetter: node.isSetter)) != null) {
On 2015/04/03 16:25:28, John Messerly wrote:
> style wise, I try to avoid assignment expression in "if". I'd probably
refactor
> this as:
> 
> if (!node.isStatic) {
>   var extensionLibrary = lookupExtensionLibrary(extensionTypes,
node.name.name,
> i
>       sGetter: node.isGetter, isSetter: node.isSetter);
>   if (extensionLibrary != null) {
>   ...
> }

Done.

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:1174: var result =
_emitDSetIfDynamic(_getTarget(lhs), lhs.propertyName, rhs);
On 2015/04/03 16:25:28, John Messerly wrote:
> ideas: _tryEmitDynamicSet? _maybeEmitDynamicSet? _handleDynamicSet?

Done.

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:2265: String _transformMemberNameHelper(String
name,
On 2015/04/03 16:25:28, John Messerly wrote:
> maybe call this one _jsMemberName, and the other one _emitMemberName? (since
> _emit methods return JS.Nodes)
that is better. done

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:2283: // methods names and regular names.
On 2015/04/03 16:28:27, John Messerly wrote:
> I think if you change _initExtensionMethodSymbol to be `new JSTemporary(name)`
> instead of `new JS.Identifier(name, allowRename: false)` then this TODO goes
> away :)
> 
> JSTemporaries are for all local variables introduced into the JS code by the
> compiler. They'll automatically be renamed to avoid conflicts.

Done.

https://codereview.chromium.org/1059583002/diff/40001/test/generated_sdk/lib/...
File test/generated_sdk/lib/core/annotations.dart (right):

https://codereview.chromium.org/1059583002/diff/40001/test/generated_sdk/lib/...
test/generated_sdk/lib/core/annotations.dart:9: class JsName {
On 2015/04/03 16:41:20, vsm wrote:
> Can you explain the use case for this annotation?


example

// The d3 library is defined off a JavaScript object named d3
@JsName(name: "d3")
library d3_js;


// We choose to pretend that the type HTMLInputElement is named InputElement
@JsName('HTMLInputElement')
class InputElement {
...
}

https://codereview.chromium.org/1059583002/diff/40001/tool/input_sdk/lib/core...
File tool/input_sdk/lib/core/annotations.dart (right):

https://codereview.chromium.org/1059583002/diff/40001/tool/input_sdk/lib/core...
tool/input_sdk/lib/core/annotations.dart:26: /// Whether an interface is allowed
to have JavaScript implementations that
On 2015/04/03 16:41:20, vsm wrote:
> Perhaps: A Dart interface may only be implemented by a native JavaScript
object
> if it is marked with this annotation.
> 
> Is that correct?

That is correct and clearer. Done.

https://codereview.chromium.org/1059583002/diff/40001/tool/input_sdk/lib/core...
tool/input_sdk/lib/core/annotations.dart:28: class SupportJsExtensionMethod {
On 2015/04/03 16:25:29, John Messerly wrote:
> supports?

This annotation name is bad. Got any better ideas?
Maybe 
@JsExtensionMethodImplementation
or
@NativeJsImplementation
?

https://codereview.chromium.org/1059583002/diff/40001/tool/input_sdk/lib/core...
File tool/input_sdk/lib/core/list.dart (right):

https://codereview.chromium.org/1059583002/diff/40001/tool/input_sdk/lib/core...
tool/input_sdk/lib/core/list.dart:57: // Method that needs to be mixed in to all
Dart subclasses that implement
On 2015/04/03 16:41:20, vsm wrote:
> I don't understand this comment.

removed.

[Jennifer Messerly, 2015-04-03 21:08:06]

I got a suggestion to fix the TODO in _isInterfaceSubTypeOf, otherwise LGTM.

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:528: // TODO(jacobr): why doesn't the generic
i1.isSubTypeOf(i2) work?
On 2015/04/03 20:25:59, Jacob wrote:
> On 2015/04/03 16:25:28, John Messerly wrote:
> > did you try rules.isSubTypeOf(i1, i2) ? that will hit our code in
> > checker/rules.dart
> 
> That doesn't work. The output changes dramatically and incorrectly if I change
> that.
> Likely because of the generic type arguments.

Doh! Well it'd be good to figure out. Something seems a bit off about writing
our own subtype check as a workaround =)

so I started debugging this by adding prints to visitClassDeclaration, and this
one seemed to work:
    var extType = rules.provider.iterableDynamicType;
    if (node.element.type.isSubtypeOf(extType)) {
      print('!!! found ${extType} subtype ${node.element.type}');
    }

does that match your code?

they make that via: _iterableDynamicType =
_iterableType.substitute4(<DartType>[_dynamicType]);
so you can make one for List<dynamic>:

var listDynamicType =
rules.provider.listType.substitute4([rules.provider.dynamicType]);

we could alternatively make a helper:

InterfaceType _fillDynamicTypeArgs(InterfaceType t) {
  var d = rules.provider.dynamicType;
  return t.substitute4(new List.filled(t.typeArguments.length, d));
}


but probably not worth it with just 1 type that needs it.

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:950: JS.Method
_visitMethodDeclaration(MethodDeclaration node, List<InterfaceType>
extensionTypes) {
On 2015/04/03 20:25:58, Jacob wrote:
> On 2015/04/03 16:25:28, John Messerly wrote:
> > _emitMethodDeclaration? At some point we sort of started a convention that
> > "visit" was for the tree visit methods, and _emit* was other methods that
> return
> > JS ASts. That said, maybe it isn't a useful convention and we should just go
> > _visit.
> > 
> > Another random thought for after this CL, should we get rid of "_" prefixes
> > everywhere? I've noticed that almost all of this file is private API, and
> using
> > so much "_" starts to get ugly. We're already in lib/src/... so it would be
> fine
> > to just go public names for methods (maybe leave fields private?)
> 
> _might _as _well _remove _the _underscores as a _followup _cl. _they _are _not
> _bothering_ _me _much _ but_ don't add_ value

haha :)

[vsm, 2015-04-03 21:15:36]

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
File lib/src/codegen/js_codegen.dart (right):

https://codereview.chromium.org/1059583002/diff/40001/lib/src/codegen/js_code...
lib/src/codegen/js_codegen.dart:528: // TODO(jacobr): why doesn't the generic
i1.isSubTypeOf(i2) work?
On 2015/04/03 21:08:05, John Messerly wrote:
> On 2015/04/03 20:25:59, Jacob wrote:
> > On 2015/04/03 16:25:28, John Messerly wrote:
> > > did you try rules.isSubTypeOf(i1, i2) ? that will hit our code in
> > > checker/rules.dart
> > 
> > That doesn't work. The output changes dramatically and incorrectly if I
change
> > that.
> > Likely because of the generic type arguments.
> 
> Doh! Well it'd be good to figure out. Something seems a bit off about writing
> our own subtype check as a workaround =)
> 
> so I started debugging this by adding prints to visitClassDeclaration, and
this
> one seemed to work:
>     var extType = rules.provider.iterableDynamicType;
>     if (node.element.type.isSubtypeOf(extType)) {
>       print('!!! found ${extType} subtype ${node.element.type}');
>     }
> 
> does that match your code?
> 
> they make that via: _iterableDynamicType =
> _iterableType.substitute4(<DartType>[_dynamicType]);
> so you can make one for List<dynamic>:
> 
> var listDynamicType =
> rules.provider.listType.substitute4([rules.provider.dynamicType]);
> 
> we could alternatively make a helper:
> 
> InterfaceType _fillDynamicTypeArgs(InterfaceType t) {
>   var d = rules.provider.dynamicType;
>   return t.substitute4(new List.filled(t.typeArguments.length, d));
> }
> 
> 
> but probably not worth it with just 1 type that needs it.
> 
> 

It'd be good to use the rules.isSubTypeOf.  I suspect - as John's getting at -
the problem is that i2 is the generic version of the type - Iterable<T> - and
not the dynamic version - Iterable<dynamic>.

[Jacob, 2015-04-03 21:41:26]

ptal. Substituted dynamic args and switched to rules.isSubTypeOf

[Jacob, 2015-04-03 21:52:09]

Committed patchset #7 (id:120001) manually as
67519d1237191fbeac00b2243bd392d59f6e3866 (presubmit successful).

[Jennifer Messerly, 2015-04-03 22:09:55]

Looks great!