Discussion of cost of super mixin breaking change

[leafpetersen]

No description provided.

[leafpetersen]

cc @vsmenon @dgrove @Hixie @lrhn @munificent @mit-mit

This issue is for discussion and analysis of the potential breakage incurred by removing support for the old experimental class based super mixins (#7).

[leafpetersen]

Some initial data.

Regexp based searching is somewhat unreliable for this, since it's a property of both the use and the definition of a class. I did some grepping of internal code and of github, looking for classes which had Mixin in their name, and which extended something other than Object. Github search results are here. Suggestions on how to refine this search welcome, but based on poking around in that query, most results are ListMixinWorkAround (cc @jmesserly :) which is a false positive, or old forks of obsolete SDK code.

[leafpetersen]

I also hacked the analyzer to look for

• Uses of super mixins (no false positives)
• Classes which make a super call and Mixin in their name (possibility of false positives).

I downloaded the top 20 or so flutter/pub packages by popularity, and ran my hacked analyzer on them including the transitive closure of the dependencies and found no examples of super mixins defined outside of the flutter framework.

I'll test out more packages from github, suggestions on other code bases to run on, or other ways of finding code to test out?

After a bit more independent poking, I would like to propose that I mail flutter-dev:

• describing the proposed change
• explaining why we believe this will be essentially non-breaking
• soliciting either known counter-examples, or publicly available codebases that people would like me to analyze for breakage.

@Hixie @dgrove, thoughts?

[Hixie]

SGTM.

[munificent]

If we really can't find any uses of super mixins outside of the two (is that all?) classes in Flutter, does this feature really carry its weight? Could those mixins in Flutter be changed to use explicit delegation instead and then we get a simpler feature?

I think dedicated mixin syntax is still a good idea, but that would let us sidestep all of the confusing semantics and syntax around specifying the superclass constraints on the mixin, and avoid the implementation and code size challenges of implementing mixins that contain super calls.

[yjbanov]

If we really can't find any uses of super mixins outside of the two (is that all?) classes in Flutter

There are more than two. Here's a few examples: SingleTickerProviderStateMixin, TickerProviderStateMixin, SchedulerBinding, GestureBinding (basically all *Binding classes), ContainerRenderObjectMixin, RenderObjectWithChildMixin, ContainerParentDataMixin.

I also estimated about 80 usages just inside the framework and core widgets. I don't have an estimate for user code.

Update: removed RenderBoxContainerDefaultsMixin from the list. It's not a super-mixin.

[Hixie]

Yeah we use this a lot. I also use it in my private code at home, though that obviously won't show up on the radar. I suspect people outside of the Flutter team don't use it because nobody knows about it, not because it's not desireable. It's a great feature.

[munificent]

Thanks, @yjbanov! I was only aware of the ticker ones.

[leafpetersen]

Ok, an update. I've added an additional check to my hacked analyzer to look for mixins of classes that have something other than Object as their super type, because the analyzer doesn't seem to be entirely reliable about recording super calls inside of classes (dart-lang/sdk#34113).

I've downloaded the first 6 pages of flutter packages from pub, ordered by popularity (see list below), and run my hacked analyzer on the lib, test, and example directories where relevant. For the flutter/plugins repository, I did the same for each sub-repo of packages.

I'm running the analyzer with --show-package-warnings, which I believe means that I am checking the transitive closure of the import graph (but probably not checking files which are in reachable packages but not reachable from the import graph, is that right @bwilkerson?)

I haven't found any definitions of super mixins. I see the following super mixins from the flutter repo referenced:

AnimationEagerListenerMixin (/Users/leafp/src/flutter/packages/flutter/lib/src/animation/listener_helpers.dart)
AnimationLazyListenerMixin (/Users/leafp/src/flutter/packages/flutter/lib/src/animation/listener_helpers.dart)
AnimationLocalListenersMixin (/Users/leafp/src/flutter/packages/flutter/lib/src/animation/listener_helpers.dart)
AnimationLocalStatusListenersMixin (/Users/leafp/src/flutter/packages/flutter/lib/src/animation/listener_helpers.dart)
AutomaticKeepAliveClientMixin (/Users/leafp/src/flutter/packages/flutter/lib/src/widgets/automatic_keep_alive.dart)
ContainerParentDataMixin (/Users/leafp/src/flutter/packages/flutter/lib/src/rendering/object.dart)
ContainerRenderObjectMixin (/Users/leafp/src/flutter/packages/flutter/lib/src/rendering/object.dart)
DebugOverflowIndicatorMixin (/Users/leafp/src/flutter/packages/flutter/lib/src/rendering/debug_overflow_indicator.dart)
GestureBinding (/Users/leafp/src/flutter/packages/flutter/lib/src/gestures/binding.dart)
LocalHistoryRoute (/Users/leafp/src/flutter/packages/flutter/lib/src/widgets/routes.dart)
PaintingBinding (/Users/leafp/src/flutter/packages/flutter/lib/src/painting/binding.dart)
RenderObjectWithChildMixin (/Users/leafp/src/flutter/packages/flutter/lib/src/rendering/object.dart)
RenderProxyBoxMixin (/Users/leafp/src/flutter/packages/flutter/lib/src/rendering/proxy_box.dart)
RendererBinding (/Users/leafp/src/flutter/packages/flutter/lib/src/rendering/binding.dart)
SchedulerBinding (/Users/leafp/src/flutter/packages/flutter/lib/src/scheduler/binding.dart)
ServicesBinding (/Users/leafp/src/flutter/packages/flutter/lib/src/services/binding.dart)
SingleTickerProviderStateMixin (/Users/leafp/src/flutter/packages/flutter/lib/src/widgets/ticker_provider.dart)
TickerProviderStateMixin (/Users/leafp/src/flutter/packages/flutter/lib/src/widgets/ticker_provider.dart)
ViewportNotificationMixin (/Users/leafp/src/flutter/packages/flutter/lib/src/widgets/scroll_notification.dart)
WidgetsBinding (/Users/leafp/src/flutter/packages/flutter/lib/src/widgets/binding.dart)
_RenderSliverPersistentHeaderForWidgetsMixin (/Users/leafp/src/flutter/packages/flutter/lib/src/widgets/sliver_persistent_header.dart)

Some sanity checking with grep doesn't find anything with Mixin in its name that I've missed in the above. @yjbanov @Hixie are there any things that you would expect to see here and don't?

I don't like that I'm not getting to see any application code (just libraries). I'll be running this on internal code overnight, which should give us some coverage, and as discussed above, will solicit example applications to test out on flutter-dev.

Packages examined so far are here:

https://github.com/dart-lang/package_resolver
https://github.com/dart-lang/watcher
https://github.com/dart-lang/convert
https://github.com/brianegan/flutter_redux
https://github.com/flutter/plugins/
https://github.com/dart-lang/stack_trace
https://github.com/dart-lang/async
https://github.com/dart-lang/args
https://github.com/ReactiveX/rxdart
https://github.com/tekartik/sqflite
https://github.com/dart-lang/charcode
https://github.com/dart-lang/source_span
https://github.com/dart-lang/string_scanner
https://github.com/dart-lang/collection
https://github.com/dart-lang/path
https://github.com/dart-lang/http
https://github.com/filiph/english_words
https://github.com/dart-lang/intl
https://github.com/flutter/cupertino_icons
https://github.com/flutter/plugins
https://github.com/brianegan/scoped_model
https://github.com/apptreesoftware/flutter_google_map_view
https://github.com/theyakka/fluro
https://github.com/dart-lang/intl_translation
https://github.com/apptreesoftware/flutter_barcode_reader
https://github.com/dart-lang/yaml
https://github.com/renggli/dart-xml
https://github.com/dart-lang/test
https://github.com/brendan-duncan/image
https://github.com/Lyokone/flutterlocation
https://github.com/Daegalus/dart-uuid
https://github.com/dart-lang/crypto
https://github.com/johnpryan/redux.dart
https://github.com/jathak/cli_repl
https://github.com/brianegan/font_awesome_flutter
https://github.com/dart-lang/json_serializable
https://github.com/dart-flitter/flutter_webview_plugin
https://github.com/dart-lang/stream_transform
https://github.com/dart-lang/typed_data
https://github.com/renefloor/flutter_cached_network_image
https://github.com/dart-lang/tuple
https://github.com/google/vector_math.dart
https://github.com/dart-lang/source_maps

[bwilkerson]

I'm running the analyzer with --show-package-warnings, which I believe means that I am checking the transitive closure of the import graph (but probably not checking files which are in reachable packages but not reachable from the import graph, is that right @bwilkerson?)

Correct.

[leafpetersen]

Ok, we have our first winners! dartdoc, file, and charts_flutter all use super mixins.

Interestingly neither of the first two actually uses super calls: they just mix in classes which have a non-Object super-class.

The dartdoc package (cc @jcollins-g) seems to define one super-mixin here:
https://github.com/dart-lang/dartdoc/blob/master/lib/src/model.dart#L1195

The file package (cc @tvolkert) defines several around forwarding, here (ForwardingFile, ForwardingDirectory, ForwardingLink):
https://github.com/google/file.dart/blob/master/packages/file/lib/src/forwarding/forwarding_file.dart#L12

The charts_flutter package has at least one definition (FlutterPanBehavior) here:
https://github.com/google/charts/blob/master/charts_flutter/lib/src/behaviors/zoom/pan_behavior.dart#L68

The charts_flutter example is the only one I've found so far that actually uses super calls in a mixin.

[tvolkert]

Yep, package:file definitely uses this. Happy to convert it to use the new syntax when available.

[yjbanov]

@leafpetersen good catch! RenderBoxContainerDefaultsMixin is not a super-mixin but merely a regular mixin, which your script correctly detected. I updated my comment #10 (comment)

[Hixie]

I'm actually surprised to see any usage outside Flutter, given that this isn't documented at all as far as I know.

[leafpetersen]

I'm actually surprised to see any usage outside Flutter, given that this isn't documented at all as far as I know.

Me too. :)

Ok, I've run a global presubmit on internal code. The only definitions of super mixins that I see are in file, charts_flutter, and flutter. Externally, there's dartdoc as well.

The file examples seem fully compatible with the super mixin proposal. There are no incompatible uses of the ForwardingXXX classes on github.

The charts_flutter example (FlutterPanBehavior) is used internally in a way that is mildly inconsistent with the proposal (it is instantiated as a class here: https://github.com/google/charts/blob/master/charts_flutter/lib/src/behaviors/zoom/pan_behavior.dart#L47), but this is trivial to fix internally there, and there are no uses on github at all outside of the package: https://github.com/search?q=FlutterPanBehavior&type=Code .

I think this is looking quite good in terms of breaking change cost. I'll mail out to flutter-dev today soliciting additional feedback and examples.

[leafpetersen]

Just a note: I searched github for extends CLASS with CLASS replaced with each of the super mixin classes identified here to understand whether there was a concern with losing the ability to extend these classes directly. I found no code which extends any of these outside of flutter itself, or the flutter2js package.

[leafpetersen]

Mail to flutter-dev went out here: https://groups.google.com/forum/#!topic/flutter-dev/dU6rwPVO-WA .

[yjbanov]

Should we record somewhere that Flutter currently ignores MIXIN_INFERENCE_INCONSISTENT_MATCHING_CLASSES in one place? Not sure if it's a big risk.

[mit-mit]

@leafpetersen I think we can close this now?

[leafpetersen]

Closing this, since this feature has landed.