From 237826304e2f0c610ad2bbd3f02ca91f316ec8ac Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Tue, 11 May 2021 11:20:35 -0400 Subject: [PATCH 1/4] Add ImmutablesInterfaceDefaultValue @Value.Immutable interface default methods should be annotated @Value.Default --- .../ImmutablesInterfaceDefaultValue.java | 65 +++++++++ .../ImmutablesInterfaceDefaultValueTest.java | 133 ++++++++++++++++++ 2 files changed, 198 insertions(+) create mode 100644 baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesInterfaceDefaultValue.java create mode 100644 baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesInterfaceDefaultValueTest.java diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesInterfaceDefaultValue.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesInterfaceDefaultValue.java new file mode 100644 index 000000000..e3bed9931 --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesInterfaceDefaultValue.java @@ -0,0 +1,65 @@ +/* + * (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.BugPattern.LinkType; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; +import com.google.errorprone.fixes.SuggestedFix; +import com.google.errorprone.fixes.SuggestedFixes; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.MethodTree; +import com.sun.tools.javac.code.Symbol.ClassSymbol; +import com.sun.tools.javac.code.Symbol.MethodSymbol; + +/** + * Checks that interface default methods in an immutables.org @Value.Immutable are marked with @Value.Default. + */ +@AutoService(BugChecker.class) +@BugPattern( + name = "ImmutablesInterfaceDefaultValue", + linkType = LinkType.CUSTOM, + link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", + severity = BugPattern.SeverityLevel.ERROR, + summary = "@Value.Immutable interface default methods should be annotated @Value.Default") +public final class ImmutablesInterfaceDefaultValue extends BugChecker implements MethodTreeMatcher { + + @Override + public Description matchMethod(MethodTree tree, VisitorState state) { + ClassSymbol enclosingClass = ASTHelpers.enclosingClass(ASTHelpers.getSymbol(tree)); + if (enclosingClass != null + && ASTHelpers.hasAnnotation(enclosingClass, "org.immutables.value.Value.Immutable", state)) { + MethodSymbol methodSymbol = ASTHelpers.getSymbol(tree); + if (methodSymbol != null + && methodSymbol.isDefault() + && !ASTHelpers.hasAnnotation(methodSymbol, "org.immutables.value.Value.Default", state) + && !ASTHelpers.hasAnnotation(methodSymbol, "org.immutables.value.Value.Derived", state) + && !ASTHelpers.hasAnnotation(methodSymbol, "org.immutables.value.Value.Lazy", state)) { + SuggestedFix.Builder builder = SuggestedFix.builder(); + String annotation = SuggestedFixes.qualifyType(state, builder, "org.immutables.value.Value.Default"); + return buildDescription(tree) + .addFix(builder.prefixWith(tree, "@" + annotation + " ").build()) + .build(); + } + } + return Description.NO_MATCH; + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesInterfaceDefaultValueTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesInterfaceDefaultValueTest.java new file mode 100644 index 000000000..0e3adfe52 --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesInterfaceDefaultValueTest.java @@ -0,0 +1,133 @@ +/* + * (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.Test; + +public class ImmutablesInterfaceDefaultValueTest { + + @Test + public void testPassesWhenDefaultMethodAnnotatedValueDefault() { + helper().addSourceLines( + "Test.java", + "import org.immutables.value.*;", + "public class Test {", + " @Value.Immutable", + " public interface InterfaceWithValueDefault {", + " String value();", + " @Value.Default", + " default String defaultValue() {", + " return \"default\";", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void testPassesWhenDefaultMethodAnnotatedValueDerived() { + helper().addSourceLines( + "Test.java", + "import org.immutables.value.*;", + "public class Test {", + " @Value.Immutable", + " public interface InterfaceWithValueDefault {", + " String value();", + " @Value.Derived", + " default String derivedValue() {", + " return value();", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void testPassesWhenDefaultMethodAnnotatedValueLazy() { + helper().addSourceLines( + "Test.java", + "import org.immutables.value.*;", + "public class Test {", + " @Value.Immutable", + " public interface InterfaceWithValueDefault {", + " String value();", + " @Value.Lazy", + " default String lazyValue() {", + " return value();", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void refactorsMissingDefaultValueAnnotation() { + refactoring() + .addInputLines( + "Test.java", + "import org.immutables.value.*;", + "public class Test {", + " @Value.Immutable", + " public interface InterfaceWithValueDefault {", + " String value();", + " // BUG: Diagnostic contains: " + + "@Value.Immutable interface default methods should be annotated @Value.Default", + " default String defaultValue() {", + " return \"default\";", + " }", + " @Value.Derived", + " default String derivedValue() {", + " return value();", + " }", + " @Value.Lazy", + " default String lazyValue() {", + " return value();", + " }", + " }", + "}") + .addOutputLines( + "Test.java", + "import org.immutables.value.*;", + "public class Test {", + " @Value.Immutable", + " public interface InterfaceWithValueDefault {", + " String value();", + " @Value.Default", + " default String defaultValue() {", + " return \"default\";", + " }", + " @Value.Derived", + " default String derivedValue() {", + " return value();", + " }", + " @Value.Lazy", + " default String lazyValue() {", + " return value();", + " }", + " }", + "}") + .doTest(); + } + + private CompilationTestHelper helper() { + return CompilationTestHelper.newInstance(ImmutablesInterfaceDefaultValue.class, getClass()); + } + + private RefactoringValidator refactoring() { + return RefactoringValidator.of(ImmutablesInterfaceDefaultValue.class, getClass()); + } +} From 4484141244bf626fc149078906541f08808ab4ad Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Tue, 11 May 2021 15:20:35 +0000 Subject: [PATCH 2/4] Add generated changelog entries --- changelog/@unreleased/pr-1761.v2.yml | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 changelog/@unreleased/pr-1761.v2.yml diff --git a/changelog/@unreleased/pr-1761.v2.yml b/changelog/@unreleased/pr-1761.v2.yml new file mode 100644 index 000000000..add184da3 --- /dev/null +++ b/changelog/@unreleased/pr-1761.v2.yml @@ -0,0 +1,8 @@ +type: improvement +improvement: + description: |- + Add ImmutablesInterfaceDefaultValue + + @Value.Immutable interface default methods should be annotated @Value.Default + links: + - https://github.com/palantir/gradle-baseline/pull/1761 From 38f97e53ccc3c6c57f81620798057747f7b64759 Mon Sep 17 00:00:00 2001 From: David Schlosnagle Date: Wed, 12 May 2021 10:31:37 -0400 Subject: [PATCH 3/4] Initial review comments --- .../ImmutablesInterfaceDefaultValue.java | 45 ++++++++++--------- .../ImmutablesInterfaceDefaultValueTest.java | 30 +++++++++++++ 2 files changed, 55 insertions(+), 20 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesInterfaceDefaultValue.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesInterfaceDefaultValue.java index e3bed9931..a725d0867 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesInterfaceDefaultValue.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesInterfaceDefaultValue.java @@ -25,13 +25,18 @@ import com.google.errorprone.fixes.SuggestedFix; import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.matchers.Description; -import com.google.errorprone.util.ASTHelpers; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.Matchers; import com.sun.source.tree.MethodTree; -import com.sun.tools.javac.code.Symbol.ClassSymbol; -import com.sun.tools.javac.code.Symbol.MethodSymbol; +import com.sun.source.tree.Tree; +import javax.lang.model.element.Modifier; /** - * Checks that interface default methods in an immutables.org @Value.Immutable are marked with @Value.Default. + * Checks that interface default methods in an immutables.org @Value.Immutable are + * annotated with + * @Value.Default , + * @Value.Derived , or + * @Value.Lazy . */ @AutoService(BugChecker.class) @BugPattern( @@ -39,26 +44,26 @@ linkType = LinkType.CUSTOM, link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", severity = BugPattern.SeverityLevel.ERROR, - summary = "@Value.Immutable interface default methods should be annotated @Value.Default") + summary = "@Value.Immutable interface default methods should be annotated with" + + " @Value.Default, @Value.Derived, or @Value.Lazy") public final class ImmutablesInterfaceDefaultValue extends BugChecker implements MethodTreeMatcher { + private static final Matcher MISSING_ANNOTATION_MATCHER = Matchers.allOf( + Matchers.enclosingClass(Matchers.hasAnnotation("org.immutables.value.Value.Immutable")), + Matchers.hasModifier(Modifier.DEFAULT), + Matchers.not(Matchers.anyOf( + Matchers.hasAnnotation("org.immutables.value.Value.Default"), + Matchers.hasAnnotation("org.immutables.value.Value.Derived"), + Matchers.hasAnnotation("org.immutables.value.Value.Lazy")))); + @Override public Description matchMethod(MethodTree tree, VisitorState state) { - ClassSymbol enclosingClass = ASTHelpers.enclosingClass(ASTHelpers.getSymbol(tree)); - if (enclosingClass != null - && ASTHelpers.hasAnnotation(enclosingClass, "org.immutables.value.Value.Immutable", state)) { - MethodSymbol methodSymbol = ASTHelpers.getSymbol(tree); - if (methodSymbol != null - && methodSymbol.isDefault() - && !ASTHelpers.hasAnnotation(methodSymbol, "org.immutables.value.Value.Default", state) - && !ASTHelpers.hasAnnotation(methodSymbol, "org.immutables.value.Value.Derived", state) - && !ASTHelpers.hasAnnotation(methodSymbol, "org.immutables.value.Value.Lazy", state)) { - SuggestedFix.Builder builder = SuggestedFix.builder(); - String annotation = SuggestedFixes.qualifyType(state, builder, "org.immutables.value.Value.Default"); - return buildDescription(tree) - .addFix(builder.prefixWith(tree, "@" + annotation + " ").build()) - .build(); - } + if (MISSING_ANNOTATION_MATCHER.matches(tree, state)) { + SuggestedFix.Builder builder = SuggestedFix.builder(); + String annotation = SuggestedFixes.qualifyType(state, builder, "org.immutables.value.Value.Default"); + return buildDescription(tree) + .addFix(builder.prefixWith(tree, "@" + annotation + " ").build()) + .build(); } return Description.NO_MATCH; } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesInterfaceDefaultValueTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesInterfaceDefaultValueTest.java index 0e3adfe52..aecb2155f 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesInterfaceDefaultValueTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesInterfaceDefaultValueTest.java @@ -20,6 +20,27 @@ public class ImmutablesInterfaceDefaultValueTest { + @Test + public void testFailsWhenDefaultMethodNotAnnotated() { + helper().addSourceLines( + "Test.java", + "import org.immutables.value.*;", + "public class Test {", + " @Value.Immutable", + " public interface InterfaceWithValueDefault {", + " String value();", + "", + " // BUG: Diagnostic contains: @Value.Immutable interface" + + " default methods should be annotated with" + + " @Value.Default, @Value.Derived, or @Value.Lazy", + " default String defaultValue() {", + " return \"default\";", + " }", + " }", + "}") + .doTest(); + } + @Test public void testPassesWhenDefaultMethodAnnotatedValueDefault() { helper().addSourceLines( @@ -29,6 +50,7 @@ public void testPassesWhenDefaultMethodAnnotatedValueDefault() { " @Value.Immutable", " public interface InterfaceWithValueDefault {", " String value();", + "", " @Value.Default", " default String defaultValue() {", " return \"default\";", @@ -47,6 +69,7 @@ public void testPassesWhenDefaultMethodAnnotatedValueDerived() { " @Value.Immutable", " public interface InterfaceWithValueDefault {", " String value();", + "", " @Value.Derived", " default String derivedValue() {", " return value();", @@ -65,6 +88,7 @@ public void testPassesWhenDefaultMethodAnnotatedValueLazy() { " @Value.Immutable", " public interface InterfaceWithValueDefault {", " String value();", + "", " @Value.Lazy", " default String lazyValue() {", " return value();", @@ -84,15 +108,18 @@ public void refactorsMissingDefaultValueAnnotation() { " @Value.Immutable", " public interface InterfaceWithValueDefault {", " String value();", + "", " // BUG: Diagnostic contains: " + "@Value.Immutable interface default methods should be annotated @Value.Default", " default String defaultValue() {", " return \"default\";", " }", + "", " @Value.Derived", " default String derivedValue() {", " return value();", " }", + "", " @Value.Lazy", " default String lazyValue() {", " return value();", @@ -106,14 +133,17 @@ public void refactorsMissingDefaultValueAnnotation() { " @Value.Immutable", " public interface InterfaceWithValueDefault {", " String value();", + "", " @Value.Default", " default String defaultValue() {", " return \"default\";", " }", + "", " @Value.Derived", " default String derivedValue() {", " return value();", " }", + "", " @Value.Lazy", " default String lazyValue() {", " return value();", From a9cdefb6e07af6a6705516abf33121bff2c59c9f Mon Sep 17 00:00:00 2001 From: Raymond Huffman Date: Wed, 27 Nov 2024 18:34:54 -0500 Subject: [PATCH 4/4] only apply check if @ImmutablesConfigStyle is present --- .../ImmutablesInterfaceDefaultValue.java | 15 +++--- .../ImmutablesInterfaceDefaultValueTest.java | 49 ++++++++++++++++++- .../style/ImmutablesConfigStyle.java | 22 +++++++++ 3 files changed, 78 insertions(+), 8 deletions(-) create mode 100644 baseline-error-prone/src/test/java/com/palantir/immutables/style/ImmutablesConfigStyle.java diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesInterfaceDefaultValue.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesInterfaceDefaultValue.java index a725d0867..0bf1175d8 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesInterfaceDefaultValue.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/ImmutablesInterfaceDefaultValue.java @@ -18,7 +18,6 @@ import com.google.auto.service.AutoService; import com.google.errorprone.BugPattern; -import com.google.errorprone.BugPattern.LinkType; import com.google.errorprone.VisitorState; import com.google.errorprone.bugpatterns.BugChecker; import com.google.errorprone.bugpatterns.BugChecker.MethodTreeMatcher; @@ -37,24 +36,28 @@ * @Value.Default , * @Value.Derived , or * @Value.Lazy . + *
+ * This check only applies to interfaces annotated with @ImmutablesConfigStyle. */ @AutoService(BugChecker.class) @BugPattern( - name = "ImmutablesInterfaceDefaultValue", - linkType = LinkType.CUSTOM, + linkType = BugPattern.LinkType.CUSTOM, link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", severity = BugPattern.SeverityLevel.ERROR, summary = "@Value.Immutable interface default methods should be annotated with" - + " @Value.Default, @Value.Derived, or @Value.Lazy") + + " @Value.Default, @Value.Derived, @Value.Lazy, or @JsonIgnore") public final class ImmutablesInterfaceDefaultValue extends BugChecker implements MethodTreeMatcher { private static final Matcher MISSING_ANNOTATION_MATCHER = Matchers.allOf( - Matchers.enclosingClass(Matchers.hasAnnotation("org.immutables.value.Value.Immutable")), + Matchers.enclosingClass(Matchers.allOf( + Matchers.hasAnnotation("org.immutables.value.Value.Immutable"), + Matchers.hasAnnotation("com.palantir.immutables.style.ImmutablesConfigStyle"))), Matchers.hasModifier(Modifier.DEFAULT), Matchers.not(Matchers.anyOf( Matchers.hasAnnotation("org.immutables.value.Value.Default"), Matchers.hasAnnotation("org.immutables.value.Value.Derived"), - Matchers.hasAnnotation("org.immutables.value.Value.Lazy")))); + Matchers.hasAnnotation("org.immutables.value.Value.Lazy"), + Matchers.hasAnnotation("com.fasterxml.jackson.annotation.JsonIgnore")))); @Override public Description matchMethod(MethodTree tree, VisitorState state) { diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesInterfaceDefaultValueTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesInterfaceDefaultValueTest.java index aecb2155f..7ee57dafe 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesInterfaceDefaultValueTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/ImmutablesInterfaceDefaultValueTest.java @@ -25,14 +25,34 @@ public void testFailsWhenDefaultMethodNotAnnotated() { helper().addSourceLines( "Test.java", "import org.immutables.value.*;", + "import com.palantir.immutables.style.ImmutablesConfigStyle;", "public class Test {", " @Value.Immutable", + " @ImmutablesConfigStyle", " public interface InterfaceWithValueDefault {", " String value();", "", " // BUG: Diagnostic contains: @Value.Immutable interface" + " default methods should be annotated with" - + " @Value.Default, @Value.Derived, or @Value.Lazy", + + " @Value.Default, @Value.Derived, @Value.Lazy, or @JsonIgnore", + " default String defaultValue() {", + " return \"default\";", + " }", + " }", + "}") + .doTest(); + } + + @Test + public void testPassesWhenInterfaceIsNotConfig() { + helper().addSourceLines( + "Test.java", + "import org.immutables.value.*;", + "public class Test {", + " @Value.Immutable", + " public interface InterfaceWithValueDefault {", + " String value();", + "", " default String defaultValue() {", " return \"default\";", " }", @@ -98,19 +118,42 @@ public void testPassesWhenDefaultMethodAnnotatedValueLazy() { .doTest(); } + @Test + public void testPassesWhenDefaultMethodAnnotatedJsonIgnore() { + helper().addSourceLines( + "Test.java", + "import com.fasterxml.jackson.annotation.JsonIgnore;", + "import org.immutables.value.*;", + "public class Test {", + " @Value.Immutable", + " public interface InterfaceWithValueDefault {", + " String value();", + "", + " @JsonIgnore", + " default String lazyValue() {", + " return value();", + " }", + " }", + "}") + .doTest(); + } + @Test public void refactorsMissingDefaultValueAnnotation() { refactoring() .addInputLines( "Test.java", "import org.immutables.value.*;", + "import com.palantir.immutables.style.ImmutablesConfigStyle;", "public class Test {", " @Value.Immutable", + " @ImmutablesConfigStyle", " public interface InterfaceWithValueDefault {", " String value();", "", " // BUG: Diagnostic contains: " - + "@Value.Immutable interface default methods should be annotated @Value.Default", + + "@Value.Immutable interface default methods should be annotated with " + + "@Value.Default, @Value.Default, @Value.Derived, @Value.Lazy, or @JsonIgnore", " default String defaultValue() {", " return \"default\";", " }", @@ -129,8 +172,10 @@ public void refactorsMissingDefaultValueAnnotation() { .addOutputLines( "Test.java", "import org.immutables.value.*;", + "import com.palantir.immutables.style.ImmutablesConfigStyle;", "public class Test {", " @Value.Immutable", + " @ImmutablesConfigStyle", " public interface InterfaceWithValueDefault {", " String value();", "", diff --git a/baseline-error-prone/src/test/java/com/palantir/immutables/style/ImmutablesConfigStyle.java b/baseline-error-prone/src/test/java/com/palantir/immutables/style/ImmutablesConfigStyle.java new file mode 100644 index 000000000..7444000d2 --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/immutables/style/ImmutablesConfigStyle.java @@ -0,0 +1,22 @@ +/* + * (c) Copyright 2024 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.immutables.style; + +/** + * Placeholder definition for use in tests. + */ +public @interface ImmutablesConfigStyle {}