Make method overrides with differing parameters a compile-time error

This adds a check to the resolver to detect when an override
has different number of parameters than in its superinterface.

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

[zundel, 2011-12-08 23:29:30]

@Peter, would you mind taking look at the updated test?  I removed the VMOptions
setting because this test doesn't involve static type errors.  I included
instantiating the type so that Frog would get an opportunity to check the class
- I know that Frog defers some checks until just before execution.

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

http://codereview.chromium.org/8632020/diff/3001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/Elements.java:59: public static
LibraryElement getLibraryElement(Element element) {
I'm expected such a method isn't already in the codebase, but I couldn't find
it.

[ahe, 2011-12-09 09:00:44]

http://codereview.chromium.org/8632020/diff/3001/tests/language/src/ClassOver...
File tests/language/src/ClassOverrideNegativeTest.dart (right):

http://codereview.chromium.org/8632020/diff/3001/tests/language/src/ClassOver...
tests/language/src/ClassOverrideNegativeTest.dart:6: // an interface method m2
and has a different number of required parameters.
This file LGTM!

[scheglov, 2011-12-09 16:49:51]

LGTM

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

http://codereview.chromium.org/8632020/diff/3001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:415:
onError(functionNode,
May be highlight only name of method.

http://codereview.chromium.org/8632020/diff/3001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:433:
onError(functionNode,
May be highlight only name of method.

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

http://codereview.chromium.org/8632020/diff/3001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/ResolverErrorCode.java:29:
CANNOT_OVERRIDE_METHOD_NUM_NAMED_PARAMS(
Formatting (indentation)

[mmendez, 2011-12-09 18:38:10]

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

http://codereview.chromium.org/8632020/diff/3001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:393: if
(ElementKind.of(currentHolder).equals(ElementKind.CLASS)) {
We should discuss offline, but I was wondering why this check did not happen as
part of the checks that the TypeAnalyzer performs?  I thought that it has some
override checking already.

[mmendez, 2011-12-09 21:09:38]

LGTM with one nit about where the check should occur.

On 2011/12/09 18:38:10, mmendez wrote:
>
http://codereview.chromium.org/8632020/diff/3001/compiler/java/com/google/dar...
> File compiler/java/com/google/dart/compiler/resolver/Resolver.java (right):
> 
>
http://codereview.chromium.org/8632020/diff/3001/compiler/java/com/google/dar...
> compiler/java/com/google/dart/compiler/resolver/Resolver.java:393: if
> (ElementKind.of(currentHolder).equals(ElementKind.CLASS)) {
> We should discuss offline, but I was wondering why this check did not happen
as
> part of the checks that the TypeAnalyzer performs?  I thought that it has some
> override checking already.

[mmendez, 2011-12-09 21:10:32]

Still LGTM, but I'm not sure why this comment was not included with the previous
post.

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

http://codereview.chromium.org/8632020/diff/13001/compiler/java/com/google/da...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:396: if
(ElementKind.of(currentHolder).equals(ElementKind.CLASS)) {
Nit: TypeAnalayzer does in fact do some of this same work and it caches
information so it should be faster.  It computes overrides, filters out private
members and then calls TypeAnalyzer.checkOverride passing the overridding method
and the overridden method.  You could consider moving these checks there.

Feel free to take or ignore this feedback.

[zundel, 2011-12-12 22:42:29]

I committed this with the check still in the resolver.

Trying to move the checks into the TypeAnalyzerTest reveals a weakness in our
test infrastructure - there is currently no way to test for resovler errors in
the TypeAnalyzer test. When I tried to turn them on I got lots of errors
reported for the existing tests, so its going to require some rework.

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

http://codereview.chromium.org/8632020/diff/3001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:393: if
(ElementKind.of(currentHolder).equals(ElementKind.CLASS)) {
On 2011/12/09 18:38:10, mmendez wrote:
> We should discuss offline, but I was wondering why this check did not happen
as
> part of the checks that the TypeAnalyzer performs?  I thought that it has some
> override checking already.

It does, but those are type errors, these are compile time errors, so I thought
it best to add them here.  These checks have nothing to do with the types of the
methods.

http://codereview.chromium.org/8632020/diff/3001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:415:
onError(functionNode,
On 2011/12/09 16:49:52, scheglov wrote:
> May be highlight only name of method.

Done.

http://codereview.chromium.org/8632020/diff/3001/compiler/java/com/google/dar...
compiler/java/com/google/dart/compiler/resolver/Resolver.java:433:
onError(functionNode,
On 2011/12/09 16:49:52, scheglov wrote:
> May be highlight only name of method.

Done.