Implement implicit constructors in mixin applications.

Implement implicit constructors in mixin applications.

R=kasperl@google.com

Committed: https://code.google.com/p/dart/source/detail?r=22367

[ahe, 2013-04-15 13:36:46]

DBC

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:908: assert(element is!
ForwardingConstructorElement);
Can you call a method instead?

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart
(right):

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:284:
element.computeSignature(compiler).optionalParameters;
This looks really bad.  Why hasn't computeSignature been called already?

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:654: abstract
class ForwardingConstructorElement extends FunctionElement {
Please document this class.

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/elements/modelx.dart (right):

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1310:
parseNode(compiler) { throw 'parseNode in synthetic constructor forward.'; }
compiler.internalErrorOnElement

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1322: Token
position() => superConstructor.position();
I'm not sure this is a good position.  Wouldn't the enclosing class be better?

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1923:
listener.cancel('can only add constructors to mixin application scope,'
Please emit a proper error instead of calling cancel.

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/enqueue.dart (right):

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/enqueue.dart:62: if (element is
ForwardingConstructorElement) {
Can you call a method instead?

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:309: if
(isConstructor && element is! ForwardingConstructorElement) {
Can you call a method instead?

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:318: } else if
(element is ForwardingConstructorElement) {
Can you call a method instead?

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:2506: if
(constructor is ForwardingConstructorElement) {
Can you call a method instead.

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:2509:
FunctionExpression tree = target.parseNode(compiler);
What happens if your superclass is itself a mixin application?

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3196: //
Create forwarding constructors for constructor defined in the super class
superclass is one word.

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3199: bool
constructorsAdded = false;
Where is this used?

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3202:
assert(!member.isFactoryConstructor());
Please use assert(invariant(...))

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:110: constructor);
Why this change?

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1259: if (constructor
is ForwardingConstructorElement) {
Is there a method you can call?

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2847: if (element is
ForwardingConstructorElement) {
Is there a method you can call?

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3377: if (constructor
is ForwardingConstructorElement) {
Is there a method you can call?

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3379: element:
constructor.superConstructor);
I don't think this position is helpful.  It doesn't tell you where the problem
is.

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/types/simple_types_inferrer.dart
(right):

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/types/simple_types_inferrer.dart:476:
if (element is ForwardingConstructorElement) {
Is there a method you can call?

https://codereview.chromium.org/14168003/diff/2001/tests/language/mixin_super...
File tests/language/mixin_super_constructor2_test.dart (right):

https://codereview.chromium.org/14168003/diff/2001/tests/language/mixin_super...
tests/language/mixin_super_constructor2_test.dart:5: import
"package:expect/expect.dart";
Do you really need this import?

[kasperl, 2013-04-24 06:01:41]

I can't see the tests in the patch set?

[karlklose, 2013-04-24 08:45:02]

I forgot to add them after resetting the branch. Here they are!

[karlklose, 2013-04-25 11:25:53]

PTAL.

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/compiler.dart:908: assert(element is!
ForwardingConstructorElement);
I changed the code to use a boolean.

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart
(right):

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/dart_backend/backend.dart:284:
element.computeSignature(compiler).optionalParameters;
It is not necessary, I forgot to delete it.

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:654: abstract
class ForwardingConstructorElement extends FunctionElement {
Obsolete, removed the class.

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/elements/modelx.dart (right):

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1310:
parseNode(compiler) { throw 'parseNode in synthetic constructor forward.'; }
Removed.

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1322: Token
position() => superConstructor.position();
Removed.

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1923:
listener.cancel('can only add constructors to mixin application scope,'
On 2013/04/15 13:36:46, ahe wrote:
> Please emit a proper error instead of calling cancel.

Done.

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:110: constructor);
Reverted.

https://codereview.chromium.org/14168003/diff/2001/sdk/lib/_internal/compiler...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:3379: element:
constructor.superConstructor);
Changed to the enclosing class.

https://codereview.chromium.org/14168003/diff/2001/tests/language/mixin_super...
File tests/language/mixin_super_constructor2_test.dart (right):

https://codereview.chromium.org/14168003/diff/2001/tests/language/mixin_super...
tests/language/mixin_super_constructor2_test.dart:5: import
"package:expect/expect.dart";
Do you prefer writing my own Expect.equals?

[kasperl, 2013-04-25 12:13:12]

Almost there, but I think there may be some issues with forwarding constructors
forwarding to other forwarding constructors.

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:251:
FunctionElement get targetConstructor;
Add a comment that explains what this is (and that it only makes sense for
forwarding constructors).

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/elements/modelx.dart (right):

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1906: bool get
hasLocalScopeMembers => constructors.isEmpty;
So it has local scope members when there are no constructors? I don't
understand.

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1913: for
(Link<Element> link = constructors;
Maybe add a lookup constructor for this and call it from here?

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1935: if
(!element.isConstructor()) {
Why not keep this as is and always throw?

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1946: 
I'd add a new method for adding a constructor and not use addToScope for this.

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3301: if
(!member.isConstructor() || member.isSynthesized) return;
Does this work if the superclass itself is a mixin application? Do you have a
test for that?

   class S { S.foo(); }
   class M1 { }
   class M2 { }
   class C extends S with M1, M2 { 
     C.foo() : super.foo();
   }

   main() {
     new C.foo();
   }

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3305:
SourceString constructorName;
Add a helper for computing the constructor name? ... and maybe a helper for
creating the forwarding constructor too?

That way, this loop becomes much more readable. Run through all the super class
constructors and create forwarders for them.

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3320:
mixinApplication.addToScope(forwarder, compiler);
I'd rather have a method for adding a new constructor to a mixin application.

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1308: if
(constructor.isForwardingConstructor) {
How does this work if the target constructor is a forwarding constructor too?
Can that happen? Are they short-circuited somewhere? You may want to assert that
constructor isn't a forwarding constructor after this.

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2888: if
(element.isForwardingConstructor) {
Same comment as above.

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/types/simple_types_inferrer.dart
(right):

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/types/simple_types_inferrer.dart:579:
if (element.isForwardingConstructor) {
Sure that element isn't a forwarding constructor after this. Add assert.

[karlklose, 2013-05-03 09:35:45]

Thanks for the review!

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/elements/elements.dart (right):

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/elements/elements.dart:251:
FunctionElement get targetConstructor;
On 2013/04/25 12:13:12, kasperl wrote:
> Add a comment that explains what this is (and that it only makes sense for
> forwarding constructors).

Done.

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/elements/modelx.dart (right):

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1906: bool get
hasLocalScopeMembers => constructors.isEmpty;
On 2013/04/25 12:13:12, kasperl wrote:
> So it has local scope members when there are no constructors? I don't
> understand.

Done.

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1913: for
(Link<Element> link = constructors;
On 2013/04/25 12:13:12, kasperl wrote:
> Maybe add a lookup constructor for this and call it from here?

Done.

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1935: if
(!element.isConstructor()) {
On 2013/04/25 12:13:12, kasperl wrote:
> Why not keep this as is and always throw? 

Done.

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1946: 
On 2013/04/25 12:13:12, kasperl wrote:
> I'd add a new method for adding a constructor and not use addToScope for this.

Done.

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/resolution/members.dart (right):

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3301: if
(!member.isConstructor() || member.isSynthesized) return;
I change it to work with these.

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3305:
SourceString constructorName;
I created a method to create the forwarding constructor.

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/resolution/members.dart:3320:
mixinApplication.addToScope(forwarder, compiler);
On 2013/04/25 12:13:12, kasperl wrote:
> I'd rather have a method for adding a new constructor to a mixin application.

Done.

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/ssa/builder.dart (right):

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:1308: if
(constructor.isForwardingConstructor) {
They are  now short-circuited.

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2888: if
(element.isForwardingConstructor) {
On 2013/04/25 12:13:12, kasperl wrote:
> Same comment as above.

Done.

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/ssa/builder.dart:2888: if
(element.isForwardingConstructor) {
I added an invariant when they are created.

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/types/simple_types_inferrer.dart
(right):

https://codereview.chromium.org/14168003/diff/18001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/types/simple_types_inferrer.dart:579:
if (element.isForwardingConstructor) {
Ditto.

[kasperl, 2013-05-03 10:22:14]

LGTM with one comment:

https://codereview.chromium.org/14168003/diff/31001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/elements/modelx.dart (right):

https://codereview.chromium.org/14168003/diff/31001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1952:
addToScope(constructor, compiler);
How does this work? Doesn't addToScope throw? Shouldn't this be addConstructor
and could you make sure you have a test case that covers this?

[karlklose, 2013-05-03 11:50:50]

Thanks, Kasper.

https://codereview.chromium.org/14168003/diff/31001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/elements/modelx.dart (right):

https://codereview.chromium.org/14168003/diff/31001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/elements/modelx.dart:1952:
addToScope(constructor, compiler);
It does not, fixed. It is already tested by every test using ListMixin.

[karlklose, 2013-05-03 12:15:52]

Committed patchset #9 manually as r22367 (presubmit successful).