Fix several issues in InstanceOfPatternMatch by cortlepp · Pull Request #489 · openrewrite/rewrite-static-analysis

cortlepp · 2025-03-26T14:16:03Z

What's changed?

always use the type in the cast for the new pattern and not the previously checked instanceof type.
when a variable name is already present in the source, don't search for duplicates and just take it as it is. Otherwise we would have to make unnecessary changes when the variable happens to be shadowing something else.
refuse to pattern-match non-wildcard generic types, as this cannot be checked due to type erasure.

What's your motivation?

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

I'll annotate some areas in the files themselves.

Anyone you would like to review specifically?

@greg-at-moderne

Have you considered any alternatives or workarounds?

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

see openrewrite#482

see openrewrite#483

see openrewrite#484

- always set the clazz of a InstanceOfPatternMatch to the casted class (don't keep the one from the original type check) - back off if the casted class is a non-wildcard generic - adapt `Arrays#string()` test to new behavior

…penrewrite#308)

…es (openrewrite#483,openrewrite#484) - doing so causes problems when the preexisting name shadows another variable, because our duplicate checking does not consider shadowing.

cortlepp · 2025-03-26T14:19:41Z

-                    null));
+                    typeCastTypeTree.getType(),
+                    null))
+                    .withClazz(typeCastTypeTree.withPrefix(currentTypeTree.getPrefix()).withId(Tree.randomId()));


It seems superfluous or possibly even wrong that we have to pass the type to the Identifier and also set it as clazz on the J.InstanceOf. Maybe this could be avoided with Java template, but I was unsure how one would look for this case.

I assume you mean a reference to line 263 here.

It seems superfluous or possibly even wrong that we have to pass the type to the Identifier and also set it as clazz on the J.InstanceOf

Not sure I understand why you find it superfluous. All identifiers need a type (so that our LST is typed - i.e. all elements have proper type information attributed). And arguably o instanceof String s1 is a declaration of o Identifier and this needs type.

I guess I thought that by giving the identifier a type in line 263, this should determine the type for the entire expression and I should not have to set it again as the clazz. To me it seems like clazz should always be the same as the type passed in line 263, therefore I thought maybe I was using the class wrong or I misunderstood something generally.

timtebeek

Thanks a lot for the multiple fixes here @cortlepp ! @greg-at-moderne will have a look shortly and see if we can get this in before the release

greg-at-moderne

I appreciate the great contribution. I consider it a piece of high quality work. Well tested, thoughtful and solving a few overlapping cases in one go!

There's one minor outstanding issue and I think we are good to merge.

(Also, I acknowledge your questions on some of the linked issues, I will get to them).

greg-at-moderne · 2025-03-26T16:04:04Z

-                    null));
+                    typeCastTypeTree.getType(),
+                    null))
+                    .withClazz(typeCastTypeTree.withPrefix(currentTypeTree.getPrefix()).withId(Tree.randomId()));


I assume you mean a reference to line 263 here.

greg-at-moderne · 2025-03-26T16:06:13Z

-                    null));
+                    typeCastTypeTree.getType(),
+                    null))
+                    .withClazz(typeCastTypeTree.withPrefix(currentTypeTree.getPrefix()).withId(Tree.randomId()));


It seems superfluous or possibly even wrong that we have to pass the type to the Identifier and also set it as clazz on the J.InstanceOf

Not sure I understand why you find it superfluous. All identifiers need a type (so that our LST is typed - i.e. all elements have proper type information attributed). And arguably o instanceof String s1 is a declaration of o Identifier and this needs type.

- no longer discover generic assignments if they aren't wildcards - adapt `catTooSpecific` test to again cover the originally reported issue

…nto fix/instance-of-pattern-match

greg-at-moderne · 2025-04-02T18:08:24Z

Does it make sense to you?
Not trying to put any pressure or anything like this. Just wondering if you are blocked by anything here?

cortlepp · 2025-04-02T19:12:20Z

