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

SafeLoggingPropagation doesn't attempt to annotate anonymous classes #2227

Merged
merged 8 commits into from
Apr 25, 2022
Merged

SafeLoggingPropagation doesn't attempt to annotate anonymous classes #2227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
39fde22
SafeLoggingPropagation doesn't attempt to annotate anonymous classes
carterkozak Apr 25, 2022
98680fc
Add generated changelog entries
svc-changelog Apr 25, 2022
7f976d7
better value type heuristic
carterkozak Apr 25, 2022
cda09a3
cyclo
carterkozak Apr 25, 2022
a0528c9
record support
carterkozak Apr 25, 2022
5af3459
retain
carterkozak Apr 25, 2022
2438eea
test records better
carterkozak Apr 25, 2022
891dadc
only immutables for now
carterkozak Apr 25, 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
3 changes: 2 additions & 1 deletion baseline-error-prone/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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']
}
73 changes: 68 additions & 5 deletions ...ne-error-prone/src/main/java/com/palantir/baseline/errorprone/SafeLoggingPropagation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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<MethodTree> NON_STATIC_NON_CTOR =
Matchers.not(Matchers.anyOf(Matchers.hasModifier(Modifier.STATIC), Matchers.methodIsConstructor()));
private static final Matcher<MethodTree> 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<VarSymbol> getRecordComponents(ClassSymbol classSymbol) {
// Can use classSymbol.getRecordComponents() in future versions
try {
return (List<VarSymbol>)
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)) {
Expand Down
108 changes: 106 additions & 2 deletions ...rror-prone/src/test/java/com/palantir/baseline/errorprone/SafeLoggingPropagationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,18 @@ 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();",
"}")
.addOutputLines(
"Test.java",
"import com.palantir.tokens.auth.*;",
"import com.palantir.logsafe.*;",
"import org.immutables.value.Value;",
"@DoNotLog",
"@Value.Immutable",
"interface Test {",
" BearerToken token();",
"}")
Expand All @@ -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();",
Expand All @@ -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();",
Expand All @@ -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();",
Expand All @@ -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();",
Expand All @@ -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();",
Expand All @@ -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();",
Expand All @@ -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",
Expand All @@ -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();",
Expand All @@ -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 \"\"; }",
Expand All @@ -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();",
Expand All @@ -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);",
Expand All @@ -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.
Expand All @@ -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<BearerToken>() {",
" @Override public BearerToken get() {",
" return BearerToken.valueOf(\"abcdefghijklmnopq\");",
" }",
" };",
"}")
.expectUnchanged()
.doTest();
}

private RefactoringValidator fix(String... args) {
return RefactoringValidator.of(SafeLoggingPropagation.class, getClass(), args);
}
}
5 changes: 5 additions & 0 deletions changelog/@unreleased/pr-2227.v2.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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