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.

Sign up for GitHub

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

Exclude anon classes from dependency registration #1507

Merged
merged 3 commits into from
Dec 6, 2024
Merged

Exclude anon classes from dependency registration #1507

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
af2004f
Exclude anon classes from dependency registration
retronym Dec 3, 2024
f1547d0
Add some notes to the saga of absent associatedFile
retronym Dec 3, 2024
c752dae
Update internal/compiler-bridge/src/main/scala/xsbt/Dependency.scala
retronym Dec 3, 2024
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
29 changes: 27 additions & 2 deletions internal/compiler-bridge/src/main/scala/xsbt/Dependency.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,27 @@ final class Dependency(val global: CallbackGlobal) extends LocateClassFile with
* but when it does we must ensure the incremental compiler tries its best no to lose
* any dependency. Therefore, we do a last-time effort to get the origin of the symbol
* by inspecting the classpath manually.
*
* UPDATE: This can also happen without compiler bugs if the symbol is simply uninitialized.
* Example, `class Client { def foo = Server.foo }`. When compiling client, the type `Foo` returned
* by `Server.foo` does not need to be initialized as we do not select from it or check its
* conformance to another type.
*
* Initializing `targetSymbol` before calling `assosicatedFile` would work but is problematic
* see zinc/zinc#949
*
* Perhaps consider this?
* val file = targetSymbol.associatedFile match {
* case NoAbstractFile => sym.rawInfo match {
* case cfl: global.loaders.ClassfileLoader =>
* val f = cfl.associatedFile(sym) // Gets the file from the loader
* if (f.exists) f else NoAbstractFile
* case f => f
* }
* }
*
* Or the status quo might just be perfectly fine -- if compilation doesn't need to force `Foo`,
* then there isn't a real dependency.
*/
val fqn = fullName(targetSymbol, '.', targetSymbol.moduleSuffix, false)
global.findAssociatedFile(fqn) match {
Copy link
Member

Choose a reason for hiding this comment

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

This is the last line I can leave comment, but a few lines down is

                  case Some((at, true)) =>
                    processExternalDependency(fqn, at)

which could explain the additional library (external) dependency. I think normally we check local, subproject, then library in order, but confusing the dependency level could potentially lead to a weird invalidation down the line.

Copy link
Member Author

Choose a reason for hiding this comment

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

confusing the dependency level could potentially lead to a weird invalidation down the line.

The customer report we're investigating was using direct-to-JAR compilation, which I can imagine cold trigger over-compilation if the spurious external is declared on the entire JAR.

Copy link
Member Author

@retronym retronym Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Indeed, if I:

  • set exportJars := true
  • add p1/src/main/demo/Other.scala
  • p2/Compile/compile

Then, Zinc overcompiles p2/src/main/demo/Client.scala:

[debug] IncrementalCompile.incrementalCompile
[debug] previous = Stamps for: 1 products, 1 sources, 2 libraries
[debug] current source = Set(${BASE}/p2/src/main/scala/Client.scala)
[debug] Invalidating '${BASE}/p1/target/scala-2.12/p1_2.12-0.1.0-SNAPSHOT.jar' because could not find class demo.Refined.select$$anon.where$$anon on the classpath.
[debug] > initialChanges = InitialChanges(Changes(added = Set(), removed = Set(), changed = Set(), unmodified = ...),Set(),Set(${BASE}/p1/target/scala-2.12/p1_2.12-0.1.0-SNAPSHOT.jar),API Changes: Set())
[debug]
[debug] Initial source changes:
[debug] 	removed: Set()
[debug] 	added: Set()
[debug] 	modified: Set()
[debug] Invalidated products: Set()
[debug] External API changes: API Changes: Set()
[debug] Modified binary dependencies: Set(${BASE}/p1/target/scala-2.12/p1_2.12-0.1.0-SNAPSHOT.jar)
[debug] Initial directly invalidated classes: Set()
[debug] Sources indirectly invalidated by:
[debug] 	product: Set()
[debug] 	binary dep: Set(${BASE}/p2/src/main/scala/Client.scala)
[debug] 	external source: Set()
[debug] all 1 sources are invalidated
[debug] Created transactional ClassFileManager with tempDir = /Users/jz/code/zinc-refinement/p2/target/scala-2.12/classes.bak
[debug] Initial set of included nodes:
[debug] Recompiling all sources: number of invalidated sources > 50.0 percent of all sources
[debug] About to delete class files:
[debug] 	Client.class
[debug] We backup class files:
[debug] 	Client.class
[debug] compilation cycle 1
[info] compiling 1 Scala source to /Users/jz/code/zinc-refinement/p2/target/scala-2.12/classes ...

Expand Down Expand Up @@ -291,9 +312,12 @@ final class Dependency(val global: CallbackGlobal) extends LocateClassFile with
assert(fromClass.isClass, Feedback.expectedClassSymbol(fromClass))
val depClass = enclOrModuleClass(dep)
val dependency = ClassDependency(fromClass, depClass)
// An anonymous class be the enclosing class of an existential type symbol inferred from refinements,
// prior to https://github.com/scala/scala/pull/10940. Allowing this here leads to a dependency on class name
// that does not exist. Guard against it here to avoid the issue with legacy compiler versions.
if (
!cache.contains(dependency) &&
!depClass.isRefinementClass
!depClass.isAnonOrRefinementClass
) {
process(dependency)
cache.add(dependency)
Expand Down Expand Up @@ -484,7 +508,8 @@ final class Dependency(val global: CallbackGlobal) extends LocateClassFile with

case m @ MacroExpansionOf(original) if inspectedOriginalTrees.add(original) =>
// TODO: typesTouchedDuringMacroExpansion can be provided by compiler
// in the form of tree attachment
// in the form
// of tree attachment
retronym marked this conversation as resolved.
Show resolved Hide resolved
val typesTouchedDuringMacroExpansion = original match {
case Apply(TypeApply(_, args), _) => args.map(_.tpe)
case TypeApply(_, args) => args.map(_.tpe)
Expand Down
Loading