Fix #28120, strong mode allows field overrides

Fix #28120, strong mode allows field overrides
Fix #28119, DDC supports field overrides without @virtual
Fix #28801, devirtualize private fields in DDC where possible
Fix #28589, stop supporting @virtual in strong mode

R=leafp@google.com, vsm@google.com

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

[Jennifer Messerly, 2017-03-27 21:48:16]

Description was changed from

==========
Fix #28120, strong mode allows field overrides
Fix #28119, DDC supports field overrides without @virtual
Fix #28801, devirtualize private fields in DDC where possible
==========

to

==========
Fix #28120, strong mode allows field overrides
Fix #28119, DDC supports field overrides without @virtual
Fix #28801, devirtualize private fields in DDC where possible
Fix #28589, stop supporting @virtual in strong mode
==========

[Jennifer Messerly, 2017-03-27 21:57:32]

Patchset #2 (id:10001) has been deleted

[Jennifer Messerly, 2017-03-27 21:59:52]

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

[Jennifer Messerly, 2017-03-27 21:59:52]

Leaf: could you review strong mode (and DDC if desired?)
Vijay: could you review DDC (and strong mode if desired?)

Thank you!

https://codereview.chromium.org/2781443003/diff/20001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/property_model.dart (right):

https://codereview.chromium.org/2781443003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/property_model.dart:155: class
ClassPropertyModel {
this is unchanged from lib/src/compiler/class_property_model.dart

https://codereview.chromium.org/2781443003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/property_model.dart:217:
fieldModel.isVirtual(field)) {
except for this 1 line

[Jennifer Messerly, 2017-03-27 22:05:58]

https://codereview.chromium.org/2781443003/diff/20001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/property_model.dart (right):

https://codereview.chromium.org/2781443003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/property_model.dart:75: if (type.isPublic &&
type.library == library) {
this should be "!type.isPublic". Fix on the way. If it's not covered by tests
I'll add one. The "private class exposed via public class" is probably not a
very well tested code path.

[vsm, 2017-03-27 22:31:27]

nice!

https://codereview.chromium.org/2781443003/diff/20001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/element_helpers.dart (right):

https://codereview.chromium.org/2781443003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/element_helpers.dart:153: for (var mixinType
in cls.mixins.reversed) {
I think we should get nested mixins (see
https://codereview.chromium.org/2778083002/).

https://codereview.chromium.org/2781443003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/element_helpers.dart:158: if
(superclass?.library != library) break;
Don't we need to keep going in case we've got something like:

foo.A extends bar.B extends foo.C

?

Or, at least, until we're out of the build unit (library cycle).

https://codereview.chromium.org/2781443003/diff/20001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/property_model.dart (right):

https://codereview.chromium.org/2781443003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/property_model.dart:20: /// when possible by
analyzing the class heirarchy and using knowledge of
heirarchy -> hierarchy

[Jennifer Messerly, 2017-03-27 23:16:28]

https://codereview.chromium.org/2781443003/diff/20001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/element_helpers.dart (right):

https://codereview.chromium.org/2781443003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/element_helpers.dart:153: for (var mixinType
in cls.mixins.reversed) {
On 2017/03/27 22:31:27, vsm wrote:
> I think we should get nested mixins (see
> https://codereview.chromium.org/2778083002/).

hmmm. Those guys unfortunately break assumptions all over the known universe,
including in Analyzer where I took this pattern from ... see
ClassElementImpl.allSupertypes and many other places. =\

I'm not sure there's any way to figure out that pattern without inspecting the
syntax? Since we're simply trying to be defensive here we could visit them. I'll
see what I can do :)

https://codereview.chromium.org/2781443003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/element_helpers.dart:158: if
(superclass?.library != library) break;
On 2017/03/27 22:31:27, vsm wrote:
> Don't we need to keep going in case we've got something like:
> 
> foo.A extends bar.B extends foo.C
> 
> ?
> 
> Or, at least, until we're out of the build unit (library cycle).

Let me investigate based on the use cases. I only care about public types that
extend private ones in the same library, thus making them extensible. It
shouldn't matter if we cross library boundaries -- once that happens, we're in
an open world situation. If we're ever analyzing outside of a library something
broken about how the analysis works.

Build units should have nothing to do with this ... it's strictly determining
what set of fields cannot be overridden outside of their immediate library.

https://codereview.chromium.org/2781443003/diff/20001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/property_model.dart (right):

https://codereview.chromium.org/2781443003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/property_model.dart:20: /// when possible by
analyzing the class heirarchy and using knowledge of
On 2017/03/27 22:31:27, vsm wrote:
> heirarchy -> hierarchy

Done.

[Leaf, 2017-03-27 23:26:19]

analyzer changes lgtm

https://codereview.chromium.org/2781443003/diff/20001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/property_model.dart (right):

https://codereview.chromium.org/2781443003/diff/20001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/property_model.dart:19: /// if those fields
should be virtual or not. In effect, we devirtualized fields
devirtualize

[Jennifer Messerly, 2017-03-28 04:36:16]

PTAL Vijay! Same logic but I think it makes more sense now?

Also should be slightly faster.

[Jennifer Messerly, 2017-03-28 04:39:47]

On 2017/03/28 04:36:16, Jennifer Messerly wrote:
> PTAL Vijay! Same logic but I think it makes more sense now?
> 
> Also should be slightly faster.

Oh ... well it's not exactly the same, it handles the strange "mixin typedef"
thingy now.

[vsm, 2017-03-28 13:28:33]

https://codereview.chromium.org/2781443003/diff/40001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/property_model.dart (right):

https://codereview.chromium.org/2781443003/diff/40001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/property_model.dart:117: if (cls.library !=
library) return;
I don't think this is quite right.  If I patch your CL, and run foo.dart below,
it prints "17", not "42" as expected.  If I remove this line, and filter out
other libraries below (e.g., `supertypes.removeWhere((cls) => cls.library !=
library)`), then I do get "42".

foo.dart:

import 'bar.dart';

class A {
  int _x = 17;
}

class C extends B {
  int _x = 42;
}

void main() {
  var a = new C();
  print(a._x);
}

bar.dart:

import 'foo.dart';

class B extends A {
  String _x = "hello";
}

[Jennifer Messerly, 2017-03-28 16:48:23]

https://codereview.chromium.org/2781443003/diff/40001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/property_model.dart (right):

https://codereview.chromium.org/2781443003/diff/40001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/property_model.dart:117: if (cls.library !=
library) return;
On 2017/03/28 13:28:32, vsm wrote:
> I don't think this is quite right.  If I patch your CL, and run foo.dart
below,
> it prints "17", not "42" as expected.  If I remove this line, and filter out
> other libraries below (e.g., `supertypes.removeWhere((cls) => cls.library !=
> library)`), then I do get "42".
> 
> foo.dart:
> 
> import 'bar.dart';
> 
> class A {
>   int _x = 17;
> }
> 
> class C extends B {
>   int _x = 42;
> }
> 
> void main() {
>   var a = new C();
>   print(a._x);
> }
> 
> bar.dart:
> 
> import 'foo.dart';
> 
> class B extends A {
>   String _x = "hello";
> }

I was trying really hard to keep the search cheap =\. I will remove that
optimization.

[vsm, 2017-03-28 17:12:33]

https://codereview.chromium.org/2781443003/diff/40001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/property_model.dart (right):

https://codereview.chromium.org/2781443003/diff/40001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/property_model.dart:117: if (cls.library !=
library) return;
On 2017/03/28 16:48:22, Jennifer Messerly wrote:
> On 2017/03/28 13:28:32, vsm wrote:
> > I don't think this is quite right.  If I patch your CL, and run foo.dart
> below,
> > it prints "17", not "42" as expected.  If I remove this line, and filter out
> > other libraries below (e.g., `supertypes.removeWhere((cls) => cls.library !=
> > library)`), then I do get "42".
> > 
> > foo.dart:
> > 
> > import 'bar.dart';
> > 
> > class A {
> >   int _x = 17;
> > }
> > 
> > class C extends B {
> >   int _x = 42;
> > }
> > 
> > void main() {
> >   var a = new C();
> >   print(a._x);
> > }
> > 
> > bar.dart:
> > 
> > import 'foo.dart';
> > 
> > class B extends A {
> >   String _x = "hello";
> > }
> 
> I was trying really hard to keep the search cheap =\. I will remove that
> optimization.

You might get most of the benefit from checking if cls.library is in the current
BuildUnit.

[Jennifer Messerly, 2017-03-28 17:12:39]

Vijay: I uploaded that (very tiny) change if you would like to take another
look.

[vsm, 2017-03-28 17:15:40]

lgtm (though see my other comment on using build unit)

[Jennifer Messerly, 2017-03-28 17:21:42]

On 2017/03/28 17:15:40, vsm wrote:
> lgtm (though see my other comment on using build unit)

yeah, I don't really wanna flow that in here because it's not something we use
for Dart analysis anywhere else to my knowledge & it doesn't store the info in
such a way that would permit fast lookups anyway.

if it ever becomes a problem we could do:

    var libraryCycle = type.library.libraryCycle.toSet();

that would enable fast bailout of things not in the cycle checks.

[Jennifer Messerly, 2017-03-28 17:26:48]

Description was changed from

==========
Fix #28120, strong mode allows field overrides
Fix #28119, DDC supports field overrides without @virtual
Fix #28801, devirtualize private fields in DDC where possible
Fix #28589, stop supporting @virtual in strong mode
==========

to

==========
Fix #28120, strong mode allows field overrides
Fix #28119, DDC supports field overrides without @virtual
Fix #28801, devirtualize private fields in DDC where possible
Fix #28589, stop supporting @virtual in strong mode

R=leafp@google.com, vsm@google.com

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

[Jennifer Messerly, 2017-03-28 17:26:49]

Committed patchset #5 (id:50001) manually as
1c504f89452d52f902a8c3591bde0f172452de21 (presubmit successful).

[vsm, 2017-03-28 17:30:00]

On 2017/03/28 17:21:42, Jennifer Messerly wrote:
> On 2017/03/28 17:15:40, vsm wrote:
> > lgtm (though see my other comment on using build unit)
> 
> yeah, I don't really wanna flow that in here because it's not something we use
> for Dart analysis anywhere else to my knowledge & it doesn't store the info in
> such a way that would permit fast lookups anyway.
> 
> if it ever becomes a problem we could do:
> 
>     var libraryCycle = type.library.libraryCycle.toSet();
> 
> that would enable fast bailout of things not in the cycle checks.

Oh, yeah - forgot that was being computed.  That would be cleaner (and usually a
lot smaller) than the build unit.  :-)