Check that interface constructors and default class constructors are compatible.

Check that interface constructors and default class constructors are compatible.
1. Number of required parameters.
2. Names and order of named parameters.
3. [disabled] type warning for non-identical types.
4. [wait for Eric, not implemented] type parameters check.


R=zundel@google.com
BUG=
TEST=

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

[zundel, 2011-12-05 14:52:17]

I guess the other implementations aren't enforcing all these checks yet based on
your changes found in the core libraries.

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

http://codereview.chromium.org/8786002/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/Elements.java:307: public static
List<String> getParametersTypes(MethodElement method) {
It isn't going to be quite this simple, as the bounds for the type parameters
also need to be compared.  Also, these are type variables, not actual types, so
maybe a name change is in order.

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

http://codereview.chromium.org/8786002/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:308: if (false &&
!hasCompileError) {
I think its best to just leave the TODOs and remove the speculative
implementation.  We know the parameter checks need more than just names.

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

http://codereview.chromium.org/8786002/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/ResolverErrorCode.java:68: //
remove stray comment

http://codereview.chromium.org/8786002/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/ResolverErrorCode.java:72:
"Interface constructor '%s' has %s of required parameters, but %s in factory
constructor '%s' in '%s'"),
string is too long.  How about:

Constructor declaration '%s' in '%s' has %s parameters, doesn't match '%s' in
'%s' with %s.

http://codereview.chromium.org/8786002/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/ResolverErrorCode.java:74:
"Interface constructor '%s' has named parameters %s, but %s in factory
constructor '%s' in '%s'"),
ditto, just a few chars too long.

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

http://codereview.chromium.org/8786002/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/TypeErrorCode.java:15:
"Interface constructor '%s' has parameters types (%s), but (%s) in factory
constructor '%s' in '%s'"),
also a few characters too long.  Can we short it to fit on one line?

http://codereview.chromium.org/8786002/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/TypeErrorCode.java:16: // XXX
remove comment

[zundel, 2011-12-05 15:46:15]

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

http://codereview.chromium.org/8786002/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/Elements.java:307: public static
List<String> getParametersTypes(MethodElement method) {
On 2011/12/05 14:52:17, zundel wrote:
> It isn't going to be quite this simple, as the bounds for the type parameters
> also need to be compared.  Also, these are type variables, not actual types,
so
> maybe a name change is in order.
Ignore this comment, I was confusing the parameters with the type parameters for
a factory.

[scheglov, 2011-12-05 19:28:01]

I'v implemented check for type names.
I have to tweak couple places in library and samples, which don't follow this
spec requirement to avoid type warnings and tests failures.

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

http://codereview.chromium.org/8786002/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:308: if (false &&
!hasCompileError) {
Implemented and moved to TypeAnalyzer (we want to have all types resolved).

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

http://codereview.chromium.org/8786002/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/ResolverErrorCode.java:72:
"Interface constructor '%s' has %s of required parameters, but %s in factory
constructor '%s' in '%s'"),
On 2011/12/05 14:52:17, zundel wrote:
> string is too long.  How about:
> 
> Constructor declaration '%s' in '%s' has %s parameters, doesn't match '%s' in
> '%s' with %s.

Done.

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

http://codereview.chromium.org/8786002/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/TypeErrorCode.java:16: // XXX
On 2011/12/05 14:52:17, zundel wrote:
> remove comment

Done.

[zundel, 2011-12-05 21:26:45]

lgtm

http://codereview.chromium.org/8786002/diff/8001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/SystemLibraryManager.java (right):

http://codereview.chromium.org/8786002/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/SystemLibraryManager.java:116: * @param
uri
spurious n/l

http://codereview.chromium.org/8786002/diff/8001/compiler/javatests/com/googl...
File compiler/javatests/com/google/dart/compiler/CompilerTestCase.java (right):

http://codereview.chromium.org/8786002/diff/8001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/CompilerTestCase.java:361:
rootNode.accept(new DartNodeTraverser<Void>() {
To simplify, you can make DartNodeTraverser<DartNewExpression>
and then just return it directly

return rootNode.accept(new DartNodeTraverser<DartNewExpression>...

See MemberBuilder.buildConstructor()

http://codereview.chromium.org/8786002/diff/8001/compiler/javatests/com/googl...
File compiler/javatests/com/google/dart/compiler/SystemLibraryManagerTest.java
(right):

http://codereview.chromium.org/8786002/diff/8001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/SystemLibraryManagerTest.java:20: }
are these your changes or did they creep in from a sync?

http://codereview.chromium.org/8786002/diff/8001/compiler/javatests/com/googl...
File compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerTest.java
(left):

http://codereview.chromium.org/8786002/diff/8001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerTest.java:328: " 
Foo(argument);",
we probably want to add the 'String'  type here so that the types of the
interface definition matches the class.

http://codereview.chromium.org/8786002/diff/8001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerTest.java:335:
analyzeFail("Baz x = new Foo('');",
TypeErrorCode.TYPE_NOT_ASSIGNMENT_COMPATIBLE);
why did you remove this test?  this statement should fail.

[scheglov, 2011-12-06 14:20:39]

http://codereview.chromium.org/8786002/diff/8001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/SystemLibraryManager.java (right):

http://codereview.chromium.org/8786002/diff/8001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/SystemLibraryManager.java:116: * @param
uri
On 2011/12/05 21:26:45, zundel wrote:
> spurious n/l

Done.

http://codereview.chromium.org/8786002/diff/8001/compiler/javatests/com/googl...
File compiler/javatests/com/google/dart/compiler/CompilerTestCase.java (right):

http://codereview.chromium.org/8786002/diff/8001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/CompilerTestCase.java:361:
rootNode.accept(new DartNodeTraverser<Void>() {
On 2011/12/05 21:26:45, zundel wrote:
> To simplify, you can make DartNodeTraverser<DartNewExpression>
> and then just return it directly
> 
> return rootNode.accept(new DartNodeTraverser<DartNewExpression>...
> 
> See MemberBuilder.buildConstructor()

Does not work because of limitations of DartNodeTraverser.

http://codereview.chromium.org/8786002/diff/8001/compiler/javatests/com/googl...
File compiler/javatests/com/google/dart/compiler/SystemLibraryManagerTest.java
(right):

http://codereview.chromium.org/8786002/diff/8001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/SystemLibraryManagerTest.java:20: }
On 2011/12/05 21:26:45, zundel wrote:
> are these your changes or did they creep in from a sync?

These are my changes.
I found that there are many broken tests and tried to fix simple ones.

http://codereview.chromium.org/8786002/diff/8001/compiler/javatests/com/googl...
File compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerTest.java
(left):

http://codereview.chromium.org/8786002/diff/8001/compiler/javatests/com/googl...
compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerTest.java:335:
analyzeFail("Baz x = new Foo('');",
TypeErrorCode.TYPE_NOT_ASSIGNMENT_COMPATIBLE);
On 2011/12/05 21:26:45, zundel wrote:
> why did you remove this test?  this statement should fail.

Done.