From 171d7593b93a2000e95f79db81bb05d860c09640 Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 6 Jun 2022 12:00:10 -0400 Subject: [PATCH 1/3] Support subclass safety overrides --- .../baseline/errorprone/MoreASTHelpers.java | 10 ++++++- .../safety/SafetyPropagationTransfer.java | 18 ++++++++++--- .../IllegalSafeLoggingArgumentTest.java | 26 +++++++++++++++++++ 3 files changed, 50 insertions(+), 4 deletions(-) diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/MoreASTHelpers.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/MoreASTHelpers.java index 2880ba603..e9e4feb99 100644 --- a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/MoreASTHelpers.java +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/MoreASTHelpers.java @@ -22,6 +22,8 @@ import com.google.errorprone.fixes.SuggestedFixes; import com.google.errorprone.util.ASTHelpers; import com.sun.source.tree.CatchTree; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.Tree; import com.sun.source.tree.TryTree; import com.sun.tools.javac.code.Symbol; import com.sun.tools.javac.code.Type; @@ -32,7 +34,7 @@ /** Utility functionality that does not exist in {@link com.google.errorprone.util.ASTHelpers}. */ @SuppressWarnings("checkstyle:AbbreviationAsWordInName") -final class MoreASTHelpers { +public final class MoreASTHelpers { /** Removes any type that is a subtype of another type in the set. */ @SuppressWarnings("ReferenceEquality") @@ -115,5 +117,11 @@ static ImmutableList expandUnion(@Nullable Type type) { return ImmutableList.of(type); } + public static Type getResultType(Tree tree) { + return tree instanceof ExpressionTree + ? ASTHelpers.getResultType((ExpressionTree) tree) + : ASTHelpers.getType(tree); + } + private MoreASTHelpers() {} } 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 ff0f2b350..cf618dc99 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 @@ -33,6 +33,7 @@ import com.google.errorprone.matchers.Matchers; import com.google.errorprone.matchers.method.MethodMatchers; import com.google.errorprone.util.ASTHelpers; +import com.palantir.baseline.errorprone.MoreASTHelpers; import com.sun.source.tree.ClassTree; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.LambdaExpressionTree; @@ -1083,9 +1084,20 @@ public TransferResult> visitTypeCast( /** Handles type changes (widen, narrow, and cast). */ private TransferResult> handleTypeConversion( Tree newType, Node original, TransferInput> input) { - Safety valueSafety = getValueOfSubNode(input, original); - Safety narrowTargetSafety = SafetyAnnotations.getSafety(newType, state); - Safety resultSafety = Safety.mergeAssumingUnknownIsSame(valueSafety, narrowTargetSafety); + Safety inputSafety = getValueOfSubNode(input, original); + Safety targetSafety = SafetyAnnotations.getSafety(newType, state); + Safety resultSafety; + if (!targetSafety.allowsValueWith(inputSafety) + && ASTHelpers.isSubtype( + MoreASTHelpers.getResultType(newType), + MoreASTHelpers.getResultType(original.getTree()), + state)) { + // In the case that the cast target is more specific than the input, propagate the safety of the cast + // target. Validation occurs when the type itself is constructed, the cast exposes previous validation. + resultSafety = targetSafety; + } else { + resultSafety = Safety.mergeAssumingUnknownIsSame(inputSafety, targetSafety); + } return noStoreChanges(resultSafety, input); } 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 6e040a96f..4c398fb0d 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 @@ -1736,6 +1736,32 @@ public void testSuperclassLessStrictThanInterfaces() { .doTest(); } + @Test + public void testCastSafety() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test {", + " @DoNotLog class DoNotLogClass {}", + " @Safe interface SafeClass {}", + " @Unsafe interface UnsafeClass {}", + " @Safe interface SafeSubclass extends UnsafeClass {}", + " void f(Object object, UnsafeClass unsafeObject) {", + " fun(object);", + " fun((SafeClass) object);", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE'", + " fun((UnsafeClass) object);", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'DO_NOT_LOG'", + " fun((DoNotLogClass) object);", + " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE'", + " fun((SafeClass) unsafeObject);", + " fun((SafeSubclass) unsafeObject);", + " }", + " private static void fun(@Safe Object obj) {}", + "}") + .doTest(); + } + private CompilationTestHelper helper() { return CompilationTestHelper.newInstance(IllegalSafeLoggingArgument.class, getClass()); } From f0423c2d944f04bc66c9d48cf063ddcc4f57872c Mon Sep 17 00:00:00 2001 From: svc-changelog Date: Mon, 6 Jun 2022 16:03:28 +0000 Subject: [PATCH 2/3] Add generated changelog entries --- changelog/@unreleased/pr-2289.v2.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelog/@unreleased/pr-2289.v2.yml diff --git a/changelog/@unreleased/pr-2289.v2.yml b/changelog/@unreleased/pr-2289.v2.yml new file mode 100644 index 000000000..a51fe85ef --- /dev/null +++ b/changelog/@unreleased/pr-2289.v2.yml @@ -0,0 +1,6 @@ +type: improvement +improvement: + description: Trust type safety on cast results, based on validation that occurred + when the type was created. + links: + - https://github.com/palantir/gradle-baseline/pull/2289 From 165dc119d200a4d88dea34564395b7966952ddab Mon Sep 17 00:00:00 2001 From: Carter Kozak Date: Mon, 6 Jun 2022 12:43:35 -0400 Subject: [PATCH 3/3] Enforce supertype agreement --- .../IllegalSafeLoggingArgument.java | 21 ++++++++++++++++++- .../errorprone/SafeLoggingPropagation.java | 12 ++--------- .../errorprone/safety/SafetyAnnotations.java | 9 ++++++++ .../IllegalSafeLoggingArgumentTest.java | 18 +++++++++++++--- 4 files changed, 46 insertions(+), 14 deletions(-) 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 c3f9506b5..d68671954 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 @@ -30,6 +30,7 @@ import com.palantir.baseline.errorprone.safety.SafetyAnalysis; import com.palantir.baseline.errorprone.safety.SafetyAnnotations; import com.sun.source.tree.AssignmentTree; +import com.sun.source.tree.ClassTree; import com.sun.source.tree.CompoundAssignmentTree; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.MethodInvocationTree; @@ -72,7 +73,8 @@ public final class IllegalSafeLoggingArgument extends BugChecker BugChecker.CompoundAssignmentTreeMatcher, BugChecker.MethodTreeMatcher, BugChecker.VariableTreeMatcher, - BugChecker.NewClassTreeMatcher { + BugChecker.NewClassTreeMatcher, + BugChecker.ClassTreeMatcher { private static final String UNSAFE_ARG = "com.palantir.logsafe.UnsafeArg"; private static final Matcher SAFE_ARG_OF_METHOD_MATCHER = MethodMatchers.staticMethod() @@ -339,4 +341,21 @@ public Description matchVariable(VariableTree tree, VisitorState state) { variableTypeSafety, parameterSafetyAnnotation)) .build(); } + + @Override + public Description matchClass(ClassTree tree, VisitorState state) { + Safety directSafety = SafetyAnnotations.getDirectSafety(ASTHelpers.getSymbol(tree), state); + if (directSafety.allowsAll()) { + return Description.NO_MATCH; + } + Safety ancestorSafety = SafetyAnnotations.getTypeSafetyFromAncestors(tree, state); + if (directSafety.allowsValueWith(ancestorSafety)) { + return Description.NO_MATCH; + } + return buildDescription(tree) + .setMessage(String.format( + "Dangerous type: annotated '%s' but ancestors declare '%s'.", + directSafety, SafetyAnnotations.getSafety(tree, state))) + .build(); + } } 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 62f97c0a4..2a41a8dcb 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 @@ -183,7 +183,7 @@ private static List getRecordComponents(ClassSymbol classSymbol) { private Description matchRecord(ClassTree classTree, ClassSymbol classSymbol, VisitorState state) { Safety existingClassSafety = SafetyAnnotations.getSafety(classTree, state); - Safety safety = getTypeSafetyFromAncestors(classTree, state); + Safety safety = SafetyAnnotations.getTypeSafetyFromAncestors(classTree, state); for (VarSymbol recordComponent : getRecordComponents(classSymbol)) { Safety symbolSafety = SafetyAnnotations.getSafety(recordComponent, state); Safety typeSymSafety = SafetyAnnotations.getSafety(recordComponent.type.tsym, state); @@ -203,7 +203,7 @@ private Description matchClassOrInterface(ClassTree classTree, ClassSymbol class @SuppressWarnings("checkstyle:CyclomaticComplexity") private Description matchImmutables(ClassTree classTree, ClassSymbol classSymbol, VisitorState state) { Safety existingClassSafety = SafetyAnnotations.getSafety(classTree, state); - Safety safety = getTypeSafetyFromAncestors(classTree, state); + Safety safety = SafetyAnnotations.getTypeSafetyFromAncestors(classTree, state); boolean hasKnownGetter = false; boolean isJson = hasJacksonAnnotation(classSymbol, state); for (Tree member : classTree.getMembers()) { @@ -251,14 +251,6 @@ private Description matchBasedOnToString(ClassTree classTree, ClassSymbol classS return handleSafety(classTree, classTree.getModifiers(), state, existingClassSafety, symbolSafety); } - 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( Tree tree, ModifiersTree treeModifiers, VisitorState state, Safety existingSafety, Safety computedSafety) { if (existingSafety != Safety.UNKNOWN && existingSafety.allowsValueWith(computedSafety)) { 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 7f132b8f0..83dec84f6 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 @@ -20,6 +20,7 @@ import com.google.errorprone.VisitorState; import com.google.errorprone.suppliers.Suppliers; import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ClassTree; import com.sun.source.tree.ExpressionTree; import com.sun.source.tree.Tree; import com.sun.tools.javac.code.Attribute; @@ -124,6 +125,14 @@ public static Safety getSafety(@Nullable Type type, VisitorState state) { return Safety.UNKNOWN; } + public 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; + } + public static Safety getDirectSafety(@Nullable Symbol symbol, VisitorState state) { if (symbol != null) { if (containsAttributeNamed(symbol, doNotLogName.get(state))) { 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 4c398fb0d..3943393e4 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 @@ -1745,8 +1745,7 @@ public void testCastSafety() { " @DoNotLog class DoNotLogClass {}", " @Safe interface SafeClass {}", " @Unsafe interface UnsafeClass {}", - " @Safe interface SafeSubclass extends UnsafeClass {}", - " void f(Object object, UnsafeClass unsafeObject) {", + " void f(Object object, UnsafeClass unsafeObject, @Unsafe Object unsafeAnnotatedObject) {", " fun(object);", " fun((SafeClass) object);", " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE'", @@ -1755,13 +1754,26 @@ public void testCastSafety() { " fun((DoNotLogClass) object);", " // BUG: Diagnostic contains: Dangerous argument value: arg is 'UNSAFE'", " fun((SafeClass) unsafeObject);", - " fun((SafeSubclass) unsafeObject);", + " fun((SafeClass) unsafeAnnotatedObject);", " }", " private static void fun(@Safe Object obj) {}", "}") .doTest(); } + @Test + public void testSubclassWithLenientSafety() { + helper().addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test {", + " @Unsafe interface UnsafeClass {}", + " // BUG: Diagnostic contains: Dangerous type: annotated 'SAFE' but ancestors declare 'SAFE'.", + " @Safe interface SafeSubclass extends UnsafeClass {}", + "}") + .doTest(); + } + private CompilationTestHelper helper() { return CompilationTestHelper.newInstance(IllegalSafeLoggingArgument.class, getClass()); }