remove duplicate checking for overrides

remove duplicate checking for overrides

We were checking for override errors in two places: strong/checker.dart and error_verifier.dart, so this removes one of them.

I went for skipping the ErrorVerifier errors in strong mode. That path produces better error messages, but it's not expressed in terms of the subtype relation directly, but rather encodes information about how function subtyping works. This makes it difficult to keep in sync with our typing logic. So for now it seems pragmatic to prefer the checker implementation.

R=brianwilkerson@google.com

Committed: https://github.com/dart-lang/sdk/commit/a7a9fc1b377ea96c7d4439f380a380c846f7f19c

[Jennifer Messerly, 2016-08-16 00:20:40]

Description was changed from

==========
remove duplicate checking for overrides

We were checking for override errors in two places: strong/checker.dart and
error_verifier.dart, so this removes one of them.

I went for skipping the ErrorVerifier errors in strong mode. That path produces
better error messages, but it's not expressed in terms of the subtype relation
directly, but rather encodes information about how function subtyping works.
This makes it difficult to keep in sync with our typing logic. So for now it
seems pragmatic to prefer the checker implementation.
==========

to

==========
remove duplicate checking for overrides

We were checking for override errors in two places: strong/checker.dart and
error_verifier.dart, so this removes one of them.

I went for skipping the ErrorVerifier errors in strong mode. That path produces
better error messages, but it's not expressed in terms of the subtype relation
directly, but rather encodes information about how function subtyping works.
This makes it difficult to keep in sync with our typing logic. So for now it
seems pragmatic to prefer the checker implementation.
==========

[Jennifer Messerly, 2016-08-16 00:20:40]

jmesserly@google.com changed reviewers:
+ brianwilkerson@google.com, leafp@google.com

[Jennifer Messerly, 2016-08-16 00:38:15]

Hey Brian & Leaf appreciate your thoughts on this change.

I had to remove one of the two checks to make way for covariance checking,
rather than implement it twice. Not sure if I picked the right one. Thoughts
appreciated.

[Brian Wilkerson, 2016-08-16 00:41:23]

> We were checking for override errors in two places:
> strong/checker.dart and error_verifier.dart, so this removes one
> of them.

Actually, it only bypasses one of them when strong mode is enabled. Both sets of
code are still there.

> I went for skipping the ErrorVerifier errors in strong mode. That
> path produces better error messages, but it's not expressed in
> terms of the subtype relation directly ...

I'd prefer to have the ErrorVerifier code fixed to be expressed in terms of the
subtype relationship and then have a single copy of the code: the one that
produces the better error messages.

[Jennifer Messerly, 2016-08-16 00:42:03]

On 2016/08/16 00:38:15, John Messerly wrote:
> Hey Brian & Leaf appreciate your thoughts on this change.
> 
> I had to remove one of the two checks to make way for covariance checking,
> rather than implement it twice. Not sure if I picked the right one. Thoughts
> appreciated.

Pros to this approach:
* simpler implementation, less code size

Cons to this approach:
* it's a bit of a regression for error message UX

Although going either way will remove the duplicate messages so that's something
:)

[Jennifer Messerly, 2016-08-16 00:45:53]

On 2016/08/16 00:41:23, Brian Wilkerson wrote:
> > We were checking for override errors in two places:
> > strong/checker.dart and error_verifier.dart, so this removes one
> > of them.
> 
> Actually, it only bypasses one of them when strong mode is enabled. Both sets
of
> code are still there.

yeah that's fair. I presume we'll remove non-strong mode support at some point
along the way to Dart2.

> > I went for skipping the ErrorVerifier errors in strong mode. That
> > path produces better error messages, but it's not expressed in
> > terms of the subtype relation directly ...
> 
> I'd prefer to have the ErrorVerifier code fixed to be expressed in terms of
the
> subtype relationship and then have a single copy of the code: the one that
> produces the better error messages.

