[avoid_classes_with_only_static_members ] consider reverting fix to flag classes w/ only static methods

[pq]

The change is dart-lang/linter@b48290e addressing #58439 and is currently blocking an SDK roll (try here).

flutter-analyze-try

   info • Avoid defining a class that contains only static members • dev/integration_tests/flutter_gallery/lib/demo/shrine/model/products_repository.dart:7:1 • avoid_classes_with_only_static_members
   info • Avoid defining a class that contains only static members • packages/flutter/lib/src/widgets/binding.dart:1234:1 • avoid_classes_with_only_static_members
   info • Avoid defining a class that contains only static members • packages/flutter/test/widgets/widget_inspector_test.dart:232:1 • avoid_classes_with_only_static_members
   info • Avoid defining a class that contains only static members • packages/flutter_driver/lib/src/common/fuchsia_compat.dart:79:1 • avoid_classes_with_only_static_members
   info • Avoid defining a class that contains only static members • packages/flutter_test/lib/src/event_simulation.dart:26:1 • avoid_classes_with_only_static_members

See: https://logs.chromium.org/logs/dart/buildbucket/cr-buildbucket.appspot.com/8841227602458710784/+/u/analyze_flutter_flutter/stdout

A few violations are in tests but 2 are API:

• flutter: https://github.com/flutter/flutter/blob/38c2af6a67ff8cefc8f6d8dee79584357fa2b941/packages/flutter/lib/src/widgets/binding.dart#L1237

• flutter_driver: https://github.com/flutter/flutter/blob/38c2af6a67ff8cefc8f6d8dee79584357fa2b941/packages/flutter_driver/lib/src/common/fuchsia_compat.dart#L85

/fyi @goderbauer @bwilkerson

[bwilkerson]

My understanding is that the fix is correct and that the lint was previously broken.

If that's true, and if the cases that the lint is flagging are intentional, then perhaps we should consider marking them with an ignore comment rather than reverting the fix.

[goderbauer]

https://github.com/flutter/flutter/blob/38c2af6a67ff8cefc8f6d8dee79584357fa2b941/packages/flutter/lib/src/widgets/binding.dart#L1237

This looks like a false positive to me that should not be flagged by the lint. The class's purpose is to combine several mixins into a usable (i.e. instantiable) class. It's arguably not defining a class that is just holding statics.

https://github.com/flutter/flutter/blob/38c2af6a67ff8cefc8f6d8dee79584357fa2b941/packages/flutter_driver/lib/src/common/fuchsia_compat.dart#L85

This one appears to be correctly flagged. I'd be OK with throwing an ignore on this one as it appears to be intentional.

[pq]

This looks like a false positive to me that should not be flagged by the lint.

Quite right. It's interesting but the implementation is ignoring inherited members entirely (and that doesn't seem right).

This one appears to be correctly flagged. I'd be OK with throwing an ignore on this one as it appears to be intentional.

Cool. Should we take the same approach for the classes in test/?

[goderbauer]

Does the lint fire if you add a private constructor to a class with static-only members to make it non-instantiable?

class Foo {
  // This class is not meant to be instantiated or extended; this constructor
  // prevents instantiation and extension.
  Foo._();

  // static members
}

---

About the other failures:

dev/integration_tests/flutter_gallery/lib/demo/shrine/model/products_repository.dart:7:1

We should refactor this to just remove the class.

packages/flutter/test/widgets/widget_inspector_test.dart:232:1

This seems like a little bit of a hack, but I think this is also a false positive since the class inherits non-static members.

packages/flutter/lib/src/widgets/binding.dart:1234:1

False positive as discussed above.

packages/flutter_driver/lib/src/common/fuchsia_compat.dart:79:1
packages/flutter_test/lib/src/event_simulation.dart:26:1

These two should get a private empty constructor to mark it as non-instantiable. That will hopefully shut up the lint. If not, ignore seems fine here.

[pq]

Does the lint fire if you add a private constructor to a class with static-only members to make it non-instantiable?

Nope. This will not fire the lint.

I'm working on a fix for the false positives but would you be up for helping with the proposed refactor? No worries either way.

[goderbauer]

I'm working on a fix for the false positives but would you be up for helping with the proposed refactor? No worries either way.

I'll put together a PR for flutter to do that refactoring and to add the private constructors.

[pq]

Thanks for the help @goderbauer! With my fixes and your changes, I think we can close this out.

🎉