Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Forwarding constructors must not have optional parameters #15101

Closed
DartBot opened this issue Nov 15, 2013 · 31 comments
Closed

Forwarding constructors must not have optional parameters #15101

DartBot opened this issue Nov 15, 2013 · 31 comments
Labels
area-language New language issues should be filed at https://github.com/dart-lang/language

Comments

@DartBot
Copy link

DartBot commented Nov 15, 2013

This issue was originally filed by @filiph


What steps will reproduce the problem?

  1. Extend a class that has an optional parameter in the constructor.
  2. Add a mixin

What is the expected output? What do you see instead?
I get a runtime error:
    forwarding constructors must not have optional parameters

I would expect this to work without problems, or at least to have better explanation (it is, for example, not clear if the culprit is the base class, the mixin, or the class being extended) and a compile time error.

But really, I find this quite limiting.

What version of the product are you using? On what operating system?
Dart Editor version 1.0.0_r30187 (DEV)
Dart SDK version 1.0.0.3_r30187
Mac OSX 10.8.5

Please provide any additional information below.

===== CODE =====

class Mixin extends Object {
  int mixinMember;
}

class BaseClass {
  int baseMember;
  BaseClass({this.baseMember: -1});
}

class Class extends BaseClass with Mixin {
  int member;
  Class(this.member);
}

void main() {
  var c = new Class(42);
}

===== /CODE =====


Attachment:
mixinoptparameters.dart (249 Bytes)

@lrhn
Copy link
Member

lrhn commented Nov 15, 2013

Added Area-VM, Triaged labels.

@lrhn
Copy link
Member

lrhn commented Nov 15, 2013

The spec actually doesn't mention optional parameters for the implicit declared constructors of mixin applications. They looks like an oversight in the spec since there is no need for the restriction.


cc @gbracha.

@iposva-google
Copy link
Contributor

cc @crelier.
Set owner to @gbracha.
Removed Area-VM label.
Added Area-Language, Accepted labels.

@gbracha
Copy link
Contributor

gbracha commented Nov 16, 2013

I agree this is unfortunate. That said, the work around is easy, and the behavior is as specified.

The error message should be better: at the least, it should say that BaseClass with Mixin does not have a constructor with no arguments because no constructor without optional arguments could be found in BaseClass, and suggest that adding such a constructor would help.

Certainly we want to liberalize this eventually - we want to lift the entire host of restrictions on mixins, all of which are limiting or annoying in one way or another.

The work around for now would be:

class BaseClass {
  int baseMember;
  BaseClass({this.baseMember: -1});
  BaseClass.x() : this(); // add a redirecting constructor to BaseClass that has no optional parameters
}

class Class extends BaseClass with Mixin {
  int member;
  Class(this.member):super.x();
}

Or, if one cannot or does not want to tamper with the API of BaseClass, define an intermediate class (probably private)

class _IntermediateClass extends BaseClass {
  _IntermediateClass();
}

class Class extends _IntermediateClass with Mixin {
  int member;
  Class(this.member);
}

To understand the issue: The constructors for mixin applications are auto-generated. They are intended to initialize any fields in the mixin and forward information to the superclass. As such, a mixin application needs to have constructors corresponding to those of its superclass so it can forward the arguments etc. However, in the interests of simplicity, the spec says that only constructors that have no optional parameters are created in this way.

The superclass of Class is an application of Mixin to BaseClass, and that class does not have a constructor at all, because no constructor without optional parameters exists in its superclass.


cc @mhausner.

@DartBot
Copy link
Author

DartBot commented Nov 16, 2013

This comment was originally written by @filiph


Thanks!

That workaround really is easy, but not immediately obvious (not to me, at
least).

Seeing that on the language level, the fix is not easy (and maybe not even
desirable?), I suggest improving the error message, and ideally giving a
warning at compile time.

Even if the warning/error just links to this bug's URL, it'll be leaps
better than the current experience.

@DartBot
Copy link
Author

DartBot commented May 28, 2014

This comment was originally written by @paulevans


Just experimenting with mixins for the first time, just bumped in to this issue.
A better error would really help.

@crelier
Copy link
Contributor

crelier commented Jun 5, 2014

Improved error message suggesting workaround submitted as r37057.

@DartBot
Copy link
Author

DartBot commented Jun 29, 2014

This comment was originally written by trinar...@gmail.com


i still don't understand this issue. here is a variant i ran into:
// variant #­1
class M {}

class A {
  A({x});
}

class B extends A with M {
  B({x}) : super(x: x);
}

void main() {
  new B(x: 1);
}

it says "error:... forwarding constructors must not have optional parameters class B extends A with M {".
the previous explanation seemed to claim that this weird error message
means that the super class of B, namely (A with M),
does not have a constructor with optional named param x. but why not?
why doesn't it just have exactly the same constructor that A has?

