Enhance InstanceOfPatternMatch to cover method invocations as well

[timo-abele]

What problem are you trying to solve?

I've come across

if (authentication.getPrincipal() instanceof UserDetails) {
    UserDetails springSecurityUser = (UserDetails) authentication.getPrincipal();
    return springSecurityUser.getUsername();
}

which is currently not covered

Describe the solution you'd like

if (authentication.getPrincipal() instanceof UserDetails springSecurityUser) {
    return springSecurityUser.getUsername();
}

[knutwannheden]

I think we don't do anything here on purpose, as we currently aren't able to determine whether a method call can have side effects or not. So if in your case the getPrincipal() method had any side effects (e.g. audit logging or generate metrics), after the refactoring there would now only be one call, rather than two. With the mentioned examples of side effects, that would arguably be better, but there could be other cases, where the side effect is required.

That being said, we could consider adding an option to allow the recipe to cover such cases. After all, there are still developers who need to review the changes. But I think I lean towards waiting until we can do the analysis regarding the side effects. What are your thoughts on this, @timtebeek ?

[timtebeek]

Indeed one of those cases where on the face of it this would be a good addition, but there's no guarantee that no one is doing something like the following contrived example, which isn't safe to convert.

if (authentication.getNext() instanceof UserDetails) {
    UserDetails springSecurityUser = (UserDetails) authentication.getNext();
    return springSecurityUser.getUsername();
}

It's at times slightly frustrating not to improve code, but we do have to be sure we do no harm, as our users have come to expect.

[timo-abele]

I came up with this feature request because IntelliJ suggested it to me (in a situation that was save). I think in most cases where this pattern comes up the method invocation is a pure getter, so requiring the method invocation to be a pure getter (e.g. one statement: return field;) would create a negligible number of false negatives (i.e. safe opportunities to convert that are skipped) and no false positives.

[knutwannheden]

IntelliJ, by comparison, has the necessary means to know if a method call is pure or not. It would be great if we could add that, too.