Skip to content

Do not pull Supertype-Cast to instanceof #577

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

Merged
merged 1 commit into from
May 29, 2025
Merged

Conversation

dnl50
Copy link
Contributor

@dnl50 dnl50 commented May 29, 2025

When the cast type expression is a supertype of the instanceof type, the recipe should back off. The code semantics could change otherwise, because more instances can potentially pass the check. Fixes GH-572.

What's changed?

Type casts after instanceof checks which use a different type than the one used in the instanceof check itself will now be ignored since changing the instanceof type might alter the code semantics.

What's your motivation?

Issue #572

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

The isCheckedCastCompatible might be extended even further to take generic type variables into account. Cases like these could also be migrated, since the type variables are the same:

// java 11
class A<T> {
   void test(List<T> o) {
       if (o instanceof ArrayList<?>) {
           ((ArrayList<T>) o).trimToSize();
       }
   }
}

// java 16+ where the reifiable type-restriction was lifted
class A<T> {
   void test(List<T> o) {
       if (o instanceof ArrayList<T> list) {
           list.trimToSize();
       }
   }
}

The isCheckedCastCompatible might be a duplicate of existing code, but I didn't find any publicly accessible methods which perform such checks ☹️

Have you considered any alternatives or workarounds?

In the first try I just flipped the type TypeUtils#isAssignableTo parameters in the registerTypeCast method, but then subtype casts were pulled up to the instanceof check, resulting in the same problem of changed semantics.

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, since the formatting differs a bit from the existing code when using the IntelliJ formatter

When the cast type expression is a supertype of the instanceof type,
the recipe should back off. The code semantics could change
otherwise, because more instances can potentially pass the check.
Fixes openrewriteGH-572.
@github-project-automation github-project-automation bot moved this to In Progress in OpenRewrite May 29, 2025
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggestions could not be made:

  • src/main/java/org/openrewrite/staticanalysis/CustomImportOrder.java
    • lines 24-24

@sambsnyd sambsnyd merged commit 3ec7c4a into openrewrite:main May 29, 2025
2 checks passed
@github-project-automation github-project-automation bot moved this from In Progress to Done in OpenRewrite May 29, 2025
@sambsnyd
Copy link
Member

Thanks @dnl50 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

InstanceofPatternMatching recipe prefers type of cast instead of instanceof check
2 participants