new lint: warn when overriding toString() in a Diagnosticable

[pq]

From flutter/flutter#29622 (comment).

[W]e could add a lint to the DiagnosticsNode lints that warns if a user overrides toString directly while implementing Diagnosticable. Overriding toString directly indicates confusion about how the API should be used and we should push users on the right path. The lint could tell users to override debugFillProperties instead.

[pq]

See: #34378

(Another case where an annotation would help.)

[pq]

Now that we have support for @nonVirtual, the next step is to update Diagnosticable.

A preliminary is updating flutter to get the latest package:meta which is causing me some trouble (flutter/flutter#44477).

[pq]

A preliminary is updating flutter to get the latest package:meta which is causing me some trouble (flutter/flutter#44477).

This is complete. 🎉

Making DiagnosticableMixin identifies 4 violations in Flutter currently:

1. CupertinoDynamicColor: https://github.com/flutter/flutter/blob/01f4f1ac5576a09dca32df6a2c9eaca1af1a7f22/packages/flutter/lib/src/cupertino/colors.dart#L1032-L1036

Seems to be exactly the kind of implementation that would be better done using debugFillProperties but I'm missing context (and rationale).

@LongCatIsLooong: any thoughts?

2. FlutterErrorDetails https://github.com/flutter/flutter/blob/a901b650b60b96413b93daf85ceadf2c869f0688/packages/flutter/lib/src/foundation/assertions.dart#L466-L468

@jacob314: thoughts on this one?

3. Alive (in list_view_test): https://github.com/flutter/flutter/blob/a901b650b60b96413b93daf85ceadf2c869f0688/packages/flutter/test/widgets/list_view_test.dart#L30

@a14n: is this adding long-term value or just used for debugging?

4. CyclicDiagnostic (in widget_inspector_test): https://github.com/flutter/flutter/blob/a901b650b60b96413b93daf85ceadf2c869f0688/packages/flutter/test/widgets/widget_inspector_test.dart#L119-L121

@jacob314: maybe we ignore this one since it seems to be legitimate for test?

[jacob314]

2. This one is tricky. Using the error style is useful for backwards compatibility. We could as a breaking change make the toString return a shorter string for this case and require users to write toStringDeep() when they really want the deep toString access.
3. The test should be cleaned up to not require overriding this.
4. This one is reasonable to ignore as it is legitimate for a regression test. Otherwise this intentionally broken code would trigger a stack overflow.

[LongCatIsLooong]

@pq thanks for the new lint!

CupertinoDynamicColor.toString() was overridden to make the current active color immediately clear, e.g. with CupertinoDynamicColor(*color = Color(0xff000000)*, darkColor = Color(0xff000001), highContrastColor = Color(0xff000003)) I know the active color is just color = Color(0xff000000) whereas to interpret CupertinoDynamicColor(color = Color(0xff000000), darkColor = Color(0xff000001), highContrastColor = Color(0xff000003), activeColor = Color(0xff000000)) I would need to go over the list of colors and find the color that has the exact hex code. Is there a way to get a similar string representation of a dynamic color without overriding toString()?

[jacob314]

you will be able to do that by extending the existing debugFillProperties method to specify which color is important.
that will also give you rendering that is more consistent with the existing debug data scheme. e.g.
CupertinoDynamicColor(*color : Color(0xff00000)*, darkColor: Color(0xff0001), ...)
You could give the summary color property DiagnosticsLevel.summary as you do appear to be wanting to indicate that diagnostic summarizes the other ones present. You would then need to customize the text rendering code to bold summary diagnostic properties when using the single line display. This would also open up the option of making the property show up as bold in debugging tools such as the inspector.
https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/foundation/diagnostics.dart#L72

[pq]

@jacob314 for FlutterErrorDetails I'd be inclined to leave it as is (w/ an ignore). Unless you feel strongly?

[pq]

@a14n: would you be up for cleaning up list_view_test?

[jacob314]

Leaving FlutterErrorDetails is reasonable. Like any ignore, it would be nice to later cleanup to avoid using an ignore but it isn't critical.

[a14n]

Removing https://github.com/flutter/flutter/blob/a901b650b60b96413b93daf85ceadf2c869f0688/packages/flutter/test/widgets/list_view_test.dart#L30 doesn't generate test failures. So you can directly remove it in your PR that adds the lint.