Hey, I'm currently on vacation until next week, I'll take a look at it as soon as I return. (Sorry I should've said something, I just forgot)

…ignments (openrewrite#482)

greg-at-moderne · 2025-04-07T07:34:52Z

Not sure what's happening with Github, might be some glitch. I have clicked the button to approve CI run a few times already for this PR.

cortlepp · 2025-04-07T07:51:35Z

I think the run went through now (on your merge commit).

greg-at-moderne

I am satisfied with this change. I appreciate the contribution.

arodionov · 2025-04-26T19:28:32Z

It looks like the current PR create an issue #528

cortlepp added 6 commits March 26, 2025 14:43

test: InstanceOfPatternMatch too specific generic cast (openrewrite#482)

03d7676

see openrewrite#482

test: InstanceOfPatternMatch class variable shadowing (openrewrite#483)

d9a0836

see openrewrite#483

test: InstanceOfPatternMatch enums (openrewrite#484)

7b7756f

see openrewrite#484

fix: InstanceOfPatternMatch generics (openrewrite#482)

3f75b5e

- always set the clazz of a InstanceOfPatternMatch to the casted class (don't keep the one from the original type check) - back off if the casted class is a non-wildcard generic - adapt `Arrays#string()` test to new behavior

test: InstanceOfPatternMatch pass raw type to parameterized method (o…

92d8d7f

…penrewrite#308)

fix: exact VariableNameStrategy strategy should not look for duplicat…

5376996

…es (openrewrite#483,openrewrite#484) - doing so causes problems when the preexisting name shadows another variable, because our duplicate checking does not consider shadowing.

moderne-meeseeks Bot added this to OpenRewrite Mar 26, 2025

github-project-automation Bot moved this to In Progress in OpenRewrite Mar 26, 2025

cortlepp commented Mar 26, 2025

View reviewed changes

timtebeek assigned cortlepp Mar 26, 2025

timtebeek requested a review from greg-at-moderne March 26, 2025 15:43

timtebeek added the bug Something isn't working label Mar 26, 2025

Merge branch 'main' into fix/instance-of-pattern-match

38411da

timtebeek reviewed Mar 26, 2025

View reviewed changes

greg-at-moderne approved these changes Mar 26, 2025

View reviewed changes

github-project-automation Bot moved this from In Progress to Ready to Review in OpenRewrite Mar 26, 2025

cortlepp added 2 commits March 26, 2025 18:45

fix: don't discover too specific generic assignments (openrewrite#482)

2183b38

- no longer discover generic assignments if they aren't wildcards - adapt `catTooSpecific` test to again cover the originally reported issue

Merge remote-tracking branch 'origin/fix/instance-of-pattern-match' i…

be34632

…nto fix/instance-of-pattern-match

cortlepp commented Mar 26, 2025

View reviewed changes

Comment thread src/main/java/org/openrewrite/staticanalysis/InstanceOfPatternMatch.java

timtebeek requested a review from greg-at-moderne March 26, 2025 22:06

cortlepp and others added 2 commits April 7, 2025 09:16

fix: don't rewrite instanceof patterns with too specific variable ass…

bd000db

…ignments (openrewrite#482)

Merge branch 'main' into fix/instance-of-pattern-match

2fc281d

greg-at-moderne approved these changes Apr 7, 2025

View reviewed changes

greg-at-moderne merged commit 3f49a05 into openrewrite:main Apr 7, 2025

github-project-automation Bot moved this from Ready to Review to Done in OpenRewrite Apr 7, 2025

arodionov mentioned this pull request Apr 26, 2025

InstanceOfPatternMatch recipe replaces wrapper types with primitives in instanceof #528

Closed

Uh oh!

Conversation

cortlepp commented Mar 26, 2025 • edited by timtebeek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What's changed?

What's your motivation?

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

Anyone you would like to review specifically?

Have you considered any alternatives or workarounds?

Any additional context

Checklist

Uh oh!

cortlepp Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

greg-at-moderne Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

greg-at-moderne Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

cortlepp Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

timtebeek left a comment

Choose a reason for hiding this comment

Uh oh!

greg-at-moderne left a comment

Choose a reason for hiding this comment

Uh oh!

greg-at-moderne Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

greg-at-moderne Mar 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

greg-at-moderne commented Apr 2, 2025

Uh oh!

cortlepp commented Apr 2, 2025

Uh oh!

greg-at-moderne commented Apr 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cortlepp commented Apr 7, 2025

Uh oh!

greg-at-moderne left a comment

Choose a reason for hiding this comment

Uh oh!

arodionov commented Apr 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cortlepp commented Mar 26, 2025 •

edited by timtebeek

Loading

greg-at-moderne commented Apr 7, 2025 •

edited

Loading