the following works:
// variant #­2
class M {}

class A {
  A({x});
  A.silly(x) : this(x: x);
}

class B extends A with M {
  B({x}) : super.silly(x);
}

void main() {
  new B(x: 1);
}
but why? the only explanation would seem to be that (A with M) inherited the silly constructor from A.
great. so why can't it inherit a constructor from A that happens to have optional named params?

no part of the explanation given in post #­4 seems to explain what
makes optional named params different from mandatory params.

also, i feel like this restriction defeats much of the point of mixins: convenience.
with this restriction, if you create a mixin M, no matter how simple,
now you must identify every class A to which you might want to apply M in advance, and either
(a) give A ((a constructor with no optional named params) that delegates to the intended constructor), or
(b) create an intermediate class _A with (a constructor with no optional named params).

if option (b) is deemed so easy and mechanical, why can't the dart compiler do it for us,
or simpler yet, just allow calling whatever constructor of A we feel like?
it's understandable that mixins should have some restrictions, but not the classes that you apply them to.

@DartBot
Copy link
Author

DartBot commented Jun 30, 2014

This comment was originally written by @mhausner


A forwarding constructor must not have optional parameters because it can't cover the case where no actual parameter is given at the call site. I.e., it can't do "if I was invoked with no parameter, call super(), else call super(x)". It can only do what was explicitly written in "variant 2" above, which is to invoke the super constructor with a value.

This may be ok if the default value is null, as it is in the example given in comment 8. But consider the case where a default value for the optional parameter is given in class A. This value must be evaluated in the context of class A, but would also have to be the default value for the optional parameter of B's super class (A with M). This can be particularly hairy if A and "A with M" are in different libraries.

@trinarytree
Copy link

thanks for the great explanation. that is amazingly subtle. if (A with M) were not publicly exposed anywhere, it might be possible to say "just make super(x: x) refer to A's constructor, not that of (A with M)", but i'm guessing (A with M) is exposed through mirrors or something.

actually, this forwarding problem seems to be more general. e.g.
f({x: 1}) {print(x);}
g({x}) {f(x: x);}
g(); // prints null. oops.

@mkustermann
Copy link
Member

Just to be clear, our 3 tools disagree here significantly ATM.

Here's an example:

$ cat test.dart
class Base {
  var opt;
  Base({this.opt});
}
class Mixin {}
class Sub extends Base with Mixin {
  Sub() : super(opt: 'opt');
}
main() => print(new Sub().opt);

Produces on analyzer

$ dartanalyzer test.dart
Analyzing [test.dart]...
No issues found

Produces on dart2js:

$ dart2js test.dart
Dart file (test.dart) compiled to JavaScript: out.js
$ d8 out.js
opt

Produces on the VM:

$ dart test.dart
'file:///tmp/test.dart': error: line 6 pos 29: forwarding constructors must not have optional parameters
class Sub extends Base with Mixin {
                            ^

I would strongly prefer if we could make it work for dartanalyzer/dart2js/dartVM in a consistent way.

@gilad: Should we support it or not?

@lrhn
Copy link
Member

lrhn commented Aug 26, 2014

The only problem I can see with mixin implicit constructors and optional parameters is that it isn't specified, not that it can't work.
Just saying that it has exactly the same signature as the corresponding superclass constructor, and that it forwards all parameters in a "super" initializer, should make it work as expected for all constructors.

Mixins on top of classes with constructors with optional parameters (... phew ...) do work in dart2js. I think the VM should make it work too.

@DartBot
Copy link
Author

DartBot commented Aug 26, 2014

This comment was originally written by @mhausner


lrn@: How does dart2js address the scenario I described in comment 9, especially with regards to default values and libraries?

@DartBot
Copy link
Author

DartBot commented Aug 26, 2014

This comment was originally written by @lrhn


I can't say how it's actually implemented, but I see no inherent problem in having the same compile-time constant value as default value in the implicit mixin-application constructor. Values are not private to libraries, only identifiers. That makes it a specification problem to state the details, but the behavior should be clear - calling without specifying that optional parameter means that it uses the same default value and passes it to the super-constructor, just as if you hadn't supplied it to the super constructor directly.

@gbracha
Copy link
Contributor

gbracha commented Aug 26, 2014

As I indicated in comment #­4, this and many other restrictions on mixins are undesirable and unnecessary, and should be removed over time. Barring that, I believe we should implement the spec as it stands. That is why we have a spec. The only relevant counter-argument is that if this already works in some implementations, reverting them to comply with the spec might be considered a breaking change. However, this argument holds for every compiler bug. It is a judgement call how to treat this. I for one don't believe this case rises to that level, but that is not my call to make.

In particular, the claim that the restriction is unnecessary is irrelevant.
The restrictions are not there because they are essential - they are there as simplifications. Whether they are "necessary" is immaterial. Note that some simplifications are important to dart2js and others to the VM.

As Matthias notes, once the restriction is removed, the behavior wrt to default values may be disappointing to some - but that is an result of our optional parameter rules, regardless of mixins. Basically, the default value in the subclass will always take precedence over the one in the superclass.

 

@gbracha
Copy link
Contributor

gbracha commented Aug 26, 2014

Issue #17610 has been merged into this issue.


cc @johnniwinther.

@gbracha
Copy link
Contributor

gbracha commented Aug 26, 2014

Issue #19226 has been merged into this issue.

@DartBot DartBot added Type-Defect area-language New language issues should be filed at https://github.com/dart-lang/language labels Aug 26, 2014
@chalin
Copy link
Contributor

chalin commented Jul 1, 2015

/sub

@Hixie
Copy link
Contributor

Hixie commented Nov 26, 2015

FWIW, the ability to forward constructors with optional arguments through mixins in this way would be of great value to Flutter's framework.

@kevmoo kevmoo added type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) and removed priority-unassigned labels Feb 29, 2016
@Hixie
Copy link
Contributor

