[java] ConsecutiveLiteralAppends false positive

[prophet1906]

PR Description:
ASTTryStatement added to BLOCK_PARENTS.
fixes #1632

[pmd-test]

	1 Message
📖	This changeset introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 0 violations, 2 errors and 2 configuration errors. Full report
✅	This changeset introduces 0 new violations, 0 new errors and 0 new configuration errors, removes 0 violations, 2 errors and 2 configuration errors. Full report

Generated by 🚫 Danger

[jsotuyod]

@Shubham-2k17 thanks for the PR! This does cover the reported scenario, but I think we can improve it a little to cover some related use cases.

[jsotuyod]

catch and finally should be added too. A simple test-case having 2 catch blocks do different appends / a catch + finally doing appends would trigger these FP too.

[jsotuyod]

I'm thinking, we actually may have similar FPs with lambdas, and we should add ASTLambdaExpression.class here…

[prophet1906]

I tried to check lambdas. No FPs.

[vbrandl]

I don't know the PMD codebase so I don't know what exactly this PR will change, but I would guess, it would just ignore literals, that are appended inside a try/catch block?

So this won't trigger the rule while it should:

final StringBuilder sb = new StringBuilder();
sb.append("foo");
try {
  sb.append("bar");
  final String res = methodThatMightThrow();
  sb.append(res);
} catch (IOException ioe) {
  ...
}

This is only a false positive, if the literal is appended inside the try/catch block, after a throwing method was called. I don't know if this case is covered by the PR.

[prophet1906]

Hi @vbrandl,
The PR is currently not covering the case where method throwing exception comes after append in try catch.

[prophet1906]

Hi @jsotuyod ,
I think we should not add ASTTryStatement, ASTCatchStatement, ASTFinallyStatement to BLOCK_PARENTS & check if the parameter inside append is a literal/constant expression. This may solve the problem. Do suggest what is the correct way.

[jsotuyod]

@Shubham-2k17 @vbrandl interesting! I think you are actually right. ASTTryStatement shouldn't be in the block parents, but catch / finally do.

I'm updating, adding that test case along with the lambda ones (which works as already stated), and merging.