extends Diagnosticable != with DiagnosticableMixin

@gspencergoog

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:

Add implements Diagnosticable to DiagnosticableMixin
Change Diagnosticable to stop using DiagnosticableMixin (and treat it as an interface, copy the signatures from the methods exposed from DiagnosticableMixin)
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

@jacob314

/cc @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.

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.

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.
}

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)?

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

Here's a reproduction snippet

import 'package:flutter/widgets.dart';

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

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.

@ds84182

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

I'm not working on a fix right now

Use DiagnosticableMixin instead of Diagnosticable for some conditiona…

I did upgrade to the libraries(pubspec.yaml) and it function.

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.

Navigation Menu

Repository navigation

extends Diagnosticable != with DiagnosticableMixin #49647

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

extends Diagnosticable != with DiagnosticableMixin #49647

Description

Activity

goderbauer commented on Jan 28, 2020

jacob314 commented on Jan 29, 2020

gspencergoog commented on Jan 29, 2020

ds84182 commented on Jan 29, 2020

gspencergoog commented on Jan 29, 2020

ds84182 commented on Jan 29, 2020

ds84182 commented on Jan 29, 2020

gspencergoog commented on Jan 29, 2020

gspencergoog commented on Jan 29, 2020

ds84182 commented on Jan 29, 2020

jimmysalto commented on Jul 29, 2020

github-actions commented on Aug 18, 2021

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions