dart-lang / pana

Package ANAlysis for Dart
https://pub.dev/packages/pana
BSD 3-Clause "New" or "Revised" License
206 stars 44 forks source link

Handle supermixins #288

Closed isoos closed 6 years ago

isoos commented 6 years ago

Dart VM supports them, web platform does not.

We should either (a) allow it and disqualify web platform when its use is detected or (b) run two analysis (enabled/disabled), and if they differ, use the enabled one and disqualify web.

eernstg commented 6 years ago

Supermixins in the form used by Flutter will not be supported in Dart 2. We expect to introduce a separate notion of mixins. So we may have a mixin ... declaration format, similar to class declarations, but specifically for mixins, capable of specifying constraints on superclasses etc. directly. This will allow us to avoid the semantic conflicts and ambiguities where one single declaration must specify both a mixin and a class.

isoos commented 6 years ago

That's good to know! Am I getting the right impression that it is not worth for us to enable supermixin in analysis, and aim for the new mixin format whenever it arrives?

lrhn commented 6 years ago

Absolutely. The VM will also not support super mixins in Dart 2. It will only exist under a flag so Flutter will have time to migrate to the new syntax that we will (hopefully) introduce in a Dart 2.1 update.

tvolkert commented 6 years ago

But the VM will support the new syntax, and that syntax will provide feature parity with the current Flutter uses?

/cc @Hixie

Hixie commented 6 years ago

Use cases we care about:

isoos commented 6 years ago

@Hixie these use cases are valid, and I believe @lrhn and @eernstg will be able to point you toward the work that will go into the tooling (e.g. dartanalyzer). However, I think that pana's and pub.dartlang.org's scope should be answering the question: "Can I use this package [on platform X]?"

Right now we are a bit too restrictive, as we are treating non-Flutter packages as they were not supporting super-mixins, which is true only for the web platform, while Dart VM could support them. Fixing that is possible, but it would be valid for only a short time period, and after that there would be different mixin syntax, rules (and appropriate dartanalyzer) around.

@tvolkert: pana and pub.dartlang.org follows the new SDK versions, and we'll pick up the new features as they come out, and will follow all the defaults with some customization.

lrhn commented 6 years ago

@Hixie Our planned feature should be able to handle those cases. Tentative syntax:

mixin Mixin on A, B, C implements W {
  /// body
  foo() => super.methodFromA();
}

and allowing extends Mixin as equivalent to extends Object with Mixin when Mixin allows Object as a superclass.

The constructor forwarding issues are separate, but we still plan to fix them.

Hixie commented 6 years ago

Wonderful!

isoos commented 6 years ago

@eernstg: what's the latest on the supermixin support in Dart2? Should we still wait with this in pana or can we already infer something from the current tooling?

eernstg commented 6 years ago

The plan is still as stated above: Dart 2 will not have support for extracting a mixin from a class whose superclass is different from Object, but we expect to introduce a new class-like declaration for mixins (mixin ... on ... implements {...}, as Lasse mentioned) soon after Dart 2 has been released.

You may be able to use --supermixin with the analyzer and run the code using the VM, but that's only relevant for exploration and curiosity: That mechanism is not going to be an official part of the language, and it differs from the expected mixin abstraction in both syntax and semantics.

So for a published package like pana you should wait.

isoos commented 6 years ago

So for a published package like pana you should wait.

I should have been more clear about the question: should pana warn packages that they are using a supermixin that they shouldn't be using going forward?

If we should, what would be the best way for it? E.g. run dartanalyzer with --supermixin (to unblock some package analysis) but still scan the parsed source codes for patterns?

eernstg commented 6 years ago

Ah, sorry, I didn't establish enough context to be able to give a reasonable response.

Flutter uses super-mixins so it would be unacceptably breaking to emit super-mixin warnings for everything that is intended to run on that platform. At some point, almost everything on Flutter will need to be adjusted in order to stop using super-mixins and start using the new mixin abstraction, but that's a known future event, not an individual problem with each class declaration which is used for extraction of a mixin somewhere else. Hopefully, it will be possible to change class C extends B ... to mixin C on B ... for every class C that is used for extraction of a mixin, and then "almost everything" just continues to work (because, out of sheer luck, C is not used in any other way ;-), but that's in any case a separate topic.

