Implementation tracking/meta Issue for Super Mixins

[JekCharlsonYu]

This issue is for tracking implementation of Super Mixins:

• Request issue: Mixins with super-calls. #6
• Feature issue: Mixin declaration allowing super-invocations. #7
• Feature specification:
  https://github.com/dart-lang/language/blob/master/accepted/2.1/super-mixins/feature-specification.md
• Implementation plan: https://github.com/dart-lang/language/blob/master/accepted/2.1/super-mixins/implementation-plan.md

TODO/Dependencies

• Discussion and feedback
  • General discussion: Mixin declaration allowing super-invocations. #7
  • Breaking change: Discussion of cost of super mixin breaking change #10
  • Syntax: Finalize on vs requires for super mixins #9
• Implementation team sign off (this issue):

P0 Blockers

• Issue for language tests
• Issue for implementation in Analyzer
  • Mixin inference
• Issue for implementation in CFE/Kernel
• Issue for implementation in VM
• Issue for implementation in Intellij
• Issue for implementation in Grok
• Issue for implementation in Dartfmt
• Issue for implementation in Documentation/Website
• Issue for linter
• Issue for migration to flutter (@yjbanov)
• Issue for updating documentation (TODO)
• Issue for implementation in Dartdoc flutter

P1 Blockers

• Issue for co19 tests
• Issue for implementation in dart2js
• Issue for implementation in DDC

Non-Blockers

• Issue for CodeMirror
• Issue for implementation in Dartpad
• Misc syntax:
  • Dart-atom: https://github.com/dart-atom/dart/tree/master/grammars
  • github markdown

**Based on meta issue template

[leafpetersen]

I have added an initial cut on an implementation plan here. If you are an assignee of this bug, please review the implementation plan and the proposal itself (if needed), and provide any feedback and comments. If you see no technical issues with the proposal, and believe that the implementation plan is reasonable, please LGTM the plan here in the issue tracker.

@a-siva for VM
@kmillikin for CFE
@munificent for dartfmt
@jwren for grok, intellij
@jcollins-g for dartdoc
@bwilkerson for analyzer
@devoncarew for dartpad
@kwalrath for documentation
@vsmenon for DDC
@johnniwinther for dart2js
@Hixie for flutter migration

cc @JekCharlsonYu @dgrove @lrhn @rakudrama @sigmundch

[leafpetersen]

cc @mit-mit @timsneath @kevmoo @sethladd @filiph

[leafpetersen]

Hmm. I don't seem to be able to assign this issue to most of you, but if you're listed above, please take a look and sign off.

[kevmoo]

I guess you could have made a "sign-off" PR and have us all approve.

LGTM

[bwilkerson]

lgtm

[Hixie]

I believe @yjbanov has volunteered to do the Flutter migration.

[Hixie]

For Flutter our usual policy is to only allow one syntax at a time, and to require lints to force us onto the new syntax if we change the allowed syntax. I would feel more comfortable if Flutter was moved to the new syntax in phase 3 after the lint has been made available in phase 2. LGTM assuming that we do the migration on Flutter's side after we've got a lint to enforce it.

[leafpetersen]

@Hixie I'm happy to handle the migration however works best for you. My worry was/is this: I can't give you a lint to change a definition of a super mixin to the new syntax, because I have no way of knowing what things are super mixins until they are used. So the lint will fire on uses, not definitions.

So I think it would be bad to land the analyzer hint that tells you are using an old style super mixin before we fix flutter. Otherwise users will see hints telling them to fix flutter... :)

But we should be able to land them (essentially) simultaneously: migrate flutter to the new syntax and roll in a new analyzer with the hint. Or maybe we can roll in the new analyzer, but initially only turn on the hint inside of flutter?

Does this seem reasonable?

[Hixie]

What we normally do is implement the lint, roll that, then turn on the lint and fix the bugs it finds at the same time (in a separate later PR).

[devoncarew]

Based on the above discussion, we should also call out above the necessary work in the linter.

[leafpetersen]

If this is just about finding super mixin definitions in flutter, I'm not sure investing in a lint is worthwhile. I can find and fix all of the super-mixins in flutter independently.

For flutter users, having a period where we a lint or hint fires is useful since it gives them time to make sure that packages get fixed.

My only concern with doing this as a lint is that removing the lint after it has served its purpose is essentially breaking, right (because people might reference it in their analysis_options.yaml)? Whereas a hint (which has no external surface area) doesn't break anything if we remove it. Or does the analyze ignore unknown lint names?

[bwilkerson]

The analyzer ignores unknown lint names, except when explicitly analyzing the analysis options file, in which case an error is generated.

[leafpetersen]

Ok, SGTM. I'll put a linter issue up there and add that to the plan.

[munificent]

The implementation plan mentions parsing in the CFE but not the analyzer front end. Is analyzer on the CFE now? Will we need to implement the syntax in the old analyzer parser?

If we're only implementing this in the new front end, I need to make sure dartfmt is migrated to use analyzer+CFE. (Maybe there isn't any migration required and analyzer still exposes the exact same API and ASTs?)

Aside from that, this LGTM. We'll want to get dartfmt support in pretty early because it will be hard for anyone to try the new syntax without that. I'll make it a priority.

I don't see a TODO for tests. :) Who owns that?

[leafpetersen]

The implementation plan mentions parsing in the CFE but not the analyzer front end. Is analyzer on the CFE now? Will we need to implement the syntax in the old analyzer parser?

Analyzer on CFE will happen before Phase 0.

If we're only implementing this in the new front end, I need to make sure dartfmt is migrated to use analyzer+CFE. (Maybe there isn't any migration required and analyzer still exposes the exact same API and ASTs?)

@bwilkerson ?

I don't see a TODO for tests. :) Who owns that?

Language team will land an initial set of tests, co19 will land a more exhaustive set. I'll update appropriately.

[bwilkerson]

Is analyzer on the CFE now?

Not yet, no ...

Will we need to implement the syntax in the old analyzer parser?

... but we should be on the fasta parser by early next week (or maybe even the end of this week), so no.

Maybe there isn't any migration required and analyzer still exposes the exact same API and ASTs?

Correct. When we move to the fasta parser (and later to CFE) the AST will not change. There might be a few minimal changes to the element model, but that won't effect dart_style.

We'll want to get dartfmt support in pretty early because it will be hard for anyone to try the new syntax without that.

I have a CL out to add the AST structures we'll need: https://dart-review.googlesource.com/c/sdk/+/70141. It's a breaking change, though, so I'm not sure exactly when we want to commit it.

[munificent]

Sounds great, @bwilkerson! As long as there's a dartfmt tracking bug, I'll get it working with the new syntax once analyzer has the ASTs for it and is on CFE.

[bwilkerson]

Will you need a published version of analyzer to get started, or only after you're ready to publish dart_style?

... and is on CFE.

And the fasta parser supports the new syntax (which will likely be a bit after analyzer is using the fasta parser).

[munificent]

Will you need a published version of analyzer to get started, or only after you're ready to publish dart_style?

A published one, preferably. Otherwise, there is no easy way to run dart_style's tests in its own repo or on Travis.

[Hixie]

Oh I guess if the lint is only valid for a short time anyway because the whole syntax is going away, then that's fine too. No need for a lint at all then. Just make the change to the syntax and fix Flutter at the same time. Or whatever works for you. Just beware that for the span of time between fixing Flutter and making it impossible to do it wrong, there will almost certainly be people who check in the wrong thing accidentally.

[leafpetersen]

Oh I guess if the lint is only valid for a short time anyway because the whole syntax is going away, then that's fine too.

Actually, this raises a legitimate question. The old way of writing super-mixins is going away. But the old way of writing ordinary mixins will be supported indefinitely, and so there will be two ways to write a non-super mixin: using a mixin declaration or a class declaration. If Flutter wants to migrate to a single syntax for all mixins, then we probably do want a long term supported lint for this. @Hixie do you expect to want to migrate all mixins?

[devoncarew]

Just to clarify from above, we expect the analyzer to be using the fasta parser shortly (there are two blocking bugs in the sdk repo, and we have to re-check the status of that change rolling into google3). Analyzer on the full CFE (including resolution, not just using the parser), has an ETA on the order of months.

[bwilkerson]

If Flutter wants to migrate to a single syntax for all mixins, then we probably do want a long term supported lint for this.

Or if we think that other users will want to use a single syntax :-)

Even without a lint we ought to have "convert class to mixin" and "convert mixin to class" assists.

[Hixie]

I could imagine desiring a lint along the lines of "only mix in classes declared with mixin", or a lint along the lines of "don't use mixin without an on clause". Certainly a goal of keeping things consistent throughout would be very nice.

We would probably just have a hack that requires that uses of the word "mixin" outside a comment are always followed by an identifier followed by the word "on", at least until we get a lint, to ensure that everything uses a single syntax. (We have many such tests already, e.g. one to verify we don't depend on package:test directly, one to verify that we are running the analyzer against the sample code in comments, one to verify that our internal imports don't form a cycle, etc.)

[leafpetersen]

I could imagine desiring a lint along the lines of "only mix in classes declared with mixin",

This is consistent with our long term goals for the language, so how about we go with adding this lint to the analyzer in Phase 1, and then turning it on in flutter and fixing all the things in Phase 2?

[johnniwinther]

LGTM

[vsmenon]

lgtm for ddc

[keertip]

@leafpetersen, don't see any mention of rolling this into google3. Does it look like there will be no/few changes to the existing code base?

[leafpetersen]

@keertip This comment describes all uses in google3: #10 (comment) . I don't expect any difficulties.

[jcollins-g]

Implementation plan is reasonable for dartdoc. Typically with new language features dartdoc has to have some minor revisions (and new published versions of analyzer) to keep from crashing when encountering the new syntax. So there will be some distributed impact over the next few months.

[mit-mit]

@JekCharlsonYu what remains on this feature?