Simplify boolean expressions in JavaScript AST

[zbynek]

In the output I noticed statements like

!!a&&a.b();

which can be simplified to

a&&a.b();

In general, if we know that we ignore the result of an evaluation or we know the result is coerced to a boolean, we can apply more simplifications.

---

Other examples from compiler output this should simplify:

a.K==(Slh(),Klh)&&false?(n.g+=TWo,n):tNg(a,n,g)
O7d(e.yb)&&Tde(e.Qb,e.yb)&&false?Tqe(e):Pqe(e)

Fixes #10280

[niloc132]

I like the patch - it seems to go a little beyond what is discussed in the description though. Can you describe a bit more how the JS is being generated that causes this - is it from Java that isn't handled correctly, or is this JSNI (or jsinterop-base's JSNI) emitting these extra !!s? Might make sense to have an issue to generally track the topic too, in case there are more examples of it.

[niloc132]

If the expr is in a statement like this, the output should be a(),b(),c(). Doesn't necessarily have to be in this patch, but I think it could be fixed by passing the context as void for the whole exprstatement?

[zbynek]

I don't think that's generally valid -- if a() evaluates to false we don't want to evaluate b().

[niloc132]

No you're right of course, my reply is nonsense.

[niloc132]

would you document the meaning of these, and what it means for evalContext to not have a key at all?

My understanding from (heh) context is that VOID stands for "value will be thrown away", and BOOL means "this will be coerced to boolean", while absent means we don't know, so retain everything

[niloc132]

Using #10263 to bring this up to date, I show that this brings samples down from 10821121 to 10800491, some 20kb, or about 0.2% - not bad for some light logic improvements!

[niloc132]

Taking a little more time to go over this, unless this is exclusively a JSNI problem I think you really want this to be moved/copied to DeadCodeElimination. Doing an optimization on constants like this earlier is extremely likely to benefit the Java optimization loop more than the JS loop (where JsStaticEval lives), as removing flow control is quite likely to also improve inlinability and enhance type tightening or pruning - something that the JS passes are unable to reason about as effectively.

I'm nearly certain the two samples at the end of the description are Java rather than JSNI:

a.K==(Slh(),Klh)&&false?(n.g+=TWo,n):tNg(a,n,g)

Slh is almost certainly a Java clinit, which cannot be pruned (tests would need to show that it is promoted), and n is probably a stringbuilder/stringbuffer, getting a constant appended to it. DCETest has a few other cases like this that are verified, writing such a test is a little harder to do on a JS AST, since you can't just parse the JS, but need to visit the JS AST and edit it after the fact, and mark them as such - com.google.gwt.dev.js.JsDuplicateFunctionRemoverTest#setAllFromJava has an example of that, and I have a PR to land eventually for #9731 that does this too...

O7d(e.yb)&&Tde(e.Qb,e.yb)&&false?Tqe(e):Pqe(e)

Harder to be sure about anything here, but I'd bet that Tqe and Pqe are Java instance methods on e that were made static. If true, also implies that the java optimization loop is where this would do its best work.

Keeping the change also in the JS loop isn't bad per se, as it will still apply to JSNI or any other work that the Java loop can't optimize out. I could easily imagine that the (apparently uninlinable) pruned instance methods could have other impacts on code size that the JS loop can't necessarily recognize.

[zbynek]

So there are two issues with the boolean expressions

