Updates dartc to recognize 'default' keyword on interface and updated factory method syntax

Updates dartc to recognize 'default' keyword on interface and updated factory method syntax.

Note: this does not have backwards compatibility for the old
factory syntax and updates the tests.

http://code.google.com/p/dart/issues/detail?id=417

Committed: https://code.google.com/p/dart/source/detail?r=2597

[zundel, 2011-12-15 00:18:26]

@Peter, could you review the updates to the unit tests?

[ahe, 2011-12-15 09:02:13]

Hi Eric,

There seems to be misunderstanding around whether or not factory clauses require
type parameters, so I'm not ready to say "go".

Otherwise, I'm impressed by the level of testing you have included in this
change.

Cheers,
Peter

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
File
compiler/javatests/com/google/dart/compiler/ast/DartToSourceVisitorTest.java
(right):

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/ast/DartToSourceVisitorTest.java:54:
"class c {\n" +
class c<E>

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
File compiler/javatests/com/google/dart/compiler/backend/js/RttTest.java
(right):

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/backend/js/RttTest.java:79: String
init = findMarkerAtOccurrence(js, "_marker_D2", DELIMETERS, 1);
I have no idea about what is going on here.

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
File
compiler/javatests/com/google/dart/compiler/backend/js/testRuntimeTypes.dart
(right):

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/backend/js/testRuntimeTypes.dart:15:
return new C<T>();
C is not a subtype of D. So this should fail in checked mode.

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/backend/js/testRuntimeTypes.dart:18:
class Main {
Consider removing this class and turn the method into a top-level method.

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
File compiler/javatests/com/google/dart/compiler/end2end/inc/my.dart (right):

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/end2end/inc/my.dart:24: var sc = new
SomeClass(1);
I assume SomeClass exists.

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
File
compiler/javatests/com/google/dart/compiler/parser/ErrorMessageLocationTest.java
(right):

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/parser/ErrorMessageLocationTest.java:23:
"interface foo bogus Bar {\n" +
I'd stick to a keyword, for example, "switch".

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
File compiler/javatests/com/google/dart/compiler/parser/NegativeParserTest.java
(left):

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/parser/NegativeParserTest.java:151:
public void test_badTopLevelFactory_withTypeParameters() {
I don't understand why you have removed this test.

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
File compiler/javatests/com/google/dart/compiler/resolver/ResolverTest.java
(right):

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/resolver/ResolverTest.java:120:
DartClass ia = makeInterface("IA", makeTypes("ID"), null);
I don't understand why you made this change.

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/resolver/ResolverTest.java:400:
ResolverErrorCode.DEFAULT_CLASS_MUST_HAVE_SAME_TYPE_PARAMS);
This test seems good, but you may also want to test that this works:

interface A<T> default B {}
class B<T> {}

You don't have to repeat the type parameters in the default factory clause.

Also, I'm not sure you're testing this case:

interface A<T> default B {}
class B<T extends Comparable> {}

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/resolver/ResolverTest.java:404: //
Invoke constructor in factory method with (wrong) type args
This test has multiple problems. I'm not sure what it is intended to test.

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/resolver/ResolverTest.java:412:
"class B<K> {",
This is an error because:

"If I has n type parameters, then the name of the ith type
parameter of I must be identical to the name of the ith type parameter of F"

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/resolver/ResolverTest.java:413: " 
factory A() { return new C<int>();}",
This is a warning because C<int> is not assignable to A<K>.

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/resolver/ResolverTest.java:418:
public void testFactoryBadTypeArgsNew4() {
Same problems as the above test.

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/resolver/ResolverTest.java:566: " 
foo() { return new Foo<T>(); }",
I agree this is an error because T is not in scope.

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
File
compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerCompilerTest.java
(left):

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerCompilerTest.java:128:
* http://code.google.com/p/dart/issues/detail?id=345
I don't understand what is going on with this bug. Did you move this test
somewhere?

http://codereview.chromium.org/8948001/diff/2001/tests/language/src/FactoryNe...
File tests/language/src/FactoryNegativeTest.dart (right):

http://codereview.chromium.org/8948001/diff/2001/tests/language/src/FactoryNe...
tests/language/src/FactoryNegativeTest.dart:7: interface Link<T> factory
LinkFactory { // Compile time error: should be LinkFactory<T>
You don't have to repeat the type parameters. This is not a compile-time error.

http://codereview.chromium.org/8948001/diff/2001/tests/language/src/FactoryNe...
tests/language/src/FactoryNegativeTest.dart:11: class LinkFactory {
However, this is a compile-time error.

http://codereview.chromium.org/8948001/diff/2001/tests/language/src/NonParame...
File tests/language/src/NonParameterizedFactory2Test.dart (right):

http://codereview.chromium.org/8948001/diff/2001/tests/language/src/NonParame...
tests/language/src/NonParameterizedFactory2Test.dart:5: interface Interface<T>
factory Factory<T> {
This is not necessary. Also 'factory' -> 'default'.

http://codereview.chromium.org/8948001/diff/2001/tests/language/src/NonParame...
File tests/language/src/NonParameterizedFactoryTest.dart (right):

http://codereview.chromium.org/8948001/diff/2001/tests/language/src/NonParame...
tests/language/src/NonParameterizedFactoryTest.dart:5: interface Interface<T>
factory Factory<T> {
factory -> default. No need to repeat the type parameter.

[zundel, 2011-12-15 14:12:50]

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
File compiler/javatests/com/google/dart/compiler/resolver/ResolverTest.java
(right):

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/resolver/ResolverTest.java:400:
ResolverErrorCode.DEFAULT_CLASS_MUST_HAVE_SAME_TYPE_PARAMS);
On 2011/12/15 09:02:13, ahe wrote:
> This test seems good, but you may also want to test that this works:
> 
> interface A<T> default B {}
> class B<T> {}
> 
> You don't have to repeat the type parameters in the default factory clause.

The spec is not very clear on when you need to specify these parameters.  The
words say, "If you specify type parameters, they need to be identical to F" so
why would you specify them at all? 
 

> 
> Also, I'm not sure you're testing this case:
> 
> interface A<T> default B {}
> class B<T extends Comparable> {}

[ahe, 2011-12-15 14:20:51]

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
File compiler/javatests/com/google/dart/compiler/resolver/ResolverTest.java
(right):

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/resolver/ResolverTest.java:400:
ResolverErrorCode.DEFAULT_CLASS_MUST_HAVE_SAME_TYPE_PARAMS);
On 2011/12/15 14:12:50, zundel wrote:
> On 2011/12/15 09:02:13, ahe wrote:
> > This test seems good, but you may also want to test that this works:
> > 
> > interface A<T> default B {}
> > class B<T> {}
> > 
> > You don't have to repeat the type parameters in the default factory clause.
> 
> The spec is not very clear on when you need to specify these parameters.  The
> words say, "If you specify type parameters, they need to be identical to F" so
> why would you specify them at all? 

You'd specify them for the interface "Map<K,V>" because "HashMap<K extends
Hashable, V>" requires K to be Hashable and you want to document that in the
interface.

> 
> > 
> > Also, I'm not sure you're testing this case:
> > 
> > interface A<T> default B {}
> > class B<T extends Comparable> {}
>

[mmendez, 2011-12-15 22:04:12]

We talked at length about this at my desk so, I'll just send a summary.  

The issue is that the current approach of using the DartParameterizedNode is
that it introduces type parameters that are not bound to those of the interface
declaration explicitly.  You'd like for this to be treated in a similar manner
to extends clause of class A<T> extends B<T>{}, but for the fact that the
factory expression can place bounds on the B<T>.

If there was a way to deal with the bounds then you'd have less special casing. 
One way might be to extend DartTypeNode to handle bounds.

http://codereview.chromium.org/8948001/diff/2001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/LibraryDepsVisitor.java (right):

http://codereview.chromium.org/8948001/diff/2001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/LibraryDepsVisitor.java:114:
if(node.isInterface() && node.getDefaultClass() != null) {
visitChildren should cause the defaultClass to be visited, unless this is as a
side effect of the DartParameterizedNode.

[zundel, 2011-12-16 21:36:29]

Updated the tests and merged up to master.  There is still something left to be
desired about the analysis of the 'default' clause. I left some TODOs in the
code.

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
File
compiler/javatests/com/google/dart/compiler/ast/DartToSourceVisitorTest.java
(right):

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/ast/DartToSourceVisitorTest.java:54:
"class c {\n" +
On 2011/12/15 09:02:13, ahe wrote:
> class c<E>

Done.

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
File
compiler/javatests/com/google/dart/compiler/backend/js/testRuntimeTypes.dart
(right):

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/backend/js/testRuntimeTypes.dart:15:
return new C<T>();
On 2011/12/15 09:02:13, ahe wrote:
> C is not a subtype of D. So this should fail in checked mode.

Done.

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/backend/js/testRuntimeTypes.dart:18:
class Main {
On 2011/12/15 09:02:13, ahe wrote:
> Consider removing this class and turn the method into a top-level method.

The test driver for this test pulls out all code from the class named 'Main' for
analysis.

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
File compiler/javatests/com/google/dart/compiler/end2end/inc/my.dart (right):

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/end2end/inc/my.dart:24: var sc = new
SomeClass(1);
On 2011/12/15 09:02:13, ahe wrote:
> I assume SomeClass exists.

Some where... in another file pulled in from my.app.dart

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
File
compiler/javatests/com/google/dart/compiler/parser/ErrorMessageLocationTest.java
(right):

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/parser/ErrorMessageLocationTest.java:23:
"interface foo bogus Bar {\n" +
On 2011/12/15 09:02:13, ahe wrote:
> I'd stick to a keyword, for example, "switch".

Done.

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
File compiler/javatests/com/google/dart/compiler/parser/NegativeParserTest.java
(left):

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/parser/NegativeParserTest.java:151:
public void test_badTopLevelFactory_withTypeParameters() {
On 2011/12/15 09:02:13, ahe wrote:
> I don't understand why you have removed this test.

because you can't parameterize factory methods directly like this any more, and
there is already another test for the factory keyword above.

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
File compiler/javatests/com/google/dart/compiler/resolver/ResolverTest.java
(right):

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/resolver/ResolverTest.java:120:
DartClass ia = makeInterface("IA", makeTypes("ID"), null);
On 2011/12/15 09:02:13, ahe wrote:
> I don't understand why you made this change.

I added it back and made it actually test something.

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/resolver/ResolverTest.java:400:
ResolverErrorCode.DEFAULT_CLASS_MUST_HAVE_SAME_TYPE_PARAMS);
On 2011/12/15 09:02:13, ahe wrote:
> This test seems good, but you may also want to test that this works:
> 
> interface A<T> default B {}
> class B<T> {}
> 
> You don't have to repeat the type parameters in the default factory clause.
> 
> Also, I'm not sure you're testing this case:
> 
> interface A<T> default B {}
> class B<T extends Comparable> {}

Added those two (as cases that should pass w/o error: testDefaultTypeArgs4,
testDefaultTypeArgs5)

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/resolver/ResolverTest.java:400:
ResolverErrorCode.DEFAULT_CLASS_MUST_HAVE_SAME_TYPE_PARAMS);
Sure, but if you left them out, nothing would go wrong.  I take it that
specifying type parameters on the default arg is just for the convenience of the
programmer?

On 2011/12/15 14:20:53, ahe wrote:
> On 2011/12/15 14:12:50, zundel wrote:
> > On 2011/12/15 09:02:13, ahe wrote:
> > > This test seems good, but you may also want to test that this works:
> > > 
> > > interface A<T> default B {}
> > > class B<T> {}
> > > 
> > > You don't have to repeat the type parameters in the default factory
clause.
> > 
> > The spec is not very clear on when you need to specify these parameters. 
The
> > words say, "If you specify type parameters, they need to be identical to F"
so
> > why would you specify them at all? 
> 
> You'd specify them for the interface "Map<K,V>" because "HashMap<K extends
> Hashable, V>" requires K to be Hashable and you want to document that in the
> interface.
> 
> > 
> > > 
> > > Also, I'm not sure you're testing this case:
> > > 
> > > interface A<T> default B {}
> > > class B<T extends Comparable> {}
> > 
>

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/resolver/ResolverTest.java:412:
"class B<K> {",
On 2011/12/15 09:02:13, ahe wrote:
> This is an error because:
> 
> "If I has n type parameters, then the name of the ith type
> parameter of I must be identical to the name of the ith type parameter of F"

I split this up into a couple of tests.

http://codereview.chromium.org/8948001/diff/2001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/resolver/ResolverTest.java:413: " 
factory A() { return new C<int>();}",
On 2011/12/15 09:02:13, ahe wrote:
> This is a warning because C<int> is not assignable to A<K>.

Done.

http://codereview.chromium.org/8948001/diff/2001/tests/language/src/FactoryNe...
File tests/language/src/FactoryNegativeTest.dart (right):

http://codereview.chromium.org/8948001/diff/2001/tests/language/src/FactoryNe...
tests/language/src/FactoryNegativeTest.dart:7: interface Link<T> factory
LinkFactory { // Compile time error: should be LinkFactory<T>
On 2011/12/15 09:02:13, ahe wrote:
> You don't have to repeat the type parameters. This is not a compile-time
error.

Removed

http://codereview.chromium.org/8948001/diff/2001/tests/language/src/FactoryNe...
tests/language/src/FactoryNegativeTest.dart:11: class LinkFactory {
On 2011/12/15 09:02:13, ahe wrote:
> However, this is a compile-time error.

Added

http://codereview.chromium.org/8948001/diff/2001/tests/language/src/NonParame...
File tests/language/src/NonParameterizedFactory2Test.dart (right):

http://codereview.chromium.org/8948001/diff/2001/tests/language/src/NonParame...
tests/language/src/NonParameterizedFactory2Test.dart:5: interface Interface<T>
factory Factory<T> {
On 2011/12/15 09:02:13, ahe wrote:
> This is not necessary. Also 'factory' -> 'default'.

Done.

http://codereview.chromium.org/8948001/diff/2001/tests/language/src/NonParame...
tests/language/src/NonParameterizedFactory2Test.dart:15: factory
Interface.withArg(value) {
I'm not sure what this is trying to test, I get a static type warning that
'value' doesn't match the interface definition:


tests/language/src/NonParameterizedFactory2Test.dart/NonParameterizedFactory2Test.dart:7:
Constructor 'Interface.withArg' in 'Interface' has parameters types (T), doesn't
match 'Interface.withArg' in 'Factory' with (<dynamic>)
     6:   Interface();
     7:   Interface.withArg(T value);

http://codereview.chromium.org/8948001/diff/2001/tests/language/src/NonParame...
File tests/language/src/NonParameterizedFactoryTest.dart (right):

http://codereview.chromium.org/8948001/diff/2001/tests/language/src/NonParame...
tests/language/src/NonParameterizedFactoryTest.dart:5: interface Interface<T>
factory Factory<T> {
On 2011/12/15 09:02:13, ahe wrote:
> factory -> default. No need to repeat the type parameter.

Done.

[zundel, 2011-12-17 11:21:23]

http://codereview.chromium.org/8948001/diff/15014/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/resolver/Resolver.java (right):

http://codereview.chromium.org/8948001/diff/15014/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:887: Element
element = context.resolveName(x.getExpression());
I know this needs rework, there are no other references to context.resolveName
in this file, and very few to recordType.  I'm thinking we should create new
TypeVariableElements in MemberBuilder and then in TypeAnalyzer, compare them to
the variables declared in the interface and in the default class.

That having been said, I'd like to do that in a followon, as   we can allow frog
and the VM to move forward with removing support for the old syntax after
committing this change.

[zundel, 2011-12-17 17:56:11]

http://codereview.chromium.org/8948001/diff/15014/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/resolver/Resolver.java (right):

http://codereview.chromium.org/8948001/diff/15014/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:887: Element
element = context.resolveName(x.getExpression());
On 2011/12/17 11:21:24, zundel wrote:
> I know this needs rework, there are no other references to context.resolveName
> in this file, and very few to recordType.  I'm thinking we should create new
> TypeVariableElements in MemberBuilder and then in TypeAnalyzer, compare them
to
> the variables declared in the interface and in the default class.
> 
> That having been said, I'd like to do that in a followon, as   we can allow
frog
> and the VM to move forward with removing support for the old syntax after
> committing this change.

Wait a minute. I don't think we need to resolve this node at all, othter than
resolving the name to the default class element.   All I really need to do for
this validation is grab the DartParameterizedTypeNode from the default class and
walk it and make sure the two of them are the same.

[zundel, 2011-12-18 02:13:07]

Updated patch and removed previous patch to avoid confusion

[mmendez, 2011-12-19 17:28:26]

- The following code produces an internal error but does not stop compilation. 
However substituting Blah for string in the sample correctly identifies that
Blah is not known 
interface IA<T> default A<T extends String> { IA(); }
class A<T extends String> implements IA<T> {
  A(){}
}

main() {}

- We need to double check how "default A<T>" should resolve in the following
code.
interface IA<T> default A<T> {}
class A<T> {}

I think that right now, the T in the default clause does not bind back to the T
declared in IA<T>.  We should double check this and that it works for the editor
guys, i.e. hitting F3 takes the user to the right decl.

- Should the following code not produce an error?
interface Foo {}
interface IA<T> default A<T extends Foo> { IA(); }
class A<T extends Foo> implements IA<T> {
  factory A(){}
}

main() {
  new IA<String>();  // Assuming the internal error is handled shouldn't this be
a warning?
}

I think that you plan on deferring this, but I wanted to make sure.

- No error if interface declares constructor and no factory is declared.  I
think you plan if deferring this to a subsequent CL.  Here is a repro case:

interface IA { IA(); } // Compile time error here, per my read of the spec.
main() {
}

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/ast/DartParameterizedTypeNode.java
(right):

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/DartParameterizedTypeNode.java:35:
public Symbol getSymbol() {
Tighten the return type here to ClassElement.

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/DartParameterizedTypeNode.java:64:
typeParameters = Collections.EMPTY_LIST;
Nit: this should produce a warning about unchecked conversion.  You can switch
it to Collections.emptyList() to get rid of the warning.

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

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:911:
contextTypeArgs,
Why is it safe to not clone here?

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:1138:
private boolean inFactoryOrStatic(ClassElement containingClass) {
It seems weird to continue to check that the member is a factory since the only
caller (inStaticNotFactory) already ensures that you are not in a factory.

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/resolver/MemberBuilder.java (right):

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/MemberBuilder.java:387: private
ElementKind getMethodKind(DartMethodDefinition method) {
In this method, I don't understand how a method name could ever be a
DartParameterizedTypeNode.

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/MemberBuilder.java:397: if (name
instanceof DartParameterizedTypeNode) {
What source code causes the name to be a DartParameterizedTypeNode?

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/resolver/Resolver.java (right):

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:295:
onError(defaultClassRef, ResolverErrorCode.TYPE_PARAMETERS_MUST_MATCH_EXACTLY);
Nit: might be nice to point out where the type param match failure starts.

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/resolver/SupertypeResolver.java
(right):

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/SupertypeResolver.java:73:
node.getDefaultClass().setSymbol(defaultClassElement);
Nit: can you add a comment about why you set the element and not the type on the
default class.

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java (right):

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:730: //
TODO(zundel): validate the factory type (and parameters) exactly match the
default
Nit: maybe update the todo section to only call out that the bounds check is
what remains to be done.

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:1070: if
(ifaceType.getElement().isInterface()) {
Could you add a comment here explaining why the substitution is necessary?

[zundel, 2011-12-19 20:17:49]

New patch up.

I fixed the problem with "internal error:  type "String" is null"

The other failing cases I'd like to deal with in a followon patch.

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/ast/DartParameterizedTypeNode.java
(right):

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/DartParameterizedTypeNode.java:35:
public Symbol getSymbol() {
On 2011/12/19 17:28:26, mmendez wrote:
> Tighten the return type here to ClassElement.

Remove get/setSybol().  now we use just the type on this node.  I think that is
more consistent with precedent in DartTypeNode

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/ast/DartParameterizedTypeNode.java:64:
typeParameters = Collections.EMPTY_LIST;
On 2011/12/19 17:28:26, mmendez wrote:
> Nit: this should produce a warning about unchecked conversion.  You can switch
> it to Collections.emptyList() to get rid of the warning.

Done.

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

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:911:
contextTypeArgs,
On 2011/12/19 17:28:26, mmendez wrote:
> Why is it safe to not clone here?

This was unintentional. reverted.

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:1138:
private boolean inFactoryOrStatic(ClassElement containingClass) {
On 2011/12/19 17:28:26, mmendez wrote:
> It seems weird to continue to check that the member is a factory since the
only
> caller (inStaticNotFactory) already ensures that you are not in a factory.

Done.

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/resolver/MemberBuilder.java (right):

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/MemberBuilder.java:387: private
ElementKind getMethodKind(DartMethodDefinition method) {
On 2011/12/19 17:28:26, mmendez wrote:
> In this method, I don't understand how a method name could ever be a
> DartParameterizedTypeNode.

Done.

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/MemberBuilder.java:397: if (name
instanceof DartParameterizedTypeNode) {
On 2011/12/19 17:28:26, mmendez wrote:
> What source code causes the name to be a DartParameterizedTypeNode?

Holdover from old factory method implementation that had type parameters. 
Removed.

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/resolver/Resolver.java (right):

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:295:
onError(defaultClassRef, ResolverErrorCode.TYPE_PARAMETERS_MUST_MATCH_EXACTLY);
On 2011/12/19 17:28:26, mmendez wrote:
> Nit: might be nice to point out where the type param match failure starts.

I started to overhaul the logic, but it isn't quite so simple.  In the case
where there are extra parameters on the end, there would be nothing to highlight
in the interface declaration other than the whole string.

Added TODO

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/resolver/SupertypeResolver.java
(right):

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/SupertypeResolver.java:73:
node.getDefaultClass().setSymbol(defaultClassElement);
On 2011/12/19 17:28:26, mmendez wrote:
> Nit: can you add a comment about why you set the element and not the type on
the
> default class.

I just made it set the type.  Neither field is being used right now, I did it
for use by the editor.

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
File compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java (right):

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:730: //
TODO(zundel): validate the factory type (and parameters) exactly match the
default
On 2011/12/19 17:28:26, mmendez wrote:
> Nit: maybe update the todo section to only call out that the bounds check is
> what remains to be done.

I think the entire TODO is obsolete, so I removed it.

http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:1070: if
(ifaceType.getElement().isInterface()) {
On 2011/12/19 17:28:26, mmendez wrote:
> Could you add a comment here explaining why the substitution is necessary?

Done.

[mmendez, 2011-12-19 21:50:34]

LGTM, but please address the open issues from my comment above in subsequent
patches.

On 2011/12/19 20:17:49, zundel wrote:
> New patch up.
> 
> I fixed the problem with "internal error:  type "String" is null"
> 
> The other failing cases I'd like to deal with in a followon patch.
> 
>
http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
> File compiler/java/com/google/dart/compiler/ast/DartParameterizedTypeNode.java
> (right):
> 
>
http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
> compiler/java/com/google/dart/compiler/ast/DartParameterizedTypeNode.java:35:
> public Symbol getSymbol() {
> On 2011/12/19 17:28:26, mmendez wrote:
> > Tighten the return type here to ClassElement.
> 
> Remove get/setSybol().  now we use just the type on this node.  I think that
is
> more consistent with precedent in DartTypeNode
> 
>
http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
> compiler/java/com/google/dart/compiler/ast/DartParameterizedTypeNode.java:64:
> typeParameters = Collections.EMPTY_LIST;
> On 2011/12/19 17:28:26, mmendez wrote:
> > Nit: this should produce a warning about unchecked conversion.  You can
switch
> > it to Collections.emptyList() to get rid of the warning.
> 
> Done.
> 
>
http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
> File
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java
> (right):
> 
>
http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
>
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:911:
> contextTypeArgs,
> On 2011/12/19 17:28:26, mmendez wrote:
> > Why is it safe to not clone here?
> 
> This was unintentional. reverted.
> 
>
http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
>
compiler/java/com/google/dart/compiler/backend/js/RuntimeTypeInjector.java:1138:
> private boolean inFactoryOrStatic(ClassElement containingClass) {
> On 2011/12/19 17:28:26, mmendez wrote:
> > It seems weird to continue to check that the member is a factory since the
> only
> > caller (inStaticNotFactory) already ensures that you are not in a factory.
> 
> Done.
> 
>
http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
> File compiler/java/com/google/dart/compiler/resolver/MemberBuilder.java
(right):
> 
>
http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
> compiler/java/com/google/dart/compiler/resolver/MemberBuilder.java:387:
private
> ElementKind getMethodKind(DartMethodDefinition method) {
> On 2011/12/19 17:28:26, mmendez wrote:
> > In this method, I don't understand how a method name could ever be a
> > DartParameterizedTypeNode.
> 
> Done.
> 
>
http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
> compiler/java/com/google/dart/compiler/resolver/MemberBuilder.java:397: if
(name
> instanceof DartParameterizedTypeNode) {
> On 2011/12/19 17:28:26, mmendez wrote:
> > What source code causes the name to be a DartParameterizedTypeNode?
> 
> Holdover from old factory method implementation that had type parameters. 
> Removed.
> 
>
http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
> File compiler/java/com/google/dart/compiler/resolver/Resolver.java (right):
> 
>
http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
> compiler/java/com/google/dart/compiler/resolver/Resolver.java:295:
> onError(defaultClassRef,
ResolverErrorCode.TYPE_PARAMETERS_MUST_MATCH_EXACTLY);
> On 2011/12/19 17:28:26, mmendez wrote:
> > Nit: might be nice to point out where the type param match failure starts.
> 
> I started to overhaul the logic, but it isn't quite so simple.  In the case
> where there are extra parameters on the end, there would be nothing to
highlight
> in the interface declaration other than the whole string.
> 
> Added TODO
> 
>
http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
> File compiler/java/com/google/dart/compiler/resolver/SupertypeResolver.java
> (right):
> 
>
http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
> compiler/java/com/google/dart/compiler/resolver/SupertypeResolver.java:73:
> node.getDefaultClass().setSymbol(defaultClassElement);
> On 2011/12/19 17:28:26, mmendez wrote:
> > Nit: can you add a comment about why you set the element and not the type on
> the
> > default class.
> 
> I just made it set the type.  Neither field is being used right now, I did it
> for use by the editor.
> 
>
http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
> File compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java (right):
> 
>
http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
> compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:730: //
> TODO(zundel): validate the factory type (and parameters) exactly match the
> default
> On 2011/12/19 17:28:26, mmendez wrote:
> > Nit: maybe update the todo section to only call out that the bounds check is
> > what remains to be done.
> 
> I think the entire TODO is obsolete, so I removed it.
> 
>
http://codereview.chromium.org/8948001/diff/37001/compiler/java/com/google/da...
> compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:1070: if
> (ifaceType.getElement().isInterface()) {
> On 2011/12/19 17:28:26, mmendez wrote:
> > Could you add a comment here explaining why the substitution is necessary?
> 
> Done.