Remove null checks before literal equality only for side-effect-free expressions

src/main/java/org/openrewrite/staticanalysis/RemoveRedundantNullCheckBeforeLiteralEquals.java

@@ -28,6 +28,8 @@
 
 import java.time.Duration;
 
+import static org.openrewrite.staticanalysis.SideEffects.mayHaveSideEffects;
+
 @EqualsAndHashCode(callSuper = false)
 @Value
 public class RemoveRedundantNullCheckBeforeLiteralEquals extends Recipe {
@@ -85,35 +87,28 @@ public J visitBinary(J.Binary binary, ExecutionContext ctx) {
             }
 
             private boolean isRedundantNullCheck(J.Binary nullCheck, J.MethodInvocation equalsCall) {
-                if (nullCheck.getOperator() != J.Binary.Type.NotEqual) {
-                    return false;
-                }
-
-                // Check if the method call is equals() on a literal string
-                if (!EQUALS_MATCHER.matches(equalsCall)) {
+                if (nullCheck.getOperator() != J.Binary.Type.NotEqual || !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);
 
-                // 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;
+
+                // 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);
             }
         };
     }

src/main/java/org/openrewrite/staticanalysis/SideEffects.java

@@ -18,18 +18,19 @@
 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;
 
 /**
- * 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}.
- * <p>
- * 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()) {

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(

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(