Fix for resolving interface constructors in factory, issue 521.

Fix for resolving interface constructors in factory, issue 521.
http://code.google.com/p/dart/issues/detail?id=521


R=zundel@google.com
BUG=
TEST=

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

[zundel, 2011-11-30 16:48:28]

I like the JUnit test you added. We should compare those tests to the current
Factory tests (especially the negative tests) and make sure there are
corresponding tests in tests/language/src

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

http://codereview.chromium.org/8747016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:939: }
From my reading of the spec, if F implements I, then there are the only 2 names
possible for the factory method.  Therefore, doesen't this seem like an
excellent opportunity to give a good message to the user, saying the compiler
expected a constructor of name Nf or Nf.id?

Also, this isn't specified in the spec, but I was wondering if the target method
needed the factory modifier on it in the case of Nf.id.   If not, is the factory
modifier illegal?

http://codereview.chromium.org/8747016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:955: }
The spec also mentions a check for # of mandatory arguments must match. I just
tested the compiler and VM, neither seem to enforce that.  It seems that would
be easy to check in this patch, but we should add an extra language test in
tests/language/src

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

http://codereview.chromium.org/8747016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/SyntheticDefaultConstructorElement.java:99:
return dynamicType;
? is this right. I  would think it would return the enclosing class type.

[zundel, 2011-11-30 16:58:33]

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

http://codereview.chromium.org/8747016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:955: }
On 2011/11/30 16:48:28, zundel wrote:
> The spec also mentions a check for # of mandatory arguments must match. I just
> tested the compiler and VM, neither seem to enforce that.  It seems that would
> be easy to check in this patch, but we should add an extra language test in
> tests/language/src

(InterfaceFactory3NegativeTest.dart which is marked failed, Bug 5387405)

[scheglov, 2011-11-30 21:11:24]

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

http://codereview.chromium.org/8747016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:939: }
On 2011/11/30 16:48:28, zundel wrote:
> From my reading of the spec, if F implements I, then there are the only 2
names
> possible for the factory method.  Therefore, doesen't this seem like an
> excellent opportunity to give a good message to the user, saying the compiler
> expected a constructor of name Nf or Nf.id?

  I've added better error message, which says specifically what name of
constructor was expected in factory class.


> Also, this isn't specified in the spec, but I was wondering if the target
method
> needed the factory modifier on it in the case of Nf.id.   If not, is the
factory
> modifier illegal?  

  It seems that "interface I factory F" and "factory method()" are independent
things despite of using same word "factory".

http://codereview.chromium.org/8747016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:955: }
On 2011/11/30 16:58:33, zundel wrote:
> On 2011/11/30 16:48:28, zundel wrote:
> > The spec also mentions a check for # of mandatory arguments must match. I
just
> > tested the compiler and VM, neither seem to enforce that.  It seems that
would
> > be easy to check in this patch, but we should add an extra language test in
> > tests/language/src
> 
> (InterfaceFactory3NegativeTest.dart which is marked failed, Bug 5387405)

  This is change in different place of Resolver, so I'd like do this in separate
CL.

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

http://codereview.chromium.org/8747016/diff/1/compiler/java/com/google/dart/c...
compiler/java/com/google/dart/compiler/resolver/SyntheticDefaultConstructorElement.java:99:
return dynamicType;
On 2011/11/30 16:48:28, zundel wrote:
> ? is this right. I  would think it would return the enclosing class type.

  For explicit constructors we also have "dynamic" return type.
  I will look on this in separate CL.

[zundel, 2011-11-30 21:22:14]

lgtm

http://codereview.chromium.org/8747016/diff/3001/compiler/java/com/google/dar...
File compiler/java/com/google/dart/compiler/resolver/ResolverErrorCode.java
(right):

http://codereview.chromium.org/8747016/diff/3001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/ResolverErrorCode.java:83:
NEW_EXPRESSION_FACTORY_CONSTRUCTOR("Can not resolve constructor with name '%s'
in factory '%s'"),
this looks more like the message the VM gives.