yeah I think we can fix it once we only have one type system to worry about. The
problem now is I'd be spending a lot of time trying to retrofit and test with 
Dart 1 and 2, which seems like not the best way to spend effort.

I could try and improve Checker.dart code path (and that code could certainly be
moved into ErrorVerifier), but on the stack rank of things I need to do for
DDC/Strong Mode I'm not sure slightly better override error UX would be the
thing.

[Jennifer Messerly, 2016-08-16 00:50:43]

to make this more concrete we are choosing between:
https://github.com/dart-lang/sdk/blob/3f9379199131ca780ff0e1602448aa6b85436b0...

vs:

https://github.com/dart-lang/sdk/blob/3f9379199131ca780ff0e1602448aa6b85436b0...
... plus the associated logic in InheritanceManager.

[Jennifer Messerly, 2016-08-16 01:00:15]

On 2016/08/16 00:50:43, John Messerly wrote:
> to make this more concrete we are choosing between:
>
https://github.com/dart-lang/sdk/blob/3f9379199131ca780ff0e1602448aa6b85436b0...
> 
> vs:
> 
>
https://github.com/dart-lang/sdk/blob/3f9379199131ca780ff0e1602448aa6b85436b0...
> ... plus the associated logic in InheritanceManager.

I guess the other thing here is the two paths aren't exactly equivalent (if you
look through them), it seems like the Checker one is supposed a few strong mode
features, judging by its comments. Though not sure if anyone can use them given
that we issue errors from both places.

[Brian Wilkerson, 2016-08-16 17:28:22]

After our off-line conversation, lgtm.

[Leaf, 2016-08-17 23:47:27]

On 2016/08/16 17:28:22, Brian Wilkerson wrote:
> After our off-line conversation, lgtm.

I'm worried about the possibility of missing checks in the strong mode version
that aren't captured by the subset of the tests which are run in strong mode.  
Or looked at differently, I think there's an implicit regression in our test
coverage with this because we're now relying solely on the set of tests that we
have for strong mode.  Is this a reasonable concern?  I'm ok with landing this
if we're reasonably sure that the existing strong mode checker does all of the
right checking, but I'm wondering whether we should make a concerted effort to
port over all of the analyzer tests to run under strong mode with the
appropriately modified expectations.

[Jennifer Messerly, 2016-08-17 23:53:06]

On 2016/08/17 23:47:27, Leaf wrote:
> On 2016/08/16 17:28:22, Brian Wilkerson wrote:
> > After our off-line conversation, lgtm.
> 
> I'm worried about the possibility of missing checks in the strong mode version
> that aren't captured by the subset of the tests which are run in strong mode. 

> Or looked at differently, I think there's an implicit regression in our test
> coverage with this because we're now relying solely on the set of tests that
we
> have for strong mode.  Is this a reasonable concern?  I'm ok with landing this
> if we're reasonably sure that the existing strong mode checker does all of the
> right checking, but I'm wondering whether we should make a concerted effort to
> port over all of the analyzer tests to run under strong mode with the
> appropriately modified expectations.

I don't understand how we're losing coverage for... anything? Very few Analyzer
tests turn on strong mode. The three main ones are inferred_type_test,
checker_test, and strong_mode_test

You're 100% right that Analyzer has a lot of good tests we should port over to
strong mode.

[Leaf, 2016-08-18 17:14:47]

