From be5ee6304c8153c46cfdcfc1d40b9f1310911941 Mon Sep 17 00:00:00 2001 From: martinfrancois Date: Sun, 9 Aug 2026 16:30:03 +0200 Subject: [PATCH 1/4] RemoveRedundantNullCheckBeforeInstanceof: only simplify side-effect-free expressions `expr != null && expr instanceof T` evaluates `expr` twice, while the `expr instanceof T` the recipe leaves behind evaluates it once. That is only equivalent when evaluating `expr` has no side effects. `SemanticallyEqual` proves that the two occurrences mean the same thing, not that evaluating them twice is the same as evaluating them once, so both the direct and the chained `&&` branch could silently drop a call: `next() != null && next() instanceof String` became `next() instanceof String`. Gate both branches on the existing package-private `SideEffects` helper, which already backs the purity guards in `RemoveDuplicateConditions`, `AllBranchesIdentical` and `SimplifyRedundantLogicalExpression`. Local variables, parameters and plain field access still simplify; method invocations, constructor calls, assignments and increments no longer do. Reviewers should note that the existing test `removeRedundantNullCheckWithMethodInvocation` asserted the old behaviour, so it becomes the no-change case `doNotChangeWhenMethodInvocation`. --- ...oveRedundantNullCheckBeforeInstanceof.java | 7 +- ...edundantNullCheckBeforeInstanceofTest.java | 108 +++++++++++++++++- 2 files changed, 111 insertions(+), 4 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeInstanceof.java b/src/main/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeInstanceof.java index 6b2714c68..ed95c9cfb 100644 --- a/src/main/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeInstanceof.java +++ b/src/main/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeInstanceof.java @@ -31,6 +31,7 @@ import java.util.Set; import static java.util.Collections.singleton; +import static org.openrewrite.staticanalysis.SideEffects.mayHaveSideEffects; @EqualsAndHashCode(callSuper = false) @Value @@ -90,7 +91,11 @@ public J visitBinary(J.Binary binary, ExecutionContext ctx) { } private boolean isRedundantNullCheck(J.Binary nullCheck, J.InstanceOf instanceOf) { - if (nullCheck.getOperator() == J.Binary.Type.NotEqual) { + // Dropping the null check evaluates the expression once where it was evaluated twice, so it is only + // equivalent when evaluating it has no side effects; `SemanticallyEqual` proves the two occurrences + // mean the same thing, not that evaluating them twice is the same as evaluating them once + if (nullCheck.getOperator() == J.Binary.Type.NotEqual && + !mayHaveSideEffects(instanceOf.getExpression())) { if (J.Literal.isLiteralValue(nullCheck.getLeft(), null)) { return SemanticallyEqual.areEqual(nullCheck.getRight(), instanceOf.getExpression()); } diff --git a/src/test/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeInstanceofTest.java b/src/test/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeInstanceofTest.java index a1ad654a6..c6a7fbc69 100644 --- a/src/test/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeInstanceofTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeInstanceofTest.java @@ -88,8 +88,9 @@ void foo(Object obj) { } + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/953") @Test - void removeRedundantNullCheckWithMethodInvocation() { + void doNotChangeWhenMethodInvocation() { rewriteRun( //language=java java( @@ -105,11 +106,21 @@ String getValue() { return "test"; } } - """, + """ + ) + ); + } + + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/953") + @Test + void doNotChangeWhenMethodInvocationWithNullOnLeft() { + rewriteRun( + //language=java + java( """ class A { void foo() { - if (getValue() instanceof String) { + if (null != getValue() && getValue() instanceof String) { System.out.println("String value"); } } @@ -123,6 +134,97 @@ String getValue() { ); } + @Test + void removeRedundantNullCheckInChainedCondition() { + rewriteRun( + //language=java + java( + """ + class A { + void foo(boolean enabled, Object obj) { + if (enabled && obj != null && obj instanceof String) { + System.out.println("String value"); + } + } + } + """, + """ + class A { + void foo(boolean enabled, Object obj) { + if (enabled && obj instanceof String) { + System.out.println("String value"); + } + } + } + """ + ) + ); + } + + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/953") + @Test + void doNotChangeWhenChainedConditionHasMethodInvocation() { + rewriteRun( + //language=java + java( + """ + class A { + void foo(boolean enabled) { + if (enabled && getValue() != null && getValue() instanceof String) { + System.out.println("String value"); + } + } + + String getValue() { + return "test"; + } + } + """ + ) + ); + } + + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/953") + @Test + void doNotChangeWhenConstructorCall() { + rewriteRun( + //language=java + java( + """ + class A { + void foo() { + if (new StringBuilder() != null && new StringBuilder() instanceof CharSequence) { + System.out.println("CharSequence value"); + } + } + } + """ + ) + ); + } + + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/953") + @Test + void doNotChangeWhenArrayIndexHasSideEffect() { + rewriteRun( + //language=java + java( + """ + class A { + Object[] values = new Object[2]; + int i; + + void foo() { + if (values[i++] != null && values[i++] instanceof String) { + System.out.println("String value"); + } + } + } + """ + ) + ); + } + @Test void removeRedundantNullCheckWithFieldAccess() { rewriteRun( From aba6f51ef4e79a74c306fada22185475fe5f8761 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 11 Aug 2026 09:23:11 +0200 Subject: [PATCH 2/4] Review fixes: require both compared operands to be side-effect free --- ...oveRedundantNullCheckBeforeInstanceof.java | 23 ++++++++--------- ...edundantNullCheckBeforeInstanceofTest.java | 25 +++++++++++++++++++ 2 files changed, 36 insertions(+), 12 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeInstanceof.java b/src/main/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeInstanceof.java index ed95c9cfb..83c8860fc 100644 --- a/src/main/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeInstanceof.java +++ b/src/main/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeInstanceof.java @@ -91,19 +91,18 @@ public J visitBinary(J.Binary binary, ExecutionContext ctx) { } private boolean isRedundantNullCheck(J.Binary nullCheck, J.InstanceOf instanceOf) { - // Dropping the null check evaluates the expression once where it was evaluated twice, so it is only - // equivalent when evaluating it has no side effects; `SemanticallyEqual` proves the two occurrences - // mean the same thing, not that evaluating them twice is the same as evaluating them once - if (nullCheck.getOperator() == J.Binary.Type.NotEqual && - !mayHaveSideEffects(instanceOf.getExpression())) { - if (J.Literal.isLiteralValue(nullCheck.getLeft(), null)) { - return SemanticallyEqual.areEqual(nullCheck.getRight(), instanceOf.getExpression()); - } - if (J.Literal.isLiteralValue(nullCheck.getRight(), null)) { - return SemanticallyEqual.areEqual(nullCheck.getLeft(), instanceOf.getExpression()); - } + if (nullCheck.getOperator() != J.Binary.Type.NotEqual) { + return false; + } + Expression checked = J.Literal.isLiteralValue(nullCheck.getLeft(), null) ? nullCheck.getRight() : + J.Literal.isLiteralValue(nullCheck.getRight(), null) ? nullCheck.getLeft() : null; + if (checked == null || !SemanticallyEqual.areEqual(checked, instanceOf.getExpression())) { + return false; } - return false; + // The rewrite evaluates the expression once where it was evaluated twice, so both occurrences must be + // side-effect free; they can differ, as `SemanticallyEqual` matches a static field access against its + // qualified form without comparing the qualifier. + return !mayHaveSideEffects(checked) && !mayHaveSideEffects(instanceOf.getExpression()); } }); } diff --git a/src/test/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeInstanceofTest.java b/src/test/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeInstanceofTest.java index c6a7fbc69..197a05af8 100644 --- a/src/test/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeInstanceofTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeInstanceofTest.java @@ -225,6 +225,31 @@ void foo() { ); } + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/953") + @Test + void doNotChangeWhenOnlyTheNullCheckedOperandHasSideEffects() { + rewriteRun( + //language=java + java( + """ + class A { + static Integer count = 1; + + A getInstance() { + return this; + } + + void foo() { + if (getInstance().count != null && count instanceof Integer) { + System.out.println("Integer value"); + } + } + } + """ + ) + ); + } + @Test void removeRedundantNullCheckWithFieldAccess() { rewriteRun( From 728edcfa018e0e2dc26ba2a848d722ff98c6937e Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Wed, 12 Aug 2026 00:25:38 +0200 Subject: [PATCH 3/4] Trim commentary --- .../RemoveRedundantNullCheckBeforeInstanceof.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeInstanceof.java b/src/main/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeInstanceof.java index 83c8860fc..b7c8d35ce 100644 --- a/src/main/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeInstanceof.java +++ b/src/main/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeInstanceof.java @@ -99,9 +99,8 @@ private boolean isRedundantNullCheck(J.Binary nullCheck, J.InstanceOf instanceOf if (checked == null || !SemanticallyEqual.areEqual(checked, instanceOf.getExpression())) { return false; } - // The rewrite evaluates the expression once where it was evaluated twice, so both occurrences must be - // side-effect free; they can differ, as `SemanticallyEqual` matches a static field access against its - // qualified form without comparing the qualifier. + // The rewrite evaluates once what was evaluated twice, so both occurrences must be side-effect + // free; they can differ, as `SemanticallyEqual` matches a static field access against its qualified form return !mayHaveSideEffects(checked) && !mayHaveSideEffects(instanceOf.getExpression()); } }); From af7f05a5befc1bf8f0bdc648d12fe12cf6c8ecda Mon Sep 17 00:00:00 2001 From: martinfrancois Date: Sun, 16 Aug 2026 15:16:18 +0200 Subject: [PATCH 4/4] Consolidate side-effect regression tests --- ...edundantNullCheckBeforeInstanceofTest.java | 52 +++---------------- 1 file changed, 7 insertions(+), 45 deletions(-) diff --git a/src/test/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeInstanceofTest.java b/src/test/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeInstanceofTest.java index 197a05af8..b369b3abe 100644 --- a/src/test/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeInstanceofTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeInstanceofTest.java @@ -87,40 +87,25 @@ void foo(Object obj) { ); } - @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/953") @Test - void doNotChangeWhenMethodInvocation() { + void doNotChangeWhenNullCheckedExpressionIsMethodInvocation() { rewriteRun( //language=java java( """ class A { - void foo() { + void direct() { if (getValue() != null && getValue() instanceof String) { System.out.println("String value"); } + if (null != getValue() && getValue() instanceof String) { + System.out.println("String value"); + } } - String getValue() { - return "test"; - } - } - """ - ) - ); - } - - @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/953") - @Test - void doNotChangeWhenMethodInvocationWithNullOnLeft() { - rewriteRun( - //language=java - java( - """ - class A { - void foo() { - if (null != getValue() && getValue() instanceof String) { + void chained(boolean enabled) { + if (enabled && getValue() != null && getValue() instanceof String) { System.out.println("String value"); } } @@ -161,29 +146,6 @@ void foo(boolean enabled, Object obj) { ); } - @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/953") - @Test - void doNotChangeWhenChainedConditionHasMethodInvocation() { - rewriteRun( - //language=java - java( - """ - class A { - void foo(boolean enabled) { - if (enabled && getValue() != null && getValue() instanceof String) { - System.out.println("String value"); - } - } - - String getValue() { - return "test"; - } - } - """ - ) - ); - } - @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/953") @Test void doNotChangeWhenConstructorCall() {