Unused import results in unnecessary compilation from 1.10.0-M1 onwards (regression from 1.9.6)

[lihaoyi]

project/build.properties

sbt.version=1.10.0-M1

build.sbt

scalaVersion := "2.13.15"

src/main/scala/build.scala

package build

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

src/main/scala/subfolder/package.scala

package build.subfolder

import build.{outer => unused}

object inner  {
  def helperFoo = { 1 }
}

• In SBT/Zinc 1.10.0-M1 onwards, adding a newline to the end of either file results in 2 files being recompiled
• In SBT/Zinc 1.9.6 and before, adding a newline to the end of either file results in only 1 file being recompiled
• Removing the unused import build.{outer => unused} results in SBT/Zinc 1.10.0-M1 behaving correctly, only re-compiling 1 file when a newline is added to the end of either of them

It seems to me that Zinc 1.9.6 is correct and Zinc 1.10.0-M1 is wrong. In none of the cases above are any signatures or implementation code changing - I am just adding newlines to the end of the file, so even the line numbers didn't change - and thus we should not need to re-compile any downstream code. But even if we did change implementations (e.g. changing = { 1 } to = { 2 }) we should not need to recompile downstream code unless signatures change

Noticed in Mill com-lihaoyi/mill#3748

Bisected the Zinc commit log and the misbehavior seems to have been introduced in 8f40c41. That code seems incorrect to me:

• We are blindly including mutually-dependent files meaning two files a <-> b that depend on one another will be forcibly recompiled even if nothing at all changed (the test case above)

• We are only including directly mutually-dependent files. That means that although a 2-file cycle a -> b -> a is considered, a three file cycle a -> b -> c -> a is not considered. I have verified this behavior experimentally, and can't imagine any scenarios where this distinction would be something we actually want

[lihaoyi]

CC @Friendseeker

[Friendseeker]

c.c. #598 (comment)

I didn't understand the exact problem either as it involved some compiler internal. I merely implemented proposed fix from @smarter to fix #598.

I think the first thing we should do is to figure out what exactly is the cause for #598, and then discuss if it is indeed a compiler issue that should be fixed at compiler side instead of Zinc side.

[Friendseeker]

@smarter Would you like to join the discussion and share your thoughts on the reported issue? Since you were the one proposing the solution and you have lots of experience working with compiler internal.

[Friendseeker]

From com-lihaoyi/mill#3751 (comment):

With this PR, whitespace changes to build.mill recompile 1 build file in about 5 seconds, whereas without this PR they would recompile 20 build file taking 10 seconds. Presumably the difference would be larger on larger projects

I must say that I did not expect mutual invalidation to have this much performance impact... This basically undos the performance improvement from Stefan's consistent analysis format.

@eed3si9n Should we revert for now?

[lihaoyi]

@Friendseeker It's not time-critical to revert the change for me, since I can just downgrade for the time being if necessary (com-lihaoyi/mill#3751). So unless it is time-critical for other people we can take our time to discuss what to do.

I suspect that "invalidate entire cycle on change" may be necessary in some cases, so the original fix is not strictly wrong. Maybe it's even not aggressive enough since it doesn't seem to handle larger cycles with n>2.

An alternative to reverting it is to just make it configurable opt-out with a flag, then people can choose their tradeoff. Whether someone prefers more occasional crashes or slower compilation for cycles would depend on the codebase and usage patterns: e.g. Mill build code currently have lots of mutual cycles and so is affected, but Mill application code does not and so is not affected by this at all. So rather than making the decision for them by reverting this we can add a config flag to let people toggle it if necessary

[Friendseeker]

I suspect that "invalidate entire cycle on change" may be necessary in some cases, so the original fix is not strictly wrong. Maybe it's even not aggressive enough since it doesn't seem to handle larger cycles with n>2.

Some case by case investigation is definitely needed. Currently there's 3 minimal crash reproductions documented, each with different crash stack trace (hence each likely have distinct causes)

• Zinc doesn't like type aliases + type projections #535
• Workaround for 'symbol is missing from the classpath' error #654
• Changing abstract class to trait causes "invalid superClass" #598 (comment)

With #535 being the most well documented. #535 has a fix planned but never released (which I guess is to let compiler traverse original type stored in tree attachment before deciding to throw).

An alternative to reverting it is to just make it configurable opt-out with a flag, then people can choose their tradeoff.

My concern with opt-out flag is that Zinc is supposed to just work out of the box. While an opt-out flag can benefit informed Zinc users, most Scala programmers would never know about the existence of such flag.

[eed3si9n]

Since sbt or Zinc hasn't had strong opinions around source-level circular dependencies, and it seems to be causing a bunch of overcompilation that are sometimes hard to identify, it does feel like invalidating the knots might not be the right default. A good thing about crash is that it would at least prevent shipping the wrong bytecode, and the users can run clean to start over, as opposed to silently succeeding to compile but failing at runtime?

[lihaoyi]

Could we detect crashes and start over by invalidating the entire cycle automatically? That would still take time, but it will turn a bunch of manual work and confusion into an automated process that shouldnt take too much longer than pre-emptively invalidating the entire cycle anyway. And in the happy path without crashes, it would be fast

[lihaoyi]

In fact I'm guessing crashes are rare enough that invalidating the entire compilation run (not just the relevant cycle) would work too. People wouldn't mind if the compiler sometimes takes a bit longer, as long as they don't have to manually investigate and intervene to resolve an issue

[lihaoyi]

"Compiler crashed, please clean and recompile" is a common enough manual workflow outside of this specific issue that maybe we should just codify it

[Friendseeker]

"Compiler crashed, please clean and recompile" is a common enough manual workflow outside of this specific issue that maybe we should just codify it

The issue is that compiler throws exception whenever there's any compile error. For example in #535, the error emitted by compiler is

[error] Bar.scala:1:26: value b is not a member of A
[error] object Bar { Foo.provide.b }

(c.c. #476)

Which is indistinguishable from common compile error.