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

Implementation Issue for SuperMixins: CFE / Kernel #34165

Closed
Tracked by #12
JekCharlsonYu opened this issue Aug 16, 2018 · 12 comments
Closed
Tracked by #12

Implementation Issue for SuperMixins: CFE / Kernel #34165

JekCharlsonYu opened this issue Aug 16, 2018 · 12 comments
Assignees
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. P1 A high priority bug; for example, a single project is unusable or has many test failures
Milestone

Comments

@JekCharlsonYu
Copy link
Contributor

JekCharlsonYu commented Aug 16, 2018

What: Issue for implementation for CFE/Kernel

ETA: 10/15 in Dart Repo.

For: SuperMixins Implementation and strawman documentation

Meta Issue: This is subsumed under the Super Mixins Meta Issue in the Language Repo

Comments: Feel free to break this further into further tasks and issues, or use as a meta issue for all the tasks in the implementation plan

@JekCharlsonYu JekCharlsonYu added P1 A high priority bug; for example, a single project is unusable or has many test failures area-analyzer-cfe labels Aug 16, 2018
@JekCharlsonYu JekCharlsonYu added this to the Dart2.1 milestone Aug 16, 2018
@kmillikin kmillikin changed the title Implementation Issue for CFE / Kernel Implementation Issue for SuperMixins: CFE / Kernel Aug 22, 2018
@bwilkerson bwilkerson added area-front-end Use area-front-end for front end / CFE / kernel format related issues. and removed area-analyzer-cfe labels Aug 28, 2018
@danrubel
Copy link

Parsing, events, parse errors, recovery, and AstBuilder mods are now complete.

@danrubel danrubel removed their assignment Aug 31, 2018
@leafpetersen
Copy link
Member

CL out with tests for mixin inference here: https://dart-review.googlesource.com/c/sdk/+/74017

@leafpetersen
Copy link
Member

Inference tests have landed.

https://dart-review.googlesource.com/c/sdk/+/74017

mixin_declaration/mixin_declaration_inference_valid_classes_test is failing and in some cases crashing.

@kmillikin
Copy link

Support for compiling mixins has landed. The static checks required at mixin applications are not yet implemented.

There are two kinds of checks on the class that the mixin is applied to: (1) that it implements the superclass constraint interfaces and (2) that it provides concrete implementations of the methods that are used in super calls in the mixin.

The first of these is straightforward to implement and can be done soon. The second of them is of moderate difficulty.

@leafpetersen
Copy link
Member

Presumably code that doesn't satisfy 2) is already broken, and can only "work" if the code is not exercised? Based on that, I'd prioritize 1) and consider letting 2) slip if necessary.

@kmillikin
Copy link

kmillikin commented Oct 11, 2018

Code that doesn't satisfy (2) -- that the superclass provides implementations of the required superclass methods -- is broken.

Checks for (1) -- that the supertype implements all the constraint interfaces -- is implemented in https://dart-review.googlesource.com/c/sdk/+/79206, landing as soon as I verify that it doesn't break dart2js.

@kmillikin
Copy link

To elaborate on mixins that contain super calls which are applied to classes that don't have implementations of those methods: either the mixin and the subclasses of the mixin application do not implement the method, in which case it is a compile-time error to instantiate it because the classes are abstract; or else the method is implemented in the mixin or a subclass of the application but it would be a runtime error to evaluate the super call in the mixin.

I also just discovered that the VM allows one to extend a mixin (and class C extends M means class C extends Object with M). I will work on statically disallowing that right now.

@kmillikin
Copy link

CFE now signals an error when a class extends a mixin (implemented here).

@dgrove
Copy link
Contributor

dgrove commented Oct 15, 2018

What remains here?

@kmillikin
Copy link

As far as the feature goes: we are missing static checks at mixin applications that the superclass has implementations of the methods that are used in super calls in the mixin declaration. Unfortunately, I think these have to get on the next release.

We also want to do some internal cleanup soon (e.g., represent mixins directly in Kernel), but that isn't tied to any particular release.

@kmillikin kmillikin modified the milestones: Dart2.1, Dart2.2 Oct 15, 2018
@dgrove
Copy link
Contributor

dgrove commented Mar 13, 2019

Can this be closed?

@kmillikin
Copy link

Yes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-front-end Use area-front-end for front end / CFE / kernel format related issues. P1 A high priority bug; for example, a single project is unusable or has many test failures
Projects
None yet
Development

No branches or pull requests

6 participants