Ban public top-level members in a test

[srawlins]

Proposal

Given a library with URI with pattern /test/.*_test.dart, report any public top-level members except main. "Top-level members in a test should be private (or relocated to within main), to prevent unused members."

Secondary proposal

Given a library which does not live in it's declaring package's public API (i.e. files in bin/, test/, and tool/), report any public top-level members except main.

Justification

This lint helps prevent code bloat. Without full-program analysis, we cannot tell if a public member is used anywhere, so it is not reported by the analyzer as unused_variable or something similar. For top-level members, only private ones can be rigorously checked for usage. But in tests (and bin/ and tool/ scripts), which are typically their own entry points and never imported by another library, we want to be able to report unused top-level members.

This includes top-level variables, constants, functions, typedefs, enums, mixins, and classes.

This is a counter-request to #685. I think this is better choice for a few reasons. #685 proposes either a change to the analyzer's unused analysis to take into account the library URI ("does it end with _test.dart?"), leading to possible false positives. I don't think that's going to go well. The alternative is to implement a new unused analysis in the linter. Unused analysis is expensive though, and adding a second one in linter (duplicating a ton of code) will make it more expensive.

Downsides

One downside to recommending private top-level members is that it might be counter to what users typically do today. I think a lot of people just write regular, public, top-level consts and helper functions. Personally, I always write private top-level members, so that I know when something becomes unused. Perhaps a study should be done on what most people do. #685 does not have this downside.

[kevmoo]

Like #1034, right?

[natebosch]

I think it would be mostly covered by #1034 but it's still worth tracking separately.

• Implementing this is likely a lot easier since it isn't blocked by the linter having an understanding of "package level" analysis.
• Even with the package level analysis, defining a public member in a *_test.dart file is something we'd discourage. You don't want to import a test file from another to use a shared utility. That kind of stuff should go in it's own library (though this does mean that the "secondary proposal" above is not feasible, since we explicitly do want to define utility libraries under test/ for shared testing code).

[kevmoo]

We should also be careful, though. Make sure the lib in question is importing package:test/test.dart etc etc

Most folks use pkg:test – but we shouldn't assume

[natebosch]

Most folks use pkg:test – but we shouldn't assume

The folks who don't shouldn't enable this lint...

[goderbauer]

As a data point: Recently did some clean-up in the framework with the help of this tool: https://pub.dev/packages/dart_code_metrics. We had accumulated quite a bit of dead code in tests: flutter/flutter#104550.

This proposal (or #685 or #2395) probably could have prevented that build-up of dead code.

[pq]

Thanks for the concrete example @goderbauer!

[a14n]

In #3453 I implemented such a lint but not specifically for tests. The lint checks every executable libraries (ie. a library containing a main top level function).

On Flutter codebase there are only few cases (among tens of thousands) that are triggered out of tests. Most of them can easily be refactored to prevent the lint (and make the code structured in a better way)

[a14n]

Any opinion about limiting the application of the lint to tests or to extend it to executable libraries (ie. a library containing a main top level function)?

The current PR (that is ready to land) applies on executable libraries but I can change it if we think it will be a too broadened application.

[a14n]

Any opinion about limiting the application of the lint to tests or to extend it to executable libraries (ie. a library containing a main top level function)?

Nobody?

[kevmoo]

That limitation sounds good to me

…

On Wed, Jun 29, 2022, 02:23 Alexandre Ardhuin ***@***.***> wrote: Any opinion about limiting the application of the lint to tests or to extend it to *executable libraries* (ie. a library containing a main top level function)? Nobody? — Reply to this email directly, view it on GitHub <#1258 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAEFCRMTFN43R4LBH5K3PLVRQIZFANCNFSM4GCL74VQ> . You are receiving this because you commented.Message ID: ***@***.***>

[a14n]

Extending the lint to executable libraries could be a way to cover/workaround Report unused public elements in an entrypoint library. User can always limits the application of the lint to /test by defining an analysis_option.yaml file inside the test directory.

My point is that this lint could have a lot of value also outside of the test directory. Should we have 2 lints (one for every executable library and one limited to test)?

[a14n]

Re-reading the issue description the lint request doesn't seem limited to tests:

Secondary proposal

Given a library which does not live in it's declaring package's public API (i.e. files in bin/, test/, and tool/), report any public top-level members except main.

I'm concern about provinding 2 lints no_top_public_members_in_executable_libraries and no_top_public_members_in_tests that do exactly the same thing but with a limited range for the latter. WDYT?

[bwilkerson]

I think we want to get some data about common usage patterns before we make a decision. How many top-level members (other than main) do people typically implement, both inside and outside of test?

I'm not familiar with test code outside the analyzer packages, but I do know of several libraries / packages that make fairly significant use of top-level members, so it's possible that this is a not-uncommon coding style.

[srawlins]

I think it's fairly common to have top-level helper functions in test files, and these would be the one's we'd want to ensure are used. I hypothesize that top-level constants like Strings and ints are cheap and not horrible if left unused, but top-level helpers can reference imported elements, so that it is more likely that an unused top-level function can represent actual bloat.

All of that being said, I'm much more in favor, these days, of just writing public top-level members, and improving our tooling to find unused elements like these. I used to write private top-level consts, helper classes, etc. in tests, but I think it does get pretty hard to read anything like that.

[a14n]

So should we close this issue in favor of #685 and #2395?

[srawlins]

I would be in favor of that. We don't have to close until one is implemented 🤣 because I think there is very good discussion.

[a14n]

The same question about the pertinence to have 2 lints doing the same thing (only on tests vs. on executable libs) remains. Any opinion on that?

[bwilkerson]

Again, I think the data would be useful. If it's common to define public top-level functions outside test, and if they're intended to be part of the public API (not just helpers that could have been private), then that would make it more important for the rules to be separate so that users could configure the behavior that they want. But if public top-level functions outside of test are rare, then it might not be worth the overhead of having two lints.

[srawlins]

I don't feel strongly about this issue any more. I'm happy with #3513 and think it is fairly non-idiomatic to declare private helpers and consts in test libraries. Anyone who feels strongly can re-open.