Consider removing the `unreachable_from_main` lint

dart-lang / ecosystem

This repository is home to general Dart Ecosystem tools and packages.

BSD 3-Clause "New" or "Revised" License

47 stars 7 forks source link

Consider removing the `unreachable_from_main` lint #281

Closed devoncarew closed 3 months ago

devoncarew commented 4 months ago

We should consider removing the unreachable_from_main lint (currently in the lint set under 'correctness').

This lint essentially does something like global dead code analysis for an entry-point file - it tells you when public members from that file are not being referenced. I have seen this lint (correctly) catch instances of public members that are no longer referenced / were never referenced - unintended dead code.

However:

we don't have a similar lint for non-entry-point files
it's not clear that users intend for all members to be reachable / that we should lint if they're not (e.g., exposing all entries in a json map as getters on a class; they might not all be referenced, but you want them all exposed)
and anecdotally, when updating code, this lint has stood out to me as requiring more // ignores than others - having a high number of false positives (the members are not referenced, but thats OK from the POV of a particular file)

devoncarew commented 4 months ago

cc @natebosch @matanlurey @Piinks @goderbauer

matanlurey commented 4 months ago

If we ever end up having a list of "we don't do this automatically, but we're not banning it" list, I think it would go there.

LGTM to remove.

natebosch commented 4 months ago

I do think we'd be losing value by dropping this.

This does catch dead code today. We can plan to backfill this utility with other checking (that admittedly can be more accurate), but we may backslide until then.
Mixing a main with definitions that get imported suggests either cyclic imports or that the details of some other binary are being coupled with this one.

having a high number of false positives (the members are not referenced, but thats OK from the POV of a particular file)

Are there any patterns that we should be looking at to loosen this lint? Looking at these ignores in google3 I see mostly legacy stuff that we wouldn't write in the same way today. Looking at these ignores on GitHub I see quite a few that looks like legitimately dead code. What are the patterns you see with good use cases for this?

goderbauer commented 4 months ago

This lint has been pretty valuable and caught a bunch of dead code in the flutter repository (see https://github.com/flutter/flutter/pull/129854 and https://github.com/flutter/flutter/pull/126266). This is particularly useful for test files where people often declare helpers as public, which they then forget to remove when the test evolves. Overall, it has only 3 ignores in all of flutter/flutter - and they are for legit unreachable code that's only there to test certain things related to unreachable code: https://github.com/search?q=repo%3Aflutter%2Fflutter%20unreachable_from_main&type=code

I think this is a valuable lint and we should not remove it. I am curious to see what patterns you've seen that require legit ignores for this lin.

devoncarew commented 4 months ago

This CL enables unreachable_from_main in the sdk tools/ dir:

https://dart-review.googlesource.com/c/sdk/+/376740

It does find legit dead code (many of the scripts in this directory are very old and have probably been refactored a few times). It also triggers on a few places where we probably want fields exposed for completeness (json getters, ...).

devoncarew commented 3 months ago

Similar to the 80 chars lint, since we don't have consensus, and good arguments to keep this, I'm going to go with 'no change'; we'll keep this rule in the rule set.