-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
org.openrewrite.staticanalysis.UnnecessaryThrows failing to detect thrown exception #269
Comments
Thanks for the report @blipper ; I've moved the issue to rewrite-static-analysis as that's where we have the The reason I ask is since the recipe uses the method types it identifies to mark exceptions as thrown rewrite-static-analysis/src/main/java/org/openrewrite/staticanalysis/UnnecessaryThrows.java Lines 105 to 122 in 88f3531
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? |
Yes this.getAllConfigs() is a Lombok generated method. There are three method calls here though. So regardless of status of getAllConfigs it can know. 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. |
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? |
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. |
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 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. |
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 #270 |
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?
What did you expect to see?
Unchaged
What did you see instead?
Blows up with
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
The text was updated successfully, but these errors were encountered: