diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java index e0efa2450..7f132b8f0 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java @@ -49,6 +49,13 @@ public final class SafetyAnnotations { public static final String UNSAFE = "com.palantir.logsafe.Unsafe"; public static final String DO_NOT_LOG = "com.palantir.logsafe.DoNotLog"; + private static final com.google.errorprone.suppliers.Supplier safeName = + VisitorState.memoize(state -> state.getName(SAFE)); + private static final com.google.errorprone.suppliers.Supplier unsafeName = + VisitorState.memoize(state -> state.getName(UNSAFE)); + private static final com.google.errorprone.suppliers.Supplier doNotLogName = + VisitorState.memoize(state -> state.getName(DO_NOT_LOG)); + private static final com.google.errorprone.suppliers.Supplier throwableSupplier = Suppliers.typeFromClass(Throwable.class); @@ -83,14 +90,9 @@ public static Safety getSafety(Tree tree, VisitorState state) { public static Safety getSafety(@Nullable Symbol symbol, VisitorState state) { if (symbol != null) { - if (ASTHelpers.hasAnnotation(symbol, DO_NOT_LOG, state)) { - return Safety.DO_NOT_LOG; - } - if (ASTHelpers.hasAnnotation(symbol, UNSAFE, state)) { - return Safety.UNSAFE; - } - if (ASTHelpers.hasAnnotation(symbol, SAFE, state)) { - return Safety.SAFE; + Safety direct = getDirectSafety(symbol, state); + if (direct != Safety.UNKNOWN) { + return direct; } // Check super-methods if (symbol instanceof MethodSymbol) { @@ -105,7 +107,7 @@ public static Safety getSafety(@Nullable Symbol symbol, VisitorState state) { } if (symbol instanceof ClassSymbol) { ClassSymbol classSymbol = (ClassSymbol) symbol; - Safety safety = Safety.UNKNOWN; + Safety safety = getSafety(classSymbol.getSuperclass().tsym, state); for (Type type : classSymbol.getInterfaces()) { safety = Safety.mergeAssumingUnknownIsSame(safety, getSafety(type.tsym, state)); } @@ -122,6 +124,30 @@ public static Safety getSafety(@Nullable Type type, VisitorState state) { return Safety.UNKNOWN; } + public static Safety getDirectSafety(@Nullable Symbol symbol, VisitorState state) { + if (symbol != null) { + if (containsAttributeNamed(symbol, doNotLogName.get(state))) { + return Safety.DO_NOT_LOG; + } + if (containsAttributeNamed(symbol, unsafeName.get(state))) { + return Safety.UNSAFE; + } + if (containsAttributeNamed(symbol, safeName.get(state))) { + return Safety.SAFE; + } + } + return Safety.UNKNOWN; + } + + private static boolean containsAttributeNamed(Symbol symbol, Name annotationName) { + for (Attribute.Compound compound : symbol.getRawAttributes()) { + if (compound.type.tsym.getQualifiedName().equals(annotationName)) { + return true; + } + } + return false; + } + private static Safety getTypeVariableSymbolSafety(TypeVariableSymbol typeVariableSymbol) { for (int i = 0; i < typeVariableSymbol.owner.getTypeParameters().size(); i++) { SymbolMetadata metadata = typeVariableSymbol.owner.getMetadata(); diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java index d27bed94b..6e040a96f 100644 --- a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java @@ -1676,6 +1676,66 @@ public void testResultOfInvocationSuperInterfaceAnnotated() { .doTest(); } + @Test + public void testSuperClassInterfaceAnnotated() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test {", + " @Unsafe", + " interface Iface {", + " }", + " static class Sup implements Iface {}", + " static class Sub extends Sup {}", + " void f(Sub value) {", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE'", + " fun(value);", + " }", + " private static void fun(@Safe Object value) {}", + "}") + .doTest(); + } + + @Test + public void testMultipleInterfacesDifferentSafety() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test {", + " @Unsafe interface UnsafeIface {}", + " @Safe interface SafeIface {}", + " @DoNotLog interface DnlIface {}", + " static class One implements SafeIface, UnsafeIface {}", + " static class Two implements SafeIface, DnlIface, UnsafeIface {}", + " void f(One one, Two two) {", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE'", + " fun(one);", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'DO_NOT_LOG'", + " fun(two);", + " }", + " private static void fun(@Safe Object value) {}", + "}") + .doTest(); + } + + @Test + public void testSuperclassLessStrictThanInterfaces() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test {", + " @DoNotLog interface DnlIface {}", + " @Safe static class Sup {}", + " static class Impl extends Sup implements DnlIface {}", + " void f(Impl value) {", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'DO_NOT_LOG'", + " fun(value);", + " }", + " private static void fun(@Safe Object value) {}", + "}") + .doTest(); + } + private CompilationTestHelper helper() { return CompilationTestHelper.newInstance(IllegalSafeLoggingArgument.class, getClass()); } diff --git a/changelog/@unreleased/pr-2271.v2.yml b/changelog/@unreleased/pr-2271.v2.yml new file mode 100644 index 000000000..64237387e --- /dev/null +++ b/changelog/@unreleased/pr-2271.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: Safety analysis detects annotations on superclasses and their interfaces + links: + - https://github.com/palantir/gradle-baseline/pull/2271