Model try-with-resources semantics during CFG construction

[msridhar]

Fixes #6037

We modify CFG construction to inject calls to close() at the appropriate place for all resource variables declared or used in a try-with-resources tree. Our modeling is based on the desugaring given in JLS 14.20.3.1:

https://docs.oracle.com/javase/specs/jls/se21/html/jls-14.html#jls-14.20.3.1

Essentially, the closing of each resource is modeled using a nested try-finally block, where the finally block includes the close() call. We currently elide the null checks and tracking of the primary exception from the JLS desugaring.

This explicit modeling in the CFG enables deletion of a good amount of custom code related to resource variables in the Must Call Checker and the Resource Leak Checker, while also enabling those checkers to handle more cases.

[msridhar]

@mernst I didn't request a review on this one since I wasn't sure whether you or @smillst would be best to review the dataflow changes

[mernst]

@msridhar Ordinarily me, but I am overwhelmed at the moment, so I will ask Suzanne to take a look.

[smillst]

I just had a few minor comments.

[smillst]

I would change just a to a use of a.

[smillst]

This should return a singleton of assignOrLocalVarNode.

[smillst]

I would change the resource is a local variable (<em>not</em> a variable declaration) to the resource is a use of a local variable.

[smillst]

Please add a change log entry for the new node subclass.

[msridhar]

I realized that in Java 9 and after try-with-resources can also be used to close a final field. Will need to rework this PR a bit to handle that. Will address comments also and then re-request a review.

[msridhar]

@smillst @kelloggm @mernst I'm thinking about re-working the approach here and would like your feedback. Right now, this PR adds ResourceNodes (modeling the declaration of a resource in a try-with-resources) at the same point in the CFG where the resource is declared. The problem is that this is not the program point where the resource is actually closed. So, in the presence of @CreatesMustCallFor, we can get false positives, e.g., here. (That false positive is reset.not.owning but I think we'd also get a false positive that the resource is not closed.)

Here is my alternative proposal. Instead of putting the ResourceNodes where resources are declared, we would place them in all copies of the finally block if it exists, and just after the doneLabel for the try if there is no finally block. Then, a ResourceNode would represent the logic to close a reference to a resource if it is not null, catching any relevant exceptions from the close() call, etc. With this representation, we could simply model a ResourceNode as a call to close() in CalledMethodsTransfer (or ResourceLeakTransfer if we don't want this for the Called Methods Checker). I think that would be all we need to model try-with-resources semantics for the RLC, so we could get rid of some code we have now in MustCallConsistencyAnalyzer and elsewhere that special-cases resource variables.

Let me know your thoughts on the above; thanks!

[kelloggm]

Here is my alternative proposal

I think this alternative proposal is sensible. I would consider changing the name from ResourceNode to something that specifically calls out the fact that the node represents the close logic, though, if we do this--for example, ResourceCloseNode or something like that---because I suspect that otherwise the presence of "ResourceNode"s near the end of try-with-resources (rather than the beginning) will be unintuitive.

[smillst]

This sounds good to me. I also agree with Martin's naming suggestion, too.

[mernst]

Thanks for all these improvements. I have a few questions/comments.

[smillst]

LGTM. I didn't merge because I'm not sure if I'm the last to review it.

[mernst]

@msridhar I think this can be merged. Thank you!