Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[java] EmptyControlStatement should not allow empty try with concise resources #4930

Closed
oowekyala opened this issue Apr 4, 2024 · 0 comments · Fixed by #4934
Closed

[java] EmptyControlStatement should not allow empty try with concise resources #4930

oowekyala opened this issue Apr 4, 2024 · 0 comments · Fixed by #4934
Labels
a:false-negative PMD doesn't flag a problematic piece of code
Milestone

Comments

@oowekyala
Copy link
Member

oowekyala commented Apr 4, 2024

Affects PMD Version: 7.0.0

Rule: EmptyControlStatement

Description:
Since #432 was closed EmptyControlStatement allows empty try-with-resources, provided the resources are named with a name like ignored.

I think this should only be done for resources that don't use shorthand form. For instance in our tests:

This looks like an anti pattern to me. If you're going to use an empty try statement for this you should write in.close() because it has the same effect but is clearer.

On the other hand the following should probably still be allowed:

try (ClientResponse ignored = execute(() -> target.request(mediaTypes).delete(), DELETE, new ExpectedResponse(status, required))) {
}

Here an explicit ignored name is given to an expression. Even if it could be written

ClientResponse response = execute(() -> target.request(mediaTypes).delete(), DELETE, new ExpectedResponse(status, required)));
response.close();

this code would pollute the enclosing scope with a useless variable name.

Another case to keep in mind is the following:

try (in) {
} catch (Foo e) {
  // do something
}

If there is at least one catch block then it should be ok to keep the try block empty, even if the resource is unnamed.

Probably the rule should actually not care about whether the variables are named ignored or the like. If the variable is not used it will be flagged by UnusedVariable already.

Expected outcome:

PMD should report a violation at line ..., but doesn't. This is a false-negative.

Running PMD through: [CLI | Ant | Maven | Gradle | Designer | Other]

@oowekyala oowekyala added the a:false-negative PMD doesn't flag a problematic piece of code label Apr 4, 2024
@oowekyala oowekyala changed the title [java] EmptyControlStatement should not allow empty try with resources [java] EmptyControlStatement should not allow empty try with concise resources Apr 4, 2024
@adangel adangel added this to the 7.2.0 milestone May 2, 2024
@adangel adangel closed this as completed in bca5c0e May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:false-negative PMD doesn't flag a problematic piece of code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants