Skip to content

Change primitive handling in RemoveObjectsIsNull#3918

Merged
timtebeek merged 4 commits into
mainfrom
RemoveObjectsIsNull-primitive-handling
Jan 18, 2024
Merged

Change primitive handling in RemoveObjectsIsNull#3918
timtebeek merged 4 commits into
mainfrom
RemoveObjectsIsNull-primitive-handling

Conversation

@timtebeek

@timtebeek timtebeek commented Jan 15, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Fixes #3915
Fixes #3916

@timtebeek timtebeek self-assigned this Jan 15, 2024
@timtebeek timtebeek added the bug Something isn't working label Jan 15, 2024
@timtebeek

timtebeek commented Jan 15, 2024

Copy link
Copy Markdown
Member Author

Turns out our recipe and tests were handling primitives weirdly; relying on a cast from boolean to Boolean before the method calls; that's corrected now, with a few additional cases covered. We should now no longer make changes when primitives are involved, unless those are Strings.

@timtebeek timtebeek mentioned this pull request Jan 15, 2024
Closed
koppor
koppor reviewed Jan 15, 2024
View reviewed changes

@koppor koppor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sponteneous comments just checking the diff.

Comment thread rewrite-java-test/src/test/java/org/openrewrite/java/RemoveObjectsIsNullTest.java Outdated
if (a != null) {
System.out.println("a is non-null");
public void test(boolean a) {
if (isNull(a)) {

@koppor koppor Jan 15, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This should be changed, shouldn't it?

@timtebeek timtebeek Jan 15, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I had my doubts about this case; when we upcast from boolean to Boolean it doesn't really make sense to even call isNull with an upcast to Boolean for that method call. Perhaps we should completely remove the method call in those cases?

@koppor koppor Jan 15, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Oh, yeah! And replace !isNull by false 😅

@timtebeek timtebeek Jan 15, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yup; I've changed the logic now to add simple true or false instead. In rewrite-launchdarkly we follow such a replacement up with SimplifyConstantIfBranchExecution, but that would require this recipe to move to rewrite-static-analysis. 🤔

Comment thread rewrite-java-test/src/test/java/org/openrewrite/java/RemoveObjectsIsNullTest.java
@timtebeek timtebeek marked this pull request as draft January 15, 2024 16:35
@timtebeek timtebeek marked this pull request as ready for review January 15, 2024 18:55
@timtebeek timtebeek merged commit 3cd3e70 into main Jan 18, 2024
@timtebeek timtebeek deleted the RemoveObjectsIsNull-primitive-handling branch January 18, 2024 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@koppor koppor koppor left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@timtebeek timtebeek

Labels

bug Something isn't working

Projects

OpenRewrite
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

RemoveObjectsIsNull should remove unused import java.util.Objects RemoveObjectsIsNull also rewrites boxed ints

2 participants

@timtebeek @koppor