Skip to content

Commit

Permalink
Support isInstanceOf(...) as implying non-null in assertion libraries. (
Browse files Browse the repository at this point in the history 
#726)

Generally speaking, all these libraries implement the assertion by calling `Class.isInstance`, which
will return `false` whenever the argument is `null`. Similar to the `instanceof` operator:
https://docs.oracle.com/javase/1.5.0/docs/api/java/lang/Class.html#isInstance(java.lang.Object)

Given this, we can avoid some redundant assertions by adding knowledge about these calls directly
in `AssertionsHandler`.
  • Loading branch information
@lazaroclapp
lazaroclapp authored Feb 1, 2023
1 parent d2e4a49 commit fc151dc
Show file tree
Hide file tree
Showing 3 changed files with 159 additions and 11 deletions.
11 changes: 8 additions & 3 deletions nullaway/src/main/java/com/uber/nullaway/handlers/AssertionHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,10 @@ public NullnessHint onDataflowVisitMethodInvocation(
methodNameUtil.initializeMethodNames(callee.name.table);
}

// Look for statements of the form: assertThat(A).isNotNull()
// Look for statements of the form: assertThat(A).isNotNull() or
// assertThat(A).isInstanceOf(Foo.class)
// A will not be NULL after this statement.
if (methodNameUtil.isMethodIsNotNull(callee)) {
if (methodNameUtil.isMethodIsNotNull(callee) || methodNameUtil.isMethodIsInstanceOf(callee)) {
Node receiver = node.getTarget().getReceiver();
if (receiver instanceof MethodInvocationNode) {
MethodInvocationNode receiver_method = (MethodInvocationNode) receiver;
Expand All @@ -80,10 +81,14 @@ public NullnessHint onDataflowVisitMethodInvocation(
// Look for statements of the form:
// * assertThat(A, is(not(nullValue())))
// * assertThat(A, is(notNullValue()))
// * assertThat(A, is(instanceOf(Foo.class)))
// * assertThat(A, isA(Foo.class))
if (methodNameUtil.isMethodHamcrestAssertThat(callee)
|| methodNameUtil.isMethodJunitAssertThat(callee)) {
List<Node> args = node.getArguments();
if (args.size() == 2 && methodNameUtil.isMatcherIsNotNull(args.get(1))) {
if (args.size() == 2
&& (methodNameUtil.isMatcherIsNotNull(args.get(1))
|| methodNameUtil.isMatcherIsInstanceOf(args.get(1)))) {
AccessPath ap = AccessPath.getAccessPathForNode(args.get(0), state, apContext);
if (ap != null) {
bothUpdates.set(ap, NONNULL);
Expand Down
53 changes: 45 additions & 8 deletions nullaway/src/main/java/com/uber/nullaway/handlers/MethodNameUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,11 @@ class MethodNameUtil {
// Strings corresponding to the names of the methods (and their owners) used to identify
// assertions in this handler.
private static final String IS_NOT_NULL_METHOD = "isNotNull";
private static final String IS_NOT_NULL_OWNER_TRUTH = "com.google.common.truth.Subject";
private static final String IS_NOT_NULL_OWNER_ASSERTJ = "org.assertj.core.api.AbstractAssert";
private static final String IS_OWNER_TRUTH_SUBJECT = "com.google.common.truth.Subject";
private static final String IS_OWNER_ASSERTJ_ABSTRACT_ASSERT =
"org.assertj.core.api.AbstractAssert";
private static final String IS_INSTANCE_OF_METHOD = "isInstanceOf";
private static final String IS_INSTANCE_OF_ANY_METHOD = "isInstanceOfAny";
private static final String IS_TRUE_METHOD = "isTrue";
private static final String IS_FALSE_METHOD = "isFalse";
private static final String IS_TRUE_OWNER_TRUTH = "com.google.common.truth.BooleanSubject";
Expand All @@ -67,16 +70,21 @@ class MethodNameUtil {
private static final String CORE_MATCHERS_CLASS = "org.hamcrest.CoreMatchers";
private static final String CORE_IS_NULL_CLASS = "org.hamcrest.core.IsNull";
private static final String IS_MATCHER = "is";
private static final String IS_A_MATCHER = "isA";
private static final String NOT_MATCHER = "not";
private static final String NOT_NULL_VALUE_MATCHER = "notNullValue";
private static final String NULL_VALUE_MATCHER = "nullValue";
private static final String INSTANCE_OF_MATCHER = "instanceOf";

// Names of the methods (and their owners) used to identify assertions in this handler. Name used
// here refers to com.sun.tools.javac.util.Name. Comparing methods using Names is faster than
// comparing using strings.
private Name isNotNull;
private Name isNotNullOwnerTruth;
private Name isNotNullOwnerAssertJ;

private Name isInstanceOf;
private Name isInstanceOfAny;
private Name isOwnerTruthSubject;
private Name isOwnerAssertJAbstractAssert;

private Name isTrue;
private Name isFalse;
Expand Down Expand Up @@ -106,15 +114,20 @@ class MethodNameUtil {
private Name coreMatchersClass;
private Name coreIsNullClass;
private Name isMatcher;
private Name isAMatcher;
private Name notMatcher;
private Name notNullValueMatcher;
private Name nullValueMatcher;
private Name instanceOfMatcher;

@Initializer
void initializeMethodNames(Name.Table table) {
isNotNull = table.fromString(IS_NOT_NULL_METHOD);
isNotNullOwnerTruth = table.fromString(IS_NOT_NULL_OWNER_TRUTH);
isNotNullOwnerAssertJ = table.fromString(IS_NOT_NULL_OWNER_ASSERTJ);
isOwnerTruthSubject = table.fromString(IS_OWNER_TRUTH_SUBJECT);
isOwnerAssertJAbstractAssert = table.fromString(IS_OWNER_ASSERTJ_ABSTRACT_ASSERT);

isInstanceOf = table.fromString(IS_INSTANCE_OF_METHOD);
isInstanceOfAny = table.fromString(IS_INSTANCE_OF_ANY_METHOD);

isTrue = table.fromString(IS_TRUE_METHOD);
isFalse = table.fromString(IS_FALSE_METHOD);
Expand Down Expand Up @@ -143,14 +156,23 @@ void initializeMethodNames(Name.Table table) {
coreMatchersClass = table.fromString(CORE_MATCHERS_CLASS);
coreIsNullClass = table.fromString(CORE_IS_NULL_CLASS);
isMatcher = table.fromString(IS_MATCHER);
isAMatcher = table.fromString(IS_A_MATCHER);
notMatcher = table.fromString(NOT_MATCHER);
notNullValueMatcher = table.fromString(NOT_NULL_VALUE_MATCHER);
nullValueMatcher = table.fromString(NULL_VALUE_MATCHER);
instanceOfMatcher = table.fromString(INSTANCE_OF_MATCHER);
}

boolean isMethodIsNotNull(Symbol.MethodSymbol methodSymbol) {
return matchesMethod(methodSymbol, isNotNull, isNotNullOwnerTruth)
|| matchesMethod(methodSymbol, isNotNull, isNotNullOwnerAssertJ);
return matchesMethod(methodSymbol, isNotNull, isOwnerTruthSubject)
|| matchesMethod(methodSymbol, isNotNull, isOwnerAssertJAbstractAssert);
}

boolean isMethodIsInstanceOf(Symbol.MethodSymbol methodSymbol) {
return matchesMethod(methodSymbol, isInstanceOf, isOwnerTruthSubject)
|| matchesMethod(methodSymbol, isInstanceOf, isOwnerAssertJAbstractAssert)
// Truth doesn't seem to have isInstanceOfAny
|| matchesMethod(methodSymbol, isInstanceOfAny, isOwnerAssertJAbstractAssert);
}

boolean isMethodAssertTrue(Symbol.MethodSymbol methodSymbol) {
Expand Down Expand Up @@ -230,6 +252,21 @@ private boolean isMatcherNull(Node node) {
|| matchesMatcherMethod(node, nullValueMatcher, coreIsNullClass);
}

boolean isMatcherIsInstanceOf(Node node) {
// Matches with
// * is(instanceOf(Some.class))
// * isA(Some.class)
if (matchesMatcherMethod(node, isMatcher, matchersClass)
|| matchesMatcherMethod(node, isMatcher, coreMatchersClass)) {
// All overloads of `is` method have exactly one argument.
Node inner = ((MethodInvocationNode) node).getArgument(0);
return matchesMatcherMethod(inner, instanceOfMatcher, matchersClass)
|| matchesMatcherMethod(inner, instanceOfMatcher, coreMatchersClass);
}
return (matchesMatcherMethod(node, isAMatcher, matchersClass)
|| matchesMatcherMethod(node, isAMatcher, coreMatchersClass));
}

private boolean matchesMatcherMethod(Node node, Name matcherName, Name matcherClass) {
if (node instanceof MethodInvocationNode) {
MethodInvocationNode methodInvocationNode = (MethodInvocationNode) node;
Expand Down
106 changes: 106 additions & 0 deletions nullaway/src/test/java/com/uber/nullaway/NullAwayAssertionLibsTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,31 @@ public void supportTruthAssertThatIsNotNull_MapKey() {
.doTest();
}

@Test
public void supportTruthAssertThatIsInstanceOf() {
makeTestHelperWithArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
"-XepOpt:NullAway:AnnotatedPackages=com.uber",
"-XepOpt:NullAway:HandleTestAssertionLibraries=true"))
.addSourceLines(
"Test.java",
"package com.uber;",
"import java.lang.Object;",
"import java.util.Objects;",
"import javax.annotation.Nullable;",
"import static com.google.common.truth.Truth.assertThat;",
"class Test {",
" private void foo(@Nullable Object o) {",
" // inInstanceOf => isNotNull!",
" assertThat(o).isInstanceOf(Object.class);",
" o.toString();",
" }",
"}")
.doTest();
}

@Test
public void doNotSupportTruthAssertThatWhenDisabled() {
makeTestHelperWithArgs(
Expand Down Expand Up @@ -208,6 +233,34 @@ public void supportHamcrestAssertThatCoreIsNotNull() {
.doTest();
}

@Test
public void supportHamcrestAssertThatIsInstanceOf() {
makeTestHelperWithArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
"-XepOpt:NullAway:AnnotatedPackages=com.uber",
"-XepOpt:NullAway:HandleTestAssertionLibraries=true"))
.addSourceLines(
"Test.java",
"package com.uber;",
"import java.lang.Object;",
"import java.util.Objects;",
"import javax.annotation.Nullable;",
"import static org.hamcrest.MatcherAssert.assertThat;",
"import static org.hamcrest.CoreMatchers.*;",
"import org.hamcrest.core.IsNull;",
"class Test {",
" private void foo(@Nullable Object a, @Nullable Object b) {",
" assertThat(a, is(instanceOf(Object.class)));",
" a.toString();",
" assertThat(b, isA(Object.class));",
" b.toString();",
" }",
"}")
.doTest();
}

@Test
public void supportJunitAssertThatIsNotNull_Object() {
makeTestHelperWithArgs(
Expand Down Expand Up @@ -235,6 +288,33 @@ public void supportJunitAssertThatIsNotNull_Object() {
.doTest();
}

@Test
public void supportJunitAssertThatIsInstanceOf() {
makeTestHelperWithArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
"-XepOpt:NullAway:AnnotatedPackages=com.uber",
"-XepOpt:NullAway:HandleTestAssertionLibraries=true"))
.addSourceLines(
"Test.java",
"package com.uber;",
"import java.lang.Object;",
"import java.util.Objects;",
"import javax.annotation.Nullable;",
"import static org.junit.Assert.assertThat;",
"import static org.hamcrest.Matchers.*;",
"class Test {",
" private void foo(@Nullable Object a, @Nullable Object b) {",
" assertThat(a, is(instanceOf(Object.class)));",
" a.toString();",
" assertThat(b, isA(Object.class));",
" b.toString();",
" }",
"}")
.doTest();
}

@Test
public void doNotSupportJunitAssertThatWhenDisabled() {
makeTestHelperWithArgs(
Expand Down Expand Up @@ -331,6 +411,32 @@ public void supportAssertJAssertThatIsNotNull_MapKey() {
.doTest();
}

@Test
public void supportAssertJAssertThatIsInstanceOf() {
makeTestHelperWithArgs(
Arrays.asList(
"-d",
temporaryFolder.getRoot().getAbsolutePath(),
"-XepOpt:NullAway:AnnotatedPackages=com.uber",
"-XepOpt:NullAway:HandleTestAssertionLibraries=true"))
.addSourceLines(
"Test.java",
"package com.uber;",
"import java.lang.Object;",
"import java.util.Objects;",
"import javax.annotation.Nullable;",
"import static org.assertj.core.api.Assertions.assertThat;",
"class Test {",
" private void foo(@Nullable Object a, @Nullable Object b) {",
" assertThat(a).isInstanceOf(Object.class);",
" a.toString();",
" assertThat(b).isInstanceOfAny(String.class, Exception.class);",
" b.toString();",
" }",
"}")
.doTest();
}

@Test
public void doNotSupportAssertJAssertThatWhenDisabled() {
makeTestHelperWithArgs(
Expand Down

0 comments on commit fc151dc

Please sign in to comment.