From 1de8267f4afab6fafd8e926b6284396801444550 Mon Sep 17 00:00:00 2001 From: martinfrancois Date: Mon, 10 Aug 2026 09:44:49 +0200 Subject: [PATCH 1/3] RemoveRedundantNullCheckBeforeLiteralEquals: only simplify side-effect-free expressions The recipe rewrote `x != null && "literal".equals(x)` to `"literal".equals(x)` whenever `SemanticallyEqual` matched the two occurrences of `x`. Semantic equality proves the two trees mean the same thing, not that evaluating the expression once is equivalent to evaluating it twice, so `next() != null && "ok".equals(next())` lost an invocation, both on its own and inside a chained `&&`. Gate the removal on `SideEffects.mayHaveSideEffects`, the helper `RemoveRedundantNullCheckBeforeInstanceof` already uses for the same pattern, so invocations, assignments and `values[i++]` keep their null check. A read of a field attributed as volatile is checked separately: it has no side effect of its own, so `SideEffects` does not report it, but it is a synchronization action that must not be elided. For review: the existing `removeRedundantNullCheckWithMethodInvocation` test pinned the invocation rewrite as desired behavior. It is replaced by `doNotChangeWhenNullCheckedExpressionIsMethodInvocation`, covering the direct and the chained form. One case stays unfixed, matching the instanceof sibling: a Groovy implicit property read backed by a getter parses as an identifier, so a shape-based check cannot see the dispatch and the null check is still removed there. See #953 --- ...RedundantNullCheckBeforeLiteralEquals.java | 28 +++ ...ndantNullCheckBeforeLiteralEqualsTest.java | 226 +++++++++++++++++- ...ndantNullCheckBeforeLiteralEqualsTest.java | 76 ++++++ 3 files changed, 319 insertions(+), 11 deletions(-) create mode 100644 src/test/java/org/openrewrite/staticanalysis/groovy/RemoveRedundantNullCheckBeforeLiteralEqualsTest.java diff --git a/src/main/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeLiteralEquals.java b/src/main/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeLiteralEquals.java index d8910c7c7..947d8eea4 100644 --- a/src/main/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeLiteralEquals.java +++ b/src/main/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeLiteralEquals.java @@ -20,13 +20,19 @@ import org.openrewrite.ExecutionContext; import org.openrewrite.Recipe; import org.openrewrite.TreeVisitor; +import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.JavaVisitor; import org.openrewrite.java.MethodMatcher; import org.openrewrite.java.search.SemanticallyEqual; import org.openrewrite.java.tree.Expression; +import org.openrewrite.java.tree.Flag; import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.JavaType; import java.time.Duration; +import java.util.concurrent.atomic.AtomicBoolean; + +import static org.openrewrite.staticanalysis.SideEffects.mayHaveSideEffects; @EqualsAndHashCode(callSuper = false) @Value @@ -106,6 +112,15 @@ private boolean isRedundantNullCheck(J.Binary nullCheck, J.MethodInvocation equa } Expression equalsArg = equalsCall.getArguments().get(0); + // 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. A read of + // a field attributed as volatile is checked separately: it has no side effect of its own, which puts + // it outside what `SideEffects` reports, but it is a synchronization action that must not be elided. + if (mayHaveSideEffects(equalsArg) || readsVolatileField(equalsArg)) { + return false; + } + // Check if the null check is for the same variable as the equals argument if (J.Literal.isLiteralValue(nullCheck.getLeft(), null)) { return SemanticallyEqual.areEqual(nullCheck.getRight(), equalsArg); @@ -115,6 +130,19 @@ private boolean isRedundantNullCheck(J.Binary nullCheck, J.MethodInvocation equa } return false; } + + private boolean readsVolatileField(Expression expression) { + return new JavaIsoVisitor() { + @Override + public J.Identifier visitIdentifier(J.Identifier identifier, AtomicBoolean result) { + JavaType.Variable fieldType = identifier.getFieldType(); + if (fieldType != null && fieldType.hasFlags(Flag.Volatile)) { + result.set(true); + } + return identifier; + } + }.reduce(expression, new AtomicBoolean(false)).get(); + } }; } } diff --git a/src/test/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeLiteralEqualsTest.java b/src/test/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeLiteralEqualsTest.java index 90c4a2787..7c98e2733 100644 --- a/src/test/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeLiteralEqualsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeLiteralEqualsTest.java @@ -17,12 +17,13 @@ import org.junit.jupiter.api.Test; import org.openrewrite.DocumentExample; +import org.openrewrite.Issue; import org.openrewrite.test.RecipeSpec; import org.openrewrite.test.RewriteTest; import static org.openrewrite.java.Assertions.java; -@SuppressWarnings({"ConstantConditions", "ConditionCoveredByFurtherCondition"}) +@SuppressWarnings({"ConstantConditions", "ConditionCoveredByFurtherCondition", "NestedAssignment", "RedundantCast"}) class RemoveRedundantNullCheckBeforeLiteralEqualsTest implements RewriteTest { @Override @@ -86,33 +87,173 @@ void foo(String value) { } @Test - void removeRedundantNullCheckWithMethodInvocation() { + void removeRedundantNullCheckWhenParenthesized() { rewriteRun( //language=java java( """ class A { + void foo(String s) { + if ((s) != null && "test".equals(s)) { + System.out.println("Parentheses around the null checked expression"); + } + } + } + """, + """ + class A { + void foo(String s) { + if ("test".equals(s)) { + System.out.println("Parentheses around the null checked expression"); + } + } + } + """ + ) + ); + } + + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/953") + @Test + void doNotChangeWhenNullCheckedExpressionIsMethodInvocation() { + rewriteRun( + //language=java + java( + """ + class A { + String next() { + return ""; + } + + boolean direct() { + return next() != null && "ok".equals(next()); + } + + boolean chained(boolean enabled) { + return enabled && next() != null && "ok".equals(next()); + } + } + """ + ) + ); + } + + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/953") + @Test + void doNotChangeWhenNullCheckedFieldIsVolatile() { + rewriteRun( + //language=java + java( + """ + class A { + volatile String value; + static volatile String shared; + void foo() { - if (getValue() != null && "expected".equals(getValue())) { - System.out.println("Match"); + if (value != null && "test".equals(value)) { + System.out.println("Volatile read must not be elided"); } } - String getValue() { - return "expected"; + void qualified() { + if (A.shared != null && "test".equals(A.shared)) { + System.out.println("Nor a qualified volatile read"); + } } } - """, + """ + ) + ); + } + + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/953") + @Test + void doNotChangeWhenNullCheckedExpressionIsAssignment() { + rewriteRun( + //language=java + java( + """ + class A { + void assignment(String s, String t) { + if ((s = t) != null && "test".equals(s = t)) { + System.out.println("Assignment"); + } + } + } + """ + ) + ); + } + + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/953") + @Test + void doNotChangeWhenArrayIndexHasSideEffect() { + rewriteRun( + //language=java + java( """ class A { + String[] values = new String[2]; + int i; + void foo() { - if ("expected".equals(getValue())) { - System.out.println("Match"); + if (values[i++] != null && "test".equals(values[i++])) { + System.out.println("Array access with increment"); } } + } + """ + ) + ); + } - String getValue() { - return "expected"; + @Test + void removeRedundantNullCheckWithArrayAccess() { + rewriteRun( + //language=java + java( + """ + class A { + void foo(String[] values) { + if (values[0] != null && "test".equals(values[0])) { + System.out.println("Array access"); + } + } + } + """, + """ + class A { + void foo(String[] values) { + if ("test".equals(values[0])) { + System.out.println("Array access"); + } + } + } + """ + ) + ); + } + + @Test + void removeRedundantNullCheckWithCast() { + rewriteRun( + //language=java + java( + """ + class A { + void foo(Object o) { + if ((String) o != null && "test".equals((String) o)) { + System.out.println("Cast"); + } + } + } + """, + """ + class A { + void foo(Object o) { + if ("test".equals((String) o)) { + System.out.println("Cast"); + } } } """ @@ -151,6 +292,69 @@ void foo() { ); } + @Test + void removeRedundantNullCheckWithStaticFieldAccess() { + rewriteRun( + //language=java + java( + """ + class A { + static String field; + + static class Inner { + static String nested; + } + + void foo() { + if (A.field != null && "constant".equals(A.field)) { + System.out.println("Static field matches"); + } + } + + void nested() { + if (A.Inner.nested != null && "constant".equals(A.Inner.nested)) { + System.out.println("Nested class field matches"); + } + } + + void fullyQualified() { + if (java.io.File.separator != null && "/".equals(java.io.File.separator)) { + System.out.println("Separator matches"); + } + } + } + """, + """ + class A { + static String field; + + static class Inner { + static String nested; + } + + void foo() { + if ("constant".equals(A.field)) { + System.out.println("Static field matches"); + } + } + + void nested() { + if ("constant".equals(A.Inner.nested)) { + System.out.println("Nested class field matches"); + } + } + + void fullyQualified() { + if ("/".equals(java.io.File.separator)) { + System.out.println("Separator matches"); + } + } + } + """ + ) + ); + } + @Test void doNotChangeWhenDifferentVariables() { rewriteRun( diff --git a/src/test/java/org/openrewrite/staticanalysis/groovy/RemoveRedundantNullCheckBeforeLiteralEqualsTest.java b/src/test/java/org/openrewrite/staticanalysis/groovy/RemoveRedundantNullCheckBeforeLiteralEqualsTest.java new file mode 100644 index 000000000..0ba21dde2 --- /dev/null +++ b/src/test/java/org/openrewrite/staticanalysis/groovy/RemoveRedundantNullCheckBeforeLiteralEqualsTest.java @@ -0,0 +1,76 @@ +/* + * Copyright 2026 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.staticanalysis.groovy; + +import org.junit.jupiter.api.Test; +import org.openrewrite.Issue; +import org.openrewrite.staticanalysis.RemoveRedundantNullCheckBeforeLiteralEquals; +import org.openrewrite.test.RecipeSpec; +import org.openrewrite.test.RewriteTest; + +import static org.openrewrite.groovy.Assertions.groovy; + +class RemoveRedundantNullCheckBeforeLiteralEqualsTest implements RewriteTest { + + @Override + public void defaults(RecipeSpec spec) { + spec.recipe(new RemoveRedundantNullCheckBeforeLiteralEquals()); + } + + @Test + void removeRedundantNullCheck() { + rewriteRun( + //language=groovy + groovy( + """ + class A { + boolean parameter(String s) { + s != null && "ok".equals(s) + } + } + """, + """ + class A { + boolean parameter(String s) { + "ok".equals(s) + } + } + """ + ) + ); + } + + @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/953") + @Test + void doNotChangeWhenNullCheckedExpressionIsMethodInvocation() { + rewriteRun( + //language=groovy + groovy( + """ + class A { + String next() { + "" + } + + boolean direct() { + next() != null && "ok".equals(next()) + } + } + """ + ) + ); + } +} From 2d222991eb259870dde63953b04cb5416b3c5652 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Tue, 11 Aug 2026 09:36:38 +0200 Subject: [PATCH 2/3] Review fixes: move the volatile-read guard into the shared SideEffects helper --- ...RedundantNullCheckBeforeLiteralEquals.java | 50 ++++--------------- .../staticanalysis/SideEffects.java | 18 +++++-- ...implifyRedundantLogicalExpressionTest.java | 19 +++++++ 3 files changed, 43 insertions(+), 44 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeLiteralEquals.java b/src/main/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeLiteralEquals.java index 947d8eea4..8a4f54625 100644 --- a/src/main/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeLiteralEquals.java +++ b/src/main/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeLiteralEquals.java @@ -20,17 +20,13 @@ import org.openrewrite.ExecutionContext; import org.openrewrite.Recipe; import org.openrewrite.TreeVisitor; -import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.JavaVisitor; import org.openrewrite.java.MethodMatcher; import org.openrewrite.java.search.SemanticallyEqual; import org.openrewrite.java.tree.Expression; -import org.openrewrite.java.tree.Flag; import org.openrewrite.java.tree.J; -import org.openrewrite.java.tree.JavaType; import java.time.Duration; -import java.util.concurrent.atomic.AtomicBoolean; import static org.openrewrite.staticanalysis.SideEffects.mayHaveSideEffects; @@ -91,57 +87,29 @@ public J visitBinary(J.Binary binary, ExecutionContext ctx) { } private boolean isRedundantNullCheck(J.Binary nullCheck, J.MethodInvocation equalsCall) { - if (nullCheck.getOperator() != J.Binary.Type.NotEqual) { + if (nullCheck.getOperator() != J.Binary.Type.NotEqual || !EQUALS_MATCHER.matches(equalsCall)) { return false; } - // Check if the method call is equals() on a literal string - if (!EQUALS_MATCHER.matches(equalsCall)) { - return false; - } - - // Check if the receiver is a literal string Expression receiver = equalsCall.getSelect(); if (!(receiver instanceof J.Literal) || !(((J.Literal) receiver).getValue() instanceof String)) { return false; } - // Get the argument passed to equals() if (equalsCall.getArguments().size() != 1) { return false; } Expression equalsArg = equalsCall.getArguments().get(0); - // 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. A read of - // a field attributed as volatile is checked separately: it has no side effect of its own, which puts - // it outside what `SideEffects` reports, but it is a synchronization action that must not be elided. - if (mayHaveSideEffects(equalsArg) || readsVolatileField(equalsArg)) { - return false; - } - - // Check if the null check is for the same variable as the equals argument - if (J.Literal.isLiteralValue(nullCheck.getLeft(), null)) { - return SemanticallyEqual.areEqual(nullCheck.getRight(), equalsArg); - } - if (J.Literal.isLiteralValue(nullCheck.getRight(), null)) { - return SemanticallyEqual.areEqual(nullCheck.getLeft(), equalsArg); - } - return false; - } + Expression nullChecked = J.Literal.isLiteralValue(nullCheck.getLeft(), null) ? nullCheck.getRight() : + J.Literal.isLiteralValue(nullCheck.getRight(), null) ? nullCheck.getLeft() : null; - private boolean readsVolatileField(Expression expression) { - return new JavaIsoVisitor() { - @Override - public J.Identifier visitIdentifier(J.Identifier identifier, AtomicBoolean result) { - JavaType.Variable fieldType = identifier.getFieldType(); - if (fieldType != null && fieldType.hasFlags(Flag.Volatile)) { - result.set(true); - } - return identifier; - } - }.reduce(expression, new AtomicBoolean(false)).get(); + // The rewrite evaluates the expression once where it was evaluated twice, so it is only equivalent + // when evaluating it is free of side effects; `SemanticallyEqual` proves the two occurrences mean the + // same thing, not that evaluating them twice is the same as evaluating them once. + return nullChecked != null && + SemanticallyEqual.areEqual(nullChecked, equalsArg) && + !mayHaveSideEffects(equalsArg); } }; } diff --git a/src/main/java/org/openrewrite/staticanalysis/SideEffects.java b/src/main/java/org/openrewrite/staticanalysis/SideEffects.java index df2fc0297..97dc1fa91 100644 --- a/src/main/java/org/openrewrite/staticanalysis/SideEffects.java +++ b/src/main/java/org/openrewrite/staticanalysis/SideEffects.java @@ -18,7 +18,9 @@ import org.jspecify.annotations.Nullable; import org.openrewrite.Tree; import org.openrewrite.java.JavaIsoVisitor; +import org.openrewrite.java.tree.Flag; import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.JavaType; import java.util.concurrent.atomic.AtomicBoolean; @@ -27,9 +29,10 @@ * delete an expression, or that stop evaluating one, are only correct when the answer is {@code false}. *

* Deliberately conservative: any method invocation, constructor call, assignment or increment counts, since - * whether those are pure cannot be decided from the LST alone. {@link org.openrewrite.java.tree.Expression#getSideEffects()} - * is not used here because it reports only the side effects of the expression's own node type, and so misses - * those nested inside a ternary or a lambda. + * whether those are pure cannot be decided from the LST alone. A read of a field attributed as volatile counts + * too: it produces no side effect of its own, but it is a synchronization action that must not be elided. + * {@link org.openrewrite.java.tree.Expression#getSideEffects()} is not used here because it reports only the + * side effects of the expression's own node type, and so misses those nested inside a ternary or a lambda. */ final class SideEffects { @@ -79,6 +82,15 @@ public J.NewClass visitNewClass(J.NewClass newClass, AtomicBoolean result) { return newClass; } + @Override + public J.Identifier visitIdentifier(J.Identifier identifier, AtomicBoolean result) { + JavaType.Variable fieldType = identifier.getFieldType(); + if (fieldType != null && fieldType.hasFlags(Flag.Volatile)) { + result.set(true); + } + return identifier; + } + @Override public @Nullable J visit(@Nullable Tree t, AtomicBoolean result) { if (result.get()) { diff --git a/src/test/java/org/openrewrite/staticanalysis/SimplifyRedundantLogicalExpressionTest.java b/src/test/java/org/openrewrite/staticanalysis/SimplifyRedundantLogicalExpressionTest.java index 121b75bb6..a6889954b 100644 --- a/src/test/java/org/openrewrite/staticanalysis/SimplifyRedundantLogicalExpressionTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/SimplifyRedundantLogicalExpressionTest.java @@ -261,6 +261,25 @@ boolean test(int x) { ); } + @Test + void doNotChangeVolatileFieldRead() { + rewriteRun( + //language=java + java( + """ + class Test { + volatile boolean flag; + + @SuppressWarnings("all") + boolean test() { + return flag && flag; + } + } + """ + ) + ); + } + @Test void simplifyLogicalAndKotlin() { rewriteRun( From 6c7f380b4b8c4426809dbf40de33ad9859d42434 Mon Sep 17 00:00:00 2001 From: Tim te Beek Date: Wed, 12 Aug 2026 00:25:08 +0200 Subject: [PATCH 3/3] Trim commentary --- ...emoveRedundantNullCheckBeforeLiteralEquals.java | 5 ++--- .../openrewrite/staticanalysis/SideEffects.java | 14 ++++++-------- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/src/main/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeLiteralEquals.java b/src/main/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeLiteralEquals.java index 8a4f54625..412b76c9d 100644 --- a/src/main/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeLiteralEquals.java +++ b/src/main/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeLiteralEquals.java @@ -104,9 +104,8 @@ private boolean isRedundantNullCheck(J.Binary nullCheck, J.MethodInvocation equa Expression nullChecked = J.Literal.isLiteralValue(nullCheck.getLeft(), null) ? nullCheck.getRight() : J.Literal.isLiteralValue(nullCheck.getRight(), null) ? nullCheck.getLeft() : null; - // The rewrite evaluates the expression once where it was evaluated twice, so it is only equivalent - // when evaluating it is free of side effects; `SemanticallyEqual` proves the two occurrences mean the - // same thing, not that evaluating them twice is the same as evaluating them once. + // The rewrite evaluates once what was evaluated twice; `SemanticallyEqual` proves the occurrences + // mean the same thing, not that evaluating them twice is the same as once return nullChecked != null && SemanticallyEqual.areEqual(nullChecked, equalsArg) && !mayHaveSideEffects(equalsArg); diff --git a/src/main/java/org/openrewrite/staticanalysis/SideEffects.java b/src/main/java/org/openrewrite/staticanalysis/SideEffects.java index 97dc1fa91..b63189fea 100644 --- a/src/main/java/org/openrewrite/staticanalysis/SideEffects.java +++ b/src/main/java/org/openrewrite/staticanalysis/SideEffects.java @@ -25,14 +25,12 @@ import java.util.concurrent.atomic.AtomicBoolean; /** - * Whether evaluating an expression might do something observable beyond producing its value. Recipes that - * delete an expression, or that stop evaluating one, are only correct when the answer is {@code false}. - *

- * Deliberately conservative: any method invocation, constructor call, assignment or increment counts, since - * whether those are pure cannot be decided from the LST alone. A read of a field attributed as volatile counts - * too: it produces no side effect of its own, but it is a synchronization action that must not be elided. - * {@link org.openrewrite.java.tree.Expression#getSideEffects()} is not used here because it reports only the - * side effects of the expression's own node type, and so misses those nested inside a ternary or a lambda. + * Whether evaluating an expression might do something observable beyond producing its value; recipes deleting an + * expression, or evaluating one fewer times, are only correct when this is {@code false}. Deliberately + * conservative: any invocation, constructor call, assignment or increment counts, as does a {@code volatile} read, + * which is a synchronization action rather than a side effect. Not {@link + * org.openrewrite.java.tree.Expression#getSideEffects()}, which reports only the expression's own node type and so + * misses anything nested inside a ternary or a lambda. */ final class SideEffects {