On 2016/08/17 23:53:06, John Messerly wrote:
> On 2016/08/17 23:47:27, Leaf wrote:
> > On 2016/08/16 17:28:22, Brian Wilkerson wrote:
> > > After our off-line conversation, lgtm.
> > 
> > I'm worried about the possibility of missing checks in the strong mode
version
> > that aren't captured by the subset of the tests which are run in strong
mode. 
> 
> > Or looked at differently, I think there's an implicit regression in our test
> > coverage with this because we're now relying solely on the set of tests that
> we
> > have for strong mode.  Is this a reasonable concern?  I'm ok with landing
this
> > if we're reasonably sure that the existing strong mode checker does all of
the
> > right checking, but I'm wondering whether we should make a concerted effort
to
> > port over all of the analyzer tests to run under strong mode with the
> > appropriately modified expectations.
> 
> I don't understand how we're losing coverage for... anything? Very few
Analyzer
> tests turn on strong mode. The three main ones are inferred_type_test,
> checker_test, and strong_mode_test
> 
> You're 100% right that Analyzer has a lot of good tests we should port over to
> strong mode.

Test coverage may not exactly be the right concept.  The concern that I have is
that there may be  error cases such that:
  1) The Dart 1.0 override checker emits an error
  2) There is a Dart 1.0 test to ensure that this doesn't regress
  3) The Dart 2.0 checker may or may not emit an error
  4) There is no Dart 2.0 test to ensure that this doesn't regress

Currently, we will get either 1 or 2 errors, and if we regress to 0 errors we
will catch it (since there is a Dart 1.0 test to ensure that we get a last 1
error).  We will not catch if regress from 2 to 1 error.

In the new setup, we will get either 0 or 1 errors, and if we regress from 1 to
0 errors we will not catch it.

I think this is a broader problem though.  I'd propose landing this, and I will
try to get something started on cleaning up/combining tests.

[Jennifer Messerly, 2016-08-18 17:25:18]

On 2016/08/18 17:14:47, Leaf wrote:
> On 2016/08/17 23:53:06, John Messerly wrote:
> > On 2016/08/17 23:47:27, Leaf wrote:
> > > On 2016/08/16 17:28:22, Brian Wilkerson wrote:
> > > > After our off-line conversation, lgtm.
> > > 
> > > I'm worried about the possibility of missing checks in the strong mode
> version
> > > that aren't captured by the subset of the tests which are run in strong
> mode. 
> > 
> > > Or looked at differently, I think there's an implicit regression in our
test
> > > coverage with this because we're now relying solely on the set of tests
that
> > we
> > > have for strong mode.  Is this a reasonable concern?  I'm ok with landing
> this
> > > if we're reasonably sure that the existing strong mode checker does all of
> the
> > > right checking, but I'm wondering whether we should make a concerted
effort
> to
> > > port over all of the analyzer tests to run under strong mode with the
> > > appropriately modified expectations.
> > 
> > I don't understand how we're losing coverage for... anything? Very few
> Analyzer
> > tests turn on strong mode. The three main ones are inferred_type_test,
> > checker_test, and strong_mode_test
> > 
> > You're 100% right that Analyzer has a lot of good tests we should port over
to
> > strong mode.
> 
> Test coverage may not exactly be the right concept.  The concern that I have
is
> that there may be  error cases such that:
>   1) The Dart 1.0 override checker emits an error
>   2) There is a Dart 1.0 test to ensure that this doesn't regress
>   3) The Dart 2.0 checker may or may not emit an error
>   4) There is no Dart 2.0 test to ensure that this doesn't regress
> 
> Currently, we will get either 1 or 2 errors, and if we regress to 0 errors we
> will catch it (since there is a Dart 1.0 test to ensure that we get a last 1
> error).  We will not catch if regress from 2 to 1 error.
> 
> In the new setup, we will get either 0 or 1 errors, and if we regress from 1
to
> 0 errors we will not catch it.
> 
> I think this is a broader problem though.  I'd propose landing this, and I
will
> try to get something started on cleaning up/combining tests.

sounds good to me. Note, the assumption that the Dart 1.0 checker issues the
right error when run under the strong mode is definitely not true... we've had
at least 3-4 bugs, those are just the ones I know of.

[Jennifer Messerly, 2016-08-19 19:27:13]

