fix: filter out methods not invoked from an identifier

[pdelagrave]

What's changed?

UseCollectionInterfaces.visitMethodInvocation() will now only proceed if the method was invoked on an expression being an identifier. Anonymous objects don't have an identifier so it doesn't make sense to try to migrate their identifier from a concrete class to an interface.

What's your motivation?

• Fixes UseCollectionInterfaces might fail with UnsupportedOperationException #592

UseCollectionInterfaces.visitMethodInvocation() was crashing trying to update the identifier type of an identifier-less anonymous object method invocations.

Anyone you would like to review specifically?

@JohannisK who worked on adding the override UseCollectionInterfaces.visitMethodInvocation() 2 months ago.

• UseCollectionInterfaces updates methodInvovations type info to enable import removal #517

Anything in particular you'd like reviewers to focus on?

Yes, see next section

Have you considered any alternatives or workarounds?

The (only) change at line 156 is:

if (mi.getSelect() != null && mi.getSelect().getType() != null && mi.getSelect() instanceof J.Identifier) {

could have been:

if (mi.getSelect() != null && mi.getSelect().getType() != null && !(mi.getSelect() instanceof J.NewClass)) {

The added test case based from the bug report did crash specifically because of the visitMethodInvocation() was processing the invocation even if it was for an invocation on an anonymous object which didn't make sense in that context. So skipping it by checking if the expression is exactly that could make sense, but that leave a lot of other expression type that could also not make sense in that context.

Specifying that we only want to process expressions that are an identifier narrows down possibilities a lot and might avoid other bugs.

But I'd like a 2nd opinion as even if so far it looks OK and all the tests are passing, this might narrow down when visitMethodInvocation() runs too much?

Any additional context

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've used the IntelliJ IDEA auto-formatter on affected files

[timtebeek]

Thanks @pdelagrave ! I've made a small change to pull forward the instanceof to where we had a null check previously, as that's then implied by the instanceof. I've also added a new test case that we also didn't cover before, but would be affected by the approach you've taken here, so figured good to add into the mix here and see that fixed as well. Would you be ok to tag team on that?

[timtebeek]

Thanks for working through this one! Good to see the issue resolved and a new case covered.