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

Investigate why changing the body of a task or method causes downstream files to re-compile #3748

Closed
lihaoyi opened this issue Oct 15, 2024 · 3 comments · Fixed by sbt/zinc#1462 or #3773
Milestone

Comments

@lihaoyi
Copy link
Member

lihaoyi commented Oct 15, 2024

In the CodeSigSubfolderTests suite, we can see that modifying the body of task or helper method in the upstream subfolder/package.mill causes the downstream build.mill to recompile:

modifyFile(
workspacePath / "subfolder/package.mill",
_.replace("running subFolderTask", "running subFolderTask2")
)
// Changing stuff in subfolder/package.mill does not invalidate unrelated tasks in build.mill
val cached3 = eval("foo")
assert(cached3.out == "")
// TODO: why is this compiling both sources when we only changed
// one file and did not change any public type signatures?
assert(cached3.err.contains("compiling 2 Scala sources"))
modifyFile(
workspacePath / "subfolder/package.mill",
_.replace("running helperFoo", "running helperFoo2")
)
val mangledHelperFoo = eval("foo")
assert(mangledHelperFoo.out.linesIterator.toSeq == Seq("running foo2", "running helperFoo2"))
// TODO: why is this compiling both sources when we only changed
// one file and did not change any public type signatures?
assert(mangledHelperFoo.err.contains("compiling 2 Scala sources"))
// Make sure changing `val`s, which only affects the Module constructor and
// not the Task method itself, causes invalidation
modifyFile(
workspacePath / "subfolder/package.mill",
_.replace("val valueFoo = 0", "val valueFoo = 10")
)
val mangledValFoo = eval("foo")
assert(mangledValFoo.out.linesIterator.toSeq == Seq("running foo2", "running helperFoo2"))
// TODO: why is this compiling both sources when we only changed
// one file and did not change any public type signatures?
assert(mangledValFoo.err.contains("compiling 2 Scala sources"))

This is at odds with how incremental compilation is meant to work, and indeed when testing out simple two-file compilation modules standalone, changing the body of a method in an upstream file without changing the signature does not cause the downstream file to recompile

@lihaoyi lihaoyi changed the title Investigate why changing the body of a task or method causes everything to re-compile Investigate why changing the body of a task or method causes downstream files to re-compile Oct 15, 2024
@lihaoyi
Copy link
Member Author

lihaoyi commented Oct 16, 2024

Here's a minimal reproduction of the problem with Zinc 1.10.2 incremental compilation outside of Mill:

build.scala

package build

object outer {
  def foo = { build.subfolder.inner.helperFoo }
}

subfolder/package.scala

package build.subfolder
import build.{outer => unused}

class Unused

object inner  {
  def helperFoo = { 1 }
}

In Scala 2.13.15:

  • With the import build.{outer => unused}, we see changes to subfolder/package.scala causing build.scala to be recompiled, even though the signatures in package.scala did not change

  • If class Unused is removed, we see changes in build.scala causing subfolder/package.scala to be recompiled, even though it does not depend on build.scala

In Scala 3.5.0:

  • With the import build.{outer => unused}, we see changes to either file causing both to re-compile, even if signatures did not change

  • class Unused has no effect

@lihaoyi
Copy link
Member Author

lihaoyi commented Oct 16, 2024

Seems like a regression in Zinc 1.10.x, starting 1.10.0-M1. Reverting to 1.9.6 seems to fix the problem (?)

@lihaoyi
Copy link
Member Author

lihaoyi commented Oct 16, 2024

sbt/zinc#1461. Not sure if this is the only issue, but it seems to make the test introduced in #3750 behave appropriately

eed3si9n pushed a commit to sbt/zinc that referenced this issue Oct 17, 2024
Ref #1284
Fixes #1461
Fixes #1420
Also fixes com-lihaoyi/mill#3748 downstream

Not sure if there's a better way to fix this? Just opening this to spark discussion

The original bugs are fine, but the solution seems incorrect, and is both overly conservative (invalidating everything based on whitespace?) and not conservative enough (doesn't handle cycles with length > 2?).
@lefou lefou added this to the 0.12.0 milestone Oct 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants