From d450981b7c6cd0150d9251813c0ea72c69285a7b Mon Sep 17 00:00:00 2001 From: Nicholas Gates Date: Thu, 16 Apr 2020 09:28:40 +0100 Subject: [PATCH 1/4] Provider implicit toString --- .../errorprone/GradleProviderToString.java | 55 +++++++++++++++++++ .../GradleProviderToStringTest.java | 49 +++++++++++++++++ 2 files changed, 104 insertions(+) create mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/GradleProviderToString.java create mode 100644 baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/GradleProviderToStringTest.java diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/GradleProviderToString.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/GradleProviderToString.java new file mode 100644 index 000000000..ea47acc8e --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/GradleProviderToString.java @@ -0,0 +1,55 @@ +/* + * (c) Copyright 2020 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.baseline.errorprone; + +import com.google.auto.service.AutoService; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.AbstractToString; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.fixes.Fix; +import com.google.errorprone.predicates.TypePredicate; +import com.google.errorprone.predicates.TypePredicates; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.Tree; +import java.util.Optional; + +@AutoService(BugChecker.class) +@BugPattern( + name = "GradleProviderToString", + summary = "Calling toString on a Provider does not render the contained value", + severity = BugPattern.SeverityLevel.ERROR) +public final class GradleProviderToString extends AbstractToString { + + private static final TypePredicate IS_PROVIDER = TypePredicates.isDescendantOf("org.gradle.api.provider.Provider"); + + @Override + protected TypePredicate typePredicate() { + return IS_PROVIDER; + } + + @Override + protected Optional implicitToStringFix(ExpressionTree tree, VisitorState state) { + // We could automatically call Provider#get, but is that always right? + return Optional.empty(); + } + + @Override + protected Optional toStringFix(Tree parent, ExpressionTree expression, VisitorState state) { + return Optional.empty(); + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/GradleProviderToStringTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/GradleProviderToStringTest.java new file mode 100644 index 000000000..cbdca4ad5 --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/GradleProviderToStringTest.java @@ -0,0 +1,49 @@ +/* + * (c) Copyright 2020 Palantir Technologies Inc. All rights reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package com.palantir.baseline.errorprone; + +import com.google.errorprone.CompilationTestHelper; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +public class GradleProviderToStringTest { + + private CompilationTestHelper compilationHelper; + + @BeforeEach + public void before() { + compilationHelper = CompilationTestHelper.newInstance(GradleProviderToString.class, getClass()); + } + + @Test + public void failsUsedInStringConcatenation() { + compilationHelper + .addSourceLines( + "Foo.java", + "import org.gradle.api.Project;", + "import org.gradle.api.Plugin;", + "import org.gradle.api.provider.Provider;", + "class Foo implements Plugin {", + " public final void apply(Project project) {", + " Provider provider = project.provider(() -> \"hello\");", + " // BUG: Diagnostic contains: Calling toString on a Provider", + " String value = \"My bad provider value: \" + provider;", + " }", + "}") + .doTest(); + } +} From 05294b065d1932bcdc845a141cac5c396a1a5542 Mon Sep 17 00:00:00 2001 From: Nicholas Gates Date: Thu, 16 Apr 2020 08:28:40 +0000 Subject: [PATCH 2/4] Add generated changelog entries --- changelog/@unreleased/pr-1322.v2.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/@unreleased/pr-1322.v2.yml diff --git a/changelog/@unreleased/pr-1322.v2.yml b/changelog/@unreleased/pr-1322.v2.yml new file mode 100644 index 000000000..62f4756ce --- /dev/null +++ b/changelog/@unreleased/pr-1322.v2.yml @@ -0,0 +1,5 @@ +type: feature +feature: + description: Forbid implicit toString of Gradle Providers. + links: + - https://github.com/palantir/gradle-baseline/pull/1322 From 31f28b55e3ce52bfc3e8229865aca69e12af54f2 Mon Sep 17 00:00:00 2001 From: Nicholas Gates Date: Wed, 6 May 2020 19:10:31 +0100 Subject: [PATCH 3/4] Include null comparison value --- .../errorprone/GradleProviderToString.java | 5 +-- .../GradleProviderToStringTest.java | 31 +++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/GradleProviderToString.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/GradleProviderToString.java index ea47acc8e..70ce742b3 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/GradleProviderToString.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/GradleProviderToString.java @@ -22,6 +22,7 @@ import com.google.errorprone.bugpatterns.AbstractToString; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.fixes.Fix; +import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.predicates.TypePredicate; import com.google.errorprone.predicates.TypePredicates; import com.sun.source.tree.ExpressionTree; @@ -44,8 +45,8 @@ protected TypePredicate typePredicate() { @Override protected Optional implicitToStringFix(ExpressionTree tree, VisitorState state) { - // We could automatically call Provider#get, but is that always right? - return Optional.empty(); + // Note that this might not always be the right thing to do, but it's right in enough cases we should do it. + return Optional.of(SuggestedFix.postfixWith(tree, ".get()")); } @Override diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/GradleProviderToStringTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/GradleProviderToStringTest.java index cbdca4ad5..05e95585e 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/GradleProviderToStringTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/GradleProviderToStringTest.java @@ -23,10 +23,12 @@ public class GradleProviderToStringTest { private CompilationTestHelper compilationHelper; + private RefactoringValidator refactoringValidator; @BeforeEach public void before() { compilationHelper = CompilationTestHelper.newInstance(GradleProviderToString.class, getClass()); + refactoringValidator = RefactoringValidator.of(new GradleProviderToString(), getClass()); } @Test @@ -39,11 +41,40 @@ public void failsUsedInStringConcatenation() { "import org.gradle.api.provider.Provider;", "class Foo implements Plugin {", " public final void apply(Project project) {", + " String nonProvider = \"foo\"", " Provider provider = project.provider(() -> \"hello\");", " // BUG: Diagnostic contains: Calling toString on a Provider", + " String value = \"My bad provider value: \" + provider + nonProvider;", + " }", + "}") + .doTest(); + } + + @Test + public void failsUsedInStringConcatenation_replacesWithGet() { + refactoringValidator + .addInputLines( + "Foo.java", + "import org.gradle.api.Project;", + "import org.gradle.api.Plugin;", + "import org.gradle.api.provider.Provider;", + "class Foo implements Plugin {", + " public final void apply(Project project) {", + " Provider provider = project.provider(() -> \"hello\");", " String value = \"My bad provider value: \" + provider;", " }", "}") + .addOutputLines( + "Foo.java", + "import org.gradle.api.Project;", + "import org.gradle.api.Plugin;", + "import org.gradle.api.provider.Provider;", + "class Foo implements Plugin {", + " public final void apply(Project project) {", + " Provider provider = project.provider(() -> \"hello\");", + " String value = \"My bad provider value: \" + provider.get();", + " }", + "}") .doTest(); } } From 5d39aa565e4c55a0058962a7659cfc77813b5814 Mon Sep 17 00:00:00 2001 From: Nicholas Gates Date: Wed, 6 May 2020 19:25:35 +0100 Subject: [PATCH 4/4] Update GradleProviderToStringTest.java --- .../baseline/errorprone/GradleProviderToStringTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/GradleProviderToStringTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/GradleProviderToStringTest.java index 05e95585e..0094541ba 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/GradleProviderToStringTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/GradleProviderToStringTest.java @@ -41,7 +41,7 @@ public void failsUsedInStringConcatenation() { "import org.gradle.api.provider.Provider;", "class Foo implements Plugin {", " public final void apply(Project project) {", - " String nonProvider = \"foo\"", + " String nonProvider = \"foo\";", " Provider provider = project.provider(() -> \"hello\");", " // BUG: Diagnostic contains: Calling toString on a Provider", " String value = \"My bad provider value: \" + provider + nonProvider;",