Invalidate package objects transitively in name hashing #2432
Conversation
The previous version of `pkg-self` covered scenario where a package object inherited directly from a class that is being modified and recompiled in subsequent steps. The sbt#2326 shows that name hashing fails to handle a package object that inherits indirectly from a modified class. In that case, name hashing fails to invalidate the source file that defines the package object. This commit expands `pkg-self` with a class B that inherits from modified class `A` and makes package object inherit from B. We mark that test as pending to show that this indeed trigger the bug in name hashing.
To aid debugging, modify Relations.toString to include `inheritance` in addition to already printed `memberRef` relation. Modify it for both name hashing and old implementation.
Modify `invalidatedPackageObjects` to look for package objects that transitively inherit from an invalidated class instead of just inheriting directly. This is the correct behavior that should have been implemented in the first place. The bug comes from a subtle copy&paste mistake. The old implementation used `publicInherited` relation and new one looks identical but uses `inheritance` relation. The difference is that `publicInherited` was a relation that was expanded transitively along inheritance chain but `inheritance` is not. We have to perform the transitive walk in `IncrementalNameHashing.invalidatedPackageObjects` implementation. Mark `pkg-self` test as passing. Fixes sbt#2326
|
Can one of the admins verify this patch? |
| invalidated flatMap relations.inheritance.internal.reverse filter { _.getName == "package.scala" } | ||
| transitiveDeps(invalidated)(relations.inheritance.internal.reverse).filter { | ||
| _.getName == "package.scala" | ||
| } |
There was a problem hiding this comment.
I realize it's not new to this commit, but what is the basis for this filter? It's not supposed to be a correctness requirement that package objects be defined in files called package.scala. It is (according to the scala specification) a convention.
There was a problem hiding this comment.
Yes, it was a quick hack. @Duhemm's idea would fix it for good.
|
LGTM. @paulp's comment is right. Maybe that we should add a flag |
|
@Duhemm: yes, that's a good way to implement package object handling. Would you like to give it a try? |
|
@gkossakowski Sure! |
Invalidate package objects transitively in name hashing
|
great, small suggestion for implementation: introduce |
|
Note if you haven't already that |
Modify
invalidatedPackageObjectsto look for package objects thattransitively inherit from an invalidated class instead of just inheriting
directly. This is the correct behavior that should have been implemented
in the first place. The bug comes from a subtle copy&paste mistake. The old
implementation used
publicInheritedrelation and new one looks identicalbut uses
inheritancerelation. The difference is thatpublicInheritedwas a relation that was expanded transitively along inheritance chain but
inheritanceis not. We have to perform the transitive walk inIncrementalNameHashing.invalidatedPackageObjectsimplementation.Mark
pkg-selftest as passing.Fixes #2326