More fixes from enabling all of code metrics #4931

jacob314 · 2022-12-10T01:20:37Z

I'm still not done but this is a good sampling to get feedback.

Some of the dead code caught was pretty amazing. Also helped clean up a bunch of really dumb async code.

#    - avoid-global-state   <- probably could be enabled as a follow up although most of our use cases are reasonable for just setting bool flags.
#    - avoid-ignoring-return-values. <-- too many false positives. wish this one could be enabled.
#    - avoid-late-keyword <-- generally we think late is ok or at least unavoidable.
#    - avoid-non-null-assertion <-- unrealistically strict for devtools.
#    - avoid-passing-async-when-sync-expected TODO(jacobr): consider re-enabliing.
# This one seems interesting but has too many false positives. Gave it a try including kitchen sink adding parameters to ignore. Generally it didn't seem to catch that many real issues but had tons of false positives.
# - no-equal-arguments:
#    ignored-parameters:
#      - height
#      - width
#      - double-literal-format
#      - defaultSortColumn
#      - left
#      - right
#      - top
#      - bottom
#      - bottomLeft
#      - topLeft
#      - enabledBorder
#   - no-magic-number
# - prefer-match-file-name
# - prefer-static-class
# - avoid-returning-widgets  This one is nice but has a lot of false positives. Would be great to figure out how to make it more tolerant of reasonable patterns where a separate widget would just make things ugly.
# - prefer-single-widget-per-file. <-- just didn't match our style.

jacob314 · 2022-12-10T01:21:09Z

packages/devtools_app/lib/src/app.dart


    _isDarkThemeEnabled = preferences.darkModeTheme.value;
