NPECheck cleanup: removing weird hypothetical states, and removing instanceof states.

[lahodaj]

There's a null checker in java/java.hints, NPECheck. I am considering some updates to it, and, as a preparation for that, I was looking at the current States there. There are some weird ones:

• NULL_HYPOTHETICAL and NOT_NULL_HYPOTHETICAL - these are meant to cover cases like:

boolean b = o != null && /*we know o is non-`null` here, marked as `NOT_NULL_HYPOTHETICAL`*/ ...;
//we can't really say much about `o` here, except that we tested it for `null`

the hypothetical states were basically used as "transient state" in expressions. I think that with the experience from pattern matching, this is overly complex: I think we can simply keep ordinary NULL/NOT_NULL states, but keep them "when true" and "when false", and then use and merge them accordingly. This also avoids the need to run conditions twice (once for ordinary run, and once for a negated one). Overall, this should make stuff simpler.

• INSTANCE_OF_FALSE and INSTANCE_OF_TRUE. I think the only real thing this was doing is code along these lines (or conversely the same with an if):

class Test {
    public void test(Object o) {
        boolean b = !(o instanceof Integer) && o./*warning here, dereferencing possible null*/toString() != null;
     }
}

Normally, the NPECheck recognizes several "possible nulls", in particular POSSIBLE_NULL, which does not produce any warning, and POSSIBLE_NULL_REPORT, which is used if there's a confirmation the variable might be null, like if there's a test for null for the variable, and which produces the "possibly dereferencing nullwarning. The instanceof states are used to make theinstanceofwork as a test for null, but, frankly, it is not: we have no firm idea whetherocan or cannot benullif theinstanceoffails. I.e. the behavior ofPOSSIBLE_NULLis more in order here, not thePOSSIBLE_NULL_REPORT`. So, this PR is stripping the instanceof states, and won't produce a warning in the above case.

---

^Add meaningful description above

Click to collapse/expand PR instructions

By opening a pull request you confirm that, unless explicitly stated otherwise, the changes -

• are all your own work, and you have the right to contribute them.
• are contributed solely under the terms and conditions of the Apache License 2.0 (see section 5 of the license for more information).

Please make sure (eg. git log) that all commits have a valid name and email address for you in the Author field.

If you're a first time contributor, see the Contributing guidelines for more information.

If you're a committer, please label the PR before pressing "Create pull request" so that the right test jobs can run.

PR approval and merge checklist:

1. Was this PR correctly labeled, did the right tests run? When did they run?
2. Is this PR squashed?
3. Are author name / email address correct? Are co-authors correctly listed? Do the commit messages need updates?
4. Does the PR title and description still fit after the Nth iteration? Is the description sufficient to appear in the release notes?

If this PR targets the delivery branch: don't merge. (full wiki article)

[mbien]

o would be no String (including null).

To assert the @NotNull requirement the method would have to add a check to the else branch.

If removing the state simplifies things this might be fine, but this does also remove a potentially useful warning, no?

[lahodaj]

The can't say anything note is meant in the context of the null checker.

The null checker currently intentionally does not produce warnings if it can't prove the variable can be null. E.g.:

    Object o = methodWithUnmarkedReturnType();
    o.toString(); //no warning here, as we can't prove o can be null

    Object o = methodWithUnmarkedReturnType();
    boolean b = o != null;
    o.toString(); //warning here, as we can prove o can be null

Now, the question is: is instanceof strong enough signal that the variable can be, "provably", null. The current checker says "yes and no". In some cases, it will produce the warning, in other cases, it won't as it was disabled under:
https://issues.apache.org/jira/browse/NETBEANS-407

It seems to cause too much inconsistencies, and it makes it increasingly difficult to reason about the checker. It feels better to err on the side of not saying anything, than annoying the user with false warnings. Alternatively, we can rollback 407, and really say "instanceof is a test for null". But the current state makes less and less sense to me.

In other words, can this remove a potentially useful warning? Yes, if that user expects the warning. It also creates noise if the user does not expect the warning.

My long-er term goal is to support JSpecify (I have some prototypes). JSpecify allows to bulk-mark types as non-null explicitly (@NullMarked), or nullable explicitly (@NullUnmarked) at which point the "automatic" "possible null, but don't report" mostly ceases to exist.

[mbien]

I see (I think). The parameter Object o isn't marked as nullable so the checker is instructed to not complain on return o; ?

but in past it could "upgrade" o to nullable since it thinks the instanceof was used as null check? (that would be the connection to NETBEANS-407?)