sbt-scalafix: enable caching by default?

[bjaglin]

There has not been any user feedback about the caching feature introduced as opt-in in scalacenter/sbt-scalafix#102 and documented in #1139. It's most likely because few users have scalafix enabled as part of the compilation and really need that incremental compilation.

I am still opening this to track the known limitations and gauge the interest of potentially making caching an opt-out rather than an opt-in.

Known false positives (cache-hit while it should cache-miss):

1. Only source files are stamped, so a classpath change or scalacOptions update could alter the bytecode and/or the semanticdb file and/or what the presentation compiler exposes to complex rules like ExplicitResultTypes, but the rule will not be re-executed. See (3) in https://gitter.im/scalacenter/scalafix?at=5ea6b3b43d7e50071c34fd67.
2. sbt-scalafix: enable caching by default? #1164 (comment) caching: remove optimization bringing false positive cache hits sbt-scalafix#145

Known shortcomings (cache-miss while it could cache-hit):

1. successive runs with different rules invalidate the previous cache, so sequential runs like Use Simulacrum Scalafix typelevel/cats#3424 (comment) cannot make use of it
   • addressed in caching: don't forget previous invocations sbt-scalafix#139
2. usage of rules compiled on the fly disables caching
3. --tool-classpath disables caching
4. --stdout invocations are not cached

[olafurpg]

I agree we should enable caching by default. Caching is enabled by default in sbt-scalafmt and I suspect most users expect caching to be enabled by default.

I just enabled caching in one of my projects and it works great in the few cases I tried so far. You implemented nice testing infrastructure for caching so that we'll be able to address bug reports from users if they experience weird behavior.

[bjaglin]

A thought: caching could be enabled by default for any "permanent" Scalafix setup (scalac options & SemanticDB compiler plugin set up in the sbt build), but remain disabled by default for adhoc/scalafixEnable usages (where it's not really making a difference anyway).

That would reduce the risk that a user non-familiar with Scalafix hits the false positive mentioned in the description - assumption being that a user installing Scalafix permanently will have read the docs and seen mention of caching and/or will be more willing to report the use-case/caching bug instead of simply "giving up".

[bjaglin]

Also, considering that --check is used mostly on CI, maybe it does not make sense to share the cache for rewrite & check invocations. In other words, --check could imply --no-cache, reducing further the risk of false positives on CI.

[bjaglin]

As a first step, I am proposing to automatically enable caching when scalafix runs as part of compile, since that's where it's the most useful.

[olafurpg]

I think I just discovered a bug in scalafixCaching. Steps to reproduce:

• run scalafixAll, all files are fixed producing diffs
• run scalafixAll, cached so no files are fixed even if the input files have changed from the initial scalafixAll

Example, in the logs below I had edited a single file and then I ran scalafixAll twice in a row. The first scalafixAll produced a diff so I expected the second run to trigger Scalafix again.

-- obtained
++ expected
sbt:sbt-scalafix> scalafixAll
[info] Compiling 1 Scala source to /Users/lgeirsson/dev/sbt-scalafix/target/scala-2.12/sbt-1.0/classes ...
[info] Running scalafix on 1 Scala sources (incremental)
[success] Total time: 4 s, completed 10-Jul-2020 10:53:28
sbt:sbt-scalafix> scalafixAll
[info] Compiling 1 Scala source to /Users/lgeirsson/dev/sbt-scalafix/target/scala-2.12/sbt-1.0/classes ...
+ [info] Running scalafix on 1 Scala sources (incremental)
[success] Total time: 2 s, completed 10-Jul-2020 10:53:33

[bjaglin]

Thanks for the clear report! I think it also applies to the scalafix task itself. I believe the problem comes from the fact that input file stamping happens after execution of all rules, to avoid re-running scalafix again. But that is correct only if we have a single rule invoked, which is not checked... I'll remove the unsafe optimization and simply stamp before rule execution.

Edit: fixed in scalacenter/sbt-scalafix#145

[bjaglin]

As a first step, I am proposing to automatically enable caching when scalafix runs as part of compile, since that's where it's the most useful.

This was shipped as part of https://github.com/scalacenter/sbt-scalafix/releases/tag/v0.9.19, so I am closing this.

[bjaglin]

We (Teads) have been using caching for over a year now (before I upstreamed it), and we haven't had any problem with it nor bug reports, so I wonder if it would make sense to make it the default now? It's currently enabled only if scalafixOnCompile is turned on.