diff --git a/baseline-error-prone/build.gradle b/baseline-error-prone/build.gradle index a0503ad66..688182976 100644 --- a/baseline-error-prone/build.gradle +++ b/baseline-error-prone/build.gradle @@ -69,4 +69,5 @@ moduleJvmArgs { 'jdk.compiler/com.sun.tools.javac.tree', 'jdk.compiler/com.sun.tools.javac.util' ] -} + opens = ['jdk.compiler/com.sun.tools.javac.comp'] +} \ 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 9276dcd6f..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 @@ -25,12 +25,16 @@ 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 com.sun.tools.javac.code.Symbol.VarSymbol; +import java.util.List; import javax.lang.model.element.Modifier; @AutoService(BugChecker.class) @@ -48,29 +52,88 @@ public final class SafeLoggingPropagation extends BugChecker implements BugCheck Matchers.isSameType(SafetyAnnotations.UNSAFE), Matchers.isSameType(SafetyAnnotations.DO_NOT_LOG)); + 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.hasModifier(Modifier.STATIC)), + NON_STATIC_NON_CTOR, Matchers.not(Matchers.methodReturns(Matchers.isVoidType())), Matchers.methodHasNoParameters()); @Override 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) { + 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 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)); + Safety getterSafety = SafetyAnnotations.getSafety(methodMember.getReturnType(), state); + if (getterSafety != Safety.UNKNOWN) { + hasKnownGetter = true; + } + safety = safety.leastUpperBound(getterSafety); } } } + // If no getter-style methods are detected, assume this is not a value type. + if (!hasKnownGetter) { + return Description.NO_MATCH; + } 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 ee157bcf4..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", @@ -124,11 +142,30 @@ void testDoesNotReplaceStrictAnnotation() { .doTest(); } + @Test + void testRecordWithUnsafeTypes() { + fix("--release", "15", "--enable-preview") + .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( "Test.java", "import com.palantir.logsafe.*;", + "import org.immutables.value.Value;", + "@Value.Immutable", "interface Test {", " @Safe", " String token();", @@ -142,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 \"\"; }", @@ -155,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();", @@ -168,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);", @@ -176,6 +219,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. @@ -191,7 +277,25 @@ void testIgnoresThrowable() { .doTest(); } - private RefactoringValidator fix() { - return RefactoringValidator.of(SafeLoggingPropagation.class, getClass()); + @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(String... args) { + return RefactoringValidator.of(SafeLoggingPropagation.class, getClass(), args); } } 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