Skip to content

WIP: fix EqualsAvoidsNull not idempotent#487

Closed
Pankraz76 wants to merge 8 commits into
openrewrite:mainfrom
Pankraz76:test-equalsAvoidsNullNonIdempotent
Closed

WIP: fix EqualsAvoidsNull not idempotent#487
Pankraz76 wants to merge 8 commits into
openrewrite:mainfrom
Pankraz76:test-equalsAvoidsNullNonIdempotent

Conversation

@Pankraz76

@Pankraz76 Pankraz76 commented Mar 25, 2025

Copy link
Copy Markdown
Contributor

What's changed?

test EqualsAvoidsNull idempotency

What's your motivation?

@github-project-automation github-project-automation Bot moved this to In Progress in OpenRewrite Mar 25, 2025
@Pankraz76

Copy link
Copy Markdown
Contributor Author

still not idempotent imho:

image

@greg-at-moderne

@Pankraz76 Pankraz76 marked this pull request as ready for review March 25, 2025 17:21
@Pankraz76 Pankraz76 changed the title WIP: EqualsAvoidsNull not idempotent WIP: fix EqualsAvoidsNull not idempotent Mar 25, 2025
@Pankraz76 Pankraz76 force-pushed the test-equalsAvoidsNullNonIdempotent branch from 6903257 to b26b5a5 Compare March 25, 2025 19:16
Comment thread src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java
Comment thread src/main/java/org/openrewrite/staticanalysis/EqualsAvoidsNull.java
Comment on lines +20 to 21
import org.jetbrains.annotations.NotNull;
import org.jspecify.annotations.Nullable;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
import org.jetbrains.annotations.NotNull;
import org.jspecify.annotations.Nullable;
import org.jspecify.annotations.Nullable;

/**
* @implNote {@link MultipleVariableDeclarationsVisitor#visitBlock(J.Block, ExecutionContext)}
*/
private J.@NotNull Block visitBlock(J.Block b) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
private J.@NotNull Block visitBlock(J.Block b) {
private J. Block visitBlock(J.Block b) {

if (!isStringComparisonMethod(m) || !hasCompatibleArgument(m) || m.getSelect() instanceof J.Literal) {
return m;
}
public J.Block visitBlock(J.Block block, ExecutionContext ctx) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The variable declaration lookup kind of works:

Variable Declaration Lookup

Now the timing is currently wrong as visitBlock gets called after the visitMethodInvocation function, which then needs to check the pre-built VARIABLE_DECLARATIONS cache to determine which ones to flip and which ones not to. I'm sure you guys can help me or easily take over to add the missing parts.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

what do you think, is this any good? @greg-at-moderne @timtebeek

@Pankraz76

Copy link
Copy Markdown
Contributor Author

feel free to overtake or advisory, as im not able to fix this.

Thanks @timtebeek

@Pankraz76

Copy link
Copy Markdown
Contributor Author

nicely fixed by @greg-at-moderne

@Pankraz76 Pankraz76 closed this Apr 8, 2025
@github-project-automation github-project-automation Bot moved this from In Progress to Done in OpenRewrite Apr 8, 2025
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.

2 participants