From 39fde22010d5680cde9a854f8f679e72b677a3c3 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 25 Apr 2022 14:50:39 -0400 Subject: [PATCH 1/8] SafeLoggingPropagation doesn't attempt to annotate anonymous classes --- .../errorprone/SafeLoggingPropagation.java | 6 ++++++ .../errorprone/SafeLoggingPropagationTest.java | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingPropagation.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingPropagation.java index 9276dcd6f..b7af5cd2f 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingPropagation.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingPropagation.java @@ -25,12 +25,14 @@ import com.google.errorprone.matchers.Description; import com.google.errorprone.matchers.Matcher; import com.google.errorprone.matchers.Matchers; +import com.google.errorprone.util.ASTHelpers; import com.palantir.baseline.errorprone.safety.Safety; import com.palantir.baseline.errorprone.safety.SafetyAnnotations; import com.sun.source.tree.AnnotationTree; import com.sun.source.tree.ClassTree; import com.sun.source.tree.MethodTree; import com.sun.source.tree.Tree; +import com.sun.tools.javac.code.Symbol.ClassSymbol; import javax.lang.model.element.Modifier; @AutoService(BugChecker.class) @@ -55,6 +57,10 @@ public final class SafeLoggingPropagation extends BugChecker implements BugCheck @Override public Description matchClass(ClassTree classTree, VisitorState state) { + ClassSymbol classSymbol = ASTHelpers.getSymbol(classTree); + if (classSymbol == null || classSymbol.isAnonymous()) { + return Description.NO_MATCH; + } Safety existingClassSafety = SafetyAnnotations.getSafety(classTree, state); Safety safety = SafetyAnnotations.getSafety(classTree.getExtendsClause(), state); for (Tree implemented : classTree.getImplementsClause()) { diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingPropagationTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingPropagationTest.java index ee157bcf4..2cd3b9016 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingPropagationTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingPropagationTest.java @@ -191,6 +191,24 @@ void testIgnoresThrowable() { .doTest(); } + @Test + void testIgnoresAnonymous() { + fix().addInputLines( + "Test.java", + "import com.palantir.logsafe.*;", + "import com.palantir.tokens.auth.*;", + "import java.util.function.*;", + "public final class Test {", + " private static final Supplier supplier = new Supplier() {", + " @Override public BearerToken get() {", + " return BearerToken.valueOf(\"abcdefghijklmnopq\");", + " }", + " };", + "}") + .expectUnchanged() + .doTest(); + } + private RefactoringValidator fix() { return RefactoringValidator.of(SafeLoggingPropagation.class, getClass()); } From 98680fc3de1a0a257bfc3b8ef3b61716a327e256 Mon Sep 17 00:00:00 2001 From: svc-changelog Date: Mon, 25 Apr 2022 19:00:36 +0000 Subject: [PATCH 2/8] Add generated changelog entries --- changelog/@unreleased/pr-2227.v2.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelog/@unreleased/pr-2227.v2.yml diff --git a/changelog/@unreleased/pr-2227.v2.yml b/changelog/@unreleased/pr-2227.v2.yml new file mode 100644 index 000000000..245935bcd --- /dev/null +++ b/changelog/@unreleased/pr-2227.v2.yml @@ -0,0 +1,5 @@ +type: fix +fix: + description: SafeLoggingPropagation doesn't attempt to annotate anonymous classes + links: + - https://github.com/palantir/gradle-baseline/pull/2227 From 7f976d7de6d065921e66c61cd6ddb5332eeb3cdf Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 25 Apr 2022 15:21:40 -0400 Subject: [PATCH 3/8] better value type heuristic --- .../errorprone/SafeLoggingPropagation.java | 31 ++++++++++--- .../SafeLoggingPropagationTest.java | 43 +++++++++++++++++++ 2 files changed, 69 insertions(+), 5 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingPropagation.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingPropagation.java index b7af5cd2f..a9ce420f0 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingPropagation.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingPropagation.java @@ -50,10 +50,10 @@ public final class SafeLoggingPropagation extends BugChecker implements BugCheck Matchers.isSameType(SafetyAnnotations.UNSAFE), Matchers.isSameType(SafetyAnnotations.DO_NOT_LOG)); + private static final Matcher NON_STATIC_METHOD_MATCHER = + Matchers.not(Matchers.hasModifier(Modifier.STATIC)); private static final Matcher GETTER_METHOD_MATCHER = Matchers.allOf( - Matchers.not(Matchers.hasModifier(Modifier.STATIC)), - Matchers.not(Matchers.methodReturns(Matchers.isVoidType())), - Matchers.methodHasNoParameters()); + Matchers.not(Matchers.methodReturns(Matchers.isVoidType())), Matchers.methodHasNoParameters()); @Override public Description matchClass(ClassTree classTree, VisitorState state) { @@ -66,14 +66,35 @@ public Description matchClass(ClassTree classTree, VisitorState state) { for (Tree implemented : classTree.getImplementsClause()) { safety = safety.leastUpperBound(SafetyAnnotations.getSafety(implemented, state)); } + boolean hasNonGetterMethod = false; + boolean hasKnownGetter = false; for (Tree member : classTree.getMembers()) { if (member instanceof MethodTree) { MethodTree methodMember = (MethodTree) member; - if (GETTER_METHOD_MATCHER.matches(methodMember, state)) { - safety = safety.leastUpperBound(SafetyAnnotations.getSafety(methodMember.getReturnType(), state)); + if (NON_STATIC_METHOD_MATCHER.matches(methodMember, state)) { + if (GETTER_METHOD_MATCHER.matches(methodMember, state)) { + Safety getterSafety = SafetyAnnotations.getSafety(methodMember.getReturnType(), state); + if (getterSafety != Safety.UNKNOWN) { + hasKnownGetter = true; + } + safety = safety.leastUpperBound(getterSafety); + } else { + hasNonGetterMethod = true; + } } } } + // If no getter-style methods are detected, assume this is not a value type. + if (!hasKnownGetter) { + return Description.NO_MATCH; + } + // non-getter methods are avoided such that we don't annotate non-value types unless we have additional + // data to suggest this is a value (immutables annotations). + if (hasNonGetterMethod + && !ASTHelpers.hasAnnotation(classSymbol, "org.immutables.value.Value.Immutable", state)) { + return Description.NO_MATCH; + } + return handleSafety(classTree, state, existingClassSafety, safety); } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingPropagationTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingPropagationTest.java index 2cd3b9016..78b0ea7b0 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingPropagationTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingPropagationTest.java @@ -176,6 +176,49 @@ void testIgnoresMethodsWithParameters() { .doTest(); } + @Test + void testIgnoresInterfacesWithMethodsWithParameters() { + fix().addInputLines( + "Test.java", + "import com.palantir.logsafe.*;", + "interface Test {", + " @DoNotLog", + " String getToken();", + " @DoNotLog", + " String token(int i);", + "}") + .expectUnchanged() + .doTest(); + } + + @Test + void testIncludesIfacesWithMethodsWithParametersIfImmutables() { + fix().addInputLines( + "Test.java", + "import com.palantir.logsafe.*;", + "import org.immutables.value.Value;", + "@Value.Immutable", + "interface Test {", + " @DoNotLog", + " String getToken();", + " @DoNotLog", + " default String token(int i) { return \"\"; };", + "}") + .addOutputLines( + "Test.java", + "import com.palantir.logsafe.*;", + "import org.immutables.value.Value;", + "@DoNotLog", + "@Value.Immutable", + "interface Test {", + " @DoNotLog", + " String getToken();", + " @DoNotLog", + " default String token(int i) { return \"\"; };", + "}") + .doTest(); + } + @Test void testIgnoresThrowable() { // exceptions are unsafe-by-default, it's unnecessary to annotate every exception as unsafe. From cda09a391e1ad015912677005ec52c66ce8223a4 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 25 Apr 2022 15:23:10 -0400 Subject: [PATCH 4/8] cyclo --- .../com/palantir/baseline/errorprone/SafeLoggingPropagation.java | 1 + 1 file changed, 1 insertion(+) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingPropagation.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingPropagation.java index a9ce420f0..829a650c9 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingPropagation.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingPropagation.java @@ -56,6 +56,7 @@ public final class SafeLoggingPropagation extends BugChecker implements BugCheck Matchers.not(Matchers.methodReturns(Matchers.isVoidType())), Matchers.methodHasNoParameters()); @Override + @SuppressWarnings("checkstyle:CyclomaticComplexity") public Description matchClass(ClassTree classTree, VisitorState state) { ClassSymbol classSymbol = ASTHelpers.getSymbol(classTree); if (classSymbol == null || classSymbol.isAnonymous()) { From a0528c9c3f75b29a3974620183c62240e1c78418 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 25 Apr 2022 15:56:58 -0400 Subject: [PATCH 5/8] record support --- baseline-error-prone/build.gradle | 7 ++- .../errorprone/SafeLoggingPropagation.java | 56 ++++++++++++++++--- .../SafeLoggingPropagationTest.java | 16 ++++++ 3 files changed, 71 insertions(+), 8 deletions(-) diff --git a/baseline-error-prone/build.gradle b/baseline-error-prone/build.gradle index a0503ad66..ab3f194cf 100644 --- a/baseline-error-prone/build.gradle +++ b/baseline-error-prone/build.gradle @@ -61,7 +61,6 @@ moduleJvmArgs { 'jdk.compiler/com.sun.tools.javac.file', 'jdk.compiler/com.sun.tools.javac.code', 'jdk.compiler/com.sun.tools.javac.util', - 'jdk.compiler/com.sun.tools.javac.comp', 'jdk.compiler/com.sun.tools.javac.main', 'jdk.compiler/com.sun.tools.javac.model', 'jdk.compiler/com.sun.tools.javac.parser', @@ -69,4 +68,10 @@ moduleJvmArgs { 'jdk.compiler/com.sun.tools.javac.tree', 'jdk.compiler/com.sun.tools.javac.util' ] + opens = ['jdk.compiler/com.sun.tools.javac.comp'] } + +javaVersions { + libraryTarget = 11 + runtime = 17 +} \ No newline at end of file diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingPropagation.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingPropagation.java index 829a650c9..057e165ca 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingPropagation.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingPropagation.java @@ -33,6 +33,8 @@ import com.sun.source.tree.MethodTree; import com.sun.source.tree.Tree; import com.sun.tools.javac.code.Symbol.ClassSymbol; +import com.sun.tools.javac.code.Symbol.VarSymbol; +import java.util.List; import javax.lang.model.element.Modifier; @AutoService(BugChecker.class) @@ -50,29 +52,61 @@ public final class SafeLoggingPropagation extends BugChecker implements BugCheck Matchers.isSameType(SafetyAnnotations.UNSAFE), Matchers.isSameType(SafetyAnnotations.DO_NOT_LOG)); - private static final Matcher NON_STATIC_METHOD_MATCHER = - Matchers.not(Matchers.hasModifier(Modifier.STATIC)); + private static final Matcher NON_STATIC_NON_CTOR = + Matchers.not(Matchers.anyOf(Matchers.hasModifier(Modifier.STATIC), Matchers.methodIsConstructor())); private static final Matcher GETTER_METHOD_MATCHER = Matchers.allOf( Matchers.not(Matchers.methodReturns(Matchers.isVoidType())), Matchers.methodHasNoParameters()); @Override - @SuppressWarnings("checkstyle:CyclomaticComplexity") public Description matchClass(ClassTree classTree, VisitorState state) { ClassSymbol classSymbol = ASTHelpers.getSymbol(classTree); if (classSymbol == null || classSymbol.isAnonymous()) { return Description.NO_MATCH; } + if (isRecord(classSymbol)) { + return matchRecord(classTree, classSymbol, state); + } else { + return matchClassOrInterface(classTree, classSymbol, state); + } + } + + private static boolean isRecord(ClassSymbol classSymbol) { + // Can use classSymbol.isRecord() in future versions + return (classSymbol.flags() & 1L << 61) != 0; + } + + @SuppressWarnings("unchecked") + private static List getRecordComponents(ClassSymbol classSymbol) { + // Can use classSymbol.getRecordComponents() in future versions + try { + return (List) + ClassSymbol.class.getMethod("getRecordComponents").invoke(classSymbol); + } catch (ReflectiveOperationException e) { + throw new RuntimeException("Failed to get record components", e); + } + } + + private Description matchRecord(ClassTree classTree, ClassSymbol classSymbol, VisitorState state) { Safety existingClassSafety = SafetyAnnotations.getSafety(classTree, state); - Safety safety = SafetyAnnotations.getSafety(classTree.getExtendsClause(), state); - for (Tree implemented : classTree.getImplementsClause()) { - safety = safety.leastUpperBound(SafetyAnnotations.getSafety(implemented, state)); + Safety safety = getTypeSafetyFromAncestors(classTree, state); + for (VarSymbol recordComponent : getRecordComponents(classSymbol)) { + Safety symbolSafety = SafetyAnnotations.getSafety(recordComponent, state); + Safety typeSymSafety = SafetyAnnotations.getSafety(recordComponent.type.tsym, state); + Safety recordComponentSafety = Safety.mergeAssumingUnknownIsSame(symbolSafety, typeSymSafety); + safety = safety.leastUpperBound(recordComponentSafety); } + return handleSafety(classTree, state, existingClassSafety, safety); + } + + private Description matchClassOrInterface(ClassTree classTree, ClassSymbol classSymbol, VisitorState state) { + Safety existingClassSafety = SafetyAnnotations.getSafety(classTree, state); + Safety safety = getTypeSafetyFromAncestors(classTree, state); boolean hasNonGetterMethod = false; boolean hasKnownGetter = false; for (Tree member : classTree.getMembers()) { if (member instanceof MethodTree) { MethodTree methodMember = (MethodTree) member; - if (NON_STATIC_METHOD_MATCHER.matches(methodMember, state)) { + if (NON_STATIC_NON_CTOR.matches(methodMember, state)) { if (GETTER_METHOD_MATCHER.matches(methodMember, state)) { Safety getterSafety = SafetyAnnotations.getSafety(methodMember.getReturnType(), state); if (getterSafety != Safety.UNKNOWN) { @@ -99,6 +133,14 @@ public Description matchClass(ClassTree classTree, VisitorState state) { return handleSafety(classTree, state, existingClassSafety, safety); } + private static Safety getTypeSafetyFromAncestors(ClassTree classTree, VisitorState state) { + Safety safety = SafetyAnnotations.getSafety(classTree.getExtendsClause(), state); + for (Tree implemented : classTree.getImplementsClause()) { + safety = safety.leastUpperBound(SafetyAnnotations.getSafety(implemented, state)); + } + return safety; + } + private Description handleSafety( ClassTree classTree, VisitorState state, Safety existingSafety, Safety computedSafety) { if (existingSafety != Safety.UNKNOWN && existingSafety.allowsValueWith(computedSafety)) { diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingPropagationTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingPropagationTest.java index 78b0ea7b0..ac0d14cf9 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingPropagationTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingPropagationTest.java @@ -124,6 +124,22 @@ void testDoesNotReplaceStrictAnnotation() { .doTest(); } + @Test + void testRecordWithUnsafeTypes() { + fix().addInputLines( + "Test.java", + "import com.palantir.tokens.auth.*;", + "import com.palantir.logsafe.*;", + "record Test(BearerToken token) {}") + .addOutputLines( + "Test.java", + "import com.palantir.tokens.auth.*;", + "import com.palantir.logsafe.*;", + "@DoNotLog", + "record Test(BearerToken token) {}") + .doTest(); + } + @Test void testDoesNotAddSafeAnnotation() { fix().addInputLines( From 5af3459d69a92422ec5cbfb218e60c3d376e81ef Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 25 Apr 2022 16:05:24 -0400 Subject: [PATCH 6/8] retain --- baseline-error-prone/build.gradle | 1 + 1 file changed, 1 insertion(+) diff --git a/baseline-error-prone/build.gradle b/baseline-error-prone/build.gradle index ab3f194cf..41d8c5f97 100644 --- a/baseline-error-prone/build.gradle +++ b/baseline-error-prone/build.gradle @@ -61,6 +61,7 @@ moduleJvmArgs { 'jdk.compiler/com.sun.tools.javac.file', 'jdk.compiler/com.sun.tools.javac.code', 'jdk.compiler/com.sun.tools.javac.util', + 'jdk.compiler/com.sun.tools.javac.comp', 'jdk.compiler/com.sun.tools.javac.main', 'jdk.compiler/com.sun.tools.javac.model', 'jdk.compiler/com.sun.tools.javac.parser', From 2438eea0f70befb7892090913ed6d84c2e1d3601 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 25 Apr 2022 16:23:57 -0400 Subject: [PATCH 7/8] test records better --- baseline-error-prone/build.gradle | 5 ----- .../baseline/errorprone/SafeLoggingPropagationTest.java | 7 ++++--- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/baseline-error-prone/build.gradle b/baseline-error-prone/build.gradle index 41d8c5f97..688182976 100644 --- a/baseline-error-prone/build.gradle +++ b/baseline-error-prone/build.gradle @@ -70,9 +70,4 @@ moduleJvmArgs { 'jdk.compiler/com.sun.tools.javac.util' ] opens = ['jdk.compiler/com.sun.tools.javac.comp'] -} - -javaVersions { - libraryTarget = 11 - runtime = 17 } \ No newline at end of file diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingPropagationTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingPropagationTest.java index ac0d14cf9..53476d5b9 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingPropagationTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingPropagationTest.java @@ -126,7 +126,8 @@ void testDoesNotReplaceStrictAnnotation() { @Test void testRecordWithUnsafeTypes() { - fix().addInputLines( + fix("--release", "15", "--enable-preview") + .addInputLines( "Test.java", "import com.palantir.tokens.auth.*;", "import com.palantir.logsafe.*;", @@ -268,7 +269,7 @@ void testIgnoresAnonymous() { .doTest(); } - private RefactoringValidator fix() { - return RefactoringValidator.of(SafeLoggingPropagation.class, getClass()); + private RefactoringValidator fix(String... args) { + return RefactoringValidator.of(SafeLoggingPropagation.class, getClass(), args); } } From 891dadc6bdb52e6ed50d3d8e3114aed053ece877 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 25 Apr 2022 16:52:25 -0400 Subject: [PATCH 8/8] only immutables for now --- .../errorprone/SafeLoggingPropagation.java | 29 +++++++------------ .../SafeLoggingPropagationTest.java | 26 +++++++++++++++++ 2 files changed, 37 insertions(+), 18 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingPropagation.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingPropagation.java index 057e165ca..99e335dd4 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingPropagation.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingPropagation.java @@ -55,7 +55,9 @@ public final class SafeLoggingPropagation extends BugChecker implements BugCheck private static final Matcher NON_STATIC_NON_CTOR = Matchers.not(Matchers.anyOf(Matchers.hasModifier(Modifier.STATIC), Matchers.methodIsConstructor())); private static final Matcher GETTER_METHOD_MATCHER = Matchers.allOf( - Matchers.not(Matchers.methodReturns(Matchers.isVoidType())), Matchers.methodHasNoParameters()); + NON_STATIC_NON_CTOR, + Matchers.not(Matchers.methodReturns(Matchers.isVoidType())), + Matchers.methodHasNoParameters()); @Override public Description matchClass(ClassTree classTree, VisitorState state) { @@ -99,23 +101,21 @@ private Description matchRecord(ClassTree classTree, ClassSymbol classSymbol, Vi } private Description matchClassOrInterface(ClassTree classTree, ClassSymbol classSymbol, VisitorState state) { + if (!ASTHelpers.hasAnnotation(classSymbol, "org.immutables.value.Value.Immutable", state)) { + return Description.NO_MATCH; + } Safety existingClassSafety = SafetyAnnotations.getSafety(classTree, state); Safety safety = getTypeSafetyFromAncestors(classTree, state); - boolean hasNonGetterMethod = false; boolean hasKnownGetter = false; for (Tree member : classTree.getMembers()) { if (member instanceof MethodTree) { MethodTree methodMember = (MethodTree) member; - if (NON_STATIC_NON_CTOR.matches(methodMember, state)) { - if (GETTER_METHOD_MATCHER.matches(methodMember, state)) { - Safety getterSafety = SafetyAnnotations.getSafety(methodMember.getReturnType(), state); - if (getterSafety != Safety.UNKNOWN) { - hasKnownGetter = true; - } - safety = safety.leastUpperBound(getterSafety); - } else { - hasNonGetterMethod = true; + if (GETTER_METHOD_MATCHER.matches(methodMember, state)) { + Safety getterSafety = SafetyAnnotations.getSafety(methodMember.getReturnType(), state); + if (getterSafety != Safety.UNKNOWN) { + hasKnownGetter = true; } + safety = safety.leastUpperBound(getterSafety); } } } @@ -123,13 +123,6 @@ private Description matchClassOrInterface(ClassTree classTree, ClassSymbol class if (!hasKnownGetter) { return Description.NO_MATCH; } - // non-getter methods are avoided such that we don't annotate non-value types unless we have additional - // data to suggest this is a value (immutables annotations). - if (hasNonGetterMethod - && !ASTHelpers.hasAnnotation(classSymbol, "org.immutables.value.Value.Immutable", state)) { - return Description.NO_MATCH; - } - return handleSafety(classTree, state, existingClassSafety, safety); } diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingPropagationTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingPropagationTest.java index 53476d5b9..c179e9a0f 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingPropagationTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingPropagationTest.java @@ -26,6 +26,8 @@ void testAddsAnnotation_dnlType() { "Test.java", "import com.palantir.tokens.auth.*;", "import com.palantir.logsafe.*;", + "import org.immutables.value.Value;", + "@Value.Immutable", "interface Test {", " BearerToken token();", "}") @@ -33,7 +35,9 @@ void testAddsAnnotation_dnlType() { "Test.java", "import com.palantir.tokens.auth.*;", "import com.palantir.logsafe.*;", + "import org.immutables.value.Value;", "@DoNotLog", + "@Value.Immutable", "interface Test {", " BearerToken token();", "}") @@ -46,6 +50,8 @@ void testMixedSafety() { "Test.java", "import com.palantir.tokens.auth.*;", "import com.palantir.logsafe.*;", + "import org.immutables.value.Value;", + "@Value.Immutable", "interface Test {", " @Safe String one();", " @Unsafe String two();", @@ -55,7 +61,9 @@ void testMixedSafety() { "Test.java", "import com.palantir.tokens.auth.*;", "import com.palantir.logsafe.*;", + "import org.immutables.value.Value;", "@Unsafe", + "@Value.Immutable", "interface Test {", " @Safe String one();", " @Unsafe String two();", @@ -70,6 +78,8 @@ void testAddsAnnotation_dnlReturnValue() { "Test.java", "import com.palantir.tokens.auth.*;", "import com.palantir.logsafe.*;", + "import org.immutables.value.Value;", + "@Value.Immutable", "interface Test {", " @DoNotLog", " String token();", @@ -78,7 +88,9 @@ void testAddsAnnotation_dnlReturnValue() { "Test.java", "import com.palantir.tokens.auth.*;", "import com.palantir.logsafe.*;", + "import org.immutables.value.Value;", "@DoNotLog", + "@Value.Immutable", "interface Test {", " @DoNotLog", " String token();", @@ -92,7 +104,9 @@ void testReplacesAnnotation_dnlReturnValue() { "Test.java", "import com.palantir.tokens.auth.*;", "import com.palantir.logsafe.*;", + "import org.immutables.value.Value;", "@Unsafe", + "@Value.Immutable", "interface Test {", " @DoNotLog", " String token();", @@ -101,7 +115,9 @@ void testReplacesAnnotation_dnlReturnValue() { "Test.java", "import com.palantir.tokens.auth.*;", "import com.palantir.logsafe.*;", + "import org.immutables.value.Value;", "@DoNotLog", + "@Value.Immutable", "interface Test {", " @DoNotLog", " String token();", @@ -115,6 +131,8 @@ void testDoesNotReplaceStrictAnnotation() { "Test.java", "import com.palantir.tokens.auth.*;", "import com.palantir.logsafe.*;", + "import org.immutables.value.Value;", + "@Value.Immutable", "@DoNotLog", "interface Test {", " @Unsafe", @@ -146,6 +164,8 @@ void testDoesNotAddSafeAnnotation() { fix().addInputLines( "Test.java", "import com.palantir.logsafe.*;", + "import org.immutables.value.Value;", + "@Value.Immutable", "interface Test {", " @Safe", " String token();", @@ -159,6 +179,8 @@ void testIgnoresStaticMethods() { fix().addInputLines( "Test.java", "import com.palantir.logsafe.*;", + "import org.immutables.value.Value;", + "@Value.Immutable", "interface Test {", " @Safe", " static String token() { return \"\"; }", @@ -172,6 +194,8 @@ void testIgnoresVoidMethods() { fix().addInputLines( "Test.java", "import com.palantir.logsafe.*;", + "import org.immutables.value.Value;", + "@Value.Immutable", "interface Test {", " @DoNotLog", " void token();", @@ -185,6 +209,8 @@ void testIgnoresMethodsWithParameters() { fix().addInputLines( "Test.java", "import com.palantir.logsafe.*;", + "import org.immutables.value.Value;", + "@Value.Immutable", "interface Test {", " @DoNotLog", " String token(int i);",