From 701fcabf467531572e4d6a8d5c7660b400328481 Mon Sep 17 00:00:00 2001 From: ydah Date: Thu, 25 Dec 2025 23:06:19 +0900 Subject: [PATCH 1/2] Fix infinite loop when rendering multiple groups of adjacent inline elements separated by line breaks Fixes: https://github.com/marcoroth/herb/issues/978 --- .../packages/formatter/src/format-printer.ts | 166 +++++++++++++++++- .../packages/formatter/test/erb/erb.test.ts | 24 +++ 2 files changed, 186 insertions(+), 4 deletions(-) diff --git a/javascript/packages/formatter/src/format-printer.ts b/javascript/packages/formatter/src/format-printer.ts index 7b45deafe..efacefdc5 100644 --- a/javascript/packages/formatter/src/format-printer.ts +++ b/javascript/packages/formatter/src/format-printer.ts @@ -1792,7 +1792,7 @@ export class FormatPrinter extends Printer { } } - const match = trimmedText.match(/^[.!?:;]+/) + const match = trimmedText.match(/^[.!?:;%]+/) if (!match) { return { @@ -1899,7 +1899,7 @@ export class FormatPrinter extends Printer { if (isNode(child, HTMLTextNode)) { const trimmed = child.content.trim() - if (trimmed && /^[.!?:;]/.test(trimmed)) { + if (trimmed && /^[.!?:;%]/.test(trimmed)) { const wrapWidth = this.maxLineLength - this.indent.length const result = this.tryMergePunctuationText(inlineContent, trimmed, wrapWidth) @@ -1963,22 +1963,180 @@ export class FormatPrinter extends Printer { }).join("") } + /** + * Count adjacent inline elements starting from a specific index + * Skips indices that are already processed + */ + private countAdjacentInlineElementsFromIndex( + children: Node[], + startIndex: number, + processedIndices: Set + ): number { + let count = 0 + let lastSignificantIndex = -1 + + for (let i = startIndex; i < children.length; i++) { + const child = children[i] + + if (isPureWhitespaceNode(child) || isNode(child, WhitespaceNode)) { + continue + } + + if (processedIndices.has(i)) { + break + } + + const isInlineOrERB = + (isNode(child, HTMLElementNode) && isInlineElement(getTagName(child))) || + isNode(child, ERBContentNode) + + if (!isInlineOrERB) { + break + } + + if (lastSignificantIndex >= 0 && hasWhitespaceBetween(children, lastSignificantIndex, i)) { + break + } + + count++ + lastSignificantIndex = i + } + + return count + } + + /** + * Render adjacent inline elements starting from a specific index + * Similar to renderAdjacentInlineElements but works from an arbitrary start point + */ + private renderAdjacentInlineElementsFromIndex( + children: Node[], + startIndex: number, + count: number, + alreadyProcessed: Set + ): { processedIndices: Set; lastIndex: number } { + let inlineContent = "" + let processedCount = 0 + let lastProcessedIndex = -1 + const processedIndices = new Set() + + for (let index = startIndex; index < children.length && processedCount < count; index++) { + const child = children[index] + + if (isPureWhitespaceNode(child) || isNode(child, WhitespaceNode)) { + continue + } + + if (alreadyProcessed.has(index)) { + continue + } + + if (isNode(child, HTMLElementNode) && isInlineElement(getTagName(child))) { + inlineContent += this.renderInlineElementAsString(child) + processedCount++ + lastProcessedIndex = index + processedIndices.add(index) + + if (inlineContent && isLineBreakingElement(child)) { + this.pushWithIndent(inlineContent) + inlineContent = "" + } + } else if (isNode(child, ERBContentNode)) { + inlineContent += this.renderERBAsString(child) + processedCount++ + lastProcessedIndex = index + processedIndices.add(index) + } + } + + if (lastProcessedIndex >= 0) { + for (let index = lastProcessedIndex + 1; index < children.length; index++) { + const child = children[index] + + if (isPureWhitespaceNode(child) || isNode(child, WhitespaceNode)) { + continue + } + + if (alreadyProcessed.has(index)) { + break + } + + if (isNode(child, ERBContentNode)) { + inlineContent += this.renderERBAsString(child) + processedIndices.add(index) + lastProcessedIndex = index + continue + } + + if (isNode(child, HTMLTextNode)) { + const trimmed = child.content.trim() + + if (trimmed && /^[.!?:;%]/.test(trimmed)) { + const wrapWidth = this.maxLineLength - this.indent.length + const result = this.tryMergePunctuationText(inlineContent, trimmed, wrapWidth) + + inlineContent = result.mergedContent + processedIndices.add(index) + lastProcessedIndex = index + + if (result.shouldStop) { + if (inlineContent) { + this.pushWithIndent(inlineContent) + } + + result.wrappedLines.forEach(line => this.push(line)) + + return { processedIndices, lastIndex: lastProcessedIndex } + } + } + } + + break + } + } + + if (inlineContent) { + this.pushWithIndent(inlineContent) + } + + return { + processedIndices, + lastIndex: lastProcessedIndex >= 0 ? lastProcessedIndex : startIndex + count - 1 + } + } + /** * Visit remaining children after processing adjacent inline elements + * Recursively handles groups of adjacent inline elements */ private visitRemainingChildren(children: Node[], processedIndices: Set): void { - for (let index = 0; index < children.length; index++) { + let index = 0 + + while (index < children.length) { const child = children[index] if (isPureWhitespaceNode(child) || isNode(child, WhitespaceNode)) { + index++ continue } if (processedIndices.has(index)) { + index++ continue } - this.visit(child) + const adjacentCount = this.countAdjacentInlineElementsFromIndex(children, index, processedIndices) + + if (adjacentCount >= 2) { + const { processedIndices: newProcessedIndices, lastIndex } = + this.renderAdjacentInlineElementsFromIndex(children, index, adjacentCount, processedIndices) + + newProcessedIndices.forEach(i => processedIndices.add(i)) + index = lastIndex + 1 + } else { + this.visit(child) + index++ + } } } diff --git a/javascript/packages/formatter/test/erb/erb.test.ts b/javascript/packages/formatter/test/erb/erb.test.ts index 1dff621d8..40367e5b1 100644 --- a/javascript/packages/formatter/test/erb/erb.test.ts +++ b/javascript/packages/formatter/test/erb/erb.test.ts @@ -270,6 +270,30 @@ describe("@herb-tools/formatter", () => { expect(secondFormat).toBe(result) }) + test("issue 978: does not duplicate content with multiple adjacent inline/ERB groups separated by br", () => { + const input = dedent` +

+ <%= t("admin.tickets.form.savings") %><%= ticket.ticketable.savings_percentage %>% +
+ <%= t("admin.tickets.form.price_per_ticket") %><%= format_price(ticket.ticketable.price_per_ticket) %> +

+ ` + + const result = formatter.format(input) + + expect(result).toBe(dedent` +

+ <%= t("admin.tickets.form.savings") %><%= ticket.ticketable.savings_percentage %>% +
+ <%= t("admin.tickets.form.price_per_ticket") %><%= format_price(ticket.ticketable.price_per_ticket) %> +

+ `) + + // Test idempotency + const secondFormat = formatter.format(result) + expect(secondFormat).toBe(result) + }) + test("https://github.com/hanakai-rb/site/blob/8adc128d9d464f3e37615be2aa29d57979904533/app/templates/pages/home.html.erb", () => { const input = dedent`

Hanakai

From f44d22199fffd490b42378d4c53f43fcd18b85c6 Mon Sep 17 00:00:00 2001 From: Marco Roth Date: Sun, 1 Mar 2026 15:02:06 +0100 Subject: [PATCH 2/2] Cleanup --- .../packages/formatter/src/format-helpers.ts | 11 +- .../packages/formatter/src/format-printer.ts | 171 +++--------------- 2 files changed, 29 insertions(+), 153 deletions(-) diff --git a/javascript/packages/formatter/src/format-helpers.ts b/javascript/packages/formatter/src/format-helpers.ts index 7fa1203ba..010748902 100644 --- a/javascript/packages/formatter/src/format-helpers.ts +++ b/javascript/packages/formatter/src/format-helpers.ts @@ -398,19 +398,24 @@ export function isContentPreserving(element: HTMLElementNode | HTMLOpenTagNode | } /** - * Count consecutive inline elements/ERB at the start of children (with no whitespace between) + * Count consecutive inline elements/ERB with no whitespace between them. + * Starts from startIndex and skips indices in processedIndices. */ -export function countAdjacentInlineElements(children: Node[]): number { +export function countAdjacentInlineElements(children: Node[], startIndex = 0, processedIndices?: Set): number { let count = 0 let lastSignificantIndex = -1 - for (let i = 0; i < children.length; i++) { + for (let i = startIndex; i < children.length; i++) { const child = children[i] if (isPureWhitespaceNode(child) || isNode(child, WhitespaceNode)) { continue } + if (processedIndices?.has(i)) { + break + } + const isInlineOrERB = (isNode(child, HTMLElementNode) && isInlineElement(getTagName(child))) || isNode(child, ERBContentNode) if (!isInlineOrERB) { diff --git a/javascript/packages/formatter/src/format-printer.ts b/javascript/packages/formatter/src/format-printer.ts index efacefdc5..3cf415255 100644 --- a/javascript/packages/formatter/src/format-printer.ts +++ b/javascript/packages/formatter/src/format-printer.ts @@ -1851,19 +1851,23 @@ export class FormatPrinter extends Printer { /** * Render adjacent inline elements together on one line */ - private renderAdjacentInlineElements(children: Node[], count: number): { processedIndices: Set } { + private renderAdjacentInlineElements(children: Node[], count: number, startIndex = 0, alreadyProcessed?: Set): { processedIndices: Set; lastIndex: number } { let inlineContent = "" let processedCount = 0 let lastProcessedIndex = -1 const processedIndices = new Set() - for (let index = 0; index < children.length && processedCount < count; index++) { + for (let index = startIndex; index < children.length && processedCount < count; index++) { const child = children[index] if (isPureWhitespaceNode(child) || isNode(child, WhitespaceNode)) { continue } + if (alreadyProcessed?.has(index)) { + continue + } + if (isNode(child, HTMLElementNode) && isInlineElement(getTagName(child))) { inlineContent += this.renderInlineElementAsString(child) processedCount++ @@ -1890,9 +1894,14 @@ export class FormatPrinter extends Printer { continue } + if (alreadyProcessed?.has(index)) { + break + } + if (isNode(child, ERBContentNode)) { inlineContent += this.renderERBAsString(child) processedIndices.add(index) + lastProcessedIndex = index continue } @@ -1905,6 +1914,7 @@ export class FormatPrinter extends Printer { inlineContent = result.mergedContent processedIndices.add(index) + lastProcessedIndex = index if (result.shouldStop) { if (inlineContent) { @@ -1913,7 +1923,7 @@ export class FormatPrinter extends Printer { result.wrappedLines.forEach(line => this.push(line)) - return { processedIndices } + return { processedIndices, lastIndex: lastProcessedIndex } } } } @@ -1926,7 +1936,10 @@ export class FormatPrinter extends Printer { this.pushWithIndent(inlineContent) } - return { processedIndices } + return { + processedIndices, + lastIndex: lastProcessedIndex >= 0 ? lastProcessedIndex : startIndex + count - 1 + } } /** @@ -1964,150 +1977,8 @@ export class FormatPrinter extends Printer { } /** - * Count adjacent inline elements starting from a specific index - * Skips indices that are already processed - */ - private countAdjacentInlineElementsFromIndex( - children: Node[], - startIndex: number, - processedIndices: Set - ): number { - let count = 0 - let lastSignificantIndex = -1 - - for (let i = startIndex; i < children.length; i++) { - const child = children[i] - - if (isPureWhitespaceNode(child) || isNode(child, WhitespaceNode)) { - continue - } - - if (processedIndices.has(i)) { - break - } - - const isInlineOrERB = - (isNode(child, HTMLElementNode) && isInlineElement(getTagName(child))) || - isNode(child, ERBContentNode) - - if (!isInlineOrERB) { - break - } - - if (lastSignificantIndex >= 0 && hasWhitespaceBetween(children, lastSignificantIndex, i)) { - break - } - - count++ - lastSignificantIndex = i - } - - return count - } - - /** - * Render adjacent inline elements starting from a specific index - * Similar to renderAdjacentInlineElements but works from an arbitrary start point - */ - private renderAdjacentInlineElementsFromIndex( - children: Node[], - startIndex: number, - count: number, - alreadyProcessed: Set - ): { processedIndices: Set; lastIndex: number } { - let inlineContent = "" - let processedCount = 0 - let lastProcessedIndex = -1 - const processedIndices = new Set() - - for (let index = startIndex; index < children.length && processedCount < count; index++) { - const child = children[index] - - if (isPureWhitespaceNode(child) || isNode(child, WhitespaceNode)) { - continue - } - - if (alreadyProcessed.has(index)) { - continue - } - - if (isNode(child, HTMLElementNode) && isInlineElement(getTagName(child))) { - inlineContent += this.renderInlineElementAsString(child) - processedCount++ - lastProcessedIndex = index - processedIndices.add(index) - - if (inlineContent && isLineBreakingElement(child)) { - this.pushWithIndent(inlineContent) - inlineContent = "" - } - } else if (isNode(child, ERBContentNode)) { - inlineContent += this.renderERBAsString(child) - processedCount++ - lastProcessedIndex = index - processedIndices.add(index) - } - } - - if (lastProcessedIndex >= 0) { - for (let index = lastProcessedIndex + 1; index < children.length; index++) { - const child = children[index] - - if (isPureWhitespaceNode(child) || isNode(child, WhitespaceNode)) { - continue - } - - if (alreadyProcessed.has(index)) { - break - } - - if (isNode(child, ERBContentNode)) { - inlineContent += this.renderERBAsString(child) - processedIndices.add(index) - lastProcessedIndex = index - continue - } - - if (isNode(child, HTMLTextNode)) { - const trimmed = child.content.trim() - - if (trimmed && /^[.!?:;%]/.test(trimmed)) { - const wrapWidth = this.maxLineLength - this.indent.length - const result = this.tryMergePunctuationText(inlineContent, trimmed, wrapWidth) - - inlineContent = result.mergedContent - processedIndices.add(index) - lastProcessedIndex = index - - if (result.shouldStop) { - if (inlineContent) { - this.pushWithIndent(inlineContent) - } - - result.wrappedLines.forEach(line => this.push(line)) - - return { processedIndices, lastIndex: lastProcessedIndex } - } - } - } - - break - } - } - - if (inlineContent) { - this.pushWithIndent(inlineContent) - } - - return { - processedIndices, - lastIndex: lastProcessedIndex >= 0 ? lastProcessedIndex : startIndex + count - 1 - } - } - - /** - * Visit remaining children after processing adjacent inline elements - * Recursively handles groups of adjacent inline elements + * Visit remaining children after processing adjacent inline elements. + * Detects and renders subsequent groups of adjacent inline elements. */ private visitRemainingChildren(children: Node[], processedIndices: Set): void { let index = 0 @@ -2125,11 +1996,11 @@ export class FormatPrinter extends Printer { continue } - const adjacentCount = this.countAdjacentInlineElementsFromIndex(children, index, processedIndices) + const adjacentCount = countAdjacentInlineElements(children, index, processedIndices) if (adjacentCount >= 2) { const { processedIndices: newProcessedIndices, lastIndex } = - this.renderAdjacentInlineElementsFromIndex(children, index, adjacentCount, processedIndices) + this.renderAdjacentInlineElements(children, adjacentCount, index, processedIndices) newProcessedIndices.forEach(i => processedIndices.add(i)) index = lastIndex + 1