diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java index b1a7e6155..3a432ad42 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/IllegalSafeLoggingArgument.java @@ -36,6 +36,8 @@ import com.sun.source.tree.MethodTree; import com.sun.source.tree.ReturnTree; import com.sun.source.tree.StatementTree; +import com.sun.source.tree.Tree; +import com.sun.source.tree.VariableTree; import com.sun.source.util.TreePath; import com.sun.tools.javac.code.Symbol.MethodSymbol; import com.sun.tools.javac.code.Symbol.VarSymbol; @@ -61,7 +63,9 @@ public final class IllegalSafeLoggingArgument extends BugChecker implements BugChecker.MethodInvocationTreeMatcher, BugChecker.ReturnTreeMatcher, BugChecker.AssignmentTreeMatcher, - BugChecker.CompoundAssignmentTreeMatcher { + BugChecker.CompoundAssignmentTreeMatcher, + BugChecker.MethodTreeMatcher, + BugChecker.VariableTreeMatcher { private static final String UNSAFE_ARG = "com.palantir.logsafe.UnsafeArg"; private static final Matcher SAFE_ARG_OF_METHOD_MATCHER = MethodMatchers.staticMethod() @@ -174,4 +178,42 @@ private Description handleAssignment( assignmentValue, variableDeclaredSafety)) .build(); } + + @Override + public Description matchMethod(MethodTree tree, VisitorState state) { + Tree returnType = tree.getReturnType(); + if (returnType == null) { + return Description.NO_MATCH; + } + Safety methodSafetyAnnotation = SafetyAnnotations.getSafety(ASTHelpers.getSymbol(tree), state); + if (methodSafetyAnnotation.allowsAll()) { + return Description.NO_MATCH; + } + Safety returnTypeSafety = SafetyAnnotations.getSafety(ASTHelpers.getSymbol(returnType), state); + if (methodSafetyAnnotation.allowsValueWith(returnTypeSafety)) { + return Description.NO_MATCH; + } + return buildDescription(returnType) + .setMessage(String.format( + "Dangerous return type: type is '%s' but the method is annotated '%s'.", + returnTypeSafety, methodSafetyAnnotation)) + .build(); + } + + @Override + public Description matchVariable(VariableTree tree, VisitorState state) { + Safety parameterSafetyAnnotation = SafetyAnnotations.getSafety(ASTHelpers.getSymbol(tree), state); + if (parameterSafetyAnnotation.allowsAll()) { + return Description.NO_MATCH; + } + Safety variableTypeSafety = SafetyAnnotations.getSafety(ASTHelpers.getSymbol(tree.getType()), state); + if (parameterSafetyAnnotation.allowsValueWith(variableTypeSafety)) { + return Description.NO_MATCH; + } + return buildDescription(tree) + .setMessage(String.format( + "Dangerous variable: type is '%s' but the variable is annotated '%s'.", + variableTypeSafety, parameterSafetyAnnotation)) + .build(); + } } diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java index 376e53cc6..d62cd5f56 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java @@ -52,6 +52,10 @@ import java.util.List; import java.util.Map; import java.util.Objects; +import java.util.Optional; +import java.util.OptionalDouble; +import java.util.OptionalInt; +import java.util.OptionalLong; import java.util.Set; import javax.annotation.Nullable; import javax.lang.model.element.VariableElement; @@ -173,9 +177,30 @@ public final class SafetyPropagationTransfer implements ForwardTransferFunction< .namedAnyOf("of", "copyOf"), MethodMatchers.staticMethod().onClass(Arrays.class.getName()).named("asList")); + private static final Matcher OPTIONAL_FACTORIES = Matchers.anyOf( + MethodMatchers.staticMethod().onClass(Optional.class.getName()).namedAnyOf("of", "ofNullable"), + MethodMatchers.staticMethod() + .onClassAny( + OptionalInt.class.getName(), OptionalLong.class.getName(), OptionalDouble.class.getName()) + .named("of")); + // These methods do not take the receiver (generally a static class) into account, only the inputs. private static final Matcher RETURNS_SAFETY_COMBINATION_OF_ARGS = - Matchers.anyOf(STRING_FORMAT, OBJECTS_TO_STRING, IMMUTABLE_COLLECTION_FACTORY); + Matchers.anyOf(STRING_FORMAT, OBJECTS_TO_STRING, IMMUTABLE_COLLECTION_FACTORY, OPTIONAL_FACTORIES); + + private static final Matcher OPTIONAL_ACCESSORS = Matchers.anyOf( + MethodMatchers.instanceMethod() + .onDescendantOf(Optional.class.getName()) + .namedAnyOf("filter", "get", "orElseThrow", "stream"), + MethodMatchers.instanceMethod() + .onDescendantOf(OptionalInt.class.getName()) + .namedAnyOf("getAsInt", "orElseThrow"), + MethodMatchers.instanceMethod() + .onDescendantOf(OptionalLong.class.getName()) + .namedAnyOf("getAsLong", "orElseThrow"), + MethodMatchers.instanceMethod() + .onDescendantOf(OptionalDouble.class.getName()) + .namedAnyOf("getAsDouble", "orElseThrow")); // Returns the safety of the receiver, e.g. myString.getBytes() returns the safety of myString. private static final Matcher RETURNS_SAFETY_OF_RECEIVER = Matchers.anyOf( @@ -190,7 +215,8 @@ public final class SafetyPropagationTransfer implements ForwardTransferFunction< .namedAnyOf("toArray", "stream", "parallelStream"), MethodMatchers.instanceMethod() .onDescendantOf(Iterable.class.getName()) - .namedAnyOf("toArray", "iterator", "spliterator")); + .namedAnyOf("toArray", "iterator", "spliterator"), + OPTIONAL_ACCESSORS); private static final Matcher RETURNS_SAFETY_OF_FIRST_ARG = Matchers.anyOf( MethodMatchers.staticMethod().onClass(Objects.class.getName()).named("requireNonNull"), 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 2c367c884..be355cb68 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 @@ -891,6 +891,67 @@ public void testFieldAnnotationAssignment() { .doTest(); } + @Test + public void disagreeingSafetyAnnotations() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test {", + " @DoNotLog static class DoNotLogClass {}", + " @Safe static class SafeClass {}", + " @Unsafe static class UnsafeClass {}", + " // BUG: Diagnostic contains: Dangerous return type: type is 'DO_NOT_LOG' " + + "but the method is annotated 'UNSAFE'.", + " @Unsafe DoNotLogClass f(", + " @Safe SafeClass superSafe,", + " // BUG: Diagnostic contains: Dangerous variable: type is 'UNSAFE' " + + "but the variable is annotated 'SAFE'.", + " @Safe UnsafeClass oops", + " ) {", + " return null;", + " }", + "}") + .doTest(); + } + + @Test + public void testOptionalUnwrapping() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "import java.util.*;", + "class Test {", + " void f(@Unsafe Optional p) {", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE' " + + "but the parameter requires 'SAFE'.", + " fun(p);", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE' " + + "but the parameter requires 'SAFE'.", + " fun(p.get());", + " }", + " void fun(@Safe Object in) {}", + "}") + .doTest(); + } + + @Test + public void testOptionalUnsafeType() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "import java.util.*;", + "class Test {", + " @Unsafe static class UnsafeClass {}", + " void f(Optional p) {", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE' " + + "but the parameter requires 'SAFE'.", + " fun(p.get());", + " }", + " void fun(@Safe Object in) {}", + "}") + .doTest(); + } + @Test public void testSafeArgOfUnsafe_recommendsUnsafeArgOf() { refactoringHelper() diff --git a/changelog/@unreleased/pr-2161.v2.yml b/changelog/@unreleased/pr-2161.v2.yml new file mode 100644 index 000000000..88bb2749c --- /dev/null +++ b/changelog/@unreleased/pr-2161.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: Validate that safety var annotations and type annotations agree + links: + - https://github.com/palantir/gradle-baseline/pull/2161