also use iteration to print binary ops (#2911)

CHANGELOG.md

@@ -35,9 +35,7 @@
 
     This edge case involves a chain of binary operators that results in an AST over 400 nodes deep. Normally this wouldn't be a problem because Go has growable call stacks, so the call stack would just grow to be as large as needed. However, WebAssembly byte code deliberately doesn't expose the ability to manipulate the stack pointer, so Go's WebAssembly translation is forced to use the fixed-size WebAssembly call stack. So esbuild's WebAssembly implementation is vulnerable to stack overflow in cases like these.
 
-    It's not unreasonable for this to cause a stack overflow, and for esbuild's answer to this problem to be "don't write code like this." That's how many other AST-manipulation tools handle this problem. However, it's possible to implement AST traversal using iteration instead of recursion to work around limited call stack space. This version of esbuild implements this code transformation for esbuild's JavaScript parser, so esbuild's WebAssembly implementation is now able to process the `grapheme-splitter` package when compiled with Go 1.20.0 and run in `node`.
-
-    This deeply-nested AST problem can also happen during AST printing. It seems like the code in the `grapheme-splitter` package isn't currently deep enough to trigger this problem, so esbuild's printer doesn't have this code transformation yet. But if it's a problem in the future, esbuild's printer can also be transformed in the same way.
+    It's not unreasonable for this to cause a stack overflow, and for esbuild's answer to this problem to be "don't write code like this." That's how many other AST-manipulation tools handle this problem. However, it's possible to implement AST traversal using iteration instead of recursion to work around limited call stack space. This version of esbuild implements this code transformation for esbuild's JavaScript parser and printer, so esbuild's WebAssembly implementation is now able to process the `grapheme-splitter` package (at least when compiled with Go 1.20.0 and run with node's WebAssembly implementation).
 
 ## 0.17.7
 

internal/js_ast/js_ast.go

@@ -170,13 +170,13 @@ const (
 	BinOpLogicalAndAssign
 )
 
-type opTableEntry struct {
+type OpTableEntry struct {
 	Text      string
 	Level     L
 	IsKeyword bool
 }
 
-var OpTable = []opTableEntry{
+var OpTable = []OpTableEntry{
 	// Prefix
 	{"+", LPrefix, false},
 	{"-", LPrefix, false},

internal/js_printer/js_printer.go

@@ -340,6 +340,7 @@ type printer struct {
 	extractedLegalComments []string
 	js                     []byte
 	jsonMetadataImports    []string
+	binaryExprStack        []binaryExprVisitor
 	options                Options
 	builder                sourcemap.ChunkBuilder
 
@@ -2884,86 +2885,159 @@ func (p *printer) printExpr(expr js_ast.Expr, level js_ast.L, flags printExprFla
 		}
 
 	case *js_ast.EBinary:
-		p.printBinary(e, level, flags)
+		// The handling of binary expressions is convoluted because we're using
+		// iteration on the heap instead of recursion on the call stack to avoid
+		// stack overflow for deeply-nested ASTs. See the comments for the similar
+		// code in the JavaScript parser for details.
+		v := binaryExprVisitor{
+			e:     e,
+			level: level,
+			flags: flags,
+		}
+
+		// Use a single stack to reduce allocation overhead
+		stackBottom := len(p.binaryExprStack)
+
+		for {
+			// Check whether this node is a special case, and stop if it is
+			if !v.checkAndPrepare(p) {
+				break
+			}
+
+			left := v.e.Left
+			leftBinary, ok := left.Data.(*js_ast.EBinary)
+
+			// Stop iterating if iteration doesn't apply to the left node
+			if !ok {
+				p.printExpr(left, v.leftLevel, v.leftFlags)
+				v.visitRightAndFinish(p)
+				break
+			}
+
+			// Manually run the code at the start of "printExpr"
+			p.printExprCommentsAtLoc(left.Loc)
+
+			// Only allocate heap memory on the stack for nested binary expressions
+			p.binaryExprStack = append(p.binaryExprStack, v)
+			v = binaryExprVisitor{
+				e:     leftBinary,
+				level: v.leftLevel,
+				flags: v.leftFlags,
+			}
+		}
+
+		// Process all binary operations from the deepest-visited node back toward
+		// our original top-level binary operation
+		for {
+			n := len(p.binaryExprStack) - 1
+			if n < stackBottom {
+				break
+			}
+			v := p.binaryExprStack[n]
+			p.binaryExprStack = p.binaryExprStack[:n]
+			v.visitRightAndFinish(p)
+		}
 
 	default:
 		panic(fmt.Sprintf("Unexpected expression of type %T", expr.Data))
 	}
 }
 
-func (p *printer) printBinary(e *js_ast.EBinary, level js_ast.L, flags printExprFlags) {
+// The handling of binary expressions is convoluted because we're using
+// iteration on the heap instead of recursion on the call stack to avoid
+// stack overflow for deeply-nested ASTs. See the comments for the similar
+// code in the JavaScript parser for details.
+type binaryExprVisitor struct {
+	// Inputs
+	e     *js_ast.EBinary
+	level js_ast.L
+	flags printExprFlags
+
+	// Input for visiting the left child
+	leftLevel js_ast.L
+	leftFlags printExprFlags
+
+	// "Local variables" passed from "checkAndPrepare" to "visitRightAndFinish"
+	entry      js_ast.OpTableEntry
+	wrap       bool
+	rightLevel js_ast.L
+}
+
+func (v *binaryExprVisitor) checkAndPrepare(p *printer) bool {
+	e := v.e
+
 	// If this is a comma operator then either the result is unused (and we
 	// should have already simplified unused expressions), or the result is used
 	// (and we can still simplify unused expressions inside the left operand)
 	if e.Op == js_ast.BinOpComma {
-		if (flags & didAlreadySimplifyUnusedExprs) == 0 {
+		if (v.flags & didAlreadySimplifyUnusedExprs) == 0 {
 			left := p.simplifyUnusedExpr(e.Left)
 			right := e.Right
-			if (flags & exprResultIsUnused) != 0 {
+			if (v.flags & exprResultIsUnused) != 0 {
 				right = p.simplifyUnusedExpr(right)
 			}
 			if left.Data != e.Left.Data || right.Data != e.Right.Data {
 				// Pass a flag so we don't needlessly re-simplify the same expression
-				p.printExpr(p.guardAgainstBehaviorChangeDueToSubstitution(js_ast.JoinWithComma(left, right), flags), level, flags|didAlreadySimplifyUnusedExprs)
-				return
+				p.printExpr(p.guardAgainstBehaviorChangeDueToSubstitution(js_ast.JoinWithComma(left, right), v.flags), v.level, v.flags|didAlreadySimplifyUnusedExprs)
+				return false
 			}
 		} else {
 			// Pass a flag so we don't needlessly re-simplify the same expression
-			flags |= didAlreadySimplifyUnusedExprs
+			v.flags |= didAlreadySimplifyUnusedExprs
 		}
 	}
 
-	entry := js_ast.OpTable[e.Op]
-	wrap := level >= entry.Level || (e.Op == js_ast.BinOpIn && (flags&forbidIn) != 0)
+	v.entry = js_ast.OpTable[e.Op]
+	v.wrap = v.level >= v.entry.Level || (e.Op == js_ast.BinOpIn && (v.flags&forbidIn) != 0)
 
 	// Destructuring assignments must be parenthesized
 	if n := len(p.js); p.stmtStart == n || p.arrowExprStart == n {
 		if _, ok := e.Left.Data.(*js_ast.EObject); ok {
-			wrap = true
+			v.wrap = true
 		}
 	}
 
-	if wrap {
+	if v.wrap {
 		p.print("(")
-		flags &= ^forbidIn
+		v.flags &= ^forbidIn
 	}
 
-	leftLevel := entry.Level - 1
-	rightLevel := entry.Level - 1
+	v.leftLevel = v.entry.Level - 1
+	v.rightLevel = v.entry.Level - 1
 
 	if e.Op.IsRightAssociative() {
-		leftLevel = entry.Level
+		v.leftLevel = v.entry.Level
 	}
 	if e.Op.IsLeftAssociative() {
-		rightLevel = entry.Level
+		v.rightLevel = v.entry.Level
 	}
 
 	switch e.Op {
 	case js_ast.BinOpNullishCoalescing:
 		// "??" can't directly contain "||" or "&&" without being wrapped in parentheses
 		if left, ok := e.Left.Data.(*js_ast.EBinary); ok && (left.Op == js_ast.BinOpLogicalOr || left.Op == js_ast.BinOpLogicalAnd) {
-			leftLevel = js_ast.LPrefix
+			v.leftLevel = js_ast.LPrefix
 		}
 		if right, ok := e.Right.Data.(*js_ast.EBinary); ok && (right.Op == js_ast.BinOpLogicalOr || right.Op == js_ast.BinOpLogicalAnd) {
-			rightLevel = js_ast.LPrefix
+			v.rightLevel = js_ast.LPrefix
 		}
 
 	case js_ast.BinOpPow:
 		// "**" can't contain certain unary expressions
 		if left, ok := e.Left.Data.(*js_ast.EUnary); ok && left.Op.UnaryAssignTarget() == js_ast.AssignTargetNone {
-			leftLevel = js_ast.LCall
+			v.leftLevel = js_ast.LCall
 		} else if _, ok := e.Left.Data.(*js_ast.EAwait); ok {
-			leftLevel = js_ast.LCall
+			v.leftLevel = js_ast.LCall
 		} else if _, ok := e.Left.Data.(*js_ast.EUndefined); ok {
 			// Undefined is printed as "void 0"
-			leftLevel = js_ast.LCall
+			v.leftLevel = js_ast.LCall
 		} else if _, ok := e.Left.Data.(*js_ast.ENumber); ok {
 			// Negative numbers are printed using a unary operator
-			leftLevel = js_ast.LCall
+			v.leftLevel = js_ast.LCall
 		} else if p.options.MinifySyntax {
 			// When minifying, booleans are printed as "!0 and "!1"
 			if _, ok := e.Left.Data.(*js_ast.EBoolean); ok {
-				leftLevel = js_ast.LCall
+				v.leftLevel = js_ast.LCall
 			}
 		}
 	}
@@ -2973,23 +3047,32 @@ func (p *printer) printBinary(e *js_ast.EBinary, level js_ast.L, flags printExpr
 		name := p.renamer.NameForSymbol(private.Ref)
 		p.addSourceMappingForName(e.Left.Loc, name, private.Ref)
 		p.printIdentifier(name)
-	} else if e.Op == js_ast.BinOpComma {
+		v.visitRightAndFinish(p)
+		return false
+	}
+
+	if e.Op == js_ast.BinOpComma {
 		// The result of the left operand of the comma operator is unused
-		p.printExpr(e.Left, leftLevel, (flags&forbidIn)|exprResultIsUnused|parentWasUnaryOrBinary)
+		v.leftFlags = (v.flags & forbidIn) | exprResultIsUnused | parentWasUnaryOrBinary
 	} else {
-		p.printExpr(e.Left, leftLevel, (flags&forbidIn)|parentWasUnaryOrBinary)
+		v.leftFlags = (v.flags & forbidIn) | parentWasUnaryOrBinary
 	}
+	return true
+}
+
+func (v *binaryExprVisitor) visitRightAndFinish(p *printer) {
+	e := v.e
 
 	if e.Op != js_ast.BinOpComma {
 		p.printSpace()
 	}
 
-	if entry.IsKeyword {
+	if v.entry.IsKeyword {
 		p.printSpaceBeforeIdentifier()
-		p.print(entry.Text)
+		p.print(v.entry.Text)
 	} else {
 		p.printSpaceBeforeOperator(e.Op)
-		p.print(entry.Text)
+		p.print(v.entry.Text)
 		p.prevOp = e.Op
 		p.prevOpEnd = len(p.js)
 	}
@@ -2998,12 +3081,12 @@ func (p *printer) printBinary(e *js_ast.EBinary, level js_ast.L, flags printExpr
 
 	if e.Op == js_ast.BinOpComma {
 		// The result of the right operand of the comma operator is unused if the caller doesn't use it
-		p.printExpr(e.Right, rightLevel, (flags&(forbidIn|exprResultIsUnused))|parentWasUnaryOrBinary)
+		p.printExpr(e.Right, v.rightLevel, (v.flags&(forbidIn|exprResultIsUnused))|parentWasUnaryOrBinary)
 	} else {
-		p.printExpr(e.Right, rightLevel, (flags&forbidIn)|parentWasUnaryOrBinary)
+		p.printExpr(e.Right, v.rightLevel, (v.flags&forbidIn)|parentWasUnaryOrBinary)
 	}
 
-	if wrap {
+	if v.wrap {
 		p.print(")")
 	}
 }

internal/js_printer/js_printer_test.go

@@ -1,6 +1,7 @@
 package js_printer
 
 import (
+	"strings"
 	"testing"
 
 	"github.com/evanw/esbuild/internal/compat"
@@ -1074,3 +1075,12 @@ func TestInfinity(t *testing.T) {
 	expectPrintedMangleMinify(t, "x = y / Infinity", "x=y/(1/0);")
 	expectPrintedMangleMinify(t, "throw Infinity", "throw 1/0;")
 }
+
+func TestBinaryOperatorVisitor(t *testing.T) {
+	// Make sure the inner "/*b*/" comment doesn't disappear due to weird binary visitor stuff
+	expectPrintedMangle(t, "x = (0, /*a*/ (0, /*b*/ (0, /*c*/ 1 == 2) + 3) * 4)", "x = /*a*/\n/*b*/\n(/*c*/\n!1 + 3) * 4;\n")
+
+	// Make sure deeply-nested ASTs don't cause a stack overflow
+	x := "x = f()" + strings.Repeat(" || f()", 10_000) + ";\n"
+	expectPrinted(t, x, x)
+}