feature: add support for first class resource in try-with-resource

[henryhchchc]

This PR fixes #4368.

[monperrus]

Thanks. Adding a new metamodel interface is a significant change in Spoon, which we do cautiously. Could you elaborate on why it's needed?

[henryhchchc]

Thanks for working on my PR. I think it is well discussed in issue #4368.

[monperrus]

@slarse @SirYwell you were involved in the conversation of #4368

WDYT of this PR?

[SirYwell]

It's basically the way I would have done it too.
The only critical point are the breaking changes. Some of the methods could be overloaded instead of replacing them, but others would require a different name...

I think we can also not get rid of the generic parameter in CtResource as long as it extends CtTypedElement, because that would mean that CtLocalVariable<T> indirectly extends both CtTypedElement<T> and CtTypedElement (raw), which isn't allowed.

Maybe it would make sense to remove CtTypedElement from CtResource, that way we can get rid of a lot of <?>. WDYT?

[slarse]

Essentially what @SirYwell said. We must modify the metamodel here because it's incorrect. We could do it in a less breaking fashion, though. addReasource and removeResource can be overloaded as suggested by @SirYwell. The others can't be overloaded in the way we want, as methods can't be overloaded on return types at all and it's also not possible to overload on type parameters (due to type erasure). So to mostly keep backwards compatibility there, we'd need to add new methods, which will make the interface more awkward.

[slarse]

@monperrus @MartinWitt We should decide what to do here, @henryhchchc has made a nice contribution and I don't like to see it hanging. This is a reasonable solution to the problem, but it is breaking. We therefore can't incorporate it without a major version bump.

Frankly, I can't see a non-breaking solution to this problem.

[monperrus]

Agree with you @slarse

@henryhchchc We need test cases to specify this new feature, would you add them?

[henryhchchc]

Agree with you @slarse

@henryhchchc We need test cases to specify this new feature, would you add them?

Sorry I am too busy to work on the test cases at this moment. 🥲

[slarse]

Sorry I am too busy to work on the test cases at this moment. smiling_face_with_tear

@henryhchchc That's entirely OK, it happens to all of us. Evidently it happened to us as we were very slow to provide feedback here :)

We'll put this on hold for the time being. If this becomes urgent for someone else, they can use your PR as a starting point. Otherwise, it will be here when you get some time again.

I'll check back in a while.

[henryhchchc]

Hi I added a test case checking whether the try-with-resource statements are correctly parsed.

[xzel23]

Hi, IMHO this is a rather important fix (at least for my own project, that's why I comment). I did not look at the new test cases, but I pulled this PR into my own fork of spoon, and it looks good to me: I have try-with-resources both with variable declaration and variable access in my code, and with the PR applied all problems with the latter are fixed while former still work as expected.

[monperrus]

yes, let's finish this one.

added the natural language contracts and a few test lines to fully cover the new code.

I will merge.

thanks a lot @henryhchchc for this valuable contribution.

[monperrus]

FYI This PR causes a regression in NpeFix see https://ci.inria.fr/sos/job/npefix/

I'm working on it for restoring backward compatibility

[slarse]

@monperrus I don't think this can be implemented to be completely non-breaking unless we add a "new api" for the same functionality, see my musings here #4371 (comment)