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

fix false positive cache invalidation caused by unscoped lookup #192

Merged
merged 1 commit into from
Dec 23, 2021
Merged
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
20 changes: 7 additions & 13 deletions plugin/src/main/scala/org/scalafmt/sbt/ScalafmtPlugin.scala
Original file line number Diff line number Diff line change
@@ -138,6 +138,7 @@ object ScalafmtPlugin extends AutoPlugin {
private class FormatSession(
config: Path,
taskStreams: TaskStreams,
cacheStoreFactory: CacheStoreFactory,
kitbellew marked this conversation as resolved.
Show resolved Hide resolved
resolvers: Seq[Resolver],
currentProject: ResolvedProject,
filterMode: String,
@@ -239,10 +240,7 @@ object ScalafmtPlugin extends AutoPlugin {
res
}

def formatTrackedSources(
cacheStoreFactory: CacheStoreFactory,
sources: Seq[File]
): Unit = {
def formatTrackedSources(sources: Seq[File]): Unit = {
val filteredSources = filterFiles(sources)
trackSourcesAndConfig(cacheStoreFactory, filteredSources) {
(outDiff, configChanged, prev) =>
@@ -272,10 +270,7 @@ object ScalafmtPlugin extends AutoPlugin {
if (cnt > 0) log.info(s"Reformatted $cnt Scala sources")
}

def checkTrackedSources(
cacheStoreFactory: CacheStoreFactory,
sources: Seq[File]
): ScalafmtAnalysis = {
def checkTrackedSources(sources: Seq[File]): ScalafmtAnalysis = {
val filteredSources = filterFiles(sources)
trackSourcesAndConfig(cacheStoreFactory, filteredSources) {
(outDiff, configChanged, prev) =>
@@ -393,15 +388,12 @@ object ScalafmtPlugin extends AutoPlugin {

private def scalafmtTask(sources: Seq[File], session: FormatSession) =
Def.task {
session.formatTrackedSources(streams.value.cacheStoreFactory, sources)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

session.formatTrackedSources(sources)
} tag (ScalafmtTagPack: _*)

private def scalafmtCheckTask(sources: Seq[File], session: FormatSession) =
Def.task {
val analysis = session.checkTrackedSources(
(scalafmt / streams).value.cacheStoreFactory,
sources
)
val analysis = session.checkTrackedSources(sources)
throwOnFailure(analysis)
} tag (ScalafmtTagPack: _*)

@@ -451,6 +443,7 @@ object ScalafmtPlugin extends AutoPlugin {
val session = new FormatSession(
config,
streams.value,
(scalafmt / streams).value.cacheStoreFactory,
fullResolvers.value,
thisProject.value,
scalafmtFilter.value,
@@ -496,6 +489,7 @@ object ScalafmtPlugin extends AutoPlugin {
new FormatSession(
scalaConfig.value,
streams.value,
(scalafmt / streams).value.cacheStoreFactory,
fullResolvers.value,
thisProject.value,
"",
4 changes: 4 additions & 0 deletions plugin/src/sbt-test/scalafmt-sbt/sbt/test
Original file line number Diff line number Diff line change
@@ -142,6 +142,10 @@ $ delete p17/src/main/scala/Test2.scala
$ copy-file changes/good.scala p17/src/main/scala/Test2.scala
> p17/scalafmtCheck
> p17/scalafmt
######## formatting other config should not invalidate the cache
$ copy-file changes/bad.scala p17/src/test/scala/Test3.scala
> p17/test:scalafmt
> p17/scalafmtCheck

# set up git
$ exec git init -b main p18