Skip to content
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

Jump to bottom

Handle assertThat in JUnit and Hamcrest. #310

Merged
merged 2 commits into from Apr 29, 2019
Merged

Handle assertThat in JUnit and Hamcrest. #310

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f104733
Adding support for assertThat in Junit and Hamcrest.
navahgar Apr 29, 2019
505dc5a
Splitting negative tests and adding static imports for assertion libr…
navahgar Apr 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
88 changes: 88 additions & 0 deletions nullaway/src/main/java/com/uber/nullaway/handlers/AssertionHandler.java
View file
Open in desktop
Expand Up @@ -31,6 +31,7 @@
import com.sun.tools.javac.util.Name;
import com.uber.nullaway.dataflow.AccessPath;
import com.uber.nullaway.dataflow.AccessPathNullnessPropagation;
import java.util.List;
import org.checkerframework.dataflow.cfg.node.MethodInvocationNode;
import org.checkerframework.dataflow.cfg.node.Node;

Expand All @@ -44,6 +45,15 @@ public class AssertionHandler extends BaseNoOpHandler {
private static final String ASSERT_THAT_METHOD = "assertThat";
private static final String ASSERT_THAT_OWNER = "com.google.common.truth.Truth";

private static final String HAMCREST_ASSERT_CLASS = "org.hamcrest.MatcherAssert";
private static final String JUNIT_ASSERT_CLASS = "org.junit.Assert";

private static final String MATCHERS_CLASS = "org.hamcrest.Matchers";
private static final String IS_MATCHER = "is";
private static final String NOT_MATCHER = "not";
private static final String NOT_NULL_VALUE_MATCHER = "notNullValue";
private static final String NULL_VALUE_MATCHER = "nullValue";

// 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.
Expand All @@ -52,6 +62,17 @@ public class AssertionHandler extends BaseNoOpHandler {
private Name assertThat;
private Name assertThatOwner;

// Names for junit assertion libraries.
private Name hamcrestAssertClass;
private Name junitAssertClass;

// Names for matchers.
navahgar marked this conversation as resolved.
Show resolved Hide resolved
private Name matchersClass;
private Name isMatcher;
private Name notMatcher;
private Name notNullValueMatcher;
private Name nullValueMatcher;

@Override
public NullnessHint onDataflowVisitMethodInvocation(
MethodInvocationNode node,
Expand Down Expand Up @@ -86,6 +107,20 @@ public NullnessHint onDataflowVisitMethodInvocation(
}
}
}

// Look for statements of the form:
// * assertThat(A, is(not(nullValue())))
// * assertThat(A, is(notNullValue()))
if (isMethodHamcrestAssertThat(callee) || isMethodJunitAssertThat(callee)) {
List<Node> args = node.getArguments();
if (args.size() == 2 && isMatcherIsNotNull(args.get(1))) {
AccessPath ap = AccessPath.getAccessPathForNodeNoMapGet(args.get(0));
if (ap != null) {
bothUpdates.set(ap, NONNULL);
}
}
}

return NullnessHint.UNKNOWN;
}

Expand All @@ -97,6 +132,50 @@ private boolean isMethodAssertThat(Symbol.MethodSymbol methodSymbol) {
return matchesMethod(methodSymbol, assertThat, assertThatOwner);
}

private boolean isMethodHamcrestAssertThat(Symbol.MethodSymbol methodSymbol) {
return matchesMethod(methodSymbol, assertThat, hamcrestAssertClass);
}

private boolean isMethodJunitAssertThat(Symbol.MethodSymbol methodSymbol) {
return matchesMethod(methodSymbol, assertThat, junitAssertClass);
}

private boolean isMatcherIsNotNull(Node node) {
// Matches with
// * is(not(nullValue()))
// * is(notNullValue())
if (matchesMatcherMethod(node, isMatcher)) {
// All overloads of `is` method have exactly one argument.
return isMatcherNotNull(((MethodInvocationNode) node).getArgument(0));
}
return false;
}

private boolean isMatcherNotNull(Node node) {
// Matches with
// * not(nullValue())
// * notNullValue()
if (matchesMatcherMethod(node, notMatcher)) {
// All overloads of `not` method have exactly one argument.
return isMatcherNull(((MethodInvocationNode) node).getArgument(0));
}
return matchesMatcherMethod(node, notNullValueMatcher);
}