1. extraneous !! operators. In the compiled code I checked (~7MB) there were over 5 thousand !! operators and more than a half of them by random inspection seemed redundant. These are not JSNI and do not exist in Java code, they are created during translation from Java to JS. So JsStaticEval sounds like the right place to fix them (other occurences of !! are already eliminated by JsStaticEval.
2. dead code not eliminated from e.g. a() && false && b()?c():d(). In the same codebase I found 36 occurences of &&false, 2 of &&true, 1 of ||false, so this is a much smaller problem but some of the eliminated bits might be big. In my last commit I made sure this is handled in DCE. For now I left the equivalent code also in JsStaticEval, but if there are concerns about increased complexity of that class, I can remove some of it.

[vjay82]

Might there be another reason to use !!, like to convert to boolean or something?

[niloc132]

I think the patch mostly looks good, just trying to understand the root issue, be sure we can't do less and get more from it. Nothing here seems like it could increase build times, and for how much we're saving already, it seems obviously worth it.

I think the !! is being added in EqualityNormalizer - all kinds of null/not-null checks are delegated to Cast.isNull/isNotNull, which are just JSNI for !src/!!src respectively, and JSNI methods are automatically eligible for the JS inlining operation (since java's MethodInliner couldn't touch them). Once inlined, JsStaticEval 400 or so should be handling them - but the issues you've discovered with booleans in the "middle" of &&s and ||s is the root cause in interrupting that?

Concrete example:

gwt/user/super/com/google/gwt/emul/java/util/TreeMap.java

Lines 828 to 834 in b7117c1

	/**
	* Returns true if <code>node</code> is red. Note that null pointers are
	* considered black.
	*/
	private boolean isRed(Node<K, V> node) {
	return node != null && node.isRed;
	}

This is inlined into removeWithState:
https://github.com/gwtproject/gwt/blob/b7117c157bbb3bcfcc022433e307722a8e12fb33/user/super/com/google/gwt/emul/java/util/TreeMap.java#L882C12-L882C17
Resulting in roughly

    if (!(!!node && node.isRed) && !$isRed(node.child[dir])) {

That !!node should definitely should be corrected, since it is unambiguously in a boolean context - your addition of visit(JsBinaryOperation,JsContext) should by itself correct this, since that will let the endVisit on the outer ! remove both:

gwt/dev/core/src/com/google/gwt/dev/js/JsStaticEval.java

Lines 399 to 418 in b7117c1

	/**
	* Change !!x to x in a boolean context.
	*/
	@Override
	public void endVisit(JsPrefixOperation x, JsContext ctx) {
	if (x.getOperator() == JsUnaryOperator.NOT) {
	evalBooleanContext.remove(x.getArg());
	}
	if (evalBooleanContext.contains(x)) {
	if ((x.getOperator() == JsUnaryOperator.NOT)
	&& (x.getArg() instanceof JsPrefixOperation)) {
	JsPrefixOperation arg = (JsPrefixOperation) x.getArg();
	if (arg.getOperator() == JsUnaryOperator.NOT) {
	ctx.replaceMe(arg.getArg());
	return;
	}
	}
	}
	}

So I have no doubts about the patch being right, I'm sorry if I've implied that. Adding the Java side to go after <expr> && false and friends in the earlier loop will improve that pass, and hopefully make at least some of the JS pass unnecessary - though clearly not the !! checks.

I have another review pass about half done, just trying to more deeply understand what's going on here. No draft comments so far on the actual contents of the patch, just the tests and one comment I think could be cleaner.

[niloc132]

Might there be another reason to use !!, like to convert to boolean or something?

Exactly - but in cases where we know it will already be coerced to boolean, the extra !! is unnecessary.

For example

alert(maybeAString || false)

will either print the string, or if "falsey", the value false. If you want true/false as a string, you would need to do

alert(!!maybeAString || false)

(this is dumb, since it should just be alert(!!maybeAString) - that's what the second set of changes in the patch addresses)

On the other hand

alert(maybeAString ? 1 : 2)

Here we don't need the !! "operator" to coerce to bool, since it can only be used in a boolean context.

[niloc132]

Sorry, taking longer than I had anticipated with the release going on.

Oddly, I'm seeing zero impact from this patch in the Java AST of the Showcase sample - I'm still trying to understand why. The original objective of cleaning up !!s is going to have to happen only in the JS AST, but the nested &&+?: examples still appear to be from Java, unless this is again just a case of JSNI method inlining. I'll keep poking, trying to find examples where we could move the work earlier.

---

gwt/user/src/com/google/gwt/user/cellview/client/CellTreeNodeView.java

Lines 150 to 169 in b7117c1

	// Outer div contains image, value, and children (when open)
	StringBuilder outerClasses = new StringBuilder(itemStyle);
	if (isOpen) {
	outerClasses.append(openStyle);
	}
	if (isRootNode) {
	outerClasses.append(topStyle);
	}
	boolean isSelected = (selectionModel != null && selectionModel.isSelected(value));
	String ariaSelected = String.valueOf(isSelected);
	if (isSelected) {
	outerClasses.append(selectedStyle);
	}
	// Inner div contains image and value
	StringBuilder innerClasses = new StringBuilder(itemStyle);
	innerClasses.append(itemImageValueStyle);
	if (isRootNode) {
	innerClasses.append(topImageValueStyle);
	}

This code is improved in JS by this patch, in ways that don't seem obvious right away - the COMMA operators are getting cleaned up due to lack of side effects on the right side.

     isOpen = $containsKey(openNodes.map, key);
     outerClasses = new StringBuilder(itemStyle);
-    isOpen && (outerClasses.string += openStyle , outerClasses);
-    isRootNode && (outerClasses.string += topStyle , outerClasses);
+    isOpen && (outerClasses.string += openStyle);
+    isRootNode && (outerClasses.string += topStyle);
     isSelected = !!selectionModel && selectionModel.isSelected(value);
     ariaSelected = '' + isSelected;
-    isSelected && (outerClasses.string += selectedStyle , outerClasses);
+    isSelected && (outerClasses.string += selectedStyle);
     innerClasses = new StringBuilder(itemStyle);
     innerClasses.string += itemImageValueStyle;
-    isRootNode && (innerClasses.string += topImageValueStyle , innerClasses);
+    isRootNode && (innerClasses.string += topImageValueStyle);
     isOpen?(image = openImage):model.isLeaf(value)?(image = ($clinit_CellTreeNodeView() , LEAF_IMAGE)):(image = closedImage);
     cellBuilder = new SafeHtmlBuilder;

Not all cases of the "return this" are impacted by this, but these improvement sure seem like obvious wins, and occur in many other places in the showcase.

Another example here, where we get a small win, but due to apparent side effects in the clinit, we can't outright remove the whole line:

gwt/user/src/com/google/gwt/user/client/ui/RootPanel.java

Lines 191 to 201 in b7117c1

	// Note that the code in this if block only happens once -
	// on the first RootPanel.get(String) or RootPanel.get()
	// call.
	if (rootPanels.size() == 0) {
	// If we're in a RTL locale, set the RTL directionality
	// on the entire document.
	if (LocaleInfo.getCurrentLocale().isRTL()) {
	BidiUtils.setDirectionOnElement(getRootElement(),
	HasDirection.Direction.RTL);
	}
	}

   if (!!rp) {
     return rp;
   }
-  $size(rootPanels) == 0 && ($clinit_LocaleInfo() , false);
+  $size(rootPanels) == 0 && $clinit_LocaleInfo();
   rp = new RootPanel$DefaultRootPanel;
   $put(rootPanels, null, rp);

Here's a case where we probably should be removing the !! on the LHS, but instead leave it in place, possibly because this is a VOID context rather than BOOLEAN? Oddly, other cases like this are cleaned up. Maybe the code is too simple here, and being optimized in the Java ast so that we still see this pattern?

gwt/user/src/com/google/gwt/cell/client/ButtonCell.java

Lines 68 to 74 in b7117c1

	public void render(Context context, SafeHtml data, SafeHtmlBuilder sb) {
	sb.appendHtmlConstant("<button type=\"button\" tabindex=\"-1\">");
	if (data != null) {
	sb.append(data);
	}
	sb.appendHtmlConstant("</button>");
	}

 _.render = function render(context, data, sb){
   $append(sb.sb, '<button type="button" tabindex="-1">');
-  !!data && ($append(sb.sb, data.html) , sb);
+  !!data && $append(sb.sb, data.html);
   $append(sb.sb, '<\/button>');
 }

This next one is probably as optimized as this patch can get it, and shows how the old code was still getting some benefits of removing the dead code, just not as complete as what you worked out

gwt/user/src/com/google/gwt/user/datepicker/client/DatePicker.java

Lines 713 to 723 in b7117c1

	protected final void refreshAll() {
	highlighted = null;
	getModel().refresh();
	getView().refresh();
	getMonthSelector().refresh();
	if (isAttached()) {
	ShowRangeEvent.fire(this, getFirstDate(), getLastDate());
	}
	getView().setAriaSelectedCell(value);
	}

 function $refreshAll(this$static){
   $refresh(this$static.view);
   $refresh(this$static.monthAndYearSelector);
-  $isAttached(this$static) && this$static.view.lastDisplayed;
+  $isAttached(this$static);
   $setAriaSelectedCell(this$static.view, this$static.value);
 }

It is possible that this patch will open up other avenues for #10242 to benefit the codebase like this one.

This makes me mildly suspicious - is it correct to drop the || 0 here? This is a "number" context, right, since we're |ing the desired event bits against the existing ones?

gwt/user/src/com/google/gwt/user/client/ui/Widget.java

Lines 239 to 245 in b7117c1

	public void sinkEvents(int eventBitsToAdd) {
	if (isOrWasAttached()) {
	super.sinkEvents(eventBitsToAdd);
	} else {
	eventsToSink |= eventBitsToAdd;
	}
	}

 _.sinkEvents = function sinkEvents(eventBitsToAdd){
-  this.eventsToSink == -1?sinkEvents(($clinit_DOM() , this.element), eventBitsToAdd | ($clinit_DOM() , ($clinit_DOM() , this.element).__eventBits || 0)):(this.eventsToSink |= eventBitsToAdd);
+  this.eventsToSink == -1?sinkEvents(($clinit_DOM() , this.element), eventBitsToAdd | ($clinit_DOM() , ($clinit_DOM() , this.element).__eventBits)):(this.eventsToSink |= eventBitsToAdd);
 }

My read is that if you simplify it, this looks like

sinkEvents(this.element), eventBitsToAdd | (this.element.__eventBits || 0));

and so we want the ||0 as "coerce to number".

[niloc132]

I'm wondering if this comment is useful here, since left-associativity means that we would view the whole
a() && false && b()
as
(a() && false) && b()

and so we would be relying on the JMultiExpr instanceof above for step two, and the lhs is the JBooleanLiteral on step three? I think a comment in the test makes sense to spell out that we're going to get all three different optimizations in a single pass, but probably just simplify here to something like

Suggested change

	// if side effect, allow rewriting a() && false && b() -> (a(), false) && b()
	// -> (a(), false && b()) -> (a(), false)
	// if side effect, allow rewriting a() && false -> (a(), false)

Also technically (a(),false) is a character longer than a()&&false - perhaps we shouldn't touch it unless we know the current binaryoperation is itself within a binaryoperation?

[zbynek]

It may be tricky to know in advance if the outer bracket is needed, if it's not, the , expression is shorter.

alert(foo()&&false)
alert(foo(),false)

[niloc132]

Agreed - my "technically" responses are mostly to mean "this is possible, but probably not easy, and doesnt save us many bytes".

[zbynek]

I can reproduce the issue with ||0 in a test, checking...

[zbynek]

This was fixed by not putting nulls into the map.

[niloc132]

Note that it isn't obvious if the first or second randomBooleanWithSideEffects() was inlined here - the code could be wrong and inlining the wrong one, but the test output is the same.

[niloc132]

Question, is this passing because it is in a void context, the value is being thrown away? Is that the goal of the test, or should this be wrapped in something (like alert()) - or should the a also be optimized out?

[zbynek]

Yes, I wanted to test that in void context !! is removed even with side effects.
Here we can't optimize a out because it decides if b() is or is not called.
We can add some tests with alert and side effects.

[niloc132]

I worked backwards from one of the <expr> && false JS cases to see what was causing them, and it seems that your Simplifier changes are at least documented as dealing with this exact case - but at least in my testing, failing to do so.

First, the method that everything is inlined into - ignore everything except the super call at the end

gwt/user/src/com/google/gwt/user/client/ui/DialogBox.java

Lines 517 to 529 in b7117c1

	protected void onPreviewNativeEvent(NativePreviewEvent event) {
	// We need to preventDefault() on mouseDown events (outside of the
	// DialogBox content) to keep text from being selected when it
	// is dragged.
	NativeEvent nativeEvent = event.getNativeEvent();
	if (!event.isCanceled() && (event.getTypeInt() == Event.ONMOUSEDOWN)
	&& isCaptionEvent(nativeEvent)) {
	nativeEvent.preventDefault();
	}
	super.onPreviewNativeEvent(event);
	}

That super call is PopupPanel.onPreviewNativeEvent:

gwt/user/src/com/google/gwt/user/client/ui/PopupPanel.java

Lines 1046 to 1052 in b7117c1

	protected void onPreviewNativeEvent(NativePreviewEvent event) {
	// Cancel the event based on the deprecated onEventPreview() method
	if (event.isFirstHandler()
	&& !onEventPreview(Event.as(event.getNativeEvent()))) {
	event.cancel();
	}
	}

which in turn calls onEventPreview (as noted, this is deprecated, so just has a default impl):

gwt/user/src/com/google/gwt/user/client/ui/PopupPanel.java

Lines 704 to 707 in b7117c1

	@Deprecated
	public boolean onEventPreview(Event event) {
	return true;
	}

The !onEventPreview naturally will be inlined as just !true and at the end of the Java optimization loop, these are fully inlined, though we keep the event.nativeEvent accessor for some reason, but !true is now false

  protected final void onPreviewNativeEvent(Event$NativePreviewEvent event){
    final NativeEvent nativeEvent = event.nativeEvent;
    !event.isCanceled && Event.$getTypeInt((Event) event.nativeEvent) == 4 && DialogBox.$isCaptionEvent(this, nativeEvent) && DOMImplStandard.$eventPreventDefault(nativeEvent);
    event.isFirstHandler && (((Event) event.nativeEvent, false)) && (event.isCanceled = true);
  }

First iteration optimization loop with JS (before this patch's changes):

_.onPreviewNativeEvent = function onPreviewNativeEvent(event){
  var nativeEvent;
  nativeEvent = event.nativeEvent;
  !event.isCanceled && $getTypeInt(event.nativeEvent) == 4 && $isCaptionEvent(this, nativeEvent) && (nativeEvent.preventDefault() , undefined);
  event.isFirstHandler && false && (event.isCanceled = true);
}
;

The JS pass got the event.nativeEvent removal right, and then your patch correctly rewrites away the rest of the code in that line (event.isFirstHandler has no side effects, and the assignment will never be called). BUT, if Java correctly cleaned up that multiexpression (which your current DCE changes are attempting to do?). The issue is the JCastOperation - except when casting to primitive, these always are marked as having a side effect (since there could be a CCE thrown).

gwt/dev/core/src/com/google/gwt/dev/jjs/ast/JCastOperation.java

Lines 53 to 63 in b7117c1

	public boolean hasSideEffects() {
	if (castType.isPrimitiveType()) {
	// Primitive casts do not throw and only have side effects if the expression has side effects.
	return expr.hasSideEffects();
	}
	// Any live non-primitive cast operations might throw a ClassCastException
	//
	// TODO: revisit this when we support the concept of whether something
	// can/must complete normally!
	return true;
	}

At the same time, Event.as() is marked as "this is always safe"

gwt/user/src/com/google/gwt/user/client/Event.java

Lines 464 to 471 in b7117c1

	/**
	* Converts the {@link NativeEvent} to Event. This is always safe.
	*
	* @param event the event to downcast
	*/
	public static Event as(NativeEvent event) {
	return (Event) event;
	}

It turns out that the compiler will make the cast be free... because both Event and NativeEvent are JSOs. Event is a subclass of NativeEvent, so TypeTightener doesnt believe it is automatically safe, but really, since it is a JSO, it shouldn't check, it should just make it a free operation.

As for why it is a free cast once we're in JS:
First, casting a JSO to a JSO is ignored by the visitor that rewrites casts to Cast.java method calls

gwt/dev/core/src/com/google/gwt/dev/jjs/impl/ImplementCastsAndTypeChecks.java

Lines 100 to 104 in b7117c1

	} else if (program.typeOracle.isEffectivelyJavaScriptObject(argType)
	&& program.typeOracle.isEffectivelyJavaScriptObject(refType)) {
	// leave the cast instance for Pruner/CFA, remove in GenJSAST
	return;
	}

Then, when we translate the Java AST to JS, we just ignore that leftover cast:

gwt/dev/core/src/com/google/gwt/dev/jjs/impl/GenerateJavaScriptAST.java

Lines 605 to 608 in b7117c1

	public JsNode transformCastOperation(JCastOperation castOperation) {
	// These are left in when cast checking is disabled.
	return transform(castOperation.getExpr());
	}

---

So for this single case, my conclusion is that it is a bug for GWT to ignore the case during JS AST generation, when it could just as easily have tightened the cast away earlier and made all of this unnecessary.

So at least for this specific example, better casting logic would have prevented this

[zbynek]

From GGB codebase

 $isUnbundledOrWhiteboard(this$static.cons.kernel.app)?this$static.cons.kernel.app.isUnbundledGeometry() && false?(locus.lineOpacity = 204):this$static.cons.kernel.app.isUnbundledGraphing() && false?(locus.lineOpacity = 204):this$static.cons.kernel.app.isUnbundledGraphing() && false?(locus.lineOpacity = 204):(locus.lineOpacity = 178):(locus.lineOpacity = 178);

which comes from setDefaultLineStyle(locus);

inlining the two methods

private void setDefaultLineStyle(GeoElement geo) {
   	if (geo instanceof GeoAngle
   			&& cons.getApplication().isUnbundledGeometry()) {
   		geo.setLineThickness(
   				EuclidianStyleConstants.OBJSTYLE_DEFAULT_LINE_THICKNESS_ANGLE_GEOMETRY);
   	} else {
   		geo.setLineThickness(
   				EuclidianStyleConstants.OBJSTYLE_DEFAULT_LINE_THICKNESS);
   	}
   	if (geo.hasLineOpacity()) {
   		if (cons.getApplication().isUnbundledOrWhiteboard()) {
   			setLineOpacity(geo);
   		} else {
   			geo.setLineOpacity(
   					EuclidianStyleConstants.OBJSTYLE_DEFAULT_LINE_OPACITY);
   		}
   	}
   }

   private void setLineOpacity(GeoElement geo) {
   	if (geo instanceof GeoAngle) {
   		geo.setLineOpacity(
   				EuclidianStyleConstants.OBJSTYLE_DEFAULT_LINE_OPACITY_ANGLE);
   	} else if (geo instanceof GeoPolygon) {
   		geo.setLineOpacity(
   				EuclidianStyleConstants.OBJSTYLE_DEFAULT_LINE_OPACITY_POLYGON);
   	} else if (geo instanceof GeoConicPart) {
   		geo.setLineOpacity(
   				EuclidianStyleConstants.OBJSTYLE_DEFAULT_LINE_OPACITY_SECTOR);
   	} else if (cons.getApplication().isUnbundledGeometry()
   			&& (geo instanceof GeoLine || geo instanceof GeoSegment
   					|| geo instanceof GeoRay || geo instanceof GeoVector
   					|| geo instanceof GeoPolyLine
   					|| geo instanceof GeoConic)) {
   		geo.setLineOpacity(
   				EuclidianStyleConstants.OBJSTYLE_DEFAULT_LINE_OPACITY_GEOMETRY);
   	} else if (cons.getApplication().isUnbundledGraphing()
   			&& (geo instanceof GeoFunction)) {
   		geo.setLineOpacity(
   				EuclidianStyleConstants.OBJSTYLE_DEFAULT_LINE_OPACITY_FUNCTION_GEOMETRY);
   	} else if (cons.getApplication().isUnbundledGraphing()
   			&& (geo instanceof GeoCurveCartesian)) {
   		geo.setLineOpacity(
   				EuclidianStyleConstants.OBJSTYLE_DEFAULT_LINE_OPACITY_CURVE_GEOMETRY);
   	} else {
   		geo.setLineOpacity(
   				EuclidianStyleConstants.OBJSTYLE_DEFAULT_LINE_OPACITY);
   	}

   }

The instanceof checks can be evaluated to false at compile time, creating the dead branches in the conditional operator.

[niloc132]

Picking the inner-most ternary's condition to preserve my sanity, from the

this$static.cons.kernel.app.isUnbundledGeometry() && false

expression, I'm guessing that we really do have side effects, and this is the "(a(),false)" vs "a()&&false" difference, still preserving side effects - the former is easier to potentially optimize out, but the latter is shorter. As it happens, Simplifier.java explicitly calls this case out:

    * isWhatever() && false -> false, unless side effects

That above is embedded in a conditional if I'm reading it right, applying the multiexpr transformation we would have (what your patch now does):

(a(),false)?(locus.lineOpacity = 204):(locus.lineOpacity = 178)

Now we're in Simplifier.simplifyConditional(), and this case should cover it:

   * (a,b,c)?d:e -> a,b,(c?d:e)

so that we end up with

a(),(false?(locus.lineOpacity = 204):(locus.lineOpacity = 178))

When simplifyConditional does that, it recursively calls itself, so would collapse that false right away, and just leave

a(),locus.lineOpacity = 178;

Does that work with the current version of the patch? At a glance, these should apply in the correct order to take place in a single pass of DCE?

As an aside, its too bad that the

      // Special case
      // if () { return ..; } else { return ..; } =>
      // return ... ? ... : ...;

optimization doesn't work on assignment - it would be an easy win for your code (and lots of other code) to add a case like

// if () { a=.. } else { a=.. } -> a = (... ? .. : ..) ;

[niloc132]

I wrote a quick test that seems to confirm that the last step at least works as expected:

    runMethodInliner = true;
    addSnippetClassDecl(
        "static class A { "
            + "static int f1;"
            + "public static boolean sideEffect() { f1++; return true; } "
        + "}");

    optimize("boolean", "return A.sideEffect() ? b : b1;").intoString(
        "return (++EntryPoint$A.f1, EntryPoint.b);");

[niloc132]

#9731 use case potentially is also improved by this - effectively we have (a(), b(), false) ? c() : d(), and the nested multiexpressions (err, JsBinExpr with COMMA) aren't being rewritten out.

The false is confusing at this time for the Java AST because it isn't a literal false, its a Boolean object with value false. In JS-land, that's the same thing, so we can optimize it here, and hopefully this pattern of nested commas is something we can handle now (or can add on, or can follow up with).