Skip to content

Conversation

eed3si9n
Copy link
Member

@eed3si9n eed3si9n commented Oct 12, 2025

Fixes #23852
Accompanying blog post: incremental compilation of Scala 3

Problem

sbt-deps phase runs after the Typer, but prior to the Inliner. This means that the Zinc won't be able to track the symbolic dependency from the use site to the generated code, unless the code exists as a form of quotation.
This often results in under-compilation of the macro use sites.

Solution

  1. Move the sbt-deps phase after the Inliner, which expands the macros
  2. Traverse the Inlined tree, and treat it like a normal code

**Problem**
sbt-deps phase runs after the Typer, but prior to the Inliner.
This means that the Zinc won't be able to track the symbolic
dependency from the use site to the generated code,
unless the code exists as a form of quotation.
This often results in under-compilation of the macro use sites.

**Solution**

1. Move the sbt-deps phase after the Inliner, which expands the macros
2. Traverse the Inlined tree, and treat it like a normal code
@sjrd
Copy link
Member

sjrd commented Oct 13, 2025

The title of this PR is a bit misleading. It fixes one (important) issue with macro invalidation. It hardly fixes incremental compilation of macros as a whole. ;-)

@eed3si9n eed3si9n changed the title fix: Fix incremental compilation of macros fix: Fix incremental compilation of (some unquoted) macros Oct 13, 2025
@Gedochao Gedochao requested review from bishabosha and jchyb October 13, 2025 05:15
@bishabosha
Copy link
Member

As @sjrd would probably mention given a chance - even with this you can still have a dependency on a symbol during the macro run that never makes it to the inlined tree, e.g. if you generate code based on presence of a symbol - which would only be fixed by instrumenting each symbol operation during typer/posttyper/inlining etc to record deps

@smarter
Copy link
Member

smarter commented Oct 15, 2025

which would only be fixed by instrumenting each symbol operation during typer/posttyper/inlining etc to record deps

Note that we already have some infrastructure for doing instrumentation in any phase where it's appropriate rather than specifically sbt-deps, this should avoid the need for moving sbt-deps itself which is likely to break things as it fixes other things indeed: #18310

@eed3si9n eed3si9n force-pushed the wip/incremental-compilation branch from 1eb1817 to dd58f75 Compare October 16, 2025 05:56
Instead of moving the entire sbt-deps phase, this leaves the phase as-is,
and adds new instrumentation in the Inlining phase for the inlined code.
@eed3si9n eed3si9n force-pushed the wip/incremental-compilation branch from dd58f75 to cd9dddf Compare October 19, 2025 23:23
@eed3si9n
Copy link
Member Author

@smarter wrote:

Note that we already have some infrastructure for doing instrumentation in any phase where it's appropriate rather than specifically sbt-deps, this should avoid the need for moving sbt-deps itself which is likely to break things as it fixes other things indeed: #18310

Thanks for the hint! Following the advice, I have returned sbt-deps phase back to its original position, and added a new tree traversal in Inlining phase, reusing the sbt-deps code as AbstractExtractDependenciesCollector#recordTree in cd9dddf. A minor implementation note is that there can be only one signal back to Zinc about the extraction and that the phase has finished, so I had to add something like this in Inlining:

    rec.sendToZinc()
    ctx.run.nn.asyncTasty.foreach(_.signalDepsExtractionComplete())

If in the future someone wants to instrument the dependency in an even later phase, they would need to move that code too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants