From 37d2b62a3cb654026281a8660ed4d58551e51d08 Mon Sep 17 00:00:00 2001 From: Cedric Champeau Date: Wed, 1 Mar 2017 17:29:20 +0100 Subject: [PATCH] Remove constant usage analysis in favor of full recompilation. This commit fixes gradle/gradle#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. --- ...entalJavaCompilationIntegrationTest.groovy | 46 +++++++++++++++++-- ...entalJavaCompilationIntegrationTest.groovy | 2 + .../asm/ClassDependenciesVisitor.java | 5 +- .../incremental/deps/ClassSetAnalysis.java | 13 ++++-- ...efaultClassDependenciesAnalyzerTest.groovy | 4 +- .../deps/ClassSetAnalysisTest.groovy | 3 ++ 6 files changed, 62 insertions(+), 11 deletions(-) diff --git a/subprojects/language-java/src/integTest/groovy/org/gradle/java/compile/incremental/AbstractCrossTaskIncrementalJavaCompilationIntegrationTest.groovy b/subprojects/language-java/src/integTest/groovy/org/gradle/java/compile/incremental/AbstractCrossTaskIncrementalJavaCompilationIntegrationTest.groovy index 1920632807af..48e9c3af6bc4 100644 --- a/subprojects/language-java/src/integTest/groovy/org/gradle/java/compile/incremental/AbstractCrossTaskIncrementalJavaCompilationIntegrationTest.groovy +++ b/subprojects/language-java/src/integTest/groovy/org/gradle/java/compile/incremental/AbstractCrossTaskIncrementalJavaCompilationIntegrationTest.groovy @@ -20,9 +20,7 @@ package org.gradle.java.compile.incremental import groovy.transform.NotYetImplemented import org.gradle.integtests.fixtures.AbstractIntegrationSpec import org.gradle.integtests.fixtures.CompilationOutputsFixture -import org.gradle.util.TestPrecondition import spock.lang.Issue -import spock.lang.Requires import spock.lang.Unroll abstract class AbstractCrossTaskIncrementalJavaCompilationIntegrationTest extends AbstractIntegrationSpec { @@ -167,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"() { java api: ["class A { public final static int x = 1; }"], impl: ["class X { int x() { return 1;} }", "class Y {}"] impl.snapshot { run "compileJava" } @@ -183,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"() { java api: ["class A {}", "class B { final static int x = 1; }"], impl: ["class ImplA extends A {}", "class ImplB extends B {}"] impl.snapshot { run "compileJava" } @@ -205,7 +207,8 @@ abstract class AbstractCrossTaskIncrementalJavaCompilationIntegrationTest extend run "impl:compileJava" then: - impl.recompiledClasses('ImplA') + // impl.recompiledClasses('ImplA') // used to work in 3.4, not anymore in 3.4.1. Can re-enable with compiler plugins. See gradle/gradle#1474 + impl.recompiledClasses('ImplA', 'ImplB') where: constantType | constantValue @@ -221,6 +224,8 @@ abstract class AbstractCrossTaskIncrementalJavaCompilationIntegrationTest extend } @Unroll + @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 causes rebuild only if same constant is used and no direct dependency (#constantType)"() { java api: ["class A {}", "class B { final static $constantType x = $constantValue; }"], impl: ["class X { $constantType foo() { return $constantValue; }}", "class Y {int foo() { return -2; }}"] impl.snapshot { run "compileJava" } @@ -246,6 +251,8 @@ abstract class AbstractCrossTaskIncrementalJavaCompilationIntegrationTest extend } @Unroll + @NotYetImplemented + // used to work in 3.4, not anymore in 3.4.1. Can re-enable with compiler plugins. See gradle/gradle#1474 def "constant value change in an upstream class causes rebuild if previous constant value was used in previous build (#constantType)"() { java api: ["class A {}", "class B { final static $constantType x = $constantValue; }"], impl: ["class X { $constantType foo() { return $constantValue; }}", "class Y {int foo() { return -2; }}"] impl.snapshot { run "compileJava" } @@ -270,6 +277,8 @@ abstract class AbstractCrossTaskIncrementalJavaCompilationIntegrationTest extend 'String' | '"foo" + "bar"' | '"bar"' } + @NotYetImplemented + // used to work in 3.4, not anymore in 3.4.1. Can re-enable with compiler plugins. See gradle/gradle#1474 def "ignores irrelevant changes to constant values"() { java api: ["class A {}", "class B { final static int x = 3; final static int y = -2; }"], impl: ["class X { int foo() { return 3; }}", "class Y {int foo() { return -2; }}"] @@ -598,7 +607,7 @@ abstract class AbstractCrossTaskIncrementalJavaCompilationIntegrationTest extend run("impl:compileJava") then: - impl.recompiledClasses('C', 'C$Inner', 'D', 'D$Inner', 'E', 'E$1', 'F', 'F$Inner') + impl.recompiledClasses('B', 'C', 'C$Inner', 'D', 'D$Inner', 'E', 'E$1', 'F', 'F$Inner') where: visibility << ['public', 'private' , ''] @@ -651,8 +660,35 @@ abstract class AbstractCrossTaskIncrementalJavaCompilationIntegrationTest extend 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 + def "only recompiles classes potentially affected by constant change"() { + java api: ["class A { public static final int FOO = 10; public static final int BAR = 20; }"], + impl: ['class B { void foo() { int x = 10; } }', 'class C { void foo() { int x = 20; } }'] + impl.snapshot { run 'compileJava' } + + when: + java api: ['class A { public static final int FOO = 100; public static final int BAR = 20; }'] + run 'impl:compileJava' + + then: + impl.recompiledClasses 'B' + } + + def "recompiles dependent class in case a constant is switched"() { + java api: ["class A { public static final int FOO = 10; public static final int BAR = 20; }"], + impl: ['class B { void foo() { int x = 10; } }', 'class C { void foo() { int x = 20; } }'] + impl.snapshot { run 'compileJava' } + + when: + java api: ['class A { public static final int FOO = 20; public static final int BAR = 10; }'] + run 'impl:compileJava' + + then: + impl.recompiledClasses 'B', 'C' + } + @Issue("gradle/gradle#1474") - @Requires({TestPrecondition.JDK8_OR_LATER}) // todo: also fix for JDK < 8 def "recompiles dependent class in case a constant is computed from another constant"() { java api: ["class A { public static final int FOO = 10; }"], impl: ['class B { public static final int BAR = 2 + A.FOO; } '] impl.snapshot { run 'compileJava' } diff --git a/subprojects/language-java/src/integTest/groovy/org/gradle/java/compile/incremental/SourceIncrementalJavaCompilationIntegrationTest.groovy b/subprojects/language-java/src/integTest/groovy/org/gradle/java/compile/incremental/SourceIncrementalJavaCompilationIntegrationTest.groovy index dfb9f22db912..d0792c45d4be 100644 --- a/subprojects/language-java/src/integTest/groovy/org/gradle/java/compile/incremental/SourceIncrementalJavaCompilationIntegrationTest.groovy +++ b/subprojects/language-java/src/integTest/groovy/org/gradle/java/compile/incremental/SourceIncrementalJavaCompilationIntegrationTest.groovy @@ -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"() { java "class A { int foo() { return 2; } }", "class B { final static int x = 1;}" outputs.snapshot { run "compileJava" } diff --git a/subprojects/language-java/src/main/java/org/gradle/api/internal/tasks/compile/incremental/asm/ClassDependenciesVisitor.java b/subprojects/language-java/src/main/java/org/gradle/api/internal/tasks/compile/incremental/asm/ClassDependenciesVisitor.java index c6e4c7e7a2f9..e1a7505258a9 100644 --- a/subprojects/language-java/src/main/java/org/gradle/api/internal/tasks/compile/incremental/asm/ClassDependenciesVisitor.java +++ b/subprojects/language-java/src/main/java/org/gradle/api/internal/tasks/compile/incremental/asm/ClassDependenciesVisitor.java @@ -152,7 +152,10 @@ private boolean isAnnotationType(String[] interfaces) { public FieldVisitor visitField(int access, String name, String desc, String signature, Object value) { maybeAddDependentType(descTypeOf(desc)); if (isAccessibleConstant(access, value) && constants != null) { - constants.add(value.hashCode()); //non-private const + // we need to compute a hash for a constant, which is based on the name of the constant + its value + // otherwise we miss the case where a class defines several constants with the same value, or when + // two values are switched + constants.add((name + '|' + value).hashCode()); //non-private const } return null; } diff --git a/subprojects/language-java/src/main/java/org/gradle/api/internal/tasks/compile/incremental/deps/ClassSetAnalysis.java b/subprojects/language-java/src/main/java/org/gradle/api/internal/tasks/compile/incremental/deps/ClassSetAnalysis.java index 5cafb9dd5421..b11e72b3a360 100644 --- a/subprojects/language-java/src/main/java/org/gradle/api/internal/tasks/compile/incremental/deps/ClassSetAnalysis.java +++ b/subprojects/language-java/src/main/java/org/gradle/api/internal/tasks/compile/incremental/deps/ClassSetAnalysis.java @@ -46,7 +46,7 @@ public DependentsSet getRelevantDependents(Iterable classes, Set children = data.classesToChildren.get(dependentClass); - if (children!=null && children.contains(cls)) { + if (children != null && children.contains(cls)) { System.out.println("children = " + children); } } @@ -63,19 +63,26 @@ public DependentsSet getRelevantDependents(String className, Set consta if (deps == null && constants.isEmpty()) { return DefaultDependentsSet.EMPTY; } + // since 3.4.1, the Set of constants here doesn't match the set of literals, so we cannot use them + // anymore. If the set above is not empty, it means a constant has been added or changed, and we need to + // recompile everything + if (!constants.isEmpty()) { + return DependencyToAll.INSTANCE; + } Set result = new HashSet(); if (deps != null && !deps.isDependencyToAll()) { recurseDependents(new HashSet(), result, deps.getDependentClasses()); } - for (Integer constant : constants) { + /*for (Integer constant : constants) { Set classes = data.literalsToClasses.get(constant); if (classes != null) { result.addAll(classes); } } - + */ result.remove(className); return new DefaultDependentsSet(result); + } public boolean isDependencyToAll(String className) { diff --git a/subprojects/language-java/src/test/groovy/org/gradle/api/internal/tasks/compile/incremental/analyzer/DefaultClassDependenciesAnalyzerTest.groovy b/subprojects/language-java/src/test/groovy/org/gradle/api/internal/tasks/compile/incremental/analyzer/DefaultClassDependenciesAnalyzerTest.groovy index 21a2cc70a0e0..b41a52dca4ea 100644 --- a/subprojects/language-java/src/test/groovy/org/gradle/api/internal/tasks/compile/incremental/analyzer/DefaultClassDependenciesAnalyzerTest.groovy +++ b/subprojects/language-java/src/test/groovy/org/gradle/api/internal/tasks/compile/incremental/analyzer/DefaultClassDependenciesAnalyzerTest.groovy @@ -69,7 +69,7 @@ class DefaultClassDependenciesAnalyzerTest extends Specification { then: analysis.classDependencies == [UsedByNonPrivateConstantsClass.name] as Set !analysis.dependencyToAll - analysis.constants == [1] as Set + analysis.constants == ['X|1'.hashCode()] as Set analysis.literals == [] as Set when: @@ -78,7 +78,7 @@ class DefaultClassDependenciesAnalyzerTest extends Specification { then: analysis.classDependencies.isEmpty() !analysis.dependencyToAll - analysis.constants == [1] as Set + analysis.constants == ['X|1'.hashCode()] as Set analysis.literals == [] as Set when: diff --git a/subprojects/language-java/src/test/groovy/org/gradle/api/internal/tasks/compile/incremental/deps/ClassSetAnalysisTest.groovy b/subprojects/language-java/src/test/groovy/org/gradle/api/internal/tasks/compile/incremental/deps/ClassSetAnalysisTest.groovy index ad6de0caf10d..a8a1dcdd7ce8 100644 --- a/subprojects/language-java/src/test/groovy/org/gradle/api/internal/tasks/compile/incremental/deps/ClassSetAnalysisTest.groovy +++ b/subprojects/language-java/src/test/groovy/org/gradle/api/internal/tasks/compile/incremental/deps/ClassSetAnalysisTest.groovy @@ -16,6 +16,7 @@ package org.gradle.api.internal.tasks.compile.incremental.deps +import groovy.transform.NotYetImplemented import spock.lang.Specification import static org.gradle.api.internal.tasks.compile.incremental.deps.DefaultDependentsSet.dependents @@ -180,6 +181,8 @@ class ClassSetAnalysisTest extends Specification { !a.isDependencyToAll("Unknown") } + @NotYetImplemented + // used to work in 3.4, not anymore in 3.4.1. Can re-enable with compiler plugins. See gradle/gradle#1474 def "adds classes with literals as dependents"() { def a = analysis([:], [A: [1,2] as Set], [1: ['B', 'C'] as Set, 2: ['D'] as Set])