org.openrewrite.staticanalysis.UnnecessaryThrows failing to detect thrown exception

[blipper]

What version of OpenRewrite are you using?

I am using 6.6.3

How are you running OpenRewrite?

What is the smallest, simplest way to reproduce the problem?

import com.fasterxml.jackson.core.JsonProcessingException;

public class PlacementConfig
    private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
    public Map<String, Object> asMap() throws JsonProcessingException
    {
        return OBJECT_MAPPER.treeToValue(OBJECT_MAPPER.valueToTree(this.getAllConfigs()), Map.class);
    }
}

What did you expect to see?

Unchaged

What did you see instead?

public class PlacementConfig
    private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
    public Map<String, Object> asMap() 
    {
        return OBJECT_MAPPER.treeToValue(OBJECT_MAPPER.valueToTree(this.getAllConfigs()), Map.class);
    }
}

Blows up with

/workplace/mnr/InsistorRedux/src/JavelinRenderingService/src/com/amazon/javelinrenderingservice/cache/model/PlacementConfig.java:147: error: unreported exception JsonProcessingException; must be caught or declared to be thrown
        return OBJECT_MAPPER.treeToValue(OBJECT_MAPPER.valueToTree(this.getAllConfigs()), Map.class);

Some how https://github.com/openrewrite/rewrite-static-analysis/blob/main/src/main/java/org/openrewrite/staticanalysis/UnnecessaryThrows.java#L106 is not filtering?

Are you interested in [contributing a fix to OpenRewrite]

Yes

[timtebeek]

Thanks for the report @blipper ; I've moved the issue to rewrite-static-analysis as that's where we have the UnnecessaryThrows recipe. In your example above, might that this.getAllConfigs() be a Lombok generated method?

The reason I ask is since the recipe uses the method types it identifies to mark exceptions as thrown https://github.com/openrewrite/rewrite-static-analysis/blob/88f3531b9a5fad4ebb97d0222b38c969012969b2/src/main/java/org/openrewrite/staticanalysis/UnnecessaryThrows.java#L105-L122

When one of the arguments to a method lacks a type, such as is often the case with Lombok generated methods, then we can't determine the type for the method being called, which in the above case would lead to the exception not being marked as used, and then removed later on.

While full blown support for Lombok is a larger effort, perhaps we can refine our type attribution to pick the correct method type despite lacking a type on an argument, if and only if there's no ambiguity due to overloaded method arguments. 🤔

Can you confirm if there might be missing method types here due to lombok or some other reason?

[blipper]

Yes this.getAllConfigs() is a Lombok generated method.

There are three method calls here though. treeToValue <- with the throws valueToTree <- with the throws getAllConfigs <- Lombok and no throws

So regardless of status of getAllConfigs it can know.
I agree with iff due to the overloaded method arguments. But you can simplify and just don't remove it if any ambiguity.

Structurally this is a fail-unsafe approach because you assume that it is okay to remove unless you can prove it is used. I would think you would want assume not okay unless you can prove NOT used.

[timtebeek]

Agree with you there that a safer default would be nice; it's tough though looking at only an import whether that's used when the type system is incomplete. What we could do, and do elsewhere is hold of on removing any unused imports if there are any missing types. That potentially will render this recipe mute in a lot of cases, in particular when any lombok annotated method is used, but would prevent accidental removal of imports where we can't be 100% sure from the type system. How would you feel about that?

[blipper]

So tactically I would change the logic such that if any nested method call (regardless of its input params are known or not) have throws it should not remove. Right now for some reason the getMethodType is null due to it not being fully known. All you need to check is if the method throws that or not.

The broader question we can probably defer since if we fix this it is likely the most common failure.

[timtebeek]

Perhaps good to note though here is that we can't determine the type of the outer method calls if the type of the inner methods is unknown.

In this case we don't know what getAllConfigs() returns (due to it being a Lombok generated method we can't see),
which then means we don't know the type of the argument going into valueToTree,
which means we can't determine the type of valueToTree and what that returns,
which means we can determine the type of the arguments or method for treeToValue.

It's a bit of a layered issue, and why I'm not a fan of not doing any replacements if any type isn't known, as this would be quite common when folks use Lombok at all.

A perhaps more promising would be to determine the method types despite missing types if there are no other overloads. Not sure if that would have helped here, but pointing it out for other cases where that might be an option.

In short: it's tough wanting to remain safe and predictable in the face of unknown types. We tend to favor safety where we can, and maybe should here as well to not make any changes when we can't find types on some LST elements.

[blipper]

Sorry yes I should have specified if there is only a single method variant (i.e. No overloading).

I filed another bug in a similar vein which is slightly different here https://github.com/openrewrite/rewrite-static-analysis/issues/270