Handle resolvedAst for forwarding constructors and callType on unnamed mixin applications.

Handle resolvedAst for forwarding constructors and callType on unnamed mixin applications.

R=sigmund@google.com

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

[Johnni Winther, 2016-05-20 12:13:04]

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

[Siggi Cherem (dart-lang), 2016-05-20 21:17:54]

lgtm

https://codereview.chromium.org/2000663002/diff/1/pkg/compiler/lib/src/common...
File pkg/compiler/lib/src/common/resolution.dart (right):

https://codereview.chromium.org/2000663002/diff/1/pkg/compiler/lib/src/common...
pkg/compiler/lib/src/common/resolution.dart:201: /// Returns `true` if [library]
is a backend specific library whose members
a backend specific => a target specific :)

https://codereview.chromium.org/2000663002/diff/1/pkg/compiler/lib/src/resolu...
File pkg/compiler/lib/src/resolution/class_members.dart (left):

https://codereview.chromium.org/2000663002/diff/1/pkg/compiler/lib/src/resolu...
pkg/compiler/lib/src/resolution/class_members.dart:28: final Compiler compiler;
woohoo! closer and closer to a compilerless compiler!

https://codereview.chromium.org/2000663002/diff/1/pkg/compiler/lib/src/serial...
File pkg/compiler/lib/src/serialization/modelz.dart (right):

https://codereview.chromium.org/2000663002/diff/1/pkg/compiler/lib/src/serial...
pkg/compiler/lib/src/serialization/modelz.dart:812: void
ensureResolved(Resolution resolution) {
FYI => "ensureResolved" is likely something I'd like to move out of the element
model.

https://codereview.chromium.org/2000663002/diff/1/pkg/compiler/lib/src/serial...
File pkg/compiler/lib/src/serialization/task.dart (right):

https://codereview.chromium.org/2000663002/diff/1/pkg/compiler/lib/src/serial...
pkg/compiler/lib/src/serialization/task.dart:8: import '../common.dart';
undo?

https://codereview.chromium.org/2000663002/diff/1/tests/compiler/dart2js/seri...
File tests/compiler/dart2js/serialization/test_data.dart (right):

https://codereview.chromium.org/2000663002/diff/1/tests/compiler/dart2js/seri...
tests/compiler/dart2js/serialization/test_data.dart:246: main() => new
UnmodifiableListView(null);
we should start looking into changing these tests to make it clear what they are
covering.

For example, it's not obvious to me that this test was added to cover fwd
constructors of unnamed mixin applications.

Some ideas:
(a) name each individual test, probably using the unittest framework would be
better for this. For example, you could write:

  test('fowarding constructors with unnamed mixins', () {
    checkSerialization(''' .... ''');
  });

(b) declare classes in each tests explicitly, so that the code is self contained
and doesn't depend on the current implementation of dart:core. This way we don't
loose test coverage if someone changes the implementation of
UnmodifiableListView.

I'd honestly do both (a) and (b).

[Johnni Winther, 2016-05-23 08:42:36]

https://codereview.chromium.org/2000663002/diff/1/pkg/compiler/lib/src/common...
File pkg/compiler/lib/src/common/resolution.dart (right):

https://codereview.chromium.org/2000663002/diff/1/pkg/compiler/lib/src/common...
pkg/compiler/lib/src/common/resolution.dart:201: /// Returns `true` if [library]
is a backend specific library whose members
On 2016/05/20 21:17:53, Siggi Cherem (dart-lang) wrote:
> a backend specific => a target specific :)

Done.

https://codereview.chromium.org/2000663002/diff/1/pkg/compiler/lib/src/serial...
File pkg/compiler/lib/src/serialization/modelz.dart (right):

https://codereview.chromium.org/2000663002/diff/1/pkg/compiler/lib/src/serial...
pkg/compiler/lib/src/serialization/modelz.dart:812: void
ensureResolved(Resolution resolution) {
On 2016/05/20 21:17:53, Siggi Cherem (dart-lang) wrote:
> FYI => "ensureResolved" is likely something I'd like to move out of the
element
> model.

I'm not a fan either but we need a point in time where a class element (or any
element?) becomes part of the universe (any existing entity) so that we can
check for instance valid super use vs mixin...

https://codereview.chromium.org/2000663002/diff/1/pkg/compiler/lib/src/serial...
File pkg/compiler/lib/src/serialization/task.dart (right):

https://codereview.chromium.org/2000663002/diff/1/pkg/compiler/lib/src/serial...
pkg/compiler/lib/src/serialization/task.dart:8: import '../common.dart';
On 2016/05/20 21:17:53, Siggi Cherem (dart-lang) wrote:
> undo?

For convenience; it adds access to debug helpers (and was used in the production
of this CL).

https://codereview.chromium.org/2000663002/diff/1/tests/compiler/dart2js/seri...
File tests/compiler/dart2js/serialization/test_data.dart (right):

https://codereview.chromium.org/2000663002/diff/1/tests/compiler/dart2js/seri...
tests/compiler/dart2js/serialization/test_data.dart:246: main() => new
UnmodifiableListView(null);
On 2016/05/20 21:17:53, Siggi Cherem (dart-lang) wrote:
> we should start looking into changing these tests to make it clear what they
are
> covering.
> 
> For example, it's not obvious to me that this test was added to cover fwd
> constructors of unnamed mixin applications.
> 
> Some ideas:
> (a) name each individual test, probably using the unittest framework would be
> better for this. For example, you could write:
> 
>   test('fowarding constructors with unnamed mixins', () {
>     checkSerialization(''' .... ''');
>   });
> 
> (b) declare classes in each tests explicitly, so that the code is self
contained
> and doesn't depend on the current implementation of dart:core. This way we
don't
> loose test coverage if someone changes the implementation of
> UnmodifiableListView.
> 
> I'd honestly do both (a) and (b).

(a) Naming, yes. The unittest package, no. (Last time I looked it interleaved
execution of asynchronous tests which makes the output incomprehensible!)

(b) The problem here is the combination of a serialized named mixin application
and a nonserialized use; i.e. it'll require a better rewrite of the test
infrastructure so I'll leave for a later CL.

[Johnni Winther, 2016-05-23 08:42:50]

Description was changed from

==========
Handle resolvedAst for forwarding constructors and callType on unnamed mixin
applications.
==========

to

==========
Handle resolvedAst for forwarding constructors and callType on unnamed mixin
applications.

R=sigmund@google.com

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

[Johnni Winther, 2016-05-23 08:42:52]

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

[Siggi Cherem (dart-lang), 2016-05-23 16:25:10]

https://codereview.chromium.org/2000663002/diff/1/tests/compiler/dart2js/seri...
File tests/compiler/dart2js/serialization/test_data.dart (right):

https://codereview.chromium.org/2000663002/diff/1/tests/compiler/dart2js/seri...
tests/compiler/dart2js/serialization/test_data.dart:246: main() => new
UnmodifiableListView(null);
> (a) Naming, yes. The unittest package, no. (Last time I looked it interleaved
> execution of asynchronous tests which makes the output incomprehensible!)

Let's give it another try... unittest should support async tests now: As long as
the test function returns a future, the unittest package will not execute any
subsequent test until the future completes.