- * 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 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 { @@ -79,6 +80,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/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/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( 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()) + } + } + """ + ) + ); + } +}