Revert "Compile-time error if mixin class has a fact

Allow factory constructors on mixin classes.

This mitigates a breaking change impacting real users caused by 851b4a995c72b484e1b4d13dabbc7e6dcfe7cc24.
The spec will be updated to allow factory constructors on mixins.

R=gbracha@google.com, iposva@google.com

Committed: https://github.com/dart-lang/sdk/commit/6a0b3210265c5fbb74ed0d1b4b6c67910454e853

[Jacob, 2016-02-16 18:57:33]

jacobr@google.com changed reviewers:
+ hausner@google.com

[Jacob, 2016-02-16 18:57:33]

This CL breaks existing Dart users who depended on being able to define factory
constructors on mixins as the functionality worked in dartium and dart2js.

[Jacob, 2016-02-16 18:58:16]

Fixes https://github.com/dart-lang/sdk/issues/25791

[Jacob, 2016-02-16 18:59:09]

Description was changed from

==========
Revert "Compile-time error if mixin class has a factory"

This reverts commit 851b4a995c72b484e1b4d13dabbc7e6dcfe7cc24.

BUG=
==========

to

==========
Revert "Compile-time error if mixin class has a factory"

This reverts commit 851b4a995c72b484e1b4d13dabbc7e6dcfe7cc24.

BUG=
==========

[Jacob, 2016-02-16 18:59:40]

Description was changed from

==========
Revert "Compile-time error if mixin class has a factory"

This reverts commit 851b4a995c72b484e1b4d13dabbc7e6dcfe7cc24.

BUG=
==========

to

==========
Revert "Compile-time error if mixin class has a factory"

This reverts commit 851b4a995c72b484e1b4d13dabbc7e6dcfe7cc24.

