Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Safety analysis detects annotations on superclasses and their interfaces #2271

Merged
merged 3 commits into from
May 13, 2022
Merged

Safety analysis detects annotations on superclasses and their interfaces #2271

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
af8d912
Safety analysis detects annotations on superclasses and their interfaces
carterkozak May 13, 2022
ad0f4f6
better
carterkozak May 13, 2022
ab04a4f
Add generated changelog entries
svc-changelog May 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 35 additions & 9 deletions ...-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyAnnotations.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Name> safeName =
VisitorState.memoize(state -> state.getName(SAFE));
private static final com.google.errorprone.suppliers.Supplier<Name> unsafeName =
VisitorState.memoize(state -> state.getName(UNSAFE));
private static final com.google.errorprone.suppliers.Supplier<Name> doNotLogName =
VisitorState.memoize(state -> state.getName(DO_NOT_LOG));

private static final com.google.errorprone.suppliers.Supplier<Type> throwableSupplier =
Suppliers.typeFromClass(Throwable.class);

Expand Down Expand Up @@ -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) {
Expand All @@ -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));
}
Expand All @@ -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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extracted to avoid duplicating effort between ASTHelpers.hasAnnotation and our annotation search. ASTHelpers.hasAnnotation searches supertypes, but not their interfaces, and there's no sense doing it twice.

I took the opportunity to use an error-prone supplier for names, which should reduce overhead a bit.

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();
Expand Down
60 changes: 60 additions & 0 deletions ...-prone/src/test/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgumentTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand Down
5 changes: 5 additions & 0 deletions changelog/@unreleased/pr-2271.v2.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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