Invalidate macro source when its dependency changes

[dwijnand]

Invalidate macro classes that transitively depend on any of the recompiled classes

The macro expansion tree can depend on the behavioural change of any upstream code change,
not just API changes, so correctness requires aggressive recompilation of downstream classes.

Technically the macro doesn't need to be recompiled - it's the classes downstream of the macro
that need to be recompiled, so that the macros can be re-expanded. But recompiling is the most
straightforward way to signal any classes downstream of the macro that they need to recompile.

Also, note, that this solution only works for behavioural changes in sources within the same
subproject as the macro. Changes in behaviour in upstream subprojects don't cause downstream
macro classes to recompile - because downstream projects only have visibility of the upstream
API, and if it changed, which is insufficient, and upstream projects have no other way than
their API to signal to downstream.

[SethTisue]

@Friendseeker you've been doing invalidation stuff recently (which is noticed and much appreciated!) — perhaps you'd like to do a little reviewing on this one?

[Friendseeker]

@Friendseeker you've been doing invalidation stuff recently (which is noticed and much appreciated!) — perhaps you'd like to do a little reviewing on this one?

Thank you for considering me for this review; I really appreciate it! I must admit, my experience with Scala macros is quite limited. However, I'm willing to take a closer look at the PR and provide my comments where I can.

[Friendseeker]

@dwijnand I've reviewed the changes in your PR and understand the modifications you've made. However, to get a complete picture, could you please provide some insight into the reasoning behind this approach? It appears to be a quick fix, and I'm curious if there were specific constraints or considerations that led to this solution.

[dwijnand]

I went with the first fix that worked, not being able to tell where I should look to slot this fix in. In particular I ended up moving my change to "initial changes" from detectAPIChanges and invalidateAfterInternalCompilation where I was originally looking. I'm open to suggestions and ideas. Meanwhile I'll study your transitive counter-example.

[Friendseeker]

LGTM in terms of transitive dependency correctness. There's probably still some edge cases left but this did cover the edge cases I can think of.

Preferably someone can do a performance review. If the performance is acceptable then I personally think this is good to go. If not, we might need to think of a way to optimize computation of thirdClassInvalidation (e.g. 2-way BFS, some tweaked version of union find...), or fall back to initial approach of checking at invalidateInitial only.

[dwijnand]

I don't think computing thirdClassInvalidation would impact performance, because invalidateClassesInternally already calls transitiveDeps - although there it operates on the classes in changes, which is a subset of recompiledClasses we're operating on here (because we care about classes that changed in behaviour, not just those that changed in API). Let me know if I'm missing something.

[Friendseeker]

Thanks for the explanation! Overall LGTM.

[eed3si9n]

Thanks for the contribution (and thanks to reviewers as well)! I'm happy to merge this as-is, but my only ask would be to elaborate the motivation for this PR in the PR description as "problem such and such doesn't work. \n\n solution here's the approach to fix it." section.

[Friendseeker]

I think I found an edge case.

If transitive dependencies of Macro lives in different projects, then transitiveDeps won't find cross-project transitive dependencies, as each project has their own Analysis (while the transitiveDeps call only uses information from the current project's analysis).

To address this issue, when performing dependency traversal, we need to traverse through both internal dependency (dependency within a project) and external dependency (project to project dependency), fetching new analyses during traversal when necessary. We can probably implement this logic via creating a new version of dependsOnClasses, or do it some other way if there's a more reasonable way to do it.

---

We can either address the edge case in this PR, or merge this PR first and create a new PR to address this edge case. I think either is fine.

[dwijnand]

We can either address the edge case in this PR, or merge this PR first and create a new PR to address this edge case. I think either is fine.

I think we should defer this part, perhaps to never.

The first reason is because I think it's common practice in projects to have a subproject dedicated for a macro, and that subproject doesn't have any dependencies. And the reason for that (IIRC and if things haven't changed) because declaring a macro dependency only puts the macro jar on the compilation classpath so it's present during macro expansion, without any of it (potential) dependencies - thus only scala-library, scala-reflect and the jdk of course. Now technically a project could develop that jar as separate subprojects and then assemble it into one jar, and then could suffer this problem, but... perhaps that reality doesn't exist and even if so, it's "only" incremental builds.

The second reason is I can't find a good way to implement that. You mentioned that to address this you need to traverse external dependencies. That won't work. The intra-subproject solution we have here takes the "recompiledClasses" (which might or might not have API changes), e.g. lib.InternalApi, and deduces if there are any macro classes, e.g. lib.Macro, that (transitively) depend on any of those recompiled classes. And if there are, the source files associated for those macro classes are recompiled. If the macro is in a downstream project, we don't have the information locally. If, instead, the current project is the one that contains the macro and the code that changes are upstream, e.g. util.InternalApi, then we don't even have visibility on util.InternalApi because lib.Macro only has visibility of util.Data the public API, and that API didn't change.

I'll detail that in the PR description and in the code.

[Friendseeker]

We can either address the edge case in this PR, or merge this PR first and create a new PR to address this edge case. I think either is fine.

I think we should defer this part, perhaps to never.

The first reason is because I think it's common practice in projects to have a subproject dedicated for a macro, and that subproject doesn't have any dependencies. And the reason for that (IIRC and if things haven't changed) because declaring a macro dependency only puts the macro jar on the compilation classpath so it's present during macro expansion, without any of it (potential) dependencies - thus only scala-library, scala-reflect and the jdk of course. Now technically a project could develop that jar as separate subprojects and then assemble it into one jar, and then could suffer this problem, but... perhaps that reality doesn't exist and even if so, it's "only" incremental builds.

The second reason is I can't find a good way to implement that. You mentioned that to address this you need to traverse external dependencies. That won't work. The intra-subproject solution we have here takes the "recompiledClasses" (which might or might not have API changes), e.g. lib.InternalApi, and deduces if there are any macro classes, e.g. lib.Macro, that (transitively) depend on any of those recompiled classes. And if there are, the source files associated for those macro classes are recompiled. If the macro is in a downstream project, we don't have the information locally. If, instead, the current project is the one that contains the macro and the code that changes are upstream, e.g. util.InternalApi, then we don't even have visibility on util.InternalApi because lib.Macro only has visibility of util.Data the public API, and that API didn't change.

I'll detail that in the PR description and in the code.

Thanks for the explanation. Let's defer it then!