Defer all recursive mixins

Defer all recursive mixins

This defers mixins along with the supertype.  It passes Leaf's example on the bug.  Will add that as a proper test.

Fixes #28334

R=jmesserly@google.com

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

[vsm, 2017-01-11 17:59:45]

Description was changed from

==========
Defer all recursive mixins

Fixes #28334
==========

to

==========
Defer all recursive mixins

This defers mixins along with the supertype.  It passes Leaf's example on the
bug.  Will add that as a proper test.

Fixes #28334
==========

[vsm, 2017-01-11 17:59:45]

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

[Jennifer Messerly, 2017-01-11 18:11:47]

lgtm

https://codereview.chromium.org/2625913003/diff/1/pkg/dev_compiler/lib/src/co...
File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2625913003/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:1252: var basetypes = [
type.superclass ]..addAll(type.mixins);
nit: run dart format

https://codereview.chromium.org/2625913003/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:1256: if
(basetypes.any((t) => _deferIfNeeded(t, element))) {
alternatively, could do this:

var baseTypes = <DartType>[];
for (var t in [type.superclass]..addAll(type.mixins)) {
  if (_deferIfNeeded(t, element)) {
    baseTypes.add(fillDynamicTypeArgs(t.element.type));
    _hasDeferredSupertype.add(element);
  } else {
    baseTypes.add(t);
  }
}


... slightly more precise on which types are being deferred. Not sure it's worth
it tho.

https://codereview.chromium.org/2625913003/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:1681: } else if
(_hasDeferredSupertype.contains(classElem)) {
incidentally ... this isn't caused by your change, but I'm never sure why we
used _hasDeferredSupertype like a global Set. It should be quite possible to
return the value from _emitClassHeritage and pass it back down to this method.

We're literally using a Set on the class rather than pass a return value and
parameter. It's strange and I've been meaning to refactor it at some point.

[vsm, 2017-01-11 18:32:31]

Description was changed from

==========
Defer all recursive mixins

This defers mixins along with the supertype.  It passes Leaf's example on the
bug.  Will add that as a proper test.

Fixes #28334
==========

to

==========
Defer all recursive mixins

This defers mixins along with the supertype.  It passes Leaf's example on the
bug.  Will add that as a proper test.

Fixes #28334

R=jmesserly@google.com

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

[vsm, 2017-01-11 18:32:32]

Committed patchset #1 (id:1) manually as
46b7f79fd50cc2c9b21fbc76fbc487b74782ae2b (presubmit successful).

[vsm, 2017-01-11 18:33:01]

https://codereview.chromium.org/2625913003/diff/1/pkg/dev_compiler/lib/src/co...
File pkg/dev_compiler/lib/src/compiler/code_generator.dart (right):

https://codereview.chromium.org/2625913003/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:1252: var basetypes = [
type.superclass ]..addAll(type.mixins);
On 2017/01/11 18:11:47, Jennifer Messerly wrote:
> nit: run dart format

Done.

https://codereview.chromium.org/2625913003/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:1256: if
(basetypes.any((t) => _deferIfNeeded(t, element))) {
On 2017/01/11 18:11:47, Jennifer Messerly wrote:
> alternatively, could do this:
> 
> var baseTypes = <DartType>[];
> for (var t in [type.superclass]..addAll(type.mixins)) {
>   if (_deferIfNeeded(t, element)) {
>     baseTypes.add(fillDynamicTypeArgs(t.element.type));
>     _hasDeferredSupertype.add(element);
>   } else {
>     baseTypes.add(t);
>   }
> }
> 
> 
> ... slightly more precise on which types are being deferred. Not sure it's
worth
> it tho.

We also don't need to fill in dynamic for all type params on a single type -
just where it's recursive.  Thought about doing this, but didn't seem worth it.

https://codereview.chromium.org/2625913003/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/code_generator.dart:1681: } else if
(_hasDeferredSupertype.contains(classElem)) {
On 2017/01/11 18:11:47, Jennifer Messerly wrote:
> incidentally ... this isn't caused by your change, but I'm never sure why we
> used _hasDeferredSupertype like a global Set. It should be quite possible to
> return the value from _emitClassHeritage and pass it back down to this method.
> 
> We're literally using a Set on the class rather than pass a return value and
> parameter. It's strange and I've been meaning to refactor it at some point.

Added a comment.  _emitClassHeritage/_emitClassExpression are called a handful
of places - wasn't obvious if we'd need to thread all of those.