Hixie commented Aug 16, 2016

I just ran into this again. The inability to use mixins when you use constructors with named arguments is a serious limitation when you make heavy use of named arguments (as Flutter does).

In this particular case, I wanted to extract some code to enable code re-use. However, I can't, because the classes involved all have constructors with named arguments. I can't change the base class, because it's a public API and we don't want to have multiple redundant constructors. I can't introduce an intermediate class because requiring that all consumers of this mixin introduce an intermediary is too ugly.

Instead, I'm going to have to put the code in the base class and this makes that class harder to use (because it now has a broader API surface, some of which is of no use to most subclasses).

We should just change the spec to forward optional and named arguments the same way it works with non-optional positional arguments.

@gavinmulligan-wf
Copy link

Is there any active work being done to address this issue?

@lrhn
Copy link
Member

lrhn commented Nov 4, 2016

I'll see what I can do.

@munificent munificent removed the type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) label Dec 15, 2016
@Hixie
Copy link
Contributor

Hixie commented Jan 3, 2017

Just ran into this again. To make finding this bug easier, I'm going to quote the error message from the analyzer that is the symptom of this issue:

[error] This mixin application is invalid because all of the constructors in the base class 'Foo' have optional parameters.

@Hixie
Copy link
Contributor

Hixie commented Apr 24, 2017

FWIW, Flutter is currently working around this by having an undesired mixin class in our class hierarchy:
https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/rendering/proxy_box.dart#L47

@tvolkert
Copy link
Contributor

@sethladd
Copy link
Contributor

sethladd commented Sep 3, 2017

Just personally ran into this. Not sure what [error] This mixin application is invalid because all of the constructors in the base class 'Foo' have optional parameters. means? Why would the presence of optional parameters prevent me from using a Mixin?

@lrhn
Copy link
Member

lrhn commented Sep 4, 2017

Why would the presence of optional parameters prevent me from using a Mixin?

In short, because the spec says so.
The rules for forwarding generative constructors from the mixin application to the superclass are only specified for constructors with no optional parameters. If all constructors of the superclass have optional parameters, the resulting mixin application will not have any generative constructors, which means you can't use it as a superclass.

The reason for not forwarding optional parameters is probably that it gets complicated to say how default values are computed. We plan to do so for Dart 2.0, so you get forwarders for all generative constructors, not just those without optional parameters.

@sethladd
Copy link
Contributor

sethladd commented Sep 4, 2017

So, are you saying this will get fixed in Dart 2.0?

@lrhn
Copy link
Member

lrhn commented Sep 6, 2017

That is the plan, yes (ob-caveat: unless something changes, as always).
The kernel project, which is part of the common Dart 2 frontend, already implements the new behavior, so by Dart 2.0, everyone should get the benefit of that.

@jcollins-g
Copy link
Contributor

@bwilkerson FYI: As of 2.0.0-dev-8.0 this is still an issue, but it looks like the analysis errors around it have changed and were not helpful in diagnosing the problem.

The analyzer says: The class 'Foo' doesn't have an unnamed constructor, complaining in subclass constructors.

Once I ran it, the VM gave me a better error, which via googling led me here.

@eernstg
Copy link
Member

eernstg commented Feb 19, 2018

Closing: As of 63c6851, the language specification of constructor forwarding does support named parameters. Issue #31543 is a request to get this feature implemented in the common front end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-language New language issues should be filed at https://github.com/dart-lang/language
Projects
None yet
Development

No branches or pull requests