On 2016/08/18 17:25:18, John Messerly wrote:
> On 2016/08/18 17:14:47, Leaf wrote:
> > On 2016/08/17 23:53:06, John Messerly wrote:
> > > On 2016/08/17 23:47:27, Leaf wrote:
> > > > On 2016/08/16 17:28:22, Brian Wilkerson wrote:
> > > > > After our off-line conversation, lgtm.
> > > > 
> > > > I'm worried about the possibility of missing checks in the strong mode
> > version
> > > > that aren't captured by the subset of the tests which are run in strong
> > mode. 
> > > 
> > > > Or looked at differently, I think there's an implicit regression in our
> test
> > > > coverage with this because we're now relying solely on the set of tests
> that
> > > we
> > > > have for strong mode.  Is this a reasonable concern?  I'm ok with
landing
> > this
> > > > if we're reasonably sure that the existing strong mode checker does all
of
> > the
> > > > right checking, but I'm wondering whether we should make a concerted
> effort
> > to
> > > > port over all of the analyzer tests to run under strong mode with the
> > > > appropriately modified expectations.
> > > 
> > > I don't understand how we're losing coverage for... anything? Very few
> > Analyzer
> > > tests turn on strong mode. The three main ones are inferred_type_test,
> > > checker_test, and strong_mode_test
> > > 
> > > You're 100% right that Analyzer has a lot of good tests we should port
over
> to
> > > strong mode.
> > 
> > Test coverage may not exactly be the right concept.  The concern that I have
> is
> > that there may be  error cases such that:
> >   1) The Dart 1.0 override checker emits an error
> >   2) There is a Dart 1.0 test to ensure that this doesn't regress
> >   3) The Dart 2.0 checker may or may not emit an error
> >   4) There is no Dart 2.0 test to ensure that this doesn't regress
> > 
> > Currently, we will get either 1 or 2 errors, and if we regress to 0 errors
we
> > will catch it (since there is a Dart 1.0 test to ensure that we get a last 1
> > error).  We will not catch if regress from 2 to 1 error.
> > 
> > In the new setup, we will get either 0 or 1 errors, and if we regress from 1
> to
> > 0 errors we will not catch it.
> > 
> > I think this is a broader problem though.  I'd propose landing this, and I
> will
> > try to get something started on cleaning up/combining tests.
> 
> sounds good to me. Note, the assumption that the Dart 1.0 checker issues the
> right error when run under the strong mode is definitely not true... we've had
> at least 3-4 bugs, those are just the ones I know of.

chatted with Leaf and this sounds good to land. I also filed
https://github.com/dart-lang/sdk/issues/27118 to track the UX issue

[Jennifer Messerly, 2016-08-19 19:27:25]

Description was changed from

==========
remove duplicate checking for overrides

We were checking for override errors in two places: strong/checker.dart and
error_verifier.dart, so this removes one of them.

I went for skipping the ErrorVerifier errors in strong mode. That path produces
better error messages, but it's not expressed in terms of the subtype relation
directly, but rather encodes information about how function subtyping works.
This makes it difficult to keep in sync with our typing logic. So for now it
seems pragmatic to prefer the checker implementation.
==========

to

==========
remove duplicate checking for overrides

We were checking for override errors in two places: strong/checker.dart and
error_verifier.dart, so this removes one of them.

I went for skipping the ErrorVerifier errors in strong mode. That path produces
better error messages, but it's not expressed in terms of the subtype relation
directly, but rather encodes information about how function subtyping works.
This makes it difficult to keep in sync with our typing logic. So for now it
seems pragmatic to prefer the checker implementation.

R=brianwilkerson@google.com

Committed:
https://github.com/dart-lang/sdk/commit/a7a9fc1b377ea96c7d4439f380a380c846f7f19c
==========

[Jennifer Messerly, 2016-08-19 19:27:26]

Committed patchset #3 (id:40001) manually as
a7a9fc1b377ea96c7d4439f380a380c846f7f19c (presubmit successful).