proposal: `<remove_unused_mixin>`

[FMorschel]

remove_unused_mixin

Description

This lint suggests removing mixins or implementations with only private declarations in Dart code.

Details

This lint aims to identify mixins or implementations that consist solely of private members without any external usage. The suggestion is to remove such elements to maintain a cleaner and more maintainable codebase.

Kind

This lint falls under the category of enforcing style advice to improve code quality and maintainability.

Bad Examples

mixin M {
  void _fn(); // The declaration '_fn' isn't referenced.
}

class C with M { // New lint: remove_unused_mixin
  void _fn() {} // The declaration '_fn' isn't referenced.
}

Good Examples

mixin M {
  @override
  void _fn(); // The declaration '_fn' isn't referenced.
  void a() {}
}

class C with M {
  void _fn() {} // The declaration '_fn' isn't referenced.
}

Discussion

The remove_unused_mixin lint is designed to encourage developers to remove mixins or implementations that do not contribute to the public API of a class and only contain private members. This enhances code clarity and reduces unnecessary complexity.

More context at #54374

I'd like to discuss about: I'm not sure if there is any use case where implementations (I am more convinced extended types would probably not fall into this category but could also be discussed) could maybe be included in this lint (which would require renaming).

Discussion checklist

• List any existing rules this proposal modifies, complements, overlaps or conflicts with.
• List any relevant issues (reported here, the [SDK Tracker], or elsewhere).
• If there's any prior art (e.g., in other linters), please add references here.
• If this proposal corresponds to [Effective Dart] or [Flutter Style Guide] advice, please call it out. (If there isn't any corresponding advice, should there be?)
• If this proposal is motivated by real-world examples, please provide as many details as you can. Demonstrating potential impact is especially valuable.

[eernstg]

I'd like to mention one scenario which could be a systematic exception (causing false positives for this lint):

class Interface {
  void _fn();
  ... // Other members, public and/or private.
}

mixin M1 implements Interface {
  void _fn() => print('Implementation 1');
  ... // A set of private member implementations.
}

mixin M2 implements Interface {
  void _fn() => print('Implementation 2');
  ... // The same set of private members with a different implementation.
}

The idea is that client code (in other libraries) can use declarations like class MyClass extends Interface with Mj ... to select a particular implementation of the given set of private members, where Mj is M1 or M2. It may or may not be OK to have both, but that's a separate issue. The point is that the client doesn't get to override (or even call) those private members, but they do get to choose one of the available sets of implementations.

I wouldn't call that an "unused" mixin.

[FMorschel]

In that scenario, I think this could be solved by considering the other declarations. If all the other declarations are private, then neither M1 nor M2 could actually do anything to MyClass since in this case as you mentioned it is a declaration in another library and could never use the private declarations of Interface, M1 or M2.

I'm not sure if that would be hard on the analyzer to depend on the other declarations inside the same scope.

[eernstg]

neither M1 nor M2 could actually do anything to MyClass

Oh, but the difference between mixing in M1 and M2 is that MyClass can behave differently in the two cases because it has different implementations of several members. The author of MyClass cannot call those members directly because they are private, but the library that declares Interface and M1 and M2 can call them.

It's a kind of "remote control": The author of MyClass gets to choose one or the other bundle of implementations of those private members, and this can change the behavior of an instance of MyClass substantially.

I don't see why we shouldn't support this kind of configurability where a client gets to mix and match some pre-packaged sets of method implementations according to their needs, but they don't get to call or override those members directly.

Of course, we'd have a different kind of lint which says that a given private member is unused. I'm just saying that an all-private-members mixin can very well be significant outside the declaring library if those members are used in any way by the declaring library.

[FMorschel]

Sorry if I wasn't clear enough, I would only mark that as unused if the actual Interface only had private members as well. That's what I meant by saying that neither could actually do anything.

If either Interface or one of its mixin implementations had public declarations, I would not consider them as unused to avoid that kind of problem.

[eernstg]

But even that isn't enough, a client might certainly have a preference for one or the other implementation of a private member if it is at all reachable:

// Library 'lib.dart'.

abstract class Interface {
  void _draw();
}

mixin M1 implements Interface {
  void _draw() => print('Create a nice drawing of a flower in a vase!');
}

mixin M2 implements Interface {
  void _draw() => print('Pull the gun and threaten some innocent bystanders!');
}

void doDraw(Interface x) => x._draw();

// Library 'main.dart'.
import 'lib.dart';

class MyClass extends Interface with M1 {}

void main() {
  doDraw(MyClass());
}

[FMorschel]

I see. But that is a public member that calls the private member of Interface, could this be taken into account? If it is called by any other method/function it would already be "used", wouldn't it?

[eernstg]

I think it should work fine to rely on each member being reachable: Each private member declaration may be used in one way or another, but if it isn't used at all then this declaration could be flagged by an unreachable_private_member lint.

We could insist that this liveness analysis should find a public member in the dependency chain, but I don't think that's necessary because otherwise the entire dependency graph would consist of private members, and the first one would be unreachable. That one would be removed based on the lint, and then the ones that it called, and so on. So we'll get the "must be callable via some public declaration" for free. (OK, we can have cycles where private members call each other and are otherwise unreachable, but let's just say that we stop here and accept that there is some reasonable way to perform an analysis about reachability of private declarations. ;-)

So let's assume that we cleaned up the code such that no such unreachable private member exists. This implies that there is a way to invoke each private member in the library (well, that is undecidable, but we can't prove that it is impossible).

With that, I think every non-empty public mixin should be considered worthwhile (even in the case where it only contains private members).

However, we can't even conclude that a mixin is useless just because it does not declare any members. For instance, we might want to use a base mixin in order to enable the creation of Fake... or Mock... classes, but only via a declaration which is marked as @visibleForTesting:

final class Foo {
  Object m(int i) => 'Foo: $i';
}

final class FooA extends Foo {
  String m(num n) => 'FooA: ${super.m(n.toInt())}';
}

final class FooB extends Foo {
  Comparable<String> m(num n) => 'FooB: ${super.m(-n.toInt())}';
}

void f(Foo foo, int i) => print(foo.m(i));

@reopen
@visibleForTesting
base mixin FooTestInterface implements Foo {}

@reopen
@visibleForTesting
base mixin FooATestInterface implements FooA {}

@reopen
@visibleForTesting
base mixin FooBTestInterface implements FooB {}

The point is that Foo, FooA and FooB are usable from other libraries as types, but given that these classes are final, other libraries can't declare a subclass of any of them and they can't declare a class/enum/mixin that has implements Foo etc. In short, the library that declares Foo... also "owns" the subtype hierarchy whose root is Foo.

However, we want to create a very specific exception: It should be possible to create a Fake... version of the Foo... classes which are intended to be used in tests. We do this by declaring a base mixin for each Foo... class which is (only) @visibleForTesting. We can then create a FakeFoo as follows:

// Testing. OK to access declarations that are `@visibleForTesting`.

base class FakeFoo with FooTestInterface { // NB: `FakeFoo` is a subtype of `Foo`.
  Object m(int _) => 'FakeFoo: 42'; // Shallow implementation, just for testing.
}

void main() {
  f(FakeFoo(), 42);
}

This shows that even an empty mixin can be useful because it can provide a certain supertype relationship to declarations where it is mixed in.

It is crucial that it is a mixin, because there cannot be more than one immediate superclass of the fake class, and we might need to use the superclass "slot" for some other purpose. In contrast, the fake class can have as many mixins as it needs. On the other hand, if we want to use class FakeFoo ... implements Foo ... then Foo cannot be final.

OK, this example is controversial (check out the discussion in dart-lang/site-www#5143), but it does illustrate that it's really difficult to characterize a particular kind of mixin declaration as useless, and getting it right. ;-)

[FMorschel]

I'm taking some time to think about it all, just one simple thing that I'd like to post here which I'm fairly certain at this point.

This shows that even an empty mixin can be useful because it can provide a certain supertype relationship to declarations where it is mixed in.

In those cases, I'd agree with srawlins comment on 54374:

If there is a desire to keep around unused code for some other purpose (other than "use"), [...], then // ignore: is a great tool, and sends a strong signal, "I know this is unused, but I want to keep it."

Even more so if one is using something like dart_code_metrics's prefer-commenting-analyzer-ignores lint so you always remember to tell others why are you sure this should be ignored.

[FMorschel]

So let's assume that we cleaned up the code such that no such unreachable private member exists. This implies that there is a way to invoke each private member in the library (well, that is undecidable, but we can't prove that it is impossible).

Could you elaborate on that a bit? I'd like to understand your parenthesis better.