From 3af987ceb79435e514ceed1121c98f3f22f97d64 Mon Sep 17 00:00:00 2001 From: Nicholas Gates Date: Wed, 7 Aug 2019 11:26:25 +0100 Subject: [PATCH 1/4] Enable applying refaster rules for repos with -Werror and -Xlint:deprecated --- .../baseline/plugins/BaselineErrorProne.java | 16 ++++++++-- ...neErrorProneRefasterIntegrationTest.groovy | 30 +++++++++++++++++++ 2 files changed, 44 insertions(+), 2 deletions(-) diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineErrorProne.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineErrorProne.java index ec5c7775a..57d011713 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineErrorProne.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineErrorProne.java @@ -110,14 +110,17 @@ public void apply(Project project) { if (project.hasProperty(PROP_REFASTER_APPLY)) { javaCompile.dependsOn(compileRefaster); - javaCompile.getOptions().setWarnings(false); + disableWerrorAndDeprecationCompilerChecks(javaCompile); + project.getLogger().error("ARGs: {}", javaCompile.getOptions().getAllCompilerArgs()); + errorProneOptions.getErrorproneArgumentProviders().add(() -> ImmutableList.of( "-XepPatchChecks:refaster:" + refasterRulesFile.get().getAbsolutePath(), "-XepPatchLocation:IN_PLACE")); // Don't attempt to cache since it won't capture the source files that might be modified javaCompile.getOutputs().cacheIf(t -> false); } else if (project.hasProperty(PROP_ERROR_PRONE_APPLY)) { - javaCompile.getOptions().setWarnings(false); + disableWerrorAndDeprecationCompilerChecks(javaCompile); + // TODO(gatesn): Is there a way to discover error-prone checks? // Maybe service-load from a ClassLoader configured with annotation processor path? // https://github.com/google/error-prone/pull/947 @@ -166,6 +169,15 @@ public void apply(Project project) { }); } + private void disableWerrorAndDeprecationCompilerChecks(JavaCompile javaCompile) { + javaCompile.getOptions().setWarnings(false); + javaCompile.getOptions().setDeprecation(false); + javaCompile.getOptions().setCompilerArgs(javaCompile.getOptions().getCompilerArgs().stream() + .filter(arg -> !arg.toLowerCase().equals("-xlintdeprecation")) + .filter(arg -> !arg.toLowerCase().equals("-werror")) + .collect(Collectors.toList())); + } + private static final class LazyConfigurationList extends AbstractList { private final FileCollection files; private List fileList; diff --git a/gradle-baseline-java/src/test/groovy/com/palantir/baseline/BaselineErrorProneRefasterIntegrationTest.groovy b/gradle-baseline-java/src/test/groovy/com/palantir/baseline/BaselineErrorProneRefasterIntegrationTest.groovy index 2ad83d80e..33857e135 100644 --- a/gradle-baseline-java/src/test/groovy/com/palantir/baseline/BaselineErrorProneRefasterIntegrationTest.groovy +++ b/gradle-baseline-java/src/test/groovy/com/palantir/baseline/BaselineErrorProneRefasterIntegrationTest.groovy @@ -35,6 +35,9 @@ class BaselineErrorProneRefasterIntegrationTest extends AbstractPluginTest { mavenLocal() jcenter() } + tasks.withType(JavaCompile) { + options.compilerArgs += ['-Werror', '-Xlint:deprecation'] + } '''.stripIndent() def 'compileJava with refaster fixes CollectionsIsEmpty'() { @@ -110,4 +113,31 @@ class BaselineErrorProneRefasterIntegrationTest extends AbstractPluginTest { } '''.stripIndent() } + + def 'compileJava with refaster fixes Utf8Length with deprecated method'() { + when: + buildFile << standardBuildFile + file('src/main/java/test/Test.java') << ''' + package test; + import com.google.common.base.CharMatcher; + import java.nio.charset.StandardCharsets; + public class Test { + CharMatcher matcher = CharMatcher.digit(); // Would normally fail with -Xlint:deprecation + int i = "hello world".getBytes(StandardCharsets.UTF_8).length; + } + '''.stripIndent() + + then: + BuildResult result = with('compileJava', '-i', '-PrefasterApply').build() + result.task(":compileJava").outcome == TaskOutcome.SUCCESS + file('src/main/java/test/Test.java').text == ''' + package test; + import com.google.common.base.CharMatcher; + import com.google.common.base.Utf8; + public class Test { + CharMatcher matcher = CharMatcher.digit(); // Would normally fail with -Xlint:deprecation + int i = Utf8.encodedLength("hello world"); + } + '''.stripIndent() + } } From 72c828dfe94e1d911287270cbc7d7d08d464a288 Mon Sep 17 00:00:00 2001 From: Nicholas Gates Date: Wed, 7 Aug 2019 11:53:36 +0100 Subject: [PATCH 2/4] Enable refaster with deprecations --- .../baseline/plugins/BaselineErrorProne.java | 74 ++++++++++++------- ...neErrorProneRefasterIntegrationTest.groovy | 1 + 2 files changed, 48 insertions(+), 27 deletions(-) diff --git a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineErrorProne.java b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineErrorProne.java index 57d011713..4364e6bf8 100644 --- a/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineErrorProne.java +++ b/gradle-baseline-java/src/main/groovy/com/palantir/baseline/plugins/BaselineErrorProne.java @@ -108,32 +108,49 @@ public void apply(Project project) { return; } - if (project.hasProperty(PROP_REFASTER_APPLY)) { - javaCompile.dependsOn(compileRefaster); - disableWerrorAndDeprecationCompilerChecks(javaCompile); - project.getLogger().error("ARGs: {}", javaCompile.getOptions().getAllCompilerArgs()); - - errorProneOptions.getErrorproneArgumentProviders().add(() -> ImmutableList.of( - "-XepPatchChecks:refaster:" + refasterRulesFile.get().getAbsolutePath(), - "-XepPatchLocation:IN_PLACE")); - // Don't attempt to cache since it won't capture the source files that might be modified - javaCompile.getOutputs().cacheIf(t -> false); - } else if (project.hasProperty(PROP_ERROR_PRONE_APPLY)) { - disableWerrorAndDeprecationCompilerChecks(javaCompile); - - // TODO(gatesn): Is there a way to discover error-prone checks? - // Maybe service-load from a ClassLoader configured with annotation processor path? - // https://github.com/google/error-prone/pull/947 - errorProneOptions.getErrorproneArgumentProviders().add(() -> ImmutableList.of( - "-XepPatchChecks:" + Joiner.on(',') - .join(errorProneExtension.getPatchChecks().get()), - "-XepPatchLocation:IN_PLACE")); + if (isRefactoring(project)) { // Don't attempt to cache since it won't capture the source files that might be modified javaCompile.getOutputs().cacheIf(t -> false); + + if (isRefasterRefactoring(project)) { + javaCompile.dependsOn(compileRefaster); + errorProneOptions.getErrorproneArgumentProviders().add(() -> ImmutableList.of( + "-XepPatchChecks:refaster:" + refasterRulesFile.get().getAbsolutePath(), + "-XepPatchLocation:IN_PLACE")); + } + + if (isErrorProneRefactoring(project)) { + // TODO(gatesn): Is there a way to discover error-prone checks? + // Maybe service-load from a ClassLoader configured with annotation processor path? + // https://github.com/google/error-prone/pull/947 + errorProneOptions.getErrorproneArgumentProviders().add(() -> ImmutableList.of( + "-XepPatchChecks:" + Joiner.on(',') + .join(errorProneExtension.getPatchChecks().get()), + "-XepPatchLocation:IN_PLACE")); + } } }); }); + // To allow refactoring of deprecated methods, even when -Xlint:deprecation is specified, we need to remove + // these compiler flags after all configuration has happened. + project.afterEvaluate(unused -> project.getTasks().withType(JavaCompile.class) + .configureEach(javaCompile -> { + if (javaCompile.equals(compileRefaster)) { + return; + } + if (isRefactoring(project)) { + javaCompile.getOptions().setWarnings(false); + javaCompile.getOptions().setDeprecation(false); + javaCompile.getOptions().setCompilerArgs(javaCompile.getOptions().getCompilerArgs() + .stream() + .filter(arg -> !arg.equals("-Werror")) + .filter(arg -> !arg.equals("-deprecation")) + .filter(arg -> !arg.equals("-Xlint:deprecation")) + .collect(Collectors.toList())); + } + })); + project.getPluginManager().withPlugin("java-gradle-plugin", appliedPlugin -> { project.getTasks().withType(JavaCompile.class).configureEach(javaCompile -> ((ExtensionAware) javaCompile.getOptions()).getExtensions() @@ -169,13 +186,16 @@ public void apply(Project project) { }); } - private void disableWerrorAndDeprecationCompilerChecks(JavaCompile javaCompile) { - javaCompile.getOptions().setWarnings(false); - javaCompile.getOptions().setDeprecation(false); - javaCompile.getOptions().setCompilerArgs(javaCompile.getOptions().getCompilerArgs().stream() - .filter(arg -> !arg.toLowerCase().equals("-xlintdeprecation")) - .filter(arg -> !arg.toLowerCase().equals("-werror")) - .collect(Collectors.toList())); + private boolean isRefactoring(Project project) { + return isRefasterRefactoring(project) || isErrorProneRefactoring(project); + } + + private boolean isRefasterRefactoring(Project project) { + return project.hasProperty(PROP_REFASTER_APPLY); + } + + private boolean isErrorProneRefactoring(Project project) { + return project.hasProperty(PROP_ERROR_PRONE_APPLY); } private static final class LazyConfigurationList extends AbstractList { diff --git a/gradle-baseline-java/src/test/groovy/com/palantir/baseline/BaselineErrorProneRefasterIntegrationTest.groovy b/gradle-baseline-java/src/test/groovy/com/palantir/baseline/BaselineErrorProneRefasterIntegrationTest.groovy index 33857e135..1df26b98a 100644 --- a/gradle-baseline-java/src/test/groovy/com/palantir/baseline/BaselineErrorProneRefasterIntegrationTest.groovy +++ b/gradle-baseline-java/src/test/groovy/com/palantir/baseline/BaselineErrorProneRefasterIntegrationTest.groovy @@ -134,6 +134,7 @@ class BaselineErrorProneRefasterIntegrationTest extends AbstractPluginTest { package test; import com.google.common.base.CharMatcher; import com.google.common.base.Utf8; + import java.nio.charset.StandardCharsets; public class Test { CharMatcher matcher = CharMatcher.digit(); // Would normally fail with -Xlint:deprecation int i = Utf8.encodedLength("hello world"); From edd21294b63eca780a2f8db85d45d6437b9014ab Mon Sep 17 00:00:00 2001 From: Nicholas Gates Date: Wed, 7 Aug 2019 10:53:36 +0000 Subject: [PATCH 3/4] Add generated changelog entries --- changelog/@unreleased/pr-742.v2.yml | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 changelog/@unreleased/pr-742.v2.yml diff --git a/changelog/@unreleased/pr-742.v2.yml b/changelog/@unreleased/pr-742.v2.yml new file mode 100644 index 000000000..f1acc4800 --- /dev/null +++ b/changelog/@unreleased/pr-742.v2.yml @@ -0,0 +1,9 @@ +type: fix +fix: + description: "Enable applying refaster rules for repos with -Xlint:deprecation\n\n## + Before this PR\nIf a repository configures -Werror or -Xlint:deprecation, then + the refaster refactoring can fail before its had a chance to refactor the code + away from the deprecated method.\n \n## After this PR\nWe disable these checks + when applying the refaster refactorings such that the refactoring is applied." + links: + - https://github.com/palantir/gradle-baseline/pull/742 From 95889e1d41b4d3b05d3dc8db69d72b1229b28afb Mon Sep 17 00:00:00 2001 From: Nicholas Gates Date: Wed, 7 Aug 2019 10:53:36 +0000 Subject: [PATCH 4/4] Add generated changelog entries --- changelog/@unreleased/pr-742.v2.yml | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/changelog/@unreleased/pr-742.v2.yml b/changelog/@unreleased/pr-742.v2.yml index f1acc4800..8085809ff 100644 --- a/changelog/@unreleased/pr-742.v2.yml +++ b/changelog/@unreleased/pr-742.v2.yml @@ -1,9 +1,5 @@ type: fix fix: - description: "Enable applying refaster rules for repos with -Xlint:deprecation\n\n## - Before this PR\nIf a repository configures -Werror or -Xlint:deprecation, then - the refaster refactoring can fail before its had a chance to refactor the code - away from the deprecated method.\n \n## After this PR\nWe disable these checks - when applying the refaster refactorings such that the refactoring is applied." + description: Enable applying refaster rules for repos with -Xlint:deprecation links: - https://github.com/palantir/gradle-baseline/pull/742