-
Notifications
You must be signed in to change notification settings - Fork 272
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove dead code #5392
base: trunk
Are you sure you want to change the base?
Remove dead code #5392
Conversation
Replaced the old Dhall config with an equivalent TOML one, and added `weeder` to the default dev shell.
This is only temporary, so we can run Weeder incrementally.
Also ones with debugging functions.
I expect that this commit will need some revision once others look at it. I’ve added some modules that I think represent “independent APIs” to the list of root modules.
Explicitly list the classes that are fine even if unused, and add a comment about running without even those classes.
This adjusts code that is only referenced by tests. There are two cases: 1. code that is unused but tested gets deleted along with the tests that test it (or the tests are adjusted to call a deeper used function more directly) and 2. code that is used _by_ the tests, but not elsewhere gets moved to the test modules.
This is exciting. I'll pick through it soon. Thanks for figuring out all the typeclass stuff in |
@@ -29,55 +27,3 @@ instance Show CausalHash where | |||
|
|||
instance Show PatchHash where | |||
show h = "PatchHash (" ++ show (unPatchHash h) ++ ")" | |||
|
|||
instance From ComponentHash Text where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure I use a bunch of these in Share BTW.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's quite as easy as doing a local analysis because Share imports many unison packages as well, and that's in a separate repo.
Please ensure that the result compiles with share-api
before merging anything, I'm certain there are a lot of things in unison
that aren't used here but are exported for use in Share.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should either tear out this module and all its dependencies entirely, or just leave the whole thing alone; picking it apart is probably just going to make it much more difficult to restore if we decide to use it later (and I still suspect that some day we'll want to use this). I.e. if we remove the whole thing we can just find the last time the files existed and restore those, but if we gut them bit by bit it's going to be hard to tell where in history they were properly intact.
The entire module is actually unused, but weeder won't pick up on that because I think it's just gated behind some conditional that's currently always false.
Overview
This (re?)enables Weeder, which does whole-program dead-code analysis. Well over 7,000 lines of code are deleted by this PR.
There are at least three different areas of the code base where I’ve discovered myself fixing code that isn’t referenced anywhere. While I fixed those areas upon encountering them, having dead code around adds noise and introduces confusion like I’ve run into (especially with there being some more-or-less duplicated subsystems, with the old being incrementally replaced by the new)
There was a Weeder config in the repo, but it used the older Dhall format instead of the newer TOML, and has a minimal-enough config that I’m not sure whether it was ever used, and certainly not recently. Weeder is now also part of the environment provided by
nix develop
(or Direnv withuse flake
).Implementation notes
First, there is the weeder.toml file, which defines the set of “roots” that are considered live. By default this contains
Main.main
andPaths_*
modules. There are a number of additional roots added, with explanations in the file as to why they are needed.The bulk of the PR is deletion of unused code. Anything that has been deleted can be restored in this PR if we add an additional root to silence Weeder’s complaint about it (and a comment as to why it should live if it doesn’t fall under one of the existing comments in the config).
There is also a new section in development.markdown that describes various ways to use Weeder.
Interesting/controversial decisions
In the later commits, some code is moved instead of deleted. This is code that is technically live, but is only used by the tests. It has been moved to the relevant test modules. This is separated from most of the code deletion.
Test coverage
Since this is (mostly) just code removal, the existing tests (and the fact that it even builds) indicates that this hasn’t deleted anything necessary. Also
weeder
(in various incantations described by development.markdown) runs cleanly, ensuring that all of the dead code is gone.Loose ends
This doesn’t add a Weeder workflow to CI. Adding one is probably desirable, but it’d be good to do things like take advantage of the cached .stack-work and use the Nix environment for consistent versioning, so I didn’t want to spend extra time on it until it seemed like the current PR was an acceptable change.