extends Diagnosticable != with DiagnosticableMixin

[ds84182]

In many places, Diagnosticable is refered to directly instead of the mixin itself. This has severe implications when it comes to class hierarchies, because classes that use the mixin are incompatible with the Diagnosticable subsystem and will instead cause TypeErrors.

As a suggested fix:

1. Add implements Diagnosticable to DiagnosticableMixin
2. Change Diagnosticable to stop using DiagnosticableMixin (and treat it as an interface, copy the signatures from the methods exposed from DiagnosticableMixin)
3. Make all subclasses use with DiagnosticableMixin instead of extends Diagnosticable

This is, unfortunately, a breaking change for anyone using extends Diagnosticable.

Right now, the Flutter Inspector stops working when a ShortcutManager is specified in a Shortcuts widget.

cc @gspencergoog @goderbauer

[goderbauer]

/cc @jacob314

[jacob314]

Yes it seems like a bug that we are ever specifying Diagnosticable as the type of a parameter rather than DiagnosticableMixin.
Diagnosticable does not add any APIs.

[gspencergoog]

Whoa, interesting. Isn't is the case that anything that mixes in DiagnosticableMixin would be considered to "be" a DiagnosticableMixin, so that classes that extend Diagnosticable would also have the mixin's interface since Diagnosticable mixes it in? If not, then consider my mind blown. :-)

What makes the inspector stop working? Does it throw an exception?

Of your three choices, I don't think you can do number 2 without removing the benefit of being able to have a mixin version of the interface, so that in ShortcutManager's case, ChangeNotifier would have to now implement Diagnosticable, or ShortcutManager would have to re-implement the whole interface itself (which seems messy).

I'd think number 1 is least invasive. Number 3 is probably how it should have been from the beginning though: this kind of interface lends itself to being a mixin.

[ds84182]

Sorry, I should've used bullet points. The suggested fix is all three of them.

In order to get the correct typing behavior, DiagnosticableMixin needs to declare that it is implementing Diagnosticable's interface. With the code currently, this breaks at compile time because it introduces a circular dependency between DiagnosticableMixin and Diagnosticable. So to fix the circular dependency introduced from making DiagnosticableMixin implement Diagnosticable, you have to stop using it as a mixin in Diagnosticable. Then, because Diagnosticable is just an interface now, every place where extends Diagnosticable is used needs to be replaced with with DiagnosticableMixin.

This is all because Dart's typing isn't structural. Currently, using DiagnosticableMixin means that a class just happens to have the same exposed interface as Diagnosticable, but doesn't implement it. It's akin to:

mixin Foo {
  void bar() => print('bar');
}

abstract class Bar {
  void bar() => print('bar');
}

class BarDerived extends Bar {}
class FooDerived = Object with Foo;

void main() {
  print(FooDerived() is Bar); // false
  print(BarDerived() is Foo); // false

  dynamic fooBar = FooDerived(); // or BarDerived();
  fooBar.bar(); // Works, but they don't share a common superclass other than Object, so unsafe.
}

[gspencergoog]

Ahh, OK that makes a lot of sense. I was wondering how number one was going to work on its own. :-)

But wouldn't just eliminating Diagnosticable entirely and having everything use the mixin also solve the problem (i.e. just do number 3)?

[ds84182]

Yep, I (personally) feel that referring to a mixin as a type is kinda weird, but that works too.

[ds84182]

Here's a reproduction snippet

import 'package:flutter/widgets.dart';

void main() {
  try {
    print(Shortcuts(manager: ShortcutManager()));
  } catch (ex, stack) {
    print(ex);
    print(stack);
  }
}

[gspencergoog]

Ahh. I was wondering why this wasn't tweaking tests, but I guess I don't have a test that prints the diagnostics of a Shortcuts widget that has a manager set.

So as long as you don't set a manager, then https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/widgets/shortcuts.dart#L371 doesn't try to do anything with the manager, so it doesn't fail.

[gspencergoog]

So, @ds84182 are you working on a fix, or should I take a look at fixing it?

[ds84182]

I'm not working on a fix right now