Skip to content

Conversation

@Hixie
Copy link
Contributor

@Hixie Hixie commented Jun 1, 2017

This prevents some of our mixins from being subclassed.

Also, move mixins to using 'extends' instead of 'implements' for
future compatibility with Dart changes.

Also, rename a class that had Mixin in the name but was not a mixin.

@Hixie
Copy link
Contributor Author

Hixie commented Jun 1, 2017

cc @HansMuller

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HitTestable.hitTest() is abstract, which means that reference to super.hitTest() at line 287 below ends up flagged as unresolved by DFE.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RendererBinding itself is abstract and is a mixin. This should just resolve to whatever ends up being the superclass of RendererBinding in the concrete class, surely.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure how to add Peter to this comment, he should be able to confirm this, but from what I can see DFE needs concrete instance methods(this does match what the spec says if I read it right: https://github.com/gbracha/lessRestrictedMixins/blob/master/proposal.md#16151-method-lookup)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, DFE expects to find a concrete method in the super class of the mixin with a super call. Remember that the RendererBinding is also a regular class, and when we're compiling it, we may not know yet that someone intends to use it as a mixin.

My understanding is that the language team is considering this aspect of mixins and allow you to declare intent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how to express what we're doing today within these constraints.

@HansMuller
Copy link
Contributor

I hate to point this out, but. I think it's "mixin" not mix-in, see for example https://www.dartlang.org/articles/language/mixins

The changes LGTM.

This prevents some of our mixins from being subclassed.

Also, move mixins to using 'extends' instead of 'implements' for
future compatibility with Dart changes.

Also, rename a class that had Mixin in the name but was not a mixin.
@Hixie
Copy link
Contributor Author

Hixie commented Jun 2, 2017

Fixed mix-in => mixin.

@Hixie
Copy link
Contributor Author

Hixie commented Jun 2, 2017

I will land this when Travis goes green. I'll follow-up with additional patches to fix the remaining DFE problems once we know how to solve them.

@Hixie Hixie merged commit 9eae8b8 into flutter:master Jun 2, 2017
@Hixie Hixie deleted the mixins-update branch June 2, 2017 22:54
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants