Skip to content

Commit

Permalink
[NETBEANS-407] Using a special State to model instanceof instead of N…
Browse files Browse the repository at this point in the history 
…OT_NULL_HYPOTHETICAL, as instanceof is not an explicit null test and may be false also for non-null input parameters.
  • Loading branch information
@jlahoda
jlahoda committed Mar 29, 2018
1 parent f978fd6 commit 3bb6c73
Show file tree
Hide file tree
Showing 2 changed files with 79 additions and 18 deletions.
46 changes: 28 additions & 18 deletions java.hints/src/org/netbeans/modules/java/hints/bugs/NPECheck.java
View file
Expand Up @@ -209,11 +209,13 @@ public static ErrorDescription unboxingConditional(HintContext ctx) {
break;
case POSSIBLE_NULL_REPORT:
case POSSIBLE_NULL:
case INSTANCE_OF_FALSE:
k = "ERR_UnboxingPotentialNullValue"; // NOI18N
break;
case NOT_NULL_BE_NPE:
case NOT_NULL:
case NOT_NULL_HYPOTHETICAL:
case INSTANCE_OF_TRUE:
return null;
default:
throw new AssertionError(s.name());
Expand All @@ -238,7 +240,7 @@ public static ErrorDescription switchExpression(HintContext ctx) {
return ErrorDescriptionFactory.forName(ctx, ctx.getPath(), displayName);
}

if (r == State.POSSIBLE_NULL_REPORT) {
if (r == State.POSSIBLE_NULL_REPORT || r == INSTANCE_OF_FALSE) {
String displayName = NbBundle.getMessage(NPECheck.class, "ERR_PossiblyDereferencingNull");

return ErrorDescriptionFactory.forName(ctx, ctx.getPath(), displayName);
Expand All @@ -259,7 +261,7 @@ public static ErrorDescription enhancedFor(HintContext ctx) {
return ErrorDescriptionFactory.forName(ctx, ctx.getPath(), displayName);
}

if (r == State.POSSIBLE_NULL_REPORT) {
if (r == State.POSSIBLE_NULL_REPORT || r == State.INSTANCE_OF_FALSE) {
String displayName = NbBundle.getMessage(NPECheck.class, "ERR_PossiblyDereferencingNull");

return ErrorDescriptionFactory.forName(ctx, ctx.getPath(), displayName);
Expand All @@ -278,7 +280,7 @@ public static ErrorDescription memberSelect(HintContext ctx) {
return ErrorDescriptionFactory.forName(ctx, ctx.getPath(), displayName);
}

if (r == State.POSSIBLE_NULL_REPORT) {
if (r == State.POSSIBLE_NULL_REPORT || r == State.INSTANCE_OF_FALSE) {
String displayName = NbBundle.getMessage(NPECheck.class, "ERR_PossiblyDereferencingNull");

return ErrorDescriptionFactory.forName(ctx, ctx.getPath(), displayName);
Expand Down Expand Up @@ -327,6 +329,7 @@ public static List<ErrorDescription> methodInvocation(HintContext ctx) {
result.add(ErrorDescriptionFactory.forTree(ctx, mit.getArguments().get(index), NbBundle.getMessage(NPECheck.class, "ERR_NULL_TO_NON_NULL_ARG")));
break;
case POSSIBLE_NULL_REPORT:
case INSTANCE_OF_FALSE:
result.add(ErrorDescriptionFactory.forTree(ctx, mit.getArguments().get(index), NbBundle.getMessage(NPECheck.class, "ERR_POSSIBLENULL_TO_NON_NULL_ARG")));
break;
}
Expand Down Expand Up @@ -455,6 +458,7 @@ public static ErrorDescription returnNull(HintContext ctx) {
if (expected.isNotNull()) key = "ERR_ReturningNullFromNonNull";
break;
case POSSIBLE_NULL_REPORT:
case INSTANCE_OF_FALSE:
if (expected.isNotNull()) key = "ERR_ReturningPossibleNullFromNonNull";
break;
}
Expand Down Expand Up @@ -717,7 +721,7 @@ public State visitMemberSelect(MemberSelectTree node, Void p) {
State expr = scan(node.getExpression(), p);
boolean wasNPE = false;

if (expr == State.NULL || expr == State.NULL_HYPOTHETICAL || expr == State.POSSIBLE_NULL || expr == State.POSSIBLE_NULL_REPORT) {
if (expr == State.NULL || expr == State.NULL_HYPOTHETICAL || expr == State.POSSIBLE_NULL || expr == State.POSSIBLE_NULL_REPORT || expr == State.INSTANCE_OF_FALSE) {
wasNPE = true;
}

Expand Down Expand Up @@ -886,8 +890,17 @@ public State visitInstanceOf(InstanceOfTree node, Void p) {

Element e = info.getTrees().getElement(new TreePath(getCurrentPath(), node.getExpression()));

if (isVariableElement(e) && (variable2State.get((VariableElement) e) == null || !variable2State.get((VariableElement) e).isNotNull()) && !not) {
variable2State.put((VariableElement) e, State.NOT_NULL_HYPOTHETICAL);
if (isVariableElement(e)) {
boolean setState = false;
State currentState = variable2State.get((VariableElement) e);
if (currentState == null) {
setState = !getStateFromAnnotations(info, e).isNotNull();
} else {
setState = !variable2State.get((VariableElement) e).isNotNull();
}
if (setState) {
variable2State.put((VariableElement) e, not ? State.INSTANCE_OF_FALSE : State.INSTANCE_OF_TRUE);
}
}

return null;
Expand Down Expand Up @@ -1479,7 +1492,7 @@ private void mergeNonHypotheticalVariable2State(Map<VariableElement, State> orig
for (Entry<VariableElement, State> e : backup.entrySet()) {
State t = e.getValue();

if (t != null && t != State.NOT_NULL_HYPOTHETICAL && t != NULL_HYPOTHETICAL) {
if (t != null && t != State.NOT_NULL_HYPOTHETICAL && t != NULL_HYPOTHETICAL && t != INSTANCE_OF_TRUE && t != INSTANCE_OF_FALSE) {
variable2State.put(e.getKey(), t);
}
}
Expand All @@ -1495,24 +1508,17 @@ private boolean isVariableElement(Element ve) {
return NPECheck.isVariableElement(ctx, ve);
}

private void clearHypothetical() {
for (Iterator<Entry<VariableElement, State>> it = variable2State.entrySet().iterator(); it.hasNext();) {
Entry<VariableElement, State> e = it.next();

if (e.getValue() == State.NOT_NULL_HYPOTHETICAL || e.getValue() == State.NULL_HYPOTHETICAL) {
it.remove();
}
}
}
}

static enum State {
NULL,
NULL_HYPOTHETICAL,
POSSIBLE_NULL,
POSSIBLE_NULL_REPORT,
INSTANCE_OF_FALSE,
NOT_NULL,
NOT_NULL_HYPOTHETICAL,
INSTANCE_OF_TRUE,
NOT_NULL_BE_NPE;

public @CheckForNull State reverse() {
Expand All @@ -1521,6 +1527,8 @@ static enum State {
return NOT_NULL;
case NULL_HYPOTHETICAL:
return NOT_NULL_HYPOTHETICAL;
case INSTANCE_OF_FALSE:
return INSTANCE_OF_TRUE;
case POSSIBLE_NULL:
case POSSIBLE_NULL_REPORT:
return this;
Expand All @@ -1529,18 +1537,20 @@ static enum State {
return NULL;
case NOT_NULL_HYPOTHETICAL:
return NULL_HYPOTHETICAL;
case INSTANCE_OF_TRUE:
return INSTANCE_OF_FALSE;
default: throw new IllegalStateException();
}
}

public boolean isNotNull() {
return this == NOT_NULL || this == NOT_NULL_BE_NPE || this == NOT_NULL_HYPOTHETICAL;
return this == NOT_NULL || this == NOT_NULL_BE_NPE || this == NOT_NULL_HYPOTHETICAL || this == INSTANCE_OF_TRUE;
}

public static State collect(State s1, State s2) {
if (s1 == s2) return s1;
if (s1 == NULL || s2 == NULL || s1 == NULL_HYPOTHETICAL || s2 == NULL_HYPOTHETICAL) return POSSIBLE_NULL_REPORT;
if (s1 == POSSIBLE_NULL_REPORT || s2 == POSSIBLE_NULL_REPORT) return POSSIBLE_NULL_REPORT;
if (s1 == POSSIBLE_NULL_REPORT || s2 == POSSIBLE_NULL_REPORT || s2 == INSTANCE_OF_FALSE) return POSSIBLE_NULL_REPORT;
if (s1 != null && s2 != null && s1.isNotNull() && s2.isNotNull()) return NOT_NULL;

return POSSIBLE_NULL;
Expand Down
51 changes: 51 additions & 0 deletions java.hints/test/unit/src/org/netbeans/modules/java/hints/bugs/NPECheckTest.java
View file
Expand Up @@ -1645,6 +1645,57 @@ public void testLambdaExpressionShouldntReturnNull() throws Exception {
.assertWarnings("17:19-17:23:verifier:ERR_ReturningNullFromNonNull");
}

public void testNETBEANS407a() throws Exception {
HintTest.create()
.input("package test;\n" +
"public class Test {\n" +
" public void test(Object o) {\n" +
" boolean b1 = o instanceof Integer;\n" +
" boolean b2 = o instanceof Integer && o != null;\n" +
" System.out.println(o.toString());\n" +
" }\n" +
"}")
.run(NPECheck.class)
.assertWarnings("4:41-4:50:verifier:ERR_NotNull");
}

public void testNETBEANS407b() throws Exception {
HintTest.create()
.input("package test;\n" +
"public class Test {\n" +
" public void test(Object o) {\n" +
" boolean b = !(o instanceof Integer) && o.toString() != null;\n" +
" }\n" +
"}")
.run(NPECheck.class)
.assertWarnings("3:45-3:53:verifier:Possibly Dereferencing null");
}

public void testNETBEANS407c() throws Exception {
HintTest.create()
.input("package test;\n" +
"public class Test {\n" +
" public void test(Object o) {\n" +
" boolean b = o != null;\n" +
" System.out.println(o.toString());\n" +
" }\n" +
"}")
.run(NPECheck.class)
.assertWarnings("4:25-4:33:verifier:Possibly Dereferencing null");
}

public void testNETBEANS407d() throws Exception {
HintTest.create()
.input("package test;\n" +
"public class Test {\n" +
" public void test(Object o) {\n" +
" boolean b = (o == null || o == \"\") && o.toString() != null;\n" +
" }\n" +
"}")
.run(NPECheck.class)
.assertWarnings("3:44-3:52:verifier:Possibly Dereferencing null");
}

private void performAnalysisTest(String fileName, String code, String... golden) throws Exception {
HintTest.create()
.input(fileName, code)
Expand Down

0 comments on commit 3bb6c73

Please sign in to comment.