Use DiagnosticableMixin instead of Diagnosticable for some conditionals.

[gspencergoog]

Description

As the code is right now, if you try and print diagnostics for a Shortcuts widget that has a manager set (a ShortcutManager), it will fail at runtime. This code:

import 'package:flutter/widgets.dart';

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

will fail with this exception:

flutter: type 'ShortcutManager' is not a subtype of type 'Diagnosticable' in type cast
flutter: #0      DiagnosticableMixin.toDiagnosticsNode (package:flutter/src/foundation/diagnostics.dart:3113:19)
#1      DiagnosticableMixin.toString.<anonymous closure> (package:flutter/src/foundation/diagnostics.dart:3095:20)
#2      DiagnosticableMixin.toString (package:flutter/src/foundation/diagnostics.dart:3097:6)
#3      DiagnosticsProperty.valueToString (package:flutter/src/foundation/diagnostics.dart:2698:61)
#4      DiagnosticsProperty.toDescription (package:flutter/src/foundation/diagnostics.dart:2712:21)
#5      TextTreeRenderer.render (package:flutter/src/foundation/diagnostics.dart:1169:31)
#6      TextTreeRenderer.render (package:flutter/src/foundation/diagnostics.dart:1279:39)
#7      DiagnosticsNode.toStringDeep (package:flutter/src/foundation/diagnostics.dart:1709:7)
#8      DiagnosticsNode.toString (package:flutter/src/foundation/diagnostics.dart:1631:14)
#9      DiagnosticableMixin.toString.<anonymous closure> (package:flutter/src/foundation/diagnostics.dart:3095:78)
#10     DiagnosticableMixin.toString (package:flutter/src/foundation/diagnostics.dart:3097:6)
#11     _StringBase._interpolateSingle (dart:core-patch/string_patch.dart:823:17)
#12     print (dart:core/print.dart:11:26)
#13     main (package:focus_samples/focus_traversal_group.dart:5:5)
...

The reasons for this are explained in #49647 (comment)

This PR fixes this by changing DiagnosticableMixin.toDiagnosticsNode and other things to accept DiagnosticableMixin instead of requiring Diagnosticable.

Eventually, after making some changes to the linter and some other external repos, we can deprecate and them remove Diagnosticable entirely and just have the mixin.

Related Issues

• Fixes extends Diagnosticable != with DiagnosticableMixin #49647

Tests

• Updated diagnostics_test.dart, although no functionality changed.

Breaking Change

• No, this is not a breaking change.

[gspencergoog]

Actually, after doing the "full" fix here, it occurred to me that there is a simpler fix to the original problem, although it still doesn't fix the structural issues with the way Diagnosticable is set up.

We can just change DiagnosticableNode to be:

class DiagnosticableNode<T extends DiagnosticableMixin> extends DiagnosticsNode {

and change DiagnosticableMixin.toDiagnosticsNode to use DiagnosticableMixin instead:

  DiagnosticsNode toDiagnosticsNode({ String name, DiagnosticsTreeStyle style }) {
    return DiagnosticableNode<DiagnosticableMixin>(
      name: name,
      value: this,
      style: style,
    );
  }

That wouldn't be a breaking change, and nothing gets deprecated. It does leave a pitfall for people that use is Diagnosticable instead of is DiagnosticableMixin, so it's not as desirable in that sense.

[ds84182]

LGTM

Later on, would it be useful to switch the names? With generalized typedefs I think you'd be able to do

mixin Diagnosticable {...}

@Deprecated(...)
typedef DiagnosticableMixin = Diagnosticable;

to move everyone over to the new name.

[gspencergoog]

I don't think I'd switch the names back, simply because Flutter has a convention of naming mixins with the suffix "Mixin", and mixin Diagnosticable wouldn't follow that convention.

What would you think of me doing the simpler change I mentioned in #49726 (comment)?

[ds84182]

Gotcha. Having them both around seems like a potential foot-gun, so I think it's better to have one way of adding DiagnosticableMixin to a class.

…

On Wed, Jan 29, 2020, 1:08 PM Greg Spencer ***@***.***> wrote: I don't think I'd switch the names back, simply because Flutter has a convention of naming mixins with the suffix "Mixin", and mixin Diagnosticable wouldn't follow that convention. What would you think of doing the simpler change I mentioned in #49726 (comment) <#49726 (comment)>? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#49726?email_source=notifications&email_token=AARJWZISBJYOHSFWLBLBTA3RAHV47A5CNFSM4KNKYQNKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKIX2VA#issuecomment-579960148>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AARJWZMLZWQXA54PNLSXF3DRAHV47ANCNFSM4KNKYQNA> .

[gspencergoog]

OK, I think deprecating and removing Diagnosticable is the right change eventually, but in order to be able to put in preemptive changes to other repos to make the transition smoother, I think I'll do the simpler fix first, and then convert other repos to use DiagnosticableMixin ahead of this change that switches all widgets to not be Diagnosticable, because any checks for is Diagnosticable would fail.

Things like the dart linter will stop reporting lints for diagnostic_describe_all_properties on classes that switch to the mixin, and the dart devtools needs changes too. If I don't make the fix first, then it would break things to switch non-Flutter-repo classes to use the mixin instead of extending Diagnosticable. They would fail in the same way as ShortcutManager when they tried to be converted to a DiagnosticNode.

Once I commit the simple fix, then I can switch other classes without any breakage, so that when I commit a change that changes the type of the widgets, nothing will stop working.

I've reverted the changes that switch widgets to use the mixin, and added implements Diagnosticable to those few classes that were just using the mixin, as well as switching anything that was testing is Diagnosticable or template args that assert <T extends Diagnosticable> to use the mixin instead, since that only broadens the match.

[lock]

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.