Issue 1575. Allow declare new named parameters in method override.

Allow declare new named parameters in method override.
http://code.google.com/p/dart/issues/detail?id=1575


R=karlklose@google.com,zundel@google.com
BUG=
TEST=

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

[karlklose, 2012-02-16 10:26:37]

First comments (mainly style issues).

I think part of the legal-override check should be in the resolver.

Specifically, the tests corresponding to the second paragraph of 8.1 must
produce compile-time errors and not only type errors. The subtype check should
stay in the TypeAnalyzer.

https://chromiumcodereview.appspot.com/9403026/diff/1/compiler/java/com/googl...
File compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java (right):

https://chromiumcodereview.appspot.com/9403026/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:1973: typeError(
Please add first argument (or even as many arguments as possible) in the first
line, and align the rest of the arguments with the first one:
  typeError(node,
            TypeErrorCode.CANNOT_OVERRIDE_TYPED_MEMBER,
            ...)
or

  typeError(node, TypeErrorCode.CANNOT_OVERRIDE_TYPED_MEMBER,
            name, superElement...

https://chromiumcodereview.appspot.com/9403026/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:1986: private
boolean checkMethodOverrideSignature(DartNode node,
Since it returns a boolean, consider changing the name to something like
hasLegal(Method)OverrideSignature.

https://chromiumcodereview.appspot.com/9403026/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:1987:
MethodElement method,
Please align parameters.

https://chromiumcodereview.appspot.com/9403026/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:1992: // # of
required parameters should be same.
# -> Number

https://chromiumcodereview.appspot.com/9403026/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:1997: onError(
Please align arguments.

https://chromiumcodereview.appspot.com/9403026/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:1997: onError(
Does calling onError from the TypeAnalyzer produce a compile time error?

https://chromiumcodereview.appspot.com/9403026/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:2018: node,
ditto.

https://chromiumcodereview.appspot.com/9403026/diff/1/compiler/javatests/com/...
File
compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerCompilerTest.java
(right):

https://chromiumcodereview.appspot.com/9403026/diff/1/compiler/javatests/com/...
compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerCompilerTest.java:838:
* <p>
The closing tag is missing.

https://chromiumcodereview.appspot.com/9403026/diff/1/compiler/javatests/com/...
compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerCompilerTest.java:858:
* <p>
ditto.

[scheglov, 2012-02-16 15:18:02]

Well, I also thought some time about where to keep override checks.
As you can see, initially they where in both places - Resolver and TypeAnalyzer,
and both places reported errors. I've decided to keep it only in TypeAnalyzer,
because it has more checks and need to know if there were "simple" cases of
parameters mismatch.

Alternatives:
1. keep code duplication;
2. make TypeAnalyzer call Resolver;
3. extract method into separate class Elements, but we need to extract also
onError() with it.
4. add some flags support for DartMethodDeclaration to allow Resolver mark that
this method is invalid override.

https://chromiumcodereview.appspot.com/9403026/diff/1/compiler/java/com/googl...
File compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java (right):

https://chromiumcodereview.appspot.com/9403026/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:1973: typeError(
On 2012/02/16 10:26:37, karlklose wrote:
> Please add first argument (or even as many arguments as possible) in the first
> line, and align the rest of the arguments with the first one:
>   typeError(node,
>             TypeErrorCode.CANNOT_OVERRIDE_TYPED_MEMBER,
>             ...)
> or
> 
>   typeError(node, TypeErrorCode.CANNOT_OVERRIDE_TYPED_MEMBER,
>             name, superElement...

Done.

https://chromiumcodereview.appspot.com/9403026/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:1986: private
boolean checkMethodOverrideSignature(DartNode node,
On 2012/02/16 10:26:37, karlklose wrote:
> Since it returns a boolean, consider changing the name to something like
> hasLegal(Method)OverrideSignature.

Done.

https://chromiumcodereview.appspot.com/9403026/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:1987:
MethodElement method,
On 2012/02/16 10:26:37, karlklose wrote:
> Please align parameters.

Done.
But I hate to see how long method name pushed _all_ parameters so far away.

https://chromiumcodereview.appspot.com/9403026/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:1992: // # of
required parameters should be same.
On 2012/02/16 10:26:37, karlklose wrote:
> # -> Number

Done.

https://chromiumcodereview.appspot.com/9403026/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:1997: onError(
On 2012/02/16 10:26:37, karlklose wrote:
> Please align arguments.

Done.

https://chromiumcodereview.appspot.com/9403026/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:1997: onError(
On 2012/02/16 10:26:37, karlklose wrote:
> Does calling onError from the TypeAnalyzer produce a compile time error?

Yes.
Technically we can report any kind of error.

https://chromiumcodereview.appspot.com/9403026/diff/1/compiler/java/com/googl...
compiler/java/com/google/dart/compiler/type/TypeAnalyzer.java:2018: node,
On 2012/02/16 10:26:37, karlklose wrote:
> ditto.

Done.

https://chromiumcodereview.appspot.com/9403026/diff/1/compiler/javatests/com/...
File
compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerCompilerTest.java
(right):

https://chromiumcodereview.appspot.com/9403026/diff/1/compiler/javatests/com/...
compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerCompilerTest.java:838:
* <p>
On 2012/02/16 10:26:37, karlklose wrote:
> The closing tag is missing.

No, it is intentionally omitted for clarity.
http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html

https://chromiumcodereview.appspot.com/9403026/diff/1/compiler/javatests/com/...
compiler/javatests/com/google/dart/compiler/type/TypeAnalyzerCompilerTest.java:858:
* <p>
On 2012/02/16 10:26:37, karlklose wrote:
> ditto.

Actually this is generally accepted practice.
I see only 3 </p> in DartC code vs 35 <p>.

[karlklose, 2012-02-21 12:52:06]

LGTM, thanks for fixing that issue.

[zundel, 2012-02-22 14:26:07]

A little late, but LGTM2