build linting to warn on unused settings during reload #5153
Conversation
Fixes sbt#3183 This implements an input task lintBuild that checks for unused settings/tasks. Because most settings are on the intermediary to other settings/tasks, they are included into the linting by default. The notable exceptions are settings used exclusively by a command. To opt-out, you can either append it to `Global / excludeLintKeys` or set the rank to invisible. On the other hand, many tasks are on the leaf (called by human), so task keys are excluded from linting by default. However there are notable tasks that trip up users, so they are opted-in using `Global / includeLintKeys`.
|
|
||
| // input task version of the lintBuild | ||
| def lintBuildTask: Def.Initialize[InputTask[Unit]] = Def.inputTask { | ||
| val _ = Def.spaceDelimited().parsed // not used yet |
There was a problem hiding this comment.
What are the plans in the future for the input of the task?
There was a problem hiding this comment.
I was thinking options that could tune the filtering, for example to show:
- unused settings introduced by plugins
- unused tasks in
build.sbt - unused settings and tasks introduced by plugins
|
Wouldn't it be better to opt-in to such linting? Either by opting into linting on load or by just running a task (or command). |
|
Great work 👍 I like that such linting is enabled by default and it doesn't require an opt-in. IMO build users should not opt-in to get some checks that are most likely always on point: if their task/setting implementation isn't used anywhere, either they have dead code, sbt has a bug in its build linting logic or they messed it up. Hopefully the first and third possibilities are the most likely. |
|
If there's high confidence then instead of warning (and going ignored, for instance in CI) let it fail the build load. |
Address other review comments
|
To be honest, I am not quite sure what the signal/noise ratio would be until people start using them, so this PR keeps the option somewhat open. Initially there might be some adjustment period needed, if there's a popular plugin that uses settings from a command. (Exclusion doesn't introduce new type, so plugins should be able to exclude without adding dependencies on sbt 1.4.x). Once the dust settles during Mx or RC, and if it's mostly good suggestions, then we can keep it on as part of I think it would also be interesting to have a mode for plugin authors to self-check if their plugin wiring is kosher. (Currently only keys from local files are checked) |
**PR Checklist** - [x] A description of the changes is added to the description of this PR. - [x] If there is a related issue, make sure it is linked to this PR. **Description of changes** Resolves #91. > Whenever build/sbt test command is run, the warnings below are printed to the console: > [warn] there are 3 keys that are not used by any other settings/tasks: > [warn] > [warn] * cli / Compile / logLevel > [warn] +- /home/runner/work/unitycatalog/unitycatalog/build.sbt:206 This issue comes from the [key linting](sbt/sbt#5153) added in sbt 1.4.0, which warns keys are used by some tasks but not by others. In our case, `Compile / logLevel` is used in `cli`, `client`, `server` but not in `apiDocs`. <!-- Please state what you've changed and how it might affect the users. --> Signed-off-by: Tai Le Manh <manhtai.lmt@gmail.com>
sbt added the `sonaDeploymentName` key with sbt#8126, released with https://github.com/sbt/sbt/releases/tag/v1.11.0 - in use, this seems to be getting lint warnings (as introduced by sbt#5153): ``` [warn] there's a key that's not used by any other settings/tasks: [warn] [warn] * scala-collection-plus / sonaDeploymentName [warn] +- /home/runner/work/scala-collection-plus/scala-collection-plus/build.sbt:3 [warn] [warn] note: a setting might still be used by a command; to exclude a key from this `lintUnused` check [warn] either append it to `Global / excludeLintKeys` or call .withRank(KeyRanks.Invisible) on the key ``` https://github.com/rtyley/scala-collection-plus/actions/runs/15326291872/job/43121748535#step:6:19 ...sbt#5153 does say that "notable exceptions are settings used exclusively by a command" - I _think_ that maybe `sonaDeploymentName` is only used by the commands `sonaRelease` & `sonaUpload`, so it's necessary to add it to `excludeLintKeys`?
Fixes #3183
This implements an input task
lintBuildthat checks for unused settings/tasks. Because most settings are on the intermediary to other settings/tasks, they are included into the linting by default. The notable exceptions are settings used exclusively by a command. To opt-out, you can either append it toGlobal / excludeLintKeysor set the rank to invisible.On the other hand, many tasks are on the leaf (called by human), so task keys are excluded from linting by default. However there are notable tasks that trip up users, so they are opted-in using
Global / includeLintKeys.lintBuildis also implemented as a function that takes aState, and I'm calling that duringreloadnow.credits
The core of the idea is straight from the snippet proposed by @havocp in 2011 unused settings thread that identified 58 unused settings. This filters the candidates down to locally defines settings only (also an idea discussed in the thread).