Perform override and inheritance checks.

Perform override and inheritance checks.

R=karlklose@google.com

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

[Johnni Winther, 2014-01-17 10:21:54]

https://codereview.chromium.org/140803002/diff/1/tests/language/language_dart...
File tests/language/language_dart2js.status (right):

https://codereview.chromium.org/140803002/diff/1/tests/language/language_dart...
tests/language/language_dart2js.status:159: #*: Fail, Pass # TODO(ahe): Triage
these tests.
This should not be commented out.

[karlklose, 2014-01-28 12:27:45]

LGTM.

https://codereview.chromium.org/140803002/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/resolution/class_members.dart
(right):

https://codereview.chromium.org/140803002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/resolution/class_members.dart:42:
Map<Object, Set<MessageKind>> reportedMessages =
Please add a comment about the possible types of keys (perhaps inline like:
'Map<Object /* a | b */, Set<MessageKind>>').

Would it make sense to use dynamic here?

https://codereview.chromium.org/140803002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/resolution/class_members.dart:65: //
Check for unimplemented member on concrete classes that neither have
'member' -> 'members'?

https://codereview.chromium.org/140803002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/resolution/class_members.dart:66: // a
@proxy annotation nor declare a `noSuchMethod` method.
'@proxy' -> '`@proxy`'.

https://codereview.chromium.org/140803002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/resolution/class_members.dart:339:
void checkInterfaceImplementation() {
Please add a comment on what this method checks.

https://codereview.chromium.org/140803002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/resolution/class_members.dart:345:
/*if (compiler.inUserCode(cls)) {
Remove debug code.

https://codereview.chromium.org/140803002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/resolution/class_members.dart:411: if
(superMember != null) {
Perhaps 'assert(declared.name == superMember.name)'.

https://codereview.chromium.org/140803002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/resolution/class_members.dart:511: }
else {
Consider changing the else-branch to be the then-branch; it is shorter than the
other branch and in this order is is easier to skip it.

https://codereview.chromium.org/140803002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/resolution/class_members.dart:515:
//print('checkValidOverride($declared,$superclass');
Remove debug code.

https://codereview.chromium.org/140803002/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/warnings.dart (right):

https://codereview.chromium.org/140803002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/warnings.dart:1527: 
I would prefer shortening the examples - at least by removing the empty lines -
to make it easier to navigate the MessageKinds without tool support.

https://codereview.chromium.org/140803002/diff/1/tests/compiler/dart2js/analy...
File tests/compiler/dart2js/analyze_api_test.dart (right):

https://codereview.chromium.org/140803002/diff/1/tests/compiler/dart2js/analy...
tests/compiler/dart2js/analyze_api_test.dart:25: "is not assignable to the type
'HtmlCollection'", // Issue 16105.
Remember to merge with https://codereview.chromium.org//140903003.

https://codereview.chromium.org/140803002/diff/1/tests/compiler/dart2js/analy...
File tests/compiler/dart2js/analyze_helper.dart (right):

https://codereview.chromium.org/140803002/diff/1/tests/compiler/dart2js/analy...
tests/compiler/dart2js/analyze_helper.dart:33: bool lastWasWhitelisted = false;
It would be nice if Infos carried a reference to the warning or error that
caused them, so we could explicitly check if that warning/error is white-listed;
because then the Info is, too.

https://codereview.chromium.org/140803002/diff/1/tests/compiler/dart2js/overr...
File tests/compiler/dart2js/override_inheritance_test.dart (right):

https://codereview.chromium.org/140803002/diff/1/tests/compiler/dart2js/overr...
tests/compiler/dart2js/override_inheritance_test.dart:30: if (errors == null)
errors = [];
Consider adding a helper like
  toList(o) => o == null ? [] : o is List ? o : [o];
for use with compareMessageKinds.

https://codereview.chromium.org/140803002/diff/1/tests/compiler/dart2js/overr...
tests/compiler/dart2js/override_inheritance_test.dart:49:
testMoreRequiredParameters() {
This method is also testing equal numbers of required parameters, as is the next
one.

Consider renaming the test methods.

https://codereview.chromium.org/140803002/diff/1/tests/compiler/dart2js/overr...
tests/compiler/dart2js/override_inheritance_test.dart:254: testNotSubtype() {
This method also tests the positive test. Consider renaming it to testSubtyping.

https://codereview.chromium.org/140803002/diff/1/tests/compiler/dart2js/overr...
tests/compiler/dart2js/override_inheritance_test.dart:344: check("""
I would argue that 'inherited(C, _)' contains 'method: (dynamic) -> dynamic' and
thus there is no warning because we check 'Class.method' against that.

Could you file a bug for Gilad to clarify and add a comment here.

https://codereview.chromium.org/140803002/diff/1/tests/compiler/dart2js/overr...
tests/compiler/dart2js/override_inheritance_test.dart:359: check("""
Ditto.

https://codereview.chromium.org/140803002/diff/1/tests/compiler/dart2js/overr...
tests/compiler/dart2js/override_inheritance_test.dart:376: check("""
Move this up after l. 357 (it is just testing a symmetric case, isn't it?)

https://codereview.chromium.org/140803002/diff/1/tests/compiler/dart2js/overr...
tests/compiler/dart2js/override_inheritance_test.dart:509: check("""
Also add a TODO to wait for clarification from Gilad.

https://codereview.chromium.org/140803002/diff/1/tests/compiler/dart2js/overr...
tests/compiler/dart2js/override_inheritance_test.dart:1482: check("""
I am not sure about this test. Please double check that @proxy is enough to
suppress the warning.

https://codereview.chromium.org/140803002/diff/1/tests/language/override_inhe...
File tests/language/override_inheritance_abstract_test.dart (right):

https://codereview.chromium.org/140803002/diff/1/tests/language/override_inhe...
tests/language/override_inheritance_abstract_test.dart:1: // Copyright (c) 2013,
the Dart project authors.  Please see the AUTHORS file
Please update the copyright year in new tests.

https://codereview.chromium.org/140803002/diff/1/tests/language/override_inhe...
File tests/language/override_inheritance_mixed_test.dart (right):

https://codereview.chromium.org/140803002/diff/1/tests/language/override_inhe...
tests/language/override_inheritance_mixed_test.dart:12: abstract class B extends
A implements I, J {
Move class after I and J?

https://codereview.chromium.org/140803002/diff/1/tests/language/override_inhe...
tests/language/override_inheritance_mixed_test.dart:13: 
Remove empty line.

https://codereview.chromium.org/140803002/diff/1/tests/language/override_inhe...
File tests/language/override_inheritance_no_such_method_test.dart (right):

https://codereview.chromium.org/140803002/diff/1/tests/language/override_inhe...
tests/language/override_inheritance_no_such_method_test.dart:26: method2(); ///
02: continued
Not sure about this one.

[Johnni Winther, 2014-01-31 12:23:05]

Committed patchset #1 manually as r32192 (presubmit successful).

[Johnni Winther, 2014-01-31 12:29:30]

https://codereview.chromium.org/140803002/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/resolution/class_members.dart
(right):

https://codereview.chromium.org/140803002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/resolution/class_members.dart:42:
Map<Object, Set<MessageKind>> reportedMessages =
On 2014/01/28 12:27:45, karlklose wrote:
> Please add a comment about the possible types of keys (perhaps inline like:
> 'Map<Object /* a | b */, Set<MessageKind>>').
> 
> Would it make sense to use dynamic here?

Done.

https://codereview.chromium.org/140803002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/resolution/class_members.dart:65: //
Check for unimplemented member on concrete classes that neither have
On 2014/01/28 12:27:45, karlklose wrote:
> 'member' -> 'members'?

Done.

https://codereview.chromium.org/140803002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/resolution/class_members.dart:66: // a
@proxy annotation nor declare a `noSuchMethod` method.
On 2014/01/28 12:27:45, karlklose wrote:
> '@proxy' -> '`@proxy`'.

Done.

https://codereview.chromium.org/140803002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/resolution/class_members.dart:339:
void checkInterfaceImplementation() {
On 2014/01/28 12:27:45, karlklose wrote:
> Please add a comment on what this method checks.

Done.

https://codereview.chromium.org/140803002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/resolution/class_members.dart:345:
/*if (compiler.inUserCode(cls)) {
On 2014/01/28 12:27:45, karlklose wrote:
> Remove debug code.

Done.

https://codereview.chromium.org/140803002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/resolution/class_members.dart:411: if
(superMember != null) {
On 2014/01/28 12:27:45, karlklose wrote:
> Perhaps 'assert(declared.name == superMember.name)'.

Done.

https://codereview.chromium.org/140803002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/resolution/class_members.dart:511: }
else {
On 2014/01/28 12:27:45, karlklose wrote:
> Consider changing the else-branch to be the then-branch; it is shorter than
the
> other branch and in this order is is easier to skip it.

Done.

https://codereview.chromium.org/140803002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/resolution/class_members.dart:515:
//print('checkValidOverride($declared,$superclass');
On 2014/01/28 12:27:45, karlklose wrote:
> Remove debug code.

Done.

https://codereview.chromium.org/140803002/diff/1/sdk/lib/_internal/compiler/i...
File sdk/lib/_internal/compiler/implementation/warnings.dart (right):

https://codereview.chromium.org/140803002/diff/1/sdk/lib/_internal/compiler/i...
sdk/lib/_internal/compiler/implementation/warnings.dart:1527: 
On 2014/01/28 12:27:45, karlklose wrote:
> I would prefer shortening the examples - at least by removing the empty lines
-
> to make it easier to navigate the MessageKinds without tool support.

Done.

https://codereview.chromium.org/140803002/diff/1/tests/compiler/dart2js/analy...
File tests/compiler/dart2js/analyze_helper.dart (right):

https://codereview.chromium.org/140803002/diff/1/tests/compiler/dart2js/analy...
tests/compiler/dart2js/analyze_helper.dart:33: bool lastWasWhitelisted = false;
On 2014/01/28 12:27:45, karlklose wrote:
> It would be nice if Infos carried a reference to the warning or error that
> caused them, so we could explicitly check if that warning/error is
white-listed;
> because then the Info is, too.

Added a TODO.

https://codereview.chromium.org/140803002/diff/1/tests/compiler/dart2js/overr...
File tests/compiler/dart2js/override_inheritance_test.dart (right):

https://codereview.chromium.org/140803002/diff/1/tests/compiler/dart2js/overr...
tests/compiler/dart2js/override_inheritance_test.dart:30: if (errors == null)
errors = [];
On 2014/01/28 12:27:45, karlklose wrote:
> Consider adding a helper like
>   toList(o) => o == null ? [] : o is List ? o : [o];
> for use with compareMessageKinds.

Done.

https://codereview.chromium.org/140803002/diff/1/tests/compiler/dart2js/overr...
tests/compiler/dart2js/override_inheritance_test.dart:49:
testMoreRequiredParameters() {
On 2014/01/28 12:27:45, karlklose wrote:
> This method is also testing equal numbers of required parameters, as is the
next
> one.
> 
> Consider renaming the test methods.

Done.

https://codereview.chromium.org/140803002/diff/1/tests/compiler/dart2js/overr...
tests/compiler/dart2js/override_inheritance_test.dart:254: testNotSubtype() {
On 2014/01/28 12:27:45, karlklose wrote:
> This method also tests the positive test. Consider renaming it to
testSubtyping.

Done.

https://codereview.chromium.org/140803002/diff/1/tests/compiler/dart2js/overr...
tests/compiler/dart2js/override_inheritance_test.dart:344: check("""
On 2014/01/28 12:27:45, karlklose wrote:
> I would argue that 'inherited(C, _)' contains 'method: (dynamic) -> dynamic'
and
> thus there is no warning because we check 'Class.method' against that.
> 
> Could you file a bug for Gilad to clarify and add a comment here.

Done.

https://codereview.chromium.org/140803002/diff/1/tests/compiler/dart2js/overr...
tests/compiler/dart2js/override_inheritance_test.dart:359: check("""
On 2014/01/28 12:27:45, karlklose wrote:
> Ditto.

Done.

https://codereview.chromium.org/140803002/diff/1/tests/compiler/dart2js/overr...
tests/compiler/dart2js/override_inheritance_test.dart:376: check("""
On 2014/01/28 12:27:45, karlklose wrote:
> Move this up after l. 357 (it is just testing a symmetric case, isn't it?)

Done.

https://codereview.chromium.org/140803002/diff/1/tests/compiler/dart2js/overr...
tests/compiler/dart2js/override_inheritance_test.dart:509: check("""
On 2014/01/28 12:27:45, karlklose wrote:
> Also add a TODO to wait for clarification from Gilad.

Done.

https://codereview.chromium.org/140803002/diff/1/tests/compiler/dart2js/overr...
tests/compiler/dart2js/override_inheritance_test.dart:1482: check("""
On 2014/01/28 12:27:45, karlklose wrote:
> I am not sure about this test. Please double check that @proxy is enough to
> suppress the warning.

The spec has changed. Filed a bug with co19.

https://codereview.chromium.org/140803002/diff/1/tests/language/override_inhe...
File tests/language/override_inheritance_abstract_test.dart (right):

https://codereview.chromium.org/140803002/diff/1/tests/language/override_inhe...
tests/language/override_inheritance_abstract_test.dart:1: // Copyright (c) 2013,
the Dart project authors.  Please see the AUTHORS file
On 2014/01/28 12:27:45, karlklose wrote:
> Please update the copyright year in new tests.

Done.

https://codereview.chromium.org/140803002/diff/1/tests/language/override_inhe...
File tests/language/override_inheritance_mixed_test.dart (right):

https://codereview.chromium.org/140803002/diff/1/tests/language/override_inhe...
tests/language/override_inheritance_mixed_test.dart:12: abstract class B extends
A implements I, J {
On 2014/01/28 12:27:45, karlklose wrote:
> Move class after I and J?

Done.

https://codereview.chromium.org/140803002/diff/1/tests/language/override_inhe...
tests/language/override_inheritance_mixed_test.dart:13: 
On 2014/01/28 12:27:45, karlklose wrote:
> Remove empty line.

Done.

https://codereview.chromium.org/140803002/diff/1/tests/language/override_inhe...
File tests/language/override_inheritance_no_such_method_test.dart (right):

https://codereview.chromium.org/140803002/diff/1/tests/language/override_inhe...
tests/language/override_inheritance_no_such_method_test.dart:26: method2(); ///
02: continued
On 2014/01/28 12:27:45, karlklose wrote:
> Not sure about this one.

Changed to expect static warning (together with 07, 10).

[ahe, 2014-04-10 12:29:03]

Please make sure to upload further patch sets so the committed code matches what
the code review tool shows.

In this case, the commit contains an unreviewed change to
tests/compiler/dart2js/message_kind_test.dart.