private boolean isMatcherNull(Node node) {
// Matches with nullValue()
return matchesMatcherMethod(node, nullValueMatcher);
navahgar marked this conversation as resolved.
Show resolved Hide resolved
}

private boolean matchesMatcherMethod(Node node, Name matcherName) {
if (node instanceof MethodInvocationNode) {
MethodInvocationNode methodInvocationNode = (MethodInvocationNode) node;
Symbol.MethodSymbol callee = ASTHelpers.getSymbol(methodInvocationNode.getTree());
return matchesMethod(callee, matcherName, matchersClass);
}
return false;
}

private boolean matchesMethod(
Symbol.MethodSymbol methodSymbol, Name toMatchMethodName, Name toMatchOwnerName) {
return methodSymbol.name.equals(toMatchMethodName)
Expand All @@ -112,5 +191,14 @@ private void initializeMethodNames(Name.Table table) {
isNotNullOwner = table.fromString(IS_NOT_NULL_OWNER);
assertThat = table.fromString(ASSERT_THAT_METHOD);
assertThatOwner = table.fromString(ASSERT_THAT_OWNER);

hamcrestAssertClass = table.fromString(HAMCREST_ASSERT_CLASS);
junitAssertClass = table.fromString(JUNIT_ASSERT_CLASS);

matchersClass = table.fromString(MATCHERS_CLASS);
isMatcher = table.fromString(IS_MATCHER);
notMatcher = table.fromString(NOT_MATCHER);
notNullValueMatcher = table.fromString(NOT_NULL_VALUE_MATCHER);
nullValueMatcher = table.fromString(NULL_VALUE_MATCHER);
}
}
70 changes: 67 additions & 3 deletions nullaway/src/test/java/com/uber/nullaway/NullAwayTest.java
View file
Open in desktop
Expand Up @@ -1030,10 +1030,74 @@ public void doNotSupportAssertThatWhenDisabled() {
"import java.util.Objects;",
"import javax.annotation.Nullable;",
"class Test {",
" private void foo(@Nullable Object o) {",
" com.google.common.truth.Truth.assertThat(o).isNotNull();",
" private void foo(@Nullable Object a, @Nullable Object b, @Nullable Object c) {",
" com.google.common.truth.Truth.assertThat(a).isNotNull();",
navahgar marked this conversation as resolved.
Show resolved Hide resolved
" // BUG: Diagnostic contains: dereferenced expression",
" o.toString();",
" a.toString();",
" org.hamcrest.MatcherAssert.assertThat(b, org.hamcrest.Matchers.is("
+ "org.hamcrest.Matchers.notNullValue()));",
" // BUG: Diagnostic contains: dereferenced expression",
" b.toString();",
" org.junit.Assert.assertThat(c, org.hamcrest.Matchers.is("
+ "org.hamcrest.Matchers.notNullValue()));",
" // BUG: Diagnostic contains: dereferenced expression",
" c.toString();",
" }",
"}")
.doTest();
}

@Test
public void supportHamcrestAssertThatIsNotNull_Object() {
compilationHelper
.setArgs(
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;",
"class Test {",
" private void foo(@Nullable Object a, @Nullable Object b) {",
" org.hamcrest.MatcherAssert.assertThat(a, org.hamcrest.Matchers.is("
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just add the static imports to make this both easier to read and more representative of idiomatic code:

import static org.hamcrest.MatcherAssert.assertThat;
...
assertThat(a, is(notNullValue));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, good point. I didn't know about static imports. Updated the tests now.

+ "org.hamcrest.Matchers.notNullValue()));",
" a.toString();",
" org.hamcrest.MatcherAssert.assertThat(b, org.hamcrest.Matchers.is("
+ "org.hamcrest.Matchers.not(org.hamcrest.Matchers.nullValue())));",
" b.toString();",
" }",
"}")
.doTest();
}

@Test
public void supportJunitAssertThatIsNotNull_Object() {
compilationHelper
.setArgs(
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;",
"class Test {",
" private void foo(@Nullable Object a, @Nullable Object b) {",
" org.junit.Assert.assertThat(a, org.hamcrest.Matchers.is("
+ "org.hamcrest.Matchers.notNullValue()));",
" a.toString();",
" org.junit.Assert.assertThat(b, org.hamcrest.Matchers.is("
+ "org.hamcrest.Matchers.not(org.hamcrest.Matchers.nullValue())));",
" b.toString();",
" }",
"}")
.doTest();
Expand Down