diff --git a/baseline-error-prone/build.gradle b/baseline-error-prone/build.gradle index 33c61aa64..f91faeb35 100644 --- a/baseline-error-prone/build.gradle +++ b/baseline-error-prone/build.gradle @@ -3,6 +3,11 @@ import net.ltgt.gradle.errorprone.CheckSeverity apply plugin: 'java-library' apply plugin: 'com.palantir.external-publish-jar' +javaVersions { + libraryTarget = 11 + runtime = 17 +} + dependencies { implementation 'com.google.errorprone:error_prone_core' // Ensure a new enough version of dataflow-errorprone is available 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 cf618dc99..b398ffd83 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 @@ -840,10 +840,13 @@ public TransferResult> visitLocalVariable( // 1. catch (SomeThrowable t) // 2. referencing a captured local variable within a lambda // 3. referencing a captured local variable within an anonymous class + // 4. Pattern matching (input instanceof String str) // Cast a wide net for all throwables (covers catch statements) if (THROWABLE_SUBTYPE.matches(node.getTree(), state)) { safety = Safety.UNSAFE.leastUpperBound(SafetyAnnotations.getSafety(node.getTree(), state)); + } else if (isPatternBinding(node, state)) { + safety = SafetyAnnotations.getSafety(node.getTree(), state); } else { // No safety information found, likely a captured reference used within a lambda or anonymous class. safety = getCapturedLocalVariableSafety(node); @@ -852,6 +855,26 @@ public TransferResult> visitLocalVariable( return noStoreChanges(safety, input); } + private static boolean isPatternBinding(LocalVariableNode node, VisitorState state) { + TreePath varPath = TreePath.getPath(state.getPath().getCompilationUnit(), node.getTree()); + if (varPath == null) { + // Synthetic expression + return false; + } + TreePath parentPath = varPath.getParentPath(); + if (parentPath == null) { + return false; + } + Tree enclosing = parentPath.getLeaf(); + if (enclosing == null) { + return false; + } + // JCBindingPattern is newer than our compilation target, so we match strings to avoid + // complex build-system configurations. + String kindString = enclosing.getKind().name(); + return "BINDING_PATTERN".equals(kindString); + } + private Safety getCapturedLocalVariableSafety(LocalVariableNode node) { Symbol symbol = ASTHelpers.getSymbol(node.getTree()); if (!(symbol instanceof VarSymbol)) { @@ -1084,6 +1107,11 @@ public TransferResult> visitTypeCast( /** Handles type changes (widen, narrow, and cast). */ private TransferResult> handleTypeConversion( Tree newType, Node original, TransferInput> input) { + return noStoreChanges(getTypeConversionSafety(newType, original, input), input); + } + + private Safety getTypeConversionSafety( + Tree newType, Node original, TransferInput> input) { Safety inputSafety = getValueOfSubNode(input, original); Safety targetSafety = SafetyAnnotations.getSafety(newType, state); Safety resultSafety; @@ -1098,13 +1126,20 @@ private TransferResult> handleTypeConversion( } else { resultSafety = Safety.mergeAssumingUnknownIsSame(inputSafety, targetSafety); } - return noStoreChanges(resultSafety, input); + return resultSafety; } @Override public TransferResult> visitInstanceOf( InstanceOfNode node, TransferInput> input) { - // types themselves are generally safe, boolean results of type checks are always safe. + LocalVariableNode bindingVariable = node.getBindingVariable(); + if (bindingVariable != null) { + Safety safety = getTypeConversionSafety(node.getTree().getType(), node.getOperand(), input); + ReadableUpdates updates = new ReadableUpdates(); + updates.set(bindingVariable, safety); + return updateRegularStore(Safety.SAFE, input, updates); + } + // Otherwise types themselves are generally safe, boolean results of type checks are always safe. return noStoreChanges(Safety.SAFE, 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 5a05154b6..e81933ae7 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 @@ -18,6 +18,7 @@ import com.google.errorprone.CompilationTestHelper; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.Timeout; class IllegalSafeLoggingArgumentTest { @@ -1594,7 +1595,7 @@ public void testTypeVariablesInConstructor() { @Test public void testRecordConstruction() { - helper().setArgs("--release", "15", "--enable-preview") + helper().setArgs("--release", "17") .addSourceLines( "Test.java", "import com.palantir.logsafe.*;", @@ -1610,7 +1611,7 @@ public void testRecordConstruction() { @Test public void testRecordComponentUsage() { - helper().setArgs("--release", "15", "--enable-preview") + helper().setArgs("--release", "17") .addSourceLines( "Test.java", "import com.palantir.logsafe.*;", @@ -1775,6 +1776,77 @@ public void testSubclassWithLenientSafety() { .doTest(); } + @Test + @Timeout(10) // https://github.com/palantir/gradle-baseline/issues/2328 + public void testPatternMatching() { + helper().setArgs("--release", "17") + .addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test {", + " @DoNotLog static class DoNotLogClass {}", + " @Unsafe Object f(Object value) {", + " if (value instanceof Integer i) {", + " return i;", + " } else if (value instanceof Float f) {", + " return f;", + " } else if (value instanceof Double d) {", + " return d;", + " } else if (value instanceof Byte b) {", + " return b;", + " } else if (value instanceof CharSequence cs) {", + " return cs;", + " } else if (value instanceof String str) {", + " return str;", + " } else if (value instanceof Throwable t) {", + " return t;", + " } else if (value instanceof DoNotLogClass dnl) {", + " // BUG: Diagnostic contains: Dangerous return value: result is 'DO_NOT_LOG'", + " return dnl;", + " }", + " return null;", + " }", + "}") + .doTest(); + } + + @Test + public void testPatternMatchingUnsafeToSafeSubtype() { + helper().setArgs("--release", "17") + .addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test {", + " @Safe static class SafeClass {}", + " @Safe Object f(@Unsafe Object value) {", + " if (value instanceof SafeClass s) {", + " return s;", + " }", + " return null;", + " }", + "}") + .doTest(); + } + + @Test + public void testPatternMatchingUnsafeToUnknown() { + helper().setArgs("--release", "17") + .addSourceLines( + "Test.java", + "import com.palantir.logsafe.*;", + "class Test {", + " @Safe static class SafeClass {}", + " @Safe Object f(@Unsafe Object value) {", + " if (value instanceof String s) {", + " // BUG: Diagnostic contains: Dangerous return value: result is 'UNSAFE'", + " return s;", + " }", + " return null;", + " }", + "}") + .doTest(); + } + private CompilationTestHelper helper() { return CompilationTestHelper.newInstance(IllegalSafeLoggingArgument.class, getClass()); }