Do not delete expressions that may have side effects

[timtebeek]

• Addresses the "missing purity guards" half of Deletion-gating recipes lack purity guards, and untyped identifier comparisons remain unsound #953. Draft because the guard is deliberately conservative and how conservative it should be is worth a discussion — see the open question at the bottom.

The problem

• Four recipes decide to delete a statement, branch or expression using a comparison that says nothing about what the deleted code does. When the deleted code has side effects, the output no longer behaves like the input. All four reproduce with complete type information, so they are ordinary missing purity guards rather than instances of the type-attribution bug that Deletion-gating recipes lack purity guards, and untyped identifier comparisons remain unsound #953 originally reported.

Recipe	Before	After	Effect
RemoveUnconditionalValueOverwrite	m.put("k", register()); m.put("k", 2);	m.put("k", 2);	register() is never called
AllBranchesIdentical	if (it.next() != null) p("x"); else p("x");	p("x");	the iterator never advances
RemoveDuplicateConditions	if (advance()) p("a"); else if (advance()) p("b");	if (advance()) p("a");	prints nothing where the original printed b
AvoidBoxedBooleanExpressions	boolean x = !b;	boolean x = Boolean.FALSE.equals(b);	a would-be NullPointerException becomes false

The fix

SimplifyRedundantLogicalExpression already carried exactly the purity check the other three need, as a private method. This moves it to SideEffects.mayHaveSideEffects and shares it:

• RemoveUnconditionalValueOverwrite — the overwritten put really is dead, but the receiver, key and value it evaluates on the way are not, so only those are checked, not the call itself. Otherwise the guard would block every case the recipe exists for.
• AllBranchesIdentical and RemoveDuplicateConditions — require every condition in the chain to be pure. For RemoveDuplicateConditions it is not enough to check the duplicated condition alone: a side effect in an intervening condition can change what the later one evaluates to, which is what makes the branch reachable.
• AvoidBoxedBooleanExpressions — visitExpression already gated its rewrite on isControlExpression; visitUnary now does too. In an if or ternary the substitution preserves meaning, which is the recipe's purpose; in an assignment it does not.

Each recipe gains a doNotChange… test built from the reproduction above, plus one for a side-effecting key in RemoveUnconditionalValueOverwrite. Full suite green (2205 tests).

Also included

• The first commit adds two RemoveSelfAssignment regression tests for Do not treat missing type attribution as proof of semantic equality rewrite#8333, the upstream fix for the original half of Deletion-gating recipes lack purity guards, and untyped identifier comparisons remain unsound #953. The Groovy one needs no harness setup at all — the Groovy parser passes null for fieldType at essentially every reference site, so this.readOnly = readOnly in a constructor reproduced the deletion directly. That is worth having as a permanent guard, since it does not depend on stripping attribution by hand. Happy to split it into its own PR if you would rather land it separately — it is independent of the purity work.

Open question

mayHaveSideEffects treats any method invocation as impure, because purity cannot be decided from the LST alone. That is the safe direction for recipes that delete code, but it does narrow RemoveDuplicateConditions in particular: if (isFoo()) … else if (isFoo()) … is a common shape for that rule and is now skipped. Every existing test still passes, so nothing currently covered regresses — but if you would rather keep firing on repeated calls and accept the risk, or gate on something narrower, say so and I will adjust.

Two smaller notes:

• Expression#getSideEffects() looked like the natural built-in, but it reports only the side effects of the expression's own node type. J.Ternary has no override, so c ? f() : g() reports as pure. The conservative subtree walk is used instead; the reasoning is in the javadoc on SideEffects.
• None of these four recipes is wired into CommonStaticAnalysis or any other collection, so exposure today is direct invocation only.

[steve-aom-elliott]

The changes make sense. I am wondering if we already have a recipe (or could make one) for being able to mark these edge scenarios that have side effects to encourage manual remediation, since code using side-effect laden conditionals or repeated assignments feels like a smell that someone should clear up.