-    preferences.darkModeTheme.addListener(() {
+    addAutoDisposeListener(preferences.darkModeTheme, () {


Fyi @polina-c this caught some cases where were adding listeners but never removing.

jacob314 · 2022-12-10T01:22:04Z

packages/devtools_app/lib/src/app.dart


  Widget _buildTabbedPage(
-    BuildContext context,
+    BuildContext _,


the lint about unused parameters was great although I wish it was a bit smarter about methods that were torn off in the same file.
I also wish it handled __ as well as _.

Could you give more details, how smarter you want it to be?

I also wish it handled __ as well as _.

Looks like a bug to me, since the rule should ignore any amount of _. Could you share an example?

_buildTabbedPage is private and the only use is within this file is where it torn off. Glad to hear any number of _ should be allowed.

Do I understand correctly: you mean that for private methods used as tear off the rule should not check for unused parameters?

correct as the unused parameters were only there to match the signature required to use it as a tearoff. Its similar to why you shouldn't complain about unused parameters for an @OverRide

Thank you! Added to backlog, will ship in the next release

jacob314 · 2022-12-10T01:22:51Z

packages/devtools_app/lib/src/app.dart

          ideTheme: ideTheme,
-          child: _providedControllers(
-            offline: true,
+          child: MultiProvider(


@kenzieschmoll not sure if this is clearer.
Technically this doesn't solve the lint as the lint still gets mad about methods returning List but I think it solves the spirit and in some ways is a little clearer than the opaque helper.

The only downside of pulling the MultiProvider out of the helper is that this creates some duplicated code where we have to manually specify MultiProvider wherever _providedControllers is used, but I do see how _providedControllers already sounds like it should return a List as is.

Why did child: _providedControllers trigger a lint to begin with?

The lint is one to guide you towards creating widgets rather than creating functions that return widgets. It catches a lot of ugly code (particularly cases where the BuildContext was passed around) as well as some false positives like this.

I wonder if the duplicate code is a little more idiomatic in that it is clear what we are doing is injecting providers.

@incendial this was the case where I couldn't keep the code idiomatic and avoid the lint about functions returning widgets. Seems odd the lint ever fires for functions returning Lists of widgets. Notice that the lint still fires with my fix here as it is upset about the method called that returns a List of all providers for the class.

I see, never seen this case before, tbh 😅. If I change the rule to ignore Providers, will this be enough? The more I look at this case, the more I don't understand how a possible "right way" without a method could look like.

Seems odd the lint ever fires for functions returning Lists of widgets

Very common case is when people create a private method returning List and then spread it in the widget's Column or Row.

So far the only case that is really blocking me is the provider case. The Column and Row cases are interesting. I do buy that those cases are probably usually bugs.

Thank you! Added to backlog, will ship in the next release

jacob314 · 2022-12-10T01:23:26Z

packages/devtools_app/lib/src/app.dart

      message: 'Settings',
      child: InkWell(
-        onTap: () async {
+        onTap: () {


here and elsewhere the lints caught a ton of cases where we were pretending we were doing useful async things when we weren't. Fyi @pq

@kenzieschmoll let me know if these cases where we are doing async stuff in tap handlers is useful or noise.

This one seems interesting but has too many false positives. Gave it a try including kitchen sink adding parameters to ignore. Generally it didn't seem to catch that many real issues but had tons of false positives. # - no-equal-arguments:

Do you mean that the amount of false positives was too much to even list in the rule config?
Yeah I was tossing tons of names in the false positives list in the rules config but not seeing any cases where the lint was adding value so I gave up.

avoid-returning-widgets This one is nice but has a lot of false positives. Would be great to figure out how to make it more tolerant of reasonable patterns where a separate widget would just make things ugly.

That's interesting. If I remember correctly, we cleaned it up from all the known false positives we had. Could you share the cases you consider false positive for this rule so I can handle them properly?
I tagged you in the comment where I tried to make this work idiomatically for a case where we needed a helper method that computed the list of all Providers to inject with a MultiProvider.

Yeah I was tossing tons of names in the false positives list in the rules config but not seeing any cases where the lint was adding value so I gave up.

This rule initially was created for models mapping, when you can have like 2 String fields and then accidentally pass same value to both fields. Maybe it should be restricted somehow to not trigger on widgets, but I'm not sure.

I tagged you in the comment where I tried to make this work idiomatically for a case where we needed a helper method that computed the list of all Providers to inject with a MultiProvider.

Thank you, answered above.

Maybe it should only apply to certain types of parameters. Seems most relevant for string fields where there is more of a sign of duplicate logic than for num fields or arbitrary class fields.

Good point, I'll take some time to think

jacob314 · 2022-12-10T01:24:08Z

packages/devtools_app/lib/src/app.dart

 /// Conditional screens can be added to this list, and they will automatically
 /// be shown or hidden based on the [Screen.conditionalLibrary] provided.
 List<DevToolsScreen> get defaultScreens {
+  final inspectorTreeController = InspectorTreeController();


@kenzieschmoll this is an attempt to placate the duplicate args lint which I gave up on. This one should probably be reverted.

what does the duplicate args lint prevent? two arguments aren't allowed to have the same value?

agreed this should be reverted as this is more verbose without an obvious benefit

yeah I don't think it is useful. I do think code is generally a bit harder to read when there are multiple args passed in with the exact same expression particularly if the args aren't named but generally I think it has far too many false positives.
Reverted.

jacob314 · 2022-12-10T01:24:28Z

packages/devtools_app/lib/src/framework/framework_core.dart

 import '../shared/survey.dart';

-typedef ErrorReporter = void Function(String title, dynamic error);
+typedef ErrorReporter = void Function(String title, Object error);


lints were good at catching a ton of uses of dynamic that were left in our code.

jacob314 · 2022-12-10T01:25:16Z

packages/devtools_app/lib/src/framework/landing_screen.dart

            const SizedBox(width: defaultSpacing),
            ElevatedButton(
-              onPressed: actionInProgress ? null : _connect,
+              onPressed: actionInProgress ? null : () => unawaited(_connect()),


this is one of the more unfortunate cases for the lints. They make you call out the cases where you are firing and forgetting async methods. On the other hand, that does make the code look a bit ugly.

Something about this makes me wonder if onPressed should be accepting an async Callback and that the Button should be accepting the responsibility of unawaiting it.

I feel like it would be prettier if we could do the following instead:
` onPressed: actionInProgress ? null : () async =>_connect()

It would be interesting if there was a version of the button class that played nicely with async callbacks. For example, the ideal version wouldn't unawait the future (which just drops it on the ground) and would instead make the button show as disabled until the onPressed callback completes. That would probably be generally useful in DevTools. We have hacked around it with custom logic on some pages but a general solution might be feasible.

jacob314 · 2022-12-10T01:26:13Z

packages/devtools_app/lib/src/framework/landing_screen.dart


  @override
  Widget build(BuildContext context) {
+    final textTheme = Theme.of(context).textTheme;


@kenzieschmoll I love this lint warning about duplicate code. It false positives a little bit with enum entries and methods with side effects but in general is tremendously useful in helping people optimize duplicate lookups in build methods.

It false positives a little bit with enum entries

Could you share an example? I'd like to fix that. Not sure that the problem with side effects is fixable, probably a candidate for // ignore.

The lint fires for no good reason if you import a library with a named prefix.
For example:

import 'library_with_enum' as libraryName void main() { print(libraryName.EnumName.entry1); print(libraryName.EnumName.entry2); }

Thank you, I've added to backlog!

jacob314 · 2022-12-10T01:27:14Z

packages/devtools_app/lib/src/framework/notifications_view.dart

+  }
+}
+
+class _NotificationOverlay extends StatelessWidget {


@kenzieschmoll I moved some but not all cases where we had functions that should have been widgets over to using widgets.

incendial · 2022-12-12T10:32:03Z

@jacob314 thank you for mentioning!

avoid-ignoring-return-values. <-- too many false positives. wish this one could be enabled.

Yeah, I like the idea behind it too, but unfortunately, it's really hard to distinguish if a value supposed to be used or was left unused by mistake. Right now this is more like a "run from time to time" type of a rule. Sorry for that.

This one seems interesting but has too many false positives. Gave it a try including kitchen sink adding parameters to ignore. Generally it didn't seem to catch that many real issues but had tons of false positives. # - no-equal-arguments:

Do you mean that the amount of false positives was too much to even list in the rule config?

avoid-returning-widgets This one is nice but has a lot of false positives. Would be great to figure out how to make it more tolerant of reasonable patterns where a separate widget would just make things ugly.

That's interesting. If I remember correctly, we cleaned it up from all the known false positives we had. Could you share the cases you consider false positive for this rule so I can handle them properly?

kenzieschmoll · 2022-12-12T17:56:24Z

packages/devtools_app/lib/src/framework/landing_screen.dart


-  Widget _buildConnectInput() {
-    return Column(
+    final connectorInput = Column(


should we make this a private helper widget instead?

I think this pattern is better for cases where you are just trying to avoid a very deep and hard to read build method. The # of params this widget would need relative to its impl would be silly and placing the code inline avoids the illusion that Flutter does anything smart optimizing it.

kenzieschmoll · 2022-12-12T17:57:44Z

packages/devtools_app/lib/src/framework/notifications_view.dart

    // Make a copy so we do not remove a notification from [_notifications]
    // while iterating over it.
-    final notifications = List<_Notification>.from(_notifications);
+    final notifications = List<_Notification>.of(_notifications);


was this caught by a lint? what is the lint preventing?

This is a lint suggested at https://github.com/dart-lang/linter/issues/2555
that Dart Code Metrics implemented.

kenzieschmoll · 2022-12-12T18:00:00Z

packages/devtools_app/lib/src/screens/app_size/app_size_table.dart

+    final Comparable valueA = getValue(a) as Comparable;
+    final Comparable valueB = getValue(b) as Comparable;


are these casts still necessary after changing the return type of getValue?

Unfortunately yes. They were hidden by using dynamic.

kenzieschmoll · 2022-12-12T18:01:07Z

packages/devtools_app/lib/src/screens/debugger/breakpoint_manager.dart


  void _updateAfterIsolateReload(
-    Event reloadEvent,
+    Event _,


probably don't need this trailing comma anymore since this should all fit on one line

kenzieschmoll · 2022-12-12T18:03:43Z

packages/devtools_app/lib/src/screens/debugger/codeview.dart

+    unawaited(() async {
+      Clipboard.setData(


this is confusing... why does Clipboard.setData need to be wrapped as an unawaited async callback?

kenzieschmoll · 2022-12-12T18:04:26Z

packages/devtools_app/lib/src/screens/debugger/controls.dart

              // a system isolate.
              onPressed: ((isPaused && !resuming) && !isSystemIsolate)
-                  ? controller.resume
+                  ? () => unawaited(controller.resume())


this is a bit uglier to have to do this everywhere

kenzieschmoll · 2022-12-12T18:05:16Z

packages/devtools_app/lib/src/screens/debugger/debugger_screen.dart


-    addAutoDisposeListener(widget.controller.isPaused, _updateStatus);
+    addAutoDisposeListener(
+        widget.controller.isPaused, () => unawaited(_updateStatus()));


nit: trailing comma

done. All regular lints are now resolved here and elsewhere.

kenzieschmoll · 2022-12-12T18:05:30Z

packages/devtools_app/lib/src/screens/debugger/debugger_screen.dart

    // todo: should we check that widget.controller != oldWidget.controller?
-    addAutoDisposeListener(widget.controller.isPaused, _updateStatus);
+    addAutoDisposeListener(
+        widget.controller.isPaused, () => unawaited(_updateStatus()));


here too. our lints should catch this

done here and elsewhere.

kenzieschmoll · 2022-12-12T18:06:24Z

packages/devtools_app/lib/src/screens/debugger/variables.dart

          displayProvider(context, variable, onPressed, debuggerController),
-      onItemSelected: (variable) async =>
-          onItemPressed(variable, debuggerController),
+      onItemSelected: (variable) async => onItemPressed(variable),


does onItemPressed need to be awaited or unawaited?

Fyi @incendial. Wonder why the lints for unneeded async didn't trigger here.
This code could be further simplified to just
onItemSelected: onItemPressed

Agree, noted

kenzieschmoll · 2022-12-12T18:07:31Z

packages/devtools_app/lib/src/screens/inspector/inspector_screen_details_tab.dart

-        );
-        await widget.controller.collapseDetailsToSelected();
-      }),
+    ga.select(


why do we no longer want to blockWhileInProgress?

@incendial this is an example of a good success story for the Dart Code Metrics lints. The lints caught that ga.select never needed to be async which triggered further lints to clarify that we never needed to be calling blockWhileInProgress as nothing async was actually happening in the whole block.

kenzieschmoll · 2022-12-12T18:10:04Z

packages/devtools_app/lib/src/screens/memory/shared/primitives/painting.dart

+    height: 6,
+    width: 20,


not your fault but these should be named consts

Agreed. FYI there is a Dart Code Metrics lint for exactly that problem we can enable next. Leaving moving these const off this CL as it is already too big and too error prone.

kenzieschmoll · 2022-12-12T18:10:40Z

packages/devtools_app/lib/src/screens/network/network_request_inspector_views.dart

  }

-  Widget _buildHttpTimeGraph(BuildContext context) {
+  Widget _buildHttpTimeGraph() {


we should move these to private helper widgets instead of helper methods

Leaving that for a follow-up enabling the DCM lint to use private helper widgets after enabling the associated lint.

kenzieschmoll · 2022-12-12T18:11:31Z

...ges/devtools_app/lib/src/screens/performance/panes/raster_stats/raster_stats_controller.dart

+  @override
+  Future<void> init() {
+    return Future.value();
+  }
+
+  @override
+  Future<void> onBecomingActive() {
+    return Future.value();
+  }


what lint is forcing these overrides?

if PerformanceFeatureController returns these values by default, do the subclasses need these empty overrides?

This was a judgment call. The base class had 3 methods tagged as async that weren't really async. 1 of the 3 methods was actually overridden everywhere so could be made abstract. These other two were overridden almost everywhere except for this case where a no-op implementation had to be added. In general, if a member should almost always be overridden I think it is cleaner to make it abstract. In addition, overriding a method returning future is very ugly as you end up awaiting the super version of the method which is quite silly given the super version was. a no-op.

kenzieschmoll · 2022-12-12T18:14:30Z

...app/lib/src/screens/performance/panes/timeline_events/perfetto/_perfetto_controller_web.dart

      ..height = '100%'
      ..width = '100%';

+    // TODO(jacobr): why does this have to be ignored?


I'm not sure on the details of why, but this is a weird flutter web thing. @ditman do you know the reasoning for this?

This needs to be ignored because the API surface of dart:ui for the web is bigger than for mobile (it has the webOnlyXXX methods that are not defined in the mobile version, for example, or the platformViewRegistry object).

analyzer flags this as you calling "things that don't exist" in the API because it only sees the proper dart:ui mobile package, not the web re-implementation with extra methods.

A workaround to this ignore is to apply a shim for the web version of dart:ui (but I've only used it in web-only packages), like this one, used here.

This is the (venerable) tracking issue: platformViewRegistry.registerViewFactory func is missing in import dart.ui flutter#41563

Thanks. Referenced the issue.

kenzieschmoll · 2022-12-12T18:16:10Z

packages/devtools_app/lib/src/screens/profiler/cpu_profile_controller.dart

-      throw Exception('Error loading data with tag "$tag": ${e.toString()}');
+      Error.throwWithStackTrace(
+          Exception('Error loading data with tag "$tag": ${e.toString()}'),
+          stackTrace);


Done. Interestingly this is a case the DCM trailing comma lint will flag. As a follow up I may try enabling it.

kenzieschmoll · 2022-12-12T18:16:23Z

packages/devtools_app/lib/src/screens/profiler/cpu_profile_controller.dart

+      data = tag == groupByUserTag || tag == groupByVmTag
+          ? CpuProfilePair.withTagRoots(
+              fullData,
+              tag == groupByUserTag
+                  ? CpuProfilerTagType.user
+                  : CpuProfilerTagType.vm,
+            )
+          : CpuProfilePair.fromUserTag(fullData, tag);


this double nest is harder to read imo

kenzieschmoll · 2022-12-12T18:17:40Z

packages/devtools_app/lib/src/shared/charts/treemap.dart

-    } else {
-      return treemapIncreaseColor;
-    }
+    return byteSize < 0 ? treemapDecreaseColor : treemapIncreaseColor;


did you merge with master? some of these changes are looking familiar from your last PR

kenzieschmoll · 2022-12-12T18:18:38Z

packages/devtools_app/lib/src/shared/ui/gtags.dart


 @JS('gtag')
-external void _gTagCommandName(String command, String name, [dynamic params]);
+external void _gTagCommandName(String command, String name, [Object params]);


should this be Object? - here and below

Done. Here and elsewhere made a lot more cases of dynamic into Object? instead of Object

kenzieschmoll · 2022-12-12T18:19:32Z

packages/devtools_app/test/test_infra/utils/file_utils.dart

-  } else {
-    packagesPath = io.Directory.current.parent.path;
-  }
+  packagesPath = io.Directory.current.path.endsWith('test')


I wonder why the duplicated code lint isn't catching that io.Directory.current and io.Directory.current.parent are duplicated

I tuned it up to 3 dupes to reduce the # of false positives. Can revisit with 2 dupes after the initial cleanup lands.

CoderDake

Took a look at part of this, only 1 comment so far

CoderDake · 2022-12-21T16:50:03Z

packages/devtools_app/lib/src/framework/landing_screen.dart

            const SizedBox(width: defaultSpacing),
            ElevatedButton(
-              onPressed: actionInProgress ? null : _connect,
+              onPressed: actionInProgress ? null : () => unawaited(_connect()),


Something about this makes me wonder if onPressed should be accepting an async Callback and that the Button should be accepting the responsibility of unawaiting it.

I feel like it would be prettier if we could do the following instead:
` onPressed: actionInProgress ? null : () async =>_connect()

jacob314 · 2022-12-21T22:28:55Z

All comments addressed and build is now green. PTAL.

jacob314 requested review from a team, CoderDake, bkonyi, elliette, kenzieschmoll and polina-c as code owners December 10, 2022 01:20

jacob314 commented Dec 10, 2022

View reviewed changes

polina-c approved these changes Dec 10, 2022

View reviewed changes

kenzieschmoll reviewed Dec 12, 2022

View reviewed changes

jacob314 force-pushed the code_metric_fixes2 branch 4 times, most recently from 4c73118 to a0e30c1 Compare December 20, 2022 22:42

CoderDake reviewed Dec 21, 2022

View reviewed changes

jacob314 force-pushed the code_metric_fixes2 branch 6 times, most recently from 047f630 to 490231d Compare December 21, 2022 22:23

Fix a number more issues by enabling Dart code metrics.

65acc47

jacob314 force-pushed the code_metric_fixes2 branch from 490231d to 65acc47 Compare December 21, 2022 22:23

kenzieschmoll approved these changes Dec 21, 2022

View reviewed changes

jacob314 merged commit 44a57db into flutter:master Dec 21, 2022

jacob314 deleted the code_metric_fixes2 branch December 21, 2022 23:59

CoderDake mentioned this pull request Dec 22, 2022

Prepare Release Notes for 2.21.0 release #4982

Closed

64 tasks

CoderDake mentioned this pull request Jan 24, 2023

test bot pr #5099

Closed

7 tasks

DartDevtoolWorkflowBot mentioned this pull request Jan 24, 2023

test bot pr #5103

Closed

7 tasks

		final Comparable valueA = getValue(a) as Comparable;
		final Comparable valueB = getValue(b) as Comparable;

More fixes from enabling all of code metrics #4931

More fixes from enabling all of code metrics #4931

Uh oh!

Conversation

jacob314 commented Dec 10, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

incendial Dec 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacob314 Dec 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

incendial commented Dec 12, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

incendial Dec 21, 2022 •

edited

Loading

jacob314 Dec 15, 2022 •

edited

Loading