fix #29766, fix #29782 - fix override checker's interface checking

fix #29766, fix #29782 - fix override checker's interface checking

R=vsm@google.com

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

[Jennifer Messerly, 2017-07-19 03:45:19]

jmesserly@google.com changed reviewers:
+ vsm@google.com

[Jennifer Messerly, 2017-07-19 03:45:21]

https://codereview.chromium.org/2981183003/diff/1/pkg/dev_compiler/tool/build...
File pkg/dev_compiler/tool/build_pkgs.dart (right):

https://codereview.chromium.org/2981183003/diff/1/pkg/dev_compiler/tool/build...
pkg/dev_compiler/tool/build_pkgs.dart:46: compileModule('collection',
unsafeForceCompile: true);
there's a soundness bug in package:collections:

[error] Invalid override. The type of 'MapKeySet.lookup' ('(E) → E') isn't a
subtype of 'Set<E>.lookup' ('(Object) → E').
(package:collection/src/wrappers.dart, line 453, col 3)

I'll send over a fix

[vsm, 2017-07-19 17:19:04]

lgtm with comments

does this logic need to be synced up somewhere in the new front end?

https://codereview.chromium.org/2981183003/diff/20001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/strong/checker.dart (right):

https://codereview.chromium.org/2981183003/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong/checker.dart:1898: // unless they are
explicitly marked as @virtual
Comment should be updated ....

https://codereview.chromium.org/2981183003/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong/checker.dart:1951: if (visited.add(t.element)
&& (filter == null || filter(t))) {
Will visited only trigger if the same type is mixed in twice?  Is it worth
moving more aggressively to make that an error?  

Otherwise, this is just the inheritance chain, right?

https://codereview.chromium.org/2981183003/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong/checker.dart:1953: t.mixins.forEach(visit);
Do the mixins end up getting added in the right order?  Does order matter?

https://codereview.chromium.org/2981183003/diff/20001/pkg/dev_compiler/tool/b...
File pkg/dev_compiler/tool/build_pkgs.dart (right):

https://codereview.chromium.org/2981183003/diff/20001/pkg/dev_compiler/tool/b...
pkg/dev_compiler/tool/build_pkgs.dart:46: compileModule('collection',
unsafeForceCompile: true);
This will probably break dartdevc snapshot generation too.  The snapshot is
built by compiling ddc against itself.  Looks like we import package:collection.

Can you land your fix there and roll it forward?  To roll forward, update this
to new tag or hash:

https://github.com/dart-lang/sdk/blob/master/DEPS#L61

[Paul Berry, 2017-07-19 17:26:47]

On 2017/07/19 17:19:04, vsm wrote:
> lgtm with comments
> 
> does this logic need to be synced up somewhere in the new front end?

There's no logic that needs to be fixed in the front end, because the logic that
would need to be fixed isn't implemented yet :) (the new front end has a small
amount of override checking logic, but it's incomplete and it happens before
inference, so it needs to be reworked anyhow).

But it would be really nice if this CL could add some test cases to
tests/language_2 so that once we get around to implementing the override
checking logic, there will already be tests to help us avoid making the same
mistake again.

[Jennifer Messerly, 2017-07-19 19:09:22]

On 2017/07/19 17:26:47, Paul Berry wrote:
> On 2017/07/19 17:19:04, vsm wrote:
> > lgtm with comments
> > 
> > does this logic need to be synced up somewhere in the new front end?
> 
> There's no logic that needs to be fixed in the front end, because the logic
that
> would need to be fixed isn't implemented yet :) (the new front end has a small
> amount of override checking logic, but it's incomplete and it happens before
> inference, so it needs to be reworked anyhow).
> 
> But it would be really nice if this CL could add some test cases to
> tests/language_2 so that once we get around to implementing the override
> checking logic, there will already be tests to help us avoid making the same
> mistake again.

oh thanks! I can try and add some tests there.

is it worth trying to implement this override checking in the new front end as
well? IDK the code base, but I have a pretty good feel for what _OverrideChecker
is doing at this point.

[Jennifer Messerly, 2017-07-20 04:48:20]

