Incremental compiler hotfixes #1483
Conversation
This commit fixes the ABI extractor, which ignored the value of constants. This was bad because if a constant value changed, we wouldn't trigger recompilation of consumers (independently of incremental compilation). Since the compiler can inline those values, incremental compilation wouldn't catch the problem either. Fixes: #1476
This commit fixes #1474, by not relying on the analysis of literals (which still exists), but rather recompiling everything whenever we detect that a constant effectively changed in a class. This doesn't prevent us from using a smarter strategy later (say, using a compiler plugin). The commit also changes the hash that is used to store constants, so that the name of the constant is used. This prevents another bug which was that if a class has multiple variants of the same value, or that values of several constants are switched, we wouldn't detect the change.
| @@ -181,6 +183,8 @@ abstract class AbstractCrossTaskIncrementalJavaCompilationIntegrationTest extend | |||
| impl.recompiledClasses("X") | |||
| } | |||
|
|
|||
| @NotYetImplemented | |||
| // used to work in 3.4, not anymore in 3.4.1. Can re-enable with compiler plugins. See gradle/gradle#1474 | |||
| def "change in an upstream class with non-private constant doesn't cause full rebuild if constant is not used"() { | |||
There was a problem hiding this comment.
The name of this method is misleading. It should say "changing an unused constant". It's not just "any change".
There was a problem hiding this comment.
Yeah I agree. I didn't change the test names, it should have been caught in a previous review :)
| @@ -165,6 +165,8 @@ abstract class AbstractCrossTaskIncrementalJavaCompilationIntegrationTest extend | |||
| impl.noneRecompiled() | |||
| } | |||
|
|
|||
| @NotYetImplemented | |||
| // used to work in 3.4, not anymore in 3.4.1. Can re-enable with compiler plugins. See gradle/gradle#1474 | |||
| def "deletion of jar with non-private constant causes rebuild if constant is used"() { | |||
There was a problem hiding this comment.
It should still cause a (full) rebuild, because a constant disappeared.
There was a problem hiding this comment.
We do a full rebuild. The method name doesn't say "full rebuild". The previous version, in 3.4, only rebuilt one of the classes, not all.
There was a problem hiding this comment.
Do we have a test that tests the full rebuild?
There was a problem hiding this comment.
Nope, and that's a fair point. That's what makes this PR a bit ugly. There were passing tests, that do not pass anymore. We want to keep them because we know we can make that work. On the other hand, we want to test the not so good behavior... So 2 tests testing the absolute opposite in the same class...
| @@ -648,4 +659,46 @@ abstract class AbstractCrossTaskIncrementalJavaCompilationIntegrationTest extend | |||
| !output.contains(':api:compileJava - is not incremental (e.g. outputs have changed, no previous execution, etc.).') | |||
| impl.recompiledClasses("ImplA") | |||
| } | |||
|
|
|||
| @NotYetImplemented | |||
| // used to work in 3.4, not anymore in 3.4.1. Can re-enable with compiler plugins. See gradle/gradle#1474 | |||
There was a problem hiding this comment.
I think we should stop writing "used to work with 3.4", that sounds negative. The logic in 3.4 was broken and ignored way too many changes. Let's just leave "Will require compiler plugin".
| @@ -257,6 +257,8 @@ class SourceIncrementalJavaCompilationIntegrationTest extends AbstractIntegratio | |||
| outputs.recompiledClasses 'B', 'A' | |||
| } | |||
|
|
|||
| @NotYetImplemented | |||
| // used to work in 3.4, not anymore in 3.4.1. Can re-enable with compiler plugins. See gradle/gradle#1474 | |||
| def "changed class with unused non-private constant incurs partial rebuild"() { | |||
There was a problem hiding this comment.
Same as above, this method name is misleading. It's not just "any change", but changing the unused constant (removing in this case).
| Set<String> result = new HashSet<String>(); | ||
| if (deps != null && !deps.isDependencyToAll()) { | ||
| recurseDependents(new HashSet<String>(), result, deps.getDependentClasses()); | ||
| } | ||
| for (Integer constant : constants) { | ||
| /*for (Integer constant : constants) { |
There was a problem hiding this comment.
Okay... I kept it explicitly because it's easier to figure out why there's a Set<Integer> as a parameter. You could argue we have git log but sometimes I find it easier to keep the code to describe why it's not there anymore.
| @@ -63,19 +63,26 @@ public DependentsSet getRelevantDependents(String className, Set<Integer> consta | |||
| if (deps == null && constants.isEmpty()) { | |||
| return DefaultDependentsSet.EMPTY; | |||
| } | |||
| // since 3.4.1, the Set<Integer> of constants here doesn't match the set of literals, so we cannot use them | |||
There was a problem hiding this comment.
I don't think this comment makes sense to anyone who didn't write this code or reads the whole git history. I'd just remove this comment, because the logic seems clearer to me without it. The code says clearly: If there are any constant changes, recompile everything.
There were comments helping the implementor (me) understand why code is commented out and why a parameter is now mostly unused. Now, please use `git log` to understand.
Fixes for #1474 and #1476.