fix false positive cache invalidation caused by unscoped lookup

[bjaglin]

Fix regression introduced in bb36031, effectively disabling the cache for projects with files in several configuration. This is particularly annoying with scalafmtOnCompile := true.

The problem is visible by looking at a stamp file on a project with 4 configurations after running sbt clean scalafixAll

# 2.4.3
bjaglin@x1:~/project/target/streams$ find -name output-diff
./it/scalafmt/_global/streams/output-diff
./uit/scalafmt/_global/streams/output-diff
./compile/scalafmt/_global/streams/output-diff
./test/scalafmt/_global/streams/output-diff

# 2.4.4
bjaglin@x1:~/project/target/streams$ find -name output-diff
./_global/_global/_global/streams/output-diff

# 2.4.5
bjaglin@x1:~/project/target/streams$ find -name output-diff
./_global/_global/_global/streams/output-diff

# 2.4.5+9-d5456570-SNAPSHOT
bjaglin@x1:~/project/target/streams$ find -name output-diff
./it/scalafmt/_global/streams/output-diff
./uit/scalafmt/_global/streams/output-diff
./compile/scalafmt/_global/streams/output-diff
./test/scalafmt/_global/streams/output-diff

[kitbellew]

would changing this to (scalafmt / streams).value.cacheStoreFactory be an equivalent single-line fix? if not, can you explain how this change actually works?

[kitbellew]

i tested this, doesn't work. mystery...

[bjaglin]

It wouldn't since we would still be missing the configuration axis (not propagated from the taskDyn, see #192 (comment)) when resolving the .value lookup.

However, an equivalent fix would be to pass the config as an argument of the method, and use (config / scalafmt / streams).value.cacheStoreFactory, but I felt like capturing the cache directory in the session was simpler.

[kitbellew]

Fix regression introduced in bb36031, effectively disabling the cache for projects with files in several configuration. This is particularly annoying with scalafmtOnCompile := true.

i am not an expert (in fact, could barely navigate through sbt dynamic tasks to get them to compile)... which part in the linked change had this effect (and how)?

[bjaglin]

@kitbellew here is an attempt at clarifying what's going on (based on my own understanding obviously). This assumes the reader is familiar with the task graph & scopes.

# build.sbt

lazy val taskLookup = taskKey[File]("")
lazy val taskDynLookup = taskKey[File]("")

inConfig(Compile)(
  Seq(
    taskLookup := Def.task(streams.value.cacheDirectory).value,
    taskDynLookup := Def.taskDyn(Def.task(streams.value.cacheDirectory)).value,
  )
)

-$ sbt "show taskLookup" | tail -n2  | head -n1
+$ sbt "show taskDynLookup" | tail -n2  | head -n1
-[info] /tmp/sbt/target/streams/compile/taskLookup/_global/streams
+[info] /tmp/sbt/target/streams/_global/_global/_global/streams

We can see that the cache directory is not resolved in the same scope depending on where the lookup happens.

• Within a task, the lookup is implicitly scoped to the enclosing config/task axes.
• Within a task referenced in a taskDyn, the enclosing scope is lost (not propagated), so we fallback to the global cache directory no matter on which config/task it's evaluated.
  • This looks more like a limitation in the underlying macro (a bit demystified in @eed3si9n's https://eed3si9n.com/sudori-part2/) than a feature to me. It has been like that in sbt for as far as I can git-blame.
  • This is why bb36031 causes cache invalidation since all configs share the same directory for their unrelated cache.

[kitbellew]

@kitbellew here is an attempt at clarifying what's going on (based on my own understanding obviously). This assumes the reader is familiar with the task graph & scopes.

thank you for the explanation, Brice. while bug in the macro is a more palatable explanation for the regression, it is true that i am more of a bull in a china shop when it comes to sbt/scopes.