[https://github.com/dart-lang/sdk/issues/25791]

BUG=
==========

[kevmoo, 2016-02-16 19:36:26]

Description was changed from

==========
Revert "Compile-time error if mixin class has a factory"

This reverts commit 851b4a995c72b484e1b4d13dabbc7e6dcfe7cc24.

[https://github.com/dart-lang/sdk/issues/25791]

BUG=
==========

to

==========
Revert "Compile-time error if mixin class has a factory"

This reverts commit 851b4a995c72b484e1b4d13dabbc7e6dcfe7cc24.

Closes https://github.com/dart-lang/sdk/issues/25791
==========

[Ivan Posva, 2016-02-17 07:51:26]

iposva@google.com changed reviewers:
+ iposva@google.com

[Ivan Posva, 2016-02-17 07:51:26]

Needs more work.

Thanks,
-Ivan

https://codereview.chromium.org/1699233002/diff/1/runtime/vm/class_finalizer.cc
File runtime/vm/class_finalizer.cc (left):

https://codereview.chromium.org/1699233002/diff/1/runtime/vm/class_finalizer....
runtime/vm/class_finalizer.cc:2233: const char* ctr_kind = func.IsFactory() ?
"factory" : "constructor";
You need to preserve the improved error message below.

https://codereview.chromium.org/1699233002/diff/1/tests/language/mixin_illega...
File tests/language/mixin_illegal_constructor_test.dart (right):

https://codereview.chromium.org/1699233002/diff/1/tests/language/mixin_illega...
tests/language/mixin_illegal_constructor_test.dart:51: new C0(1,2,3);   /// 13:
static type warning, runtime error
As far as I understood from Gilad even if we allowed mixins with factory
constructors, the resulting class should not contain the factory constructors.

[hausner, 2016-02-17 11:25:04]

I do not approve of this change. If we can't fix bugs, we might as well stop the
Dart project now.

The implications of changing the spec have not been thought through.

[Jacob, 2016-02-17 16:54:51]

jacobr@google.com changed reviewers:
+ gbracha@google.com

[Jacob, 2016-02-17 17:35:55]

jacobr@google.com changed reviewers:
+ kevmoo@google.com

[Jacob, 2016-02-17 17:35:56]

I completely agree that we need to be able to fix bugs. However, this case is
more nuanced than fixing a bug. The CL disabled a reasonable and useful but
unspecced feature used by real users that was properly implemented in both
Dart2Js and the DartVM. 

Please sync with Gilad if you have reasons why he shouldn't update the spec to
match  the existing implementations that support mixin classes with factory
constructors. I talked to Gilad briefly yesterday and got the impression he
believes making the minor updates to the spec to match the existing
implementations is the right approach  given the new information that actual
users depend on the feature as implemented.

If you want to see examples of how customers use this feature just let me know
and I'm happy to take you a tour of existing customer code broken by this
change.

[Jacob, 2016-02-19 20:53:03]

Gilad please review the CL to confirm this matches anticipated minor changes to
the language spec to match the existing language tests and implementations that
allow factory constructors.

https://codereview.chromium.org/1699233002/diff/1/runtime/vm/class_finalizer.cc
File runtime/vm/class_finalizer.cc (left):

https://codereview.chromium.org/1699233002/diff/1/runtime/vm/class_finalizer....
runtime/vm/class_finalizer.cc:2233: const char* ctr_kind = func.IsFactory() ?
"factory" : "constructor";
On 2016/02/17 07:51:26, Ivan Posva wrote:
> You need to preserve the improved error message below.

Done.

https://codereview.chromium.org/1699233002/diff/1/tests/language/mixin_illega...
File tests/language/mixin_illegal_constructor_test.dart (right):

https://codereview.chromium.org/1699233002/diff/1/tests/language/mixin_illega...
tests/language/mixin_illegal_constructor_test.dart:51: new C0(1,2,3);   /// 13:
static type warning, runtime error
On 2016/02/17 07:51:26, Ivan Posva wrote:
> As far as I understood from Gilad even if we allowed mixins with factory
> constructors, the resulting class should not contain the factory constructors.
Correct.

[Jacob, 2016-02-19 20:53:04]

Gilad please review the CL to confirm this matches anticipated minor changes to
the language spec to match the existing language tests and implementations that
allow factory constructors.

[Ivan Posva, 2016-02-19 22:35:26]

Given the comments by Gilad in the area-language bug, I am fine removing the
restriction again.

Please update the CL description as it is not a straight revert, otherwise LGTM.

Thanks,
-Ivan

[gbracha, 2016-02-19 23:29:35]

test lgtm.

[Jacob, 2016-02-22 18:15:53]

Description was changed from

==========
Revert "Compile-time error if mixin class has a factory"

This reverts commit 851b4a995c72b484e1b4d13dabbc7e6dcfe7cc24.

Closes https://github.com/dart-lang/sdk/issues/25791
==========

to

==========
Allow factory constructors on mixin classes.

This mitigates a breaking change impacting real users caused by
851b4a995c72b484e1b4d13dabbc7e6dcfe7cc24.
The spec will be updated to allow factory constructors on mixins.
==========

[Jacob, 2016-02-22 18:19:53]

Description was changed from

==========
Allow factory constructors on mixin classes.

This mitigates a breaking change impacting real users caused by
851b4a995c72b484e1b4d13dabbc7e6dcfe7cc24.
The spec will be updated to allow factory constructors on mixins.
==========

to

==========
Allow factory constructors on mixin classes.

This mitigates a breaking change impacting real users caused by
851b4a995c72b484e1b4d13dabbc7e6dcfe7cc24.
The spec will be updated to allow factory constructors on mixins.

R=gbracha@google.com, iposva@google.com

Committed:
https://github.com/dart-lang/sdk/commit/6a0b3210265c5fbb74ed0d1b4b6c67910454e853
==========

[Jacob, 2016-02-22 18:19:54]

Committed patchset #3 (id:40001) manually as
6a0b3210265c5fbb74ed0d1b4b6c67910454e853 (presubmit successful).