On 2017/07/19 19:09:22, Jennifer Messerly wrote:
> On 2017/07/19 17:26:47, Paul Berry wrote:
> > On 2017/07/19 17:19:04, vsm wrote:
> > > lgtm with comments
> > > 
> > > does this logic need to be synced up somewhere in the new front end?
> > 
> > There's no logic that needs to be fixed in the front end, because the logic
> that
> > would need to be fixed isn't implemented yet :) (the new front end has a
small
> > amount of override checking logic, but it's incomplete and it happens before
> > inference, so it needs to be reworked anyhow).
> > 
> > But it would be really nice if this CL could add some test cases to
> > tests/language_2 so that once we get around to implementing the override
> > checking logic, there will already be tests to help us avoid making the same
> > mistake again.
> 
> oh thanks! I can try and add some tests there.
> 
> is it worth trying to implement this override checking in the new front end as
> well? IDK the code base, but I have a pretty good feel for what
_OverrideChecker
> is doing at this point.

I'm not totally sure what to do with the tests yet (or how to mark them as
failing in the new front end) ... so I filed
https://github.com/dart-lang/sdk/issues/30214 so I remember to follow up :)

[Jennifer Messerly, 2017-07-20 19:15:50]

thanks Vijay! addressed those if you want to take another look

https://codereview.chromium.org/2981183003/diff/20001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/strong/checker.dart (right):

https://codereview.chromium.org/2981183003/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong/checker.dart:1898: // unless they are
explicitly marked as @virtual
On 2017/07/19 17:19:04, vsm wrote:
> Comment should be updated ....

Sigh. Yes. Historical comment from better times. =(

https://codereview.chromium.org/2981183003/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong/checker.dart:1951: if (visited.add(t.element)
&& (filter == null || filter(t))) {
On 2017/07/19 17:19:04, vsm wrote:
> Will visited only trigger if the same type is mixed in twice?  Is it worth
> moving more aggressively to make that an error?  
> 
> Otherwise, this is just the inheritance chain, right?

good catch. fixed & added a test for multiple mixins.

https://codereview.chromium.org/2981183003/diff/20001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong/checker.dart:1953: t.mixins.forEach(visit);
On 2017/07/19 17:19:04, vsm wrote:
> Do the mixins end up getting added in the right order?  Does order matter?

good catch! fixed

https://codereview.chromium.org/2981183003/diff/20001/pkg/dev_compiler/tool/b...
File pkg/dev_compiler/tool/build_pkgs.dart (right):

https://codereview.chromium.org/2981183003/diff/20001/pkg/dev_compiler/tool/b...
pkg/dev_compiler/tool/build_pkgs.dart:46: compileModule('collection',
unsafeForceCompile: true);
On 2017/07/19 17:19:04, vsm wrote:
> This will probably break dartdevc snapshot generation too.  The snapshot is
> built by compiling ddc against itself.  Looks like we import
package:collection.
> 
> Can you land your fix there and roll it forward?  To roll forward, update this
> to new tag or hash:
> 
> https://github.com/dart-lang/sdk/blob/master/DEPS#L61

done

[vsm, 2017-07-20 23:07:15]

lgtm

[Jennifer Messerly, 2017-07-21 23:02:56]

Description was changed from

==========
fix #29766, fix #29782 - fix override checker's interface checking
==========

to

==========
fix #29766, fix #29782 - fix override checker's interface checking

R=vsm@google.com

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

[Jennifer Messerly, 2017-07-21 23:02:58]

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

[Siggi Cherem (dart-lang), 2017-07-24 18:45:05]

sigmund@google.com changed reviewers:
+ sigmund@google.com

[Siggi Cherem (dart-lang), 2017-07-24 18:45:06]

https://codereview.chromium.org/2981183003/diff/40001/pkg/analyzer/lib/src/ta...
File pkg/analyzer/lib/src/task/strong/checker.dart (right):

https://codereview.chromium.org/2981183003/diff/40001/pkg/analyzer/lib/src/ta...
pkg/analyzer/lib/src/task/strong/checker.dart:1700: for (int i = 0; i <
type.mixins.length; i++) {
After this CL landed, we noticed about 30 strong mode errors in dart2js related
to this, but I believe we are getting false negatives. Most of them happen to be
about a mixin member overriden by another mixin further down.

I don't have the full context of the CL, but do I understand correctly that the
intent was to not report an error in that case either?

Here is a repro of the problem:

  abstract class I1 { num get x; }  
  abstract class I2 extends I1 { int get x; }

  class M1 { num get x => 0; }
  class M2 { int get x => 0; }

  class Base extends Object with M1 implements I1 {}
  class Child extends Base with M2 implements I2 {}

This produces the error:
  Base class introduces an invalid override.
  The type of 'M1.x' ('() → num') isn't a subtype of 'I2.x' ('() → int')

Note however that in Child M1.x is overriden by M2.x. If I were to inline M2
directly in the child class, for example, if I do:

  class Child2a extends Base { int get x => 0; }
  class Child2b extends Child2a implements I2 {}

Then I get no error.