Function type checking

Function type checking

Replaces http://codereview.chromium.org/8479049/
Rebased with master
Add RTT.createFunction() to handle all functions.  Update code to call RTT.createFunction() directly instead of hooking into the class' lookup. This should save memory as all functions should share the same name, "cls:Function".

FunctionDeclarations do not work.  Some code exists to support in another cl.
Named optional parameters do not work.
FunctionAlias types as paramters do not work.

R=mmendez,zundel,fabiomfv
BUG=
TEST=

[zundel, 2011-11-16 22:17:15]

Here are some comments so far, I didn't get through the entire patch.

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
File
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java
(right):

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java:193:
}
style: add n/l between functions/class definitions

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java:613:
private void generateMethodGetter(MethodElement methodElement, String
NameOverride) {
style: argument name should start with lower case letter nameOverride

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java:636:
//              ret = $bind(<class>.prototype.<member>$lookupRTT, this); return
ret; }
The commented code doesn't make any sense to me. It looks like we perform a
bind, then throw it away?  (my guess from reading the code is that one line
applies to this part of the if, the second line applies to the else block)

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java:647:
//              ret = $bind(<class>.prototype.<member>$lookupRTT, this); return
ret; }
Same comment as above.

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java:663:
bindMethodCall = AstUtil.newInvocation(new JsNameRef("$bind"), methodToCall,
IMHO, re-using the variable bindMethodCall is confusing.

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/backend/js/JsNameProvider.java
(right):

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/JsNameProvider.java:65: ||
ElementKind.of(symbol).equals(ElementKind.FUNCTION_TYPE_ALIAS) : "Only classes
or typedefs can be lazily declared. Undeclared: "
> 100 chars

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java
(right):

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:411:
if(interfaceElement instanceof DynamicElement) {
style: space between if and (

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:435:
private void generateRTTLookupMethod(DartMethodDefinition x) {
is it just me, or dos this duplicate a lot of code in the other
gnerateRTTLookupMethod() override?

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:440:
if (methodElement instanceof ConstructorElement
ElementKind.of(methodElement).equals(CONSTRUCTOR) might catch a couple of these
cases in one swoop.

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:460:
JsInvocation callLookup = call(null,newQualifiedNameRef("RTT.createFunction"));
style nit: space after ,

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:495:
if (methodElement instanceof ConstructorElement
again, ElementKind.of(...).equals(CONSTRUCTOR) might make this simpler (don't
know)

[codefu, 2011-11-16 22:39:55]

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
File
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java
(right):

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java:636:
//              ret = $bind(<class>.prototype.<member>$lookupRTT, this); return
ret; }
On 2011/11/16 22:17:15, zundel wrote:
> The commented code doesn't make any sense to me. It looks like we perform a
> bind, then throw it away?  (my guess from reading the code is that one line
> applies to this part of the if, the second line applies to the else block)

Bad comment. The second part should read "ret.lookupRTT"

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java:647:
//              ret = $bind(<class>.prototype.<member>$lookupRTT, this); return
ret; }
On 2011/11/16 22:17:15, zundel wrote:
> Same comment as above.

Done.

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/backend/js/JsNameProvider.java
(right):

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/JsNameProvider.java:65: ||
ElementKind.of(symbol).equals(ElementKind.FUNCTION_TYPE_ALIAS) : "Only classes
or typedefs can be lazily declared. Undeclared: "
On 2011/11/16 22:17:15, zundel wrote:
> > 100 chars

I ran the formatter on this one only to have to scrunch up parameters. Will
change.

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java
(right):

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:411:
if(interfaceElement instanceof DynamicElement) {
On 2011/11/16 22:17:15, zundel wrote:
> style: space between if and (

Done.

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:435:
private void generateRTTLookupMethod(DartMethodDefinition x) {
On 2011/11/16 22:17:15, zundel wrote:
> is it just me, or dos this duplicate a lot of code in the other
> gnerateRTTLookupMethod() override?

There are now 4 generateRTTLookupMethod().  One existing one for classes, three
new ones for the three different function representations.  I've really cleaned
up the FunctionExpression version which could get consolidated.

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:440:
if (methodElement instanceof ConstructorElement
On 2011/11/16 22:17:15, zundel wrote:
> ElementKind.of(methodElement).equals(CONSTRUCTOR) might catch a couple of
these
> cases in one swoop.

Borrowed code. Will look into it.

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:460:
JsInvocation callLookup = call(null,newQualifiedNameRef("RTT.createFunction"));
On 2011/11/16 22:17:15, zundel wrote:
> style nit: space after ,

Done.

[zundel, 2011-11-16 23:54:55]

a few more comments

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java
(right):

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:634:
private void generateRTTLookupMethod(DartFunctionTypeAlias x) {
Add a comment with an example of the JS this generates.

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:662:
//generateRTTLookupForReturnType
not sure what these comments mean

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:1060:
return generateRefiedInterfaceTypeComparison(lhs, aliasType, currentClass, src);
this code as is just looks redundant.  I can't make sense of the comment.

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/resolver/ResolveVisitor.java
(right):

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/ResolveVisitor.java:76:
TypeVariableElement variable = (TypeVariableElement) typeParameter.getElement();
Could we consolidate the resolution of type parameters? It seems to be repeated
2x in this function and again in resolveFunction()

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/ResolveVisitor.java:96:
continue;
I don't understand this continue.  Maybe this should be an error? What's going
on here?

http://codereview.chromium.org/8566022/diff/1/compiler/lib/implementation/rtt.js
File compiler/lib/implementation/rtt.js (right):

http://codereview.chromium.org/8566022/diff/1/compiler/lib/implementation/rtt...
compiler/lib/implementation/rtt.js:107: for(var x = this.typeArgs.length - 1; x
>= 0; x--) {
style: space between for and (

http://codereview.chromium.org/8566022/diff/1/compiler/lib/implementation/rtt...
compiler/lib/implementation/rtt.js:109: found = false;
I find the use of 'found' in this method a bit confusing:

What you really want I think is to continue at the outer for loop.  You could
use a label on the outer loop and then use continue LABEL.

OUTER: for (var i = 0; i < probs.length; i++) {
    for (...) {
      continue OUTER;
    }
}
then you can get rid of the found variable all together.

[codefu, 2011-11-17 21:17:10]

Working on pushing a new patch soon

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java
(right):

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:634:
private void generateRTTLookupMethod(DartFunctionTypeAlias x) {
On 2011/11/16 23:54:56, zundel wrote:
> Add a comment with an example of the JS this generates.

Definitely here and other places.

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:662:
//generateRTTLookupForReturnType
On 2011/11/16 23:54:56, zundel wrote:
> not sure what these comments mean

Left-over cruft from when I was first writing this function.  Will remove.

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:1060:
return generateRefiedInterfaceTypeComparison(lhs, aliasType, currentClass, src);
On 2011/11/16 23:54:56, zundel wrote:
> this code as is just looks redundant.  I can't make sense of the comment.

This is redundant. The comment is left over from when this code path did
something different and I had a copy/paste comment from John Lenz

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
File compiler/java/com/google/dart/compiler/resolver/ResolveVisitor.java
(right):

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/ResolveVisitor.java:76:
TypeVariableElement variable = (TypeVariableElement) typeParameter.getElement();
On 2011/11/16 23:54:56, zundel wrote:
> Could we consolidate the resolution of type parameters? It seems to be
repeated
> 2x in this function and again in resolveFunction()

Certainly.

http://codereview.chromium.org/8566022/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/ResolveVisitor.java:96:
continue;
On 2011/11/16 23:54:56, zundel wrote:
> I don't understand this continue.  Maybe this should be an error? What's going
> on here?

Cleaned up

http://codereview.chromium.org/8566022/diff/1/compiler/lib/implementation/rtt.js
File compiler/lib/implementation/rtt.js (right):

http://codereview.chromium.org/8566022/diff/1/compiler/lib/implementation/rtt...
compiler/lib/implementation/rtt.js:107: for(var x = this.typeArgs.length - 1; x
>= 0; x--) {
On 2011/11/16 23:54:56, zundel wrote:
> style: space between for and (

Done.

http://codereview.chromium.org/8566022/diff/1/compiler/lib/implementation/rtt...
compiler/lib/implementation/rtt.js:109: found = false;
On 2011/11/16 23:54:56, zundel wrote:
> I find the use of 'found' in this method a bit confusing:
> 
> What you really want I think is to continue at the outer for loop.  You could
> use a label on the outer loop and then use continue LABEL.
> 
> OUTER: for (var i = 0; i < probs.length; i++) {
>     for (...) {
>       continue OUTER;
>     }
> }
> then you can get rid of the found variable all together.

Nice!

[codefu, 2011-11-17 22:39:03]

PTAL

[zundel, 2011-11-18 18:45:00]

http://codereview.chromium.org/8566022/diff/7001/compiler/java/com/google/dar...
File
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java
(right):

http://codereview.chromium.org/8566022/diff/7001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java:639:
//              return ret; }
this comment is wrong for static methods - there is no $bind() needed (and none
created)

http://codereview.chromium.org/8566022/diff/7001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java:644:
JsName rttLatch = scope.declareFreshName("ret.$lookupRTT");
Also for static methods, the $lookupRTT property setting can be done once at the
top level, there is no need to re-assign the property each time in the getter.

http://codereview.chromium.org/8566022/diff/7001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java:3046:
// TODO(codefu): Properlly support hoisted function's types 
TAG:codefuHoistedFunctionTypes
style: // comments are at the same indent level as code.
Properlly --> Properly

[codefu, 2011-11-18 20:06:16]

http://codereview.chromium.org/8566022/diff/7001/compiler/java/com/google/dar...
File
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java
(right):

http://codereview.chromium.org/8566022/diff/7001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java:639:
//              return ret; }
On 2011/11/18 18:45:00, zundel wrote:
> this comment is wrong for static methods - there is no $bind() needed (and
none
> created)

Yep, my bad (copy and paste got the best of me).  The current implementation
would have no bind and this references.

http://codereview.chromium.org/8566022/diff/7001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java:644:
JsName rttLatch = scope.declareFreshName("ret.$lookupRTT");
On 2011/11/18 18:45:00, zundel wrote:
> Also for static methods, the $lookupRTT property setting can be done once at
the
> top level, there is no need to re-assign the property each time in the getter.

Sounds perfectly good to me.  I'll follow up with another CL to bring in this
optimization.  The only difference is that generating the lookupRTT now requires
an extra $named in the function name; other locations that refer to this need to
be updated as well.

http://codereview.chromium.org/8566022/diff/7001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java:3046:
// TODO(codefu): Properlly support hoisted function's types 
TAG:codefuHoistedFunctionTypes
On 2011/11/18 18:45:00, zundel wrote:
> style: // comments are at the same indent level as code.
> Properlly --> Properly

I meant for these to be very temporary, but I can certainly tab them out.

[mmendez, 2011-11-18 21:19:27]

As we discussed at my desk, I think that the following maybe an issue.

typedef int foo();

class A {
  int mine() {...}
}

main() {
  A a = new A();
  var x = a.mine;
  if (x is List<foo>) {
  }
}

You will get the following incorrect code:

function unnamedc4c8e6$main$member(){
  var a = unnamedc4c8e6$A$Dart.A$$Factory();
  var x = a.mine$getter();
  if (List$Dart.$lookupRTT([RTT.placeholderType]).implementedBy(x.$lookupRTT()))
{
  }
}

I think that you want to model this more like the existing RTT for generics
which would yield the following code:

function unnamedc4c8e6$main$member(){
  var a = unnamedc4c8e6$A$Dart.A$$Factory();
  var x = a.mine$getter();
  if
(List$Dart.$lookupRTT([unnamedc4c8e6$func$Dart.$lookupRTT]).implementedBy(x)) {
  }
}

http://codereview.chromium.org/8566022/diff/7001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/backend/js/DartMangler.java (right):

http://codereview.chromium.org/8566022/diff/7001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/backend/js/DartMangler.java:106: public
String mangleLookupMethod(MethodElement method, LibraryElement currentLibrary);
Nit: Do we really need these additional methods?  If so, please add java doc to
one of the variants.

http://codereview.chromium.org/8566022/diff/7001/compiler/java/com/google/dar...
File
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java
(right):

http://codereview.chromium.org/8566022/diff/7001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java:182:
static class FunctionAliasDiscover extends DartNodeTraverser<Void> {
This code looks a little suspect in that it always goes "deeper"...

http://codereview.chromium.org/8566022/diff/7001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java:205:
class GenerateJavascriptVisitor
Nits: If you make this class non static to access the outer members, then you
should also remove the extra args from the constructor since they are now
unused.

[codefu, 2011-11-23 17:59:18]

Patch set 3 has a major clean up with logic changes:
  $getter methods no longer bind $lookupRTT methods (revert)
  $lookupRTT methods still exist, but added to '$named.'
  $bind is updated to add a $lookupRTT function has a $lookupRTT property
  Updated implementedByTypeFunc() to check for assignability (13.5 Dart
specification).
  Update implementedByType() to switch to handle functions passed as parameters

This makes several more tests pass.  The only ones not passing are for optional
/ named parameters.

[zundel, 2011-11-29 21:53:46]

I didn't find anything significant

http://codereview.chromium.org/8566022/diff/14001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java
(right):

http://codereview.chromium.org/8566022/diff/14001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:160:
// 1) create static type information lookup function
this comment is kind of useless.

http://codereview.chromium.org/8566022/diff/14001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:168:
// 1) create static type information lookup function
this comment is kind of useless.

http://codereview.chromium.org/8566022/diff/14001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:452:
*         List$Dart.$lookupRTT([RTT.getTypeArg(typeArgs, 0)]));
there is a common sub-expression that is called twice acc'd to the comment. 
Could this be one call and save the results?

http://codereview.chromium.org/8566022/diff/14001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:459:
|| Elements.isNonFactoryConstructor(methodElement)
This will never get here because methodElement instanceof ConstructorElement
above.  Inside of isNonFactoryConstructor it tests:
ElementKind.of(method).equals(ElementKind.CONSTRUCTOR)

I believe that isFactory() also is only set for ConstructorElement types, but
that is less clear.

http://codereview.chromium.org/8566022/diff/14001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:539:
if (element instanceof ConstructorElement ||
Elements.isNonFactoryConstructor(element)
same comment as above.

http://codereview.chromium.org/8566022/diff/14001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:590:
if (elementExpr == null) {
This looks odd - Is this a normal occurrence? Does it just happen when there are
some unresolved types that make it to code generation?

http://codereview.chromium.org/8566022/diff/14001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:837:
if( contextClassElement instanceof FunctionAliasElementImplementation) {
better to use ElementKind.of()

http://codereview.chromium.org/8566022/diff/14001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/resolver/ResolveVisitor.java
(right):

http://codereview.chromium.org/8566022/diff/14001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/ResolveVisitor.java:58: if
(!(typeNode.getType().getElement() instanceof TypeVariableElement)) {
better to use ElementKind.of()

http://codereview.chromium.org/8566022/diff/14001/compiler/lib/implementation...
File compiler/lib/implementation/rtt.js (right):

http://codereview.chromium.org/8566022/diff/14001/compiler/lib/implementation...
compiler/lib/implementation/rtt.js:116: * @return {boolean} Whether this type is
implement by other
implement --> implemented

[mmendez, 2011-11-29 22:18:23]

I'm going to look at it some more this evening, but it looks like there is an
issue with the type checking of functions when parameters and arguments are
involved, at least with the version of the patch that I looked at.  I believe
that the code below should fail, but it doesn't.

Here is the repro code:

typedef MainType(double);

foo(int i) {
}

main() {
  var y = foo;
  if (y is MainType) {
    print("success");
  } else {
    print("failure");
  }
}

[codefu, 2011-11-29 22:33:52]

On 2011/11/29 22:18:23, mmendez wrote:
> I'm going to look at it some more this evening, but it looks like there is an
> issue with the type checking of functions when parameters and arguments are
> involved, at least with the version of the patch that I looked at.  I believe
> that the code below should fail, but it doesn't.
> 
> Here is the repro code:
> 
> typedef MainType(double);
> 
> foo(int i) {
> }
> 
> main() {
>   var y = foo;
>   if (y is MainType) {
>     print("success");
>   } else {
>     print("failure");
>   }
> }

Correct.  There was a bad co19 test that I was trying to pass and it wasn't
valid.  Addressed in patch set 4

[codefu, 2011-11-29 22:42:13]

Addressing Eric's review comments.

http://codereview.chromium.org/8566022/diff/14001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java
(right):

http://codereview.chromium.org/8566022/diff/14001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:160:
// 1) create static type information lookup function
On 2011/11/29 21:53:46, zundel wrote:
> this comment is kind of useless.

Done.

http://codereview.chromium.org/8566022/diff/14001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:168:
// 1) create static type information lookup function
On 2011/11/29 21:53:46, zundel wrote:
> this comment is kind of useless.

Done.

http://codereview.chromium.org/8566022/diff/14001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:452:
*         List$Dart.$lookupRTT([RTT.getTypeArg(typeArgs, 0)]));
On 2011/11/29 21:53:46, zundel wrote:
> there is a common sub-expression that is called twice acc'd to the comment. 
> Could this be one call and save the results?

I can update the example, the index can be different, give a different set of
generic parameters.  e.g.  B foo<A,B,C>(A a, B b, C c), the return type would
look up index 1 of the typeArgs[].

http://codereview.chromium.org/8566022/diff/14001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:459:
|| Elements.isNonFactoryConstructor(methodElement)
On 2011/11/29 21:53:46, zundel wrote:
> This will never get here because methodElement instanceof ConstructorElement
> above.  Inside of isNonFactoryConstructor it tests:
> ElementKind.of(method).equals(ElementKind.CONSTRUCTOR)
> 
> I believe that isFactory() also is only set for ConstructorElement types, but
> that is less clear.

I believe factory is set if the factory keyword is used (parseFieldOrMethod() in
DartParser).  Would it be cleaner to change to:
    if (ElementKind.of(methodElement).equals(ElementKind.CONSTRUCTOR)
        || methodElement.getModifiers().isNative()) {

I removed the isFactory and ran the tests with assert and didn't notice any
errors, so I'm removing it.

http://codereview.chromium.org/8566022/diff/14001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:590:
if (elementExpr == null) {
On 2011/11/29 21:53:46, zundel wrote:
> This looks odd - Is this a normal occurrence? Does it just happen when there
are
> some unresolved types that make it to code generation?

This is probably an artifact from when generateRTTLookupForType() failed on
function expressions.  Looking at that function now, it should always return (or
assert).

http://codereview.chromium.org/8566022/diff/14001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:837:
if( contextClassElement instanceof FunctionAliasElementImplementation) {
On 2011/11/29 21:53:46, zundel wrote:
> better to use ElementKind.of()

Done.

http://codereview.chromium.org/8566022/diff/14001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:925:
FunctionType functionType = (FunctionType) type;
Hey hey, check out that tab.

http://codereview.chromium.org/8566022/diff/14001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/resolver/ResolveVisitor.java
(right):

http://codereview.chromium.org/8566022/diff/14001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/ResolveVisitor.java:58: if
(!(typeNode.getType().getElement() instanceof TypeVariableElement)) {
On 2011/11/29 21:53:46, zundel wrote:
> better to use ElementKind.of()

Done.

http://codereview.chromium.org/8566022/diff/14001/compiler/lib/implementation...
File compiler/lib/implementation/rtt.js (right):

http://codereview.chromium.org/8566022/diff/14001/compiler/lib/implementation...
compiler/lib/implementation/rtt.js:116: * @return {boolean} Whether this type is
implement by other
On 2011/11/29 21:53:46, zundel wrote:
> implement --> implemented

Done.

[mmendez, 2011-11-30 18:49:39]

This is getting closer John.  I only have one concern at this point and that is
the effect on tree shaking.  With the current formulation, if any $lookupRTT
properties are accessed then all of those properties attached to the $named
methods survive into the output.

I think that your previous formulation of adding the RTT in the method getters
works better towards minification because if the method is never accessed
through a getter then its corresponding RTT drops out also.

Can we sit down tomorrow when you are back in the office and have high-bandwidth
conversation about this?  I think that we can get this landed tomorrow once we
talk.

http://codereview.chromium.org/8566022/diff/25001/compiler/java/com/google/da...
File
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java
(left):

http://codereview.chromium.org/8566022/diff/25001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java:3632:
assert (qualifier instanceof DartIdentifier);
Why could this be removed again?

http://codereview.chromium.org/8566022/diff/25001/compiler/java/com/google/da...
File
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java
(right):

http://codereview.chromium.org/8566022/diff/25001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java:1356:
inFactoryOrStaticContext = element.getModifiers().isFactory()
You might as well make the inFactoryOrStaticContext computation a helper method
and unify it with the one on line 1098.

http://codereview.chromium.org/8566022/diff/25001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java
(right):

http://codereview.chromium.org/8566022/diff/25001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:396:
// Codefu: Random addition to keep BlackListed13 tests failing.
Where are the BlackListed13 tests again?

http://codereview.chromium.org/8566022/diff/25001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:454:
boolean hasTypes = false;
Nit: should this be hasTypeArguments?

http://codereview.chromium.org/8566022/diff/25001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:480:
return; // Error, Do not generate a lookup function
Nit: double check me, but I think that generateTypeArrayFromElements will never
actually return null.  If so, we should probably update its javadoc and add an
assertion.

[codefu, 2011-11-30 22:42:51]

Lets get together tomorrow morning and hammer out the RTT bind vs getter
methods.

http://codereview.chromium.org/8566022/diff/25001/compiler/java/com/google/da...
File
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java
(left):

http://codereview.chromium.org/8566022/diff/25001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java:3632:
assert (qualifier instanceof DartIdentifier);
On 2011/11/30 18:49:40, mmendez wrote:
> Why could this be removed again?

It could be a property access for a prefixed class (liba.B).

http://codereview.chromium.org/8566022/diff/25001/compiler/java/com/google/da...
File
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java
(right):

http://codereview.chromium.org/8566022/diff/25001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/GenerateJavascriptAST.java:1356:
inFactoryOrStaticContext = element.getModifiers().isFactory()
On 2011/11/30 18:49:40, mmendez wrote:
> You might as well make the inFactoryOrStaticContext computation a helper
method
> and unify it with the one on line 1098.

Done.

http://codereview.chromium.org/8566022/diff/25001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java
(right):

http://codereview.chromium.org/8566022/diff/25001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:396:
// Codefu: Random addition to keep BlackListed13 tests failing.
On 2011/11/30 18:49:40, mmendez wrote:
> Where are the BlackListed13 tests again?

tests/language/src/BlackListedTest.dart

http://codereview.chromium.org/8566022/diff/25001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:454:
boolean hasTypes = false;
On 2011/11/30 18:49:40, mmendez wrote:
> Nit: should this be hasTypeArguments?

Done.

http://codereview.chromium.org/8566022/diff/25001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:480:
return; // Error, Do not generate a lookup function
On 2011/11/30 18:49:40, mmendez wrote:
> Nit: double check me, but I think that generateTypeArrayFromElements will
never
> actually return null.  If so, we should probably update its javadoc and add an
> assertion.

You are correct.  I will update this and other locations.

[mmendez, 2011-12-06 17:36:46]

This is the root cause behind the optimzed mode failures.  Take a look and we
can discuss.

http://codereview.chromium.org/8566022/diff/32012/compiler/lib/implementation...
File compiler/lib/implementation/core.js (right):

http://codereview.chromium.org/8566022/diff/32012/compiler/lib/implementation...
compiler/lib/implementation/core.js:22: return fn;
I think that part of the problem is that these bind overloads should be
returning fun instead of fn.   Otherwise, the body of these is dead code.

[codefu, 2011-12-07 16:31:30]

Closing and merging to head. New CL located at:
http://codereview.chromium.org/8845002

http://codereview.chromium.org/8566022/diff/32012/compiler/lib/implementation...
File compiler/lib/implementation/core.js (right):

http://codereview.chromium.org/8566022/diff/32012/compiler/lib/implementation...
compiler/lib/implementation/core.js:22: return fn;
On 2011/12/06 17:36:46, mmendez wrote:
> I think that part of the problem is that these bind overloads should be
> returning fun instead of fn.   Otherwise, the body of these is dead code.

Good catch.  I was fairly certain I had fixed these, but apparently I lost that
change.