Use .equals() for nodeValues if == does not work; fixes #3484

[mernst]

Merge after #3522

[wmdietl]

This would break one of the basic assumptions of the CFG, quoting from https://github.com/typetools/checker-framework/blob/master/dataflow/src/main/java/org/checkerframework/dataflow/cfg/node/Node.java#L29:

"Note that two {@code Node}s can be {@code .equals} but represent different CFG nodes. Take care to use reference equality, maps that handle identity {@code IdentityHashMap}, and sets like {@code IdentityMostlySingleton}."

So even though it might fix this issue, it would introduce a lot of problems about our CFG representation.
I'll look at your email with alternative suggestions and we can discuss this in more detail in our next meeting.

[mernst]

This PR does not violate the assumption, because it does not add any new items to the map, nor does it create non-identity maps or sets.
This PR does does do a lookup that is beyond what those data structures support, but that isn't violating the assumption.

[wmdietl]

https://github.com/typetools/checker-framework/pull/3523/files#diff-d4108228a0a5407e00c090c2e97d8b6dR178 uses equals to compare two Node instances.
So this change can return an arbitrary result for a different Node that happens to be equals to what is being queried.
How is that not a violation of the assumptions?

If that node should have a value, we should figure out why there is no result for it, instead of returning a possibly incorrect result.

[mernst]

The assumption you quoted is about the class's representation. An accessor (which is the only thing being changed here) cannot violate representation invariants.

This pull request is not the best way to solve the problem in the long run, but it fixes multiple problems without introducing any new ones. It seems appropriate to merge this change, and open an issue to fix the CFG construction algorithm.

[wmdietl]

The assumption you quoted is about the class's representation. An accessor (which is the only thing being changed here) cannot violate representation invariants.

This pull request is not the best way to solve the problem in the long run, but it fixes multiple problems without introducing any new ones. It seems appropriate to merge this change, and open an issue to fix the CFG construction algorithm.

I really don't see what you are trying to argue. You are not using the datastructure correctly and incorrectly use .equals. This returns incorrect results.

I also don't see how this fixes the issue in any real setting where there is more than one use of the variable. Take this example, based on the "fixed" test case from the PR:

class Demo3523 {
    void unreachableCatch(String[] xs) {
        String t = "";
        t.toString();
        t.hashCode();
        try {
        } catch (Throwable e) {
            // :: error: (dereference.of.nullable)
            t.toString();
        }
    }
}

Running the Nullness Checker on that still gives the unexpected

Demo3523.java:9: error: [dereference.of.nullable] dereference of possibly-null reference t
            t.toString();
            ^
1 error

because nodeValues now contains two Nodes for the two preceding accesses of t.
Does this PR actually fix the original issue that triggered filing #3484?

So if you insist on committing this incorrect code, at least add a big disclaimer to the method that these changes should be undone and file a follow-up issue to actually fix the problem.

[mernst]

I agree entirely that this needs a big comment warning that it is not the right fix and should be undone when CFG construction is fixed. I was waiting for #3522 to be merged to write that comment, because once that is merged I can open an issue about CFG construction, and refer to the issue in the comment.