So if you have already detected that a given package is intended to be used on Flutter only, then I'd recommend running dartanalyzer --supermixin in order to remain consistent with the approach used on Flutter in general. For example, I'd expect an import 'package:flutter/...'; anywhere in the transitive import graph from a library to be sufficient to take this route.

For anything that might be used on some other platform than Flutter, I'd recommend running dartanalyzer without --supermixin, because that's the sane approach for any such package. That will be fine for lots of packages.

Otherwise there is this tricky case where super-mixins are actually used, and that's because the developers of that package want it so, but there is no concrete evidence that the given package is Flutter-only (it might be intended to be Flutter+VM, or it might be intended to be Flutter-only).

Do you have any kind of options for pana, that is, could a package declare (maybe in analysis_options.yaml or in some other configuration file in the package, say, pana.yaml) that it is intended to use --supermixin? In that case it would make sense to detect that and use it.

I'm not quite sure what 'scan the parsed source codes for patterns' means, so I won't comment on that.

isoos commented 6 years ago

Thanks, that's good insight, and I have one follow-up question that could influence what actions we shall take now: would the web platform (dart2js, or dev compiler) support the mixin format that is coming after Dart2?

eernstg commented 6 years ago

would the web platform (dart2js, or dev compiler) support the mixin format that is coming after Dart2?

Definitely yes.

isoos commented 6 years ago

would the web platform (dart2js, or dev compiler) support the mixin format that is coming after Dart2?

Definitely yes.

In that case the current lack of supermixin support in web should be considered temporary, and pana should be handling it as the exception. My current resolution for this would be the following:

We could change the order of processing, and do the platform analysis before calling dartanalyzer. That way we would have some signal of what the package is for, and we could allow supermixin for every package except the ones that do use web platform APIs.

eernstg commented 6 years ago

Are you generally running the analyses (language checks, lints, custom analyzers, whatever it is that you're running) in a maximally permissive configuration? That is, do you want to have as few diagnostic messages as possible, skipping silently over every issue that you could have chosen to flag, simply because there is a way (say, an option) to ignore it? If that's your policy then it makes sense to allow the use of supermixins in every situation where it could possibly work.

However, there are several reasons why this might be a little too optimistic.

Let's use F to denote is the feature that has been known as 'super-mixins' for a while, and let G denote the new mixin abstraction. Then we have the following situation:

Flutter and the VM have feature F and the web platform does not have F. In the future we will add feature G to all platforms and delete F. G has a different syntax and a different semantics than F, but we will strive to make the transition as smooth as possible. It is true that G can be used to express roughly the same thing as F, but there may be a need to introduce new entities (say, adding new classes along with the refactoring that changes some existing classes into mixins), and other adjustments may be needed as well. Now, it sounds like your description skips over F != G:

... the current lack of supermixin support in web should be considered temporary

That would match the situation if we were simply planning to add F to the web platform, but it doesn't match the situation so well when F is going to die and G will be added to all platforms.

The following sounds like a very useful approach:

... do the platform analysis before calling dartanalyzer

.. because that would make it possible to detect Flutter-only packages (where supermixins must be allowed). However, the approach you mention is much more permissive:

... allow supermixin for every package except the ones that do use web platform APIs

.. that is, allow the use of super-mixins also in all packages that aren't exclusively for the web platform, e.g., every package that is intended to be used on more than one platform.

So if you want to be as permissive as possible you can of course use this approach. But it's definitely not a good idea to actively support the use of super-mixins outside Flutter at this point, so I would prefer a mechanism where supermixins are not automatically accepted for anything except "Flutter-only" packages.

isoos commented 6 years ago

Now, it sounds like your description skips over F != G:

Yeah, that's a good point. Even if we unblock the not-too-many packages that are using e.g. dart:io with supermixins, they will need to change their package eventually. Thanks for the discussion!

But it's definitely not a good idea to actively support the use of super-mixins outside Flutter at this point, so I would prefer a mechanism where supermixins are not automatically accepted for anything except "Flutter-only" packages.

This is exactly what we have today. Let's keep it the current way for now, I'm closing this issue. I have a few updates from the half-baked-PR related to this change that are worth to submit, regardless of the supermixin support, but will the main thing intact.

eernstg commented 6 years ago

Sounds good, thanks!