PatternMatchingInstanceof leaves unnecessary parentheses

[PhilippWendler]

Since ab45284 the PatternMatchingInstanceof check leaves parentheses that are flagged by the UnnecessaryParentheses check. I guess it is not desired that one Error Prone check produces code that another check than complains about?

One particular important case is:

if (o instanceof Test) {
  Test t = ((Test) o);
}

The parentheses make Error Prone miss the fact that it can eliminate the variable declaration and directly use o instanceof Test t.

Other cases found inside our code case and flagged by UnnecessaryParentheses are in return statements, inside divisions, and ternary operators. It is easy to extend PatternMatchingInstanceof.requiresParentheses() and fix particular cases, and I could do so in a PR, but I wonder whether this is the best approach, or whether the function should instead use a list of cases with required parentheses? So I wanted to clarify this upfront.

[cushon]

It would be nice if PatternMatchingInstanceof handled all these cases. Would you not get the same result by running UnnecessaryParentheses first? Maybe that check does things you don't want?

I wonder whether this is the best approach, or whether the function should instead use a list of cases with required parentheses?

Maybe, I'm not sure. The risk of checking for cases where parentheses are required is that if we miss them the check produces bad findings, instead of missing a finding. Either way I find it hard to remember all of the cases where they are/aren't required. I'm open to being convinced that handling the cases where they're required is simpler :)

[PhilippWendler]

It would be nice if PatternMatchingInstanceof handled all these cases. Would you not get the same result by running UnnecessaryParentheses first? Maybe that check does things you don't want?

Unfortunately, no. We do run Error Prone as part of our standard build already and have all suggestions of UnnecessaryParentheses applied.

For the assignment example from my post, extending UnnecessaryParentheses to flag it could make sense. But cases like boolean foo = (bar == baz); imply for me that UnnecessaryParentheses should not flag all top-level parentheses on the right-hand side of an assignment, so it would need some more complex logic, and I don't think the complexity seems worth it.

And furthermore, for other cases I noticed in our code base, like ((Double) d) / 2.0, I can understand that developers would like to have the parentheses (if they are not even required), but after applying PatternMatchingInstanceof this should of course be d / 2.0 and not (d) / 2.0.

I wonder whether this is the best approach, or whether the function should instead use a list of cases with required parentheses?

Maybe, I'm not sure. The risk of checking for cases where parentheses are required is that if we miss them the check produces bad findings, instead of missing a finding. Either way I find it hard to remember all of the cases where they are/aren't required. I'm open to being convinced that handling the cases where they're required is simpler :)

I am also unsure, that's why I asked. :-) Ideal would be a way to distinguish this kind of parentheses from parentheses that are part of the syntax of some other construct. But I don't know whether the AST provides this information.

[graememorgan]

PatternMatchingInstanceof.requiresParentheses

There's no such method, so I'm not quite sure what you mean. I mailed a fix though.

I'd be in favour of UnnecessaryParentheses firing on foo = (a); even for the case in the above comment, but I can see that being controversial.

[PhilippWendler]

PatternMatchingInstanceof.requiresParentheses

There's no such method, so I'm not quite sure what you mean. I mailed a fix though.

I was referring to this:

error-prone/core/src/main/java/com/google/errorprone/bugpatterns/PatternMatchingInstanceof.java

Line 272 in b20b019

private static boolean requiresParentheses(TreePath path) {

If you have a fix, great, I am willing to test.

[graememorgan]

Apparently I should have gone to Specsavers. :)