search

unisonweb / unison

A friendly programming language from the future
https://unison-lang.org
Other
5.81k stars 271 forks source link

Remove dead code #5392

Open sellout opened 1 month ago

sellout commented 1 month ago

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.

aryairani commented 1 month ago

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.