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

document scalafixOnCompileConfig key & scalafixOnCompile shortcomings #1211

Closed
wants to merge 2 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 36 additions & 1 deletion docs/users/installation.md
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,8 @@ Great! You are all set to use Scalafix with sbt :)
| `scalafixCaching` | `SettingKey[Boolean]` | Controls whether 2 successive invocations of the `scalafix` `InputTask` with the same arguments & configuration should be incremental. Defaults to `true` if `scalafixOnCompile := true`, `false` otherwise. When enabled, use the `--no-cache` argument to force an exhaustive run.
| `scalafixConfig` | `SettingKey[Option[File]]` | `.scalafix.conf` file to specify which scalafix rules should run, together with their potential options. Defaults to `.scalafix.conf` in the root directory, if it exists.
| `scalafixDependencies` | `SettingKey[Seq[ModuleID]]` | Dependencies making [custom rules](#run-custom-rules) available via their simple name. Must be set in `ThisBuild`. Defaults to `Nil`.
| `scalafixOnCompile` | `SettingKey[Boolean]` | When `true`, Scalafix rule(s) declared in `scalafixConfig` are run on compilation, applying rewrites and failing on lint errors. Defaults to `false`.
| `scalafixOnCompile` | `SettingKey[Boolean]` | When `true`, Scalafix rule(s) declared in `scalafixConfig` (or `scalafixOnCompileConfig` when set) are run on compilation, applying rewrites and failing on lint errors. Defaults to `false`.
| `scalafixOnCompileConfig` | `SettingKey[File]` | When this is set and `scalafixOnCompile := true`, the rules and settings from this .scalafix.conf file will be used when Scalafix runs automatically after compilation.
| `scalafixResolvers` | `SettingKey[Seq[Repository]]` | Custom resolvers where `scalafixDependencies` are resolved from. Must be set in `ThisBuild`. Defaults to: Ivy2 local, Maven Central, Sonatype releases & Sonatype snapshots.
| `scalafixScalaBinaryVersion` | `SettingKey[String]` | Scala binary version used for Scalafix execution. Defaults to 2.12. For advanced rules such as ExplicitResultTypes to work, it must match the binary version defined in the build for compiling sources. Note that `scalafixDependencies` artifacts must be published against that Scala version.

Expand Down Expand Up @@ -190,6 +191,40 @@ Both the `scalafix` & the `scalafixAll` task aggregate like the `compile`
and `test` tasks. To run Scalafix on all projects for both main and test
sources you can execute `scalafixAll`.

### Run Scalafix automatically on compile

If you set `scalafixOnCompile` to `true`, the rules declared in `.scalafix.conf`
(or in the file located by `scalafixConfig`) will run automatically each time
`compile` is invoked either explicitly or implicitly (for example when
executing `test` or `publishLocal`). Lint errors will fail that invocation,
while rewrites will be applied.

Although this looks like an easy way to use Scalafix as a linter, use this
feature with care as it has several shortcomings, for example:

1. Some rules such as `RemoveUnused` can be counter-productive if applied too
often/early, as the work-in-progress code that was just added might disappear
after a simple `test`.
1. If you run many semantic rules by default, the last one(s) to run might see
stale information and fail the invocation, which needs to be re-run manually.
This is [not specific to `scalafixOnCompile`](https://github.com/scalacenter/scalafix/issues/1204),
but the problem becomes much more visible with it.
1. To keep the overhad minimal, `scalafixCaching` is automatically enabled when
`scalafixOnCompile` is, which can cause unexpected behaviors if you run into
false positive cache hits. `scalafixCaching` can explicitly be set to
`false` in that case.
1. Non-idempotent rewrite rules might get you in an infinite loop where sources
never converge - not specific to `scalafixOnCompile` either, but rather
confusing when triggered automatically.
1. Bugs in rule implementations can prevent you from getting a successul
`compile`, blocking testing or publishing for example

Some of these shortcomings can be mitigated by the `scalafixOnCompileConfig`
setting key, which allows to use a separate configuration file whenever
Scalafix is triggered by `compile`. This way, Scalafix can execute a
different, safer set of rules compared to the regular `scalafixConfig` that
remains used for explicit `scalafix` & `scalafixAll` invocations.
Comment on lines +202 to +226
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@taisukeoe I have (shamelessly) included your own concern in this list. Feedback very much welcomed!

Copy link
Contributor

@taisukeoe taisukeoe Jul 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bjaglin Great capture 👍 Thank you very much!

In the future, it would be nicer to describe what kind of rules are recommended for on-compile, although I haven't found the adequate words yet.

The safest way is to recommend only diagnostic rules such as DisableSyntax, while there are few rules only for diagnostics. I feel it is a bit too conservative and might miss some useful combinations.

So at this moment, I'd agree with your explanation.


### Enforce in CI

To automatically enforce that Scalafix has been run on all sources, use
Expand Down