Skip to content

fix #2328 Handle safety flow through instanceof pattern matching #2331

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

Merged
bulldozer-bot merged 4 commits into from
Jul 26, 2022
Merged

fix #2328 Handle safety flow through instanceof pattern matching #2331

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3c3d093
fix #2328 Handle safety flow through instanceof pattern matching
Jul 25, 2022
34f5b68
Add generated changelog entries
svc-changelog Jul 25, 2022
e64fd84
no more preview flags
Jul 25, 2022
f5ef87e
fix
Jul 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
5 changes: 5 additions & 0 deletions baseline-error-prone/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@ import net.ltgt.gradle.errorprone.CheckSeverity
apply plugin: 'java-library'
apply plugin: 'com.palantir.external-publish-jar'

javaVersion {
target = 11
runtime = 17
}

dependencies {
implementation 'com.google.errorprone:error_prone_core'
// Ensure a new enough version of dataflow-errorprone is available
Expand Down
39 changes: 37 additions & 2 deletions ...rone/src/main/java/com/palantir/baseline/errorprone/safety/SafetyPropagationTransfer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -840,10 +840,13 @@ public TransferResult<Safety, AccessPathStore<Safety>> 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);
Expand All @@ -852,6 +855,26 @@ public TransferResult<Safety, AccessPathStore<Safety>> 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)) {
Expand Down Expand Up @@ -1084,6 +1107,11 @@ public TransferResult<Safety, AccessPathStore<Safety>> visitTypeCast(
/** Handles type changes (widen, narrow, and cast). */
private TransferResult<Safety, AccessPathStore<Safety>> handleTypeConversion(
Tree newType, Node original, TransferInput<Safety, AccessPathStore<Safety>> input) {
return noStoreChanges(getTypeConversionSafety(newType, original, input), input);
}

private Safety getTypeConversionSafety(
Tree newType, Node original, TransferInput<Safety, AccessPathStore<Safety>> input) {
Safety inputSafety = getValueOfSubNode(input, original);
Safety targetSafety = SafetyAnnotations.getSafety(newType, state);
Safety resultSafety;
Expand All @@ -1098,13 +1126,20 @@ private TransferResult<Safety, AccessPathStore<Safety>> handleTypeConversion(
} else {
resultSafety = Safety.mergeAssumingUnknownIsSame(inputSafety, targetSafety);
}
return noStoreChanges(resultSafety, input);
return resultSafety;
}

@Override
public TransferResult<Safety, AccessPathStore<Safety>> visitInstanceOf(
InstanceOfNode node, TransferInput<Safety, AccessPathStore<Safety>> 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);
}

Expand Down
76 changes: 74 additions & 2 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 @@ -18,6 +18,7 @@

import com.google.errorprone.CompilationTestHelper;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.Timeout;

class IllegalSafeLoggingArgumentTest {

Expand Down Expand Up @@ -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.*;",
Expand All @@ -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.*;",
Expand Down Expand Up @@ -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());
}
Expand Down
2 changes: 1 addition & 1 deletion ...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 @@ -211,7 +211,7 @@ void testMethodDoesNotReturn() {

@Test
void testRecordWithUnsafeTypes() {
fix("--release", "15", "--enable-preview")
fix("--release", "17")
.addInputLines(
"Test.java",
"import com.palantir.tokens.auth.*;",
Expand Down
4 changes: 2 additions & 2 deletions ...-error-prone/src/test/java/com/palantir/baseline/errorprone/StrictUnusedVariableTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,10 +314,10 @@ public void fixes_previously_suppressed_variables() {
}

@Test
@DisabledForJreRange(max = JRE.JAVA_13)
@DisabledForJreRange(max = JRE.JAVA_16)
public void testRecord() {
compilationHelper = CompilationTestHelper.newInstance(StrictUnusedVariable.class, getClass())
.setArgs("--enable-preview", "--release", "15");
.setArgs("--release", "17");

compilationHelper
.addSourceLines("Test.java", "class Test {", " record Foo(int bar) {}", "}")
Expand Down
1 change: 1 addition & 0 deletions baseline-refaster-rules/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,5 @@ moduleJvmArgs {
'jdk.compiler/com.sun.tools.javac.tree',
'jdk.compiler/com.sun.tools.javac.util'
]
opens = ['jdk.compiler/com.sun.tools.javac.comp']
Copy link
Copy Markdown
Contributor

@fawind fawind Jul 26, 2022

Choose a reason for hiding this comment

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

For my understanding, why do we need the additional "opens" declaration now? I don't see any new usage in the diff here.

Copy link
Copy Markdown
Contributor Author

@carterkozak carterkozak Jul 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This allows this module to pass tests using jdk17. Not required for the build, but initially I tried using jdk17 everywhere, but some tests are more difficult to fix.

}
5 changes: 5 additions & 0 deletions changelog/@unreleased/pr-2331.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: Handle safety flow through instanceof pattern matching
links:
- https://github.com/palantir/gradle-baseline/pull/2331