Linker not noticing instance test changes #5131

VelizarHristov · 2025-02-18T16:35:20Z

In some cases, the linker fails to update a file and this can result in Javascript errors which always go away after clean and re-linking. Here's one example where this might happen:

Make sure to use small modules in the build:

scalaJSLinkerConfig ~= {
    _.withModuleKind(ModuleKind.ESModule)
      .withModuleSplitStyle(
        ModuleSplitStyle.SmallModulesFor(List("hello_world")))
  }

Use whichever version of Scala and SBT (I've reproduced it on many versions), and the latest ScalaJS.

Declare the following in a file:

package hello_world
trait A
case class B() extends A
case class C() extends A

And this is your main method in a different file:

package hello_world
@main
def main(): Unit =
    val ls = Seq(new B(), new C())
    println(ls.collect { case a: B => a })

Now link it by running fastLinkJS from sbt - so far everything will work;
Then change B to C in the last line, the code should look like this: println(ls.collect { case a: C => a })
Then run fastLinkJS again.
At this point the resulting JS is broken. It no longer prints a console log message, and it instead shows a JS error.
What's broken specifically is in the generated C.js code - it is missing the function starting with $as_, which is expected from the file which I will abbreviate as $$anon$1.js.

(The bug didn't reproduce when I declared the trait and classes in the same file as the main method; however, it reproduces when all of A, B, C are classes (B, C must extend A), even with parameters)

The problem can be narrowed down to the code here: https://github.com/scala-js/scala-js/blob/main/linker/shared/src/main/scala/org/scalajs/linker/backend/emitter/Emitter.scala#L696-L701
This code is not updating changed in the case where linkedClass.hasInstanceTests changes, while nothing in the class itself changes. Note that we're not modifying the implementation of the case class C, we're only modifying its caller, so the linker incorrectly assumes that the class has not changed and doesn't check anything referencing it. I couldn't find a specific part where the code was wrong, it's more that this check needs to be added to prevent this bug, and I don't know what is the best way to do this.

A seemingly much less important case of the same bug: If I'm doing an instance test, then I remove it without making changes to the file declaring the referenced instance, then a different js file will be constructed by the emitter, but changed will be false so it won't replace the old file. This doesn't break anything, it only leaves in unused code. To reproduce follow the same steps as above, but check the generated JS code for B, and you'll see that it has instance check functions that are unused and that will go away if cleaned and re-linked.

Also I am not sure whether classEmitter.needInstanceTests can toggle in other situations with changed remaining false - I don't know what .isAncestorOfHijackedClass will be because I don't know what a hijacked class is, and I did not dig into it, it just seemed suspicious that this might possibly happen. Food for thought for someone more familiar with the codebase than I am.

The text was updated successfully, but these errors were encountered:

… flag. That flag is used to decide if a module needs to be rewritten. Previously, we failed to notice cases where *only* one of the `Analysis` flags changed (`hasInstances`, `hasRuntimeTypeInfo` and `hasInstanceTests`). In those cases, all the *cached trees* are kept. The only thing that changes is the subset thereof that is actullay included. That does not require invalidation of the caches, but it does require rewriting the module.

…Class. Previously, we were missing a number of cases where we should have set the `changed` flag to true. That flag is used to decide if a module needs to be rewritten. Previously, we failed to notice cases where *only* one of the `Analysis` flags changed (`hasInstances`, `hasRuntimeTypeInfo` and `hasInstanceTests`). In those cases, all the *cached trees* are kept. The only thing that changes is the subset thereof that is actullay included. That does not require invalidation of the caches, but it does require rewriting the module. Overall, it had become quite tricky to follow what we are allowed to use where. This commit overhauls how we track everything. First, we include the `kind` of the class in its cache identity, because too many things depend on it. If it changes, it is likely that everything will be invalidated anyway. Other than that, dangerous variables, whose uses must be justified, are clearly marked with `foo_!` names. Boolean decisions whose outcome is never cached are tracked separately; if any of them changes, we marked `changed` to `true`. This particular change is what fixes scala-js#5131.

…Class. Previously, we were missing a number of cases where we should have set the `changed` flag to true. That flag is used to decide if a module needs to be rewritten. Previously, we failed to notice cases where *only* one of the `Analysis` flags changed (`hasInstances`, `hasRuntimeTypeInfo` and `hasInstanceTests`). In those cases, all the *cached trees* are kept. The only thing that changes is the subset thereof that is actullay included. That does not require invalidation of the caches, but it does require rewriting the module. Likewise, we failed to invalidate the module if only the *set* of static-like methods retained by the analysis changed. Overall, it had become quite tricky to follow what we are allowed to use where. This commit overhauls how we track everything. First, we include the `kind` of the class in its cache identity, because too many things depend on it. If it changes, it is likely that everything will be invalidated anyway. Other than that, dangerous variables, whose uses must be justified, are clearly marked with `foo_!` names. Boolean decisions whose outcome is never cached are tracked separately; if any of them changes, we marked `changed` to `true`. This particular change is what fixes scala-js#5131. Finally, we add dedicated tracking of the set of generated static-like methods, like we do for instance methods.

…h-modules Fix #5131: Overhaul justifications for caching in Emitter.genClass.

sjrd self-assigned this Feb 18, 2025

sjrd added the bug Confirmed bug. Needs to be fixed. label Feb 18, 2025

sjrd added this to the v1.19.0 milestone Feb 18, 2025

sjrd linked a pull request Mar 11, 2025 that will close this issue

Fix #5131: Overhaul justifications for caching in Emitter.genClass. #5140

Merged

sjrd mentioned this issue Apr 6, 2025

Fix #5131: Overhaul justifications for caching in Emitter.genClass. #5140

Merged

sjrd closed this as completed in #5140 Apr 6, 2025

sjrd added a commit that referenced this issue Apr 6, 2025

Merge pull request #5140 from sjrd/fix-incremental-instance-tests-wit…

7d15aad

…h-modules Fix #5131: Overhaul justifications for caching in Emitter.genClass.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Linker not noticing instance test changes #5131

Linker not noticing instance test changes #5131

Linker not noticing instance test changes #5131

Linker not noticing instance test changes #5131

Comments