Skip to content
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

Open
wants to merge 9 commits into
base: trunk
Choose a base branch
from
Open

Remove dead code #5392

wants to merge 9 commits into from

Conversation

sellout
Copy link
Contributor

@sellout sellout commented Oct 7, 2024

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 with use 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 and Paths_* 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.

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.
@aryairani
Copy link
Contributor

This is exciting. I'll pick through it soon. Thanks for figuring out all the typeclass stuff in weeder.toml, that had been a barrier in the past.

@@ -29,55 +27,3 @@ instance Show CausalHash where

instance Show PatchHash where
show h = "PatchHash (" ++ show (unPatchHash h) ++ ")"

instance From ComponentHash Text where
Copy link
Contributor

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.

Copy link
Contributor

@ChrisPenner ChrisPenner left a 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.

Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants