proposal: `consider_making_members_private`

[KyleFin]

consider_making_members_private

Description

Consider making public members private (or annotate them as visibleForTesting , internal, etc)

Details

When a class, mixin, enum, or extension is annotated as @visibleForTesting, you may want to make constructors and other public members private or annotate them as visibleForTesting , internal , etc. This can prevent accidentally exposing internals.

Kind

This may prevent serious errors that could occur when code authors misunderstand what protection they get by annotating a class as @visibleForTesting (or @internal etc) and publicly expose members that were intended to be private.

Bad Examples

// EXAMPLE 1
// Constructor and field m are publicly exposed with no lint warnings for improper access.
@visibleForTesting
class SomeWidget extends StatelessWidget {
  const SomeWidget();

  int m = 0;
}

// Intention of visibleForTesting may have been to allow tests to use `find.byType(SomeWidget)`.
// Otherwise the entire class may be private.
// Non-test code will have analyzer warnings for:
SomeWidget w;

// Non-test code will NOT have analyzer warnings for:
final w = SomeWidget();
parent.someWidget.m;

// EXAMPLE 2
// Extension members are publicly exposed with no lint warnings for improper access.
@visibleForTesting
extension MyExt on Widget {
  int get x => 0;
}

// Intention of visibleForTesting may have been to allow tests to use extension methods.
// Non-test code will have analyzer warnings for:
import my_lib show MyExt;

// Non-test code will NOT have analyzer warnings for:
someWidget.x;

Good Examples

// EXAMPLE 1
// Annotate constructor and make field private.
@visibleForTesting
class SomeWidget extends StatelessWidget {
  @visibleForTesting
  const SomeWidget();

  int _m = 0;
}

// EXAMPLE 2
// Annotate members (extension names are rarely referenced directly).
extension MyExt on Widget {
  @visibleForTesting
  int get x => 0;
}

Discussion

Effective Dart recommends to prefer-making-declarations-private and suggests that if something is public it is a signal that other libraries can and should access that member. It is also a commitment on your library’s part to support that and behave properly when it happens..

@visibleForTesting (and similarly internal and protected) is not enforced by the complier but is only an analyzer warning. This means code authors may add @visibleForTesting and assume the analyzer will warn for improper uses, but it may be far more risky than it appears (especially if your library is a dependency for projects beyond your control).

I realized this when I was creating a widget in a UI package to be imported by Flutter apps. This widget has many complex layers in its visual appearance so for testing I wanted to be able to use find.byType(SomeWidget) to validate the specific layers are present or hidden and have the expected state. To achieve this I made the classes public and marked them visibleForTesting (as in EXAMPLE 1). I later realized that this meant I was publicly exposing all class state within my package and to all apps that import my package as a dependency. At best, this may be a bit annoying as their IDE may suggest using constructors or members that were intended to be package-private. For many fields this may not matter. But at worst, callers could modify internal state, even for final fields. For example, one of my widget's inputs was a list, so even though the member variable was final it could be accessed, and because lists are mutable it could be modified.

In other languages like Java this may not be as much of a concern because there are compiler-enforced visibility levels between public and private like protected, but Dart doesn't have compiler-enforced protected visibility.

This also relates to https://github.com/dart-lang/linter/issues/3262

Discussion checklist

• [x] List any existing rules this proposal modifies, complements, overlaps or conflicts with.
• [x] List any relevant issues (reported here, the SDK Tracker, or elsewhere).
• [x] If there's any prior art (e.g., in other linters), please add references here.
• [x] 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?)
• [x] If this proposal is motivated by real-world examples, please provide as many details as you can. Demonstrating potential impact is especially valuable.

[srawlins]

This is a great proposal. We haven't formally written anything, but generally speaking, annotations like @visibleForTesting which can be applied to classes (mixins, etc) and instance members prompt a question: is the annotation inherited? if not, should it explicitly annotate overrides or subtypes?

This asks a similar question: if a class is @visibleForTesting (or, conceptually simpler, @experimental or @internal or @deprecated), then should the child elements (like instance members, constructors, etc) inherit the annotation? Should it be explicitly required. I think this proposal goes in a good direction: require child elements to explicitly match the parent's behavior (either be private so its not a concern, or have a matching annotation, for public members).

[bwilkerson]

if a class is @visibleForTesting ... then should the child elements ... inherit the annotation?

I'd much rather define the annotation to automatically imply behavior related to the members than to have to have a lint to enforce that members be annotated.

It might be too late for @visibleForTesting, but it might be worth checking to see how breaking this would be. I'd actually expect most people expected the members to also only be visible for testing.

[KyleFin]

I agree @bwilkerson. If it turns out that changing @visibleForTesting to apply to members would be "too breaking", what about adding a new annotation (something like visibleForTestingIncludingMembers?). Then we could have a lint to suggest using visibleForTestingIncludingMembers if class is annotated but members aren't? Seems better to update one annotation instead of having to annotate all members.

@srawlins those are interesting questions about how annotation should relate to subtypes.