Checkpoint for more inheritance error codes 'Missing inherited member' warnings.

Checkpoint for more inheritance error codes 'Missing inherited member' warnings.
Added isAbstract to the PropertyAccessorElement

R=brianwilkerson@google.com, scheglov@google.com

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

[Brian Wilkerson, 2013-05-20 22:51:49]

LGTM

https://codereview.chromium.org/15500002/diff/22/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/element/PropertyAccessorElement.java
(right):

https://codereview.chromium.org/15500002/diff/22/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/element/PropertyAccessorElement.java:43:
* Return {@code true} if this method is abstract. Methods are abstract if they
are not external
nit: "method" --> "accessor", "Methods" --> "Accessors"

https://codereview.chromium.org/15500002/diff/22/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/builder/ElementBuilder.java
(right):

https://codereview.chromium.org/15500002/diff/22/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/builder/ElementBuilder.java:529:
&& !matches(node.getExternalKeyword(), Keyword.EXTERNAL));
I think you can replace the second half of this condition (and the one below)
with "node.getExternalKeyword() != null".

https://codereview.chromium.org/15500002/diff/22/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ErrorVerifier.java
(right):

https://codereview.chromium.org/15500002/diff/22/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ErrorVerifier.java:815:
MethodElement[] methods = enclosingClass.getMethods();
Seems like it would be faster to build a set containing the names of locally
defined instance methods (and re-use it throughout this loop).

https://codereview.chromium.org/15500002/diff/22/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ErrorVerifier.java:818:
if (methodName.equals(method.getName()) && method.isAccessibleIn(currentLibrary)
If the method is defined in the current class, which is defined in the current
library, then it will always be accessible in the current library.

https://codereview.chromium.org/15500002/diff/22/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ErrorVerifier.java:832:
// If the accessor is abstract, then verify that this class has a method which
overrides the
nit: "method" --> "accessor" (twice)

https://codereview.chromium.org/15500002/diff/22/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/internal/builder/ElementBuilderTest.java
(right):

https://codereview.chromium.org/15500002/diff/22/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/internal/builder/ElementBuilderTest.java:548:
assertFalse(getter.isAbstract());
There aren't any tests to ensure that isAbstract() will ever return true.

[scheglov, 2013-05-20 22:54:23]

lgtm

[jwren, 2013-05-21 18:21:30]

https://codereview.chromium.org/15500002/diff/22/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/element/PropertyAccessorElement.java
(right):

https://codereview.chromium.org/15500002/diff/22/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/element/PropertyAccessorElement.java:43:
* Return {@code true} if this method is abstract. Methods are abstract if they
are not external
On 2013/05/20 22:51:49, Brian Wilkerson wrote:
> nit: "method" --> "accessor", "Methods" --> "Accessors"

Done.

https://codereview.chromium.org/15500002/diff/22/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/builder/ElementBuilder.java
(right):

https://codereview.chromium.org/15500002/diff/22/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/builder/ElementBuilder.java:529:
&& !matches(node.getExternalKeyword(), Keyword.EXTERNAL));
On 2013/05/20 22:51:49, Brian Wilkerson wrote:
> I think you can replace the second half of this condition (and the one below)
> with "node.getExternalKeyword() != null".

Done.

https://codereview.chromium.org/15500002/diff/22/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ErrorVerifier.java
(right):

https://codereview.chromium.org/15500002/diff/22/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ErrorVerifier.java:815:
MethodElement[] methods = enclosingClass.getMethods();
On 2013/05/20 22:51:49, Brian Wilkerson wrote:
> Seems like it would be faster to build a set containing the names of locally
> defined instance methods (and re-use it throughout this loop).

Done.

https://codereview.chromium.org/15500002/diff/22/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ErrorVerifier.java:818:
if (methodName.equals(method.getName()) && method.isAccessibleIn(currentLibrary)
We can also get rid of the isStatic test, this work is done for us in the
InheritanceManager.

https://codereview.chromium.org/15500002/diff/22/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ErrorVerifier.java:832:
// If the accessor is abstract, then verify that this class has a method which
overrides the
On 2013/05/20 22:51:49, Brian Wilkerson wrote:
> nit: "method" --> "accessor" (twice)

Done.

https://codereview.chromium.org/15500002/diff/22/editor/tools/plugins/com.goo...
File
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/internal/builder/ElementBuilderTest.java
(right):

https://codereview.chromium.org/15500002/diff/22/editor/tools/plugins/com.goo...
editor/tools/plugins/com.google.dart.engine_test/src/com/google/dart/engine/internal/builder/ElementBuilderTest.java:548:
assertFalse(getter.isAbstract());
On 2013/05/20 22:51:49, Brian Wilkerson wrote:
> There aren't any tests to ensure that isAbstract() will ever return true.

Done.

[jwren, 2013-05-21 18:26:19]

Committed patchset #5 manually as r22949 (presubmit successful).

[Brian Wilkerson, 2013-05-21 18:38:25]

https://codereview.chromium.org/15500002/diff/14001/editor/tools/plugins/com....
File
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ErrorVerifier.java
(right):

https://codereview.chromium.org/15500002/diff/14001/editor/tools/plugins/com....
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ErrorVerifier.java:811:
methodsInEnclosingClass.add(method.getName());
I think you only want to add the names of non-static members.

https://codereview.chromium.org/15500002/diff/14001/editor/tools/plugins/com....
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ErrorVerifier.java:828:
boolean foundOverridingMethod = false;
Note that neither the variable nor the 'if' is necessary. The condition
"!foundOverridingMethod" could be replaced by
"!methodsInEnclosingClass.contains(methodName)".

https://codereview.chromium.org/15500002/diff/14001/editor/tools/plugins/com....
editor/tools/plugins/com.google.dart.engine/src/com/google/dart/engine/internal/verifier/ErrorVerifier.java:864:
if (elt instanceof MethodElement && !((MethodElement) elt).isAbstract()) {
Do we want to include the test for isAbstract()? Consider the following case:

abstract class A { void m(); }
abstract class B { void m(); }
class C extends A implements B {}

I think that with the test for isAbstract() the verifier will produce two errors
while without it only one error will be produced. I think one error is better.