Skip to content

Commit a330459

Browse files
authored
Linter: Implement autofix for erb-no-duplicate-branch-elements (#1302)
Uses #1300 to implement an autofix for #1301 https://github.com/user-attachments/assets/2f19df6f-dc33-43c8-9450-cfae6cc67db5
1 parent ca8b28c commit a330459

File tree

14 files changed

+509
-82
lines changed

14 files changed

+509
-82
lines changed

javascript/packages/core/src/ast-utils.ts

Lines changed: 114 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,14 +28,16 @@ import {
2828
isHTMLCommentNode,
2929
isHTMLElementNode,
3030
isHTMLOpenTagNode,
31+
isHTMLTextNode,
32+
isWhitespaceNode,
3133
isHTMLAttributeNameNode,
3234
isHTMLAttributeValueNode,
3335
areAllOfType,
3436
filterLiteralNodes,
3537
filterHTMLAttributeNodes
3638
} from "./node-type-guards.js"
3739

38-
import type { Location } from "./location.js"
40+
import { Location } from "./location.js"
3941
import type { Position } from "./position.js"
4042

4143
export type ERBOutputNode = ERBNode & {
@@ -497,10 +499,14 @@ export function forEachAttribute(node: HTMLElementNode | HTMLOpenTagNode, callba
497499
// --- Class Name Grouping Utilities ---
498500

499501
/**
500-
* Checks if a node is a whitespace-only literal (no visible content)
502+
* Checks if a node is a whitespace-only literal or text node (no visible content)
501503
*/
502-
export function isWhitespaceLiteral(node: Node): boolean {
503-
return isLiteralNode(node) && !node.content.trim()
504+
export function isPureWhitespaceNode(node: Node): boolean {
505+
if (isWhitespaceNode(node)) return true
506+
if (isLiteralNode(node)) return !node.content.trim()
507+
if (isHTMLTextNode(node)) return !(node.content ?? "").trim()
508+
509+
return false
504510
}
505511

506512
/**
@@ -566,7 +572,7 @@ export function groupNodesByClass(nodes: Node[]): Node[][] {
566572
startNewGroup = false
567573
} else if (previousNode && !isLiteralNode(previousNode)) {
568574
startNewGroup = true
569-
} else if (currentGroup.every(member => isWhitespaceLiteral(member))) {
575+
} else if (currentGroup.every(member => isPureWhitespaceNode(member))) {
570576
startNewGroup = true
571577
}
572578
} else {
@@ -747,3 +753,106 @@ export function isEquivalentElement(first: HTMLElementNode, second: HTMLElementN
747753

748754
return isEquivalentOpenTag(first.open_tag, second.open_tag)
749755
}
756+
757+
// --- AST Mutation Utilities ---
758+
759+
const CHILD_ARRAY_PROPS = ["children", "body", "statements", "conditions"]
760+
const LINKED_NODE_PROPS = ["subsequent", "else_clause"]
761+
762+
/**
763+
* Finds the array containing a target node in the AST, along with its index.
764+
* Traverses child arrays and linked node properties (e.g., `subsequent`, `else_clause`).
765+
*
766+
* Useful for autofix operations that need to splice nodes in/out of their parent array.
767+
*
768+
* @param root - The root node to search from
769+
* @param target - The node to find
770+
* @returns The containing array and the target's index, or null if not found
771+
*/
772+
export function findParentArray(root: Node, target: Node): { array: Node[], index: number } | null {
773+
const search = (node: Node): { array: Node[], index: number } | null => {
774+
const record = node as Record<string, any>
775+
776+
for (const prop of CHILD_ARRAY_PROPS) {
777+
const array = record[prop]
778+
779+
if (Array.isArray(array)) {
780+
const index = array.indexOf(target)
781+
782+
if (index !== -1) {
783+
return { array, index }
784+
}
785+
}
786+
}
787+
788+
for (const prop of CHILD_ARRAY_PROPS) {
789+
const array = record[prop]
790+
791+
if (Array.isArray(array)) {
792+
for (const child of array) {
793+
if (child && typeof child === 'object' && 'type' in child) {
794+
const result = search(child)
795+
796+
if (result) {
797+
return result
798+
}
799+
}
800+
}
801+
}
802+
}
803+
804+
for (const prop of LINKED_NODE_PROPS) {
805+
const value = record[prop]
806+
807+
if (value && typeof value === 'object' && 'type' in value) {
808+
const result = search(value)
809+
810+
if (result) {
811+
return result
812+
}
813+
}
814+
}
815+
816+
return null
817+
}
818+
819+
return search(root)
820+
}
821+
822+
/**
823+
* Removes a node from an array, also removing an adjacent preceding
824+
* whitespace-only literal if present.
825+
*/
826+
export function removeNodeFromArray(array: Node[], node: Node): void {
827+
const index = array.indexOf(node)
828+
if (index === -1) return
829+
830+
if (index > 0 && isPureWhitespaceNode(array[index - 1])) {
831+
array.splice(index - 1, 2)
832+
} else {
833+
array.splice(index, 1)
834+
}
835+
}
836+
837+
/**
838+
* Replaces an element in an array with its body (children), effectively unwrapping it.
839+
*/
840+
export function replaceNodeWithBody(array: Node[], element: HTMLElementNode): void {
841+
const index = array.indexOf(element)
842+
if (index === -1) return
843+
844+
array.splice(index, 1, ...element.body)
845+
}
846+
847+
/**
848+
* Creates a synthetic LiteralNode with the given content and zero location.
849+
* Useful for inserting whitespace or newlines during AST mutations.
850+
*/
851+
export function createLiteral(content: string): LiteralNode {
852+
return new LiteralNode({
853+
type: "AST_LITERAL_NODE",
854+
content,
855+
location: Location.zero,
856+
errors: [],
857+
})
858+
}

javascript/packages/formatter/src/format-helpers.ts

Lines changed: 3 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { isNode, isERBNode, getTagName, isAnyOf, isERBControlFlowNode, hasERBOutput, getStaticAttributeValue, getTokenList } from "@herb-tools/core"
1+
import { isNode, isERBNode, getTagName, isAnyOf, isERBControlFlowNode, hasERBOutput, getStaticAttributeValue, getTokenList, isPureWhitespaceNode } from "@herb-tools/core"
22
import { Node, HTMLDoctypeNode, HTMLTextNode, HTMLElementNode, HTMLCommentNode, HTMLOpenTagNode, HTMLCloseTagNode, ERBIfNode, ERBContentNode, WhitespaceNode } from "@herb-tools/core"
33

44
// --- Types ---
@@ -81,21 +81,11 @@ export const SPACEABLE_CONTAINERS = new Set([
8181

8282
// --- Node Utility Functions ---
8383

84-
/**
85-
* Check if a node is pure whitespace (empty text node with only whitespace)
86-
*/
87-
export function isPureWhitespaceNode(node: Node): boolean {
88-
return isNode(node, HTMLTextNode) && node.content.trim() === ""
89-
}
90-
9184
/**
9285
* Check if a node is non-whitespace (has meaningful content)
9386
*/
9487
export function isNonWhitespaceNode(node: Node): boolean {
95-
if (isNode(node, WhitespaceNode)) return false
96-
if (isNode(node, HTMLTextNode)) return node.content.trim() !== ""
97-
98-
return true
88+
return !isPureWhitespaceNode(node)
9989
}
10090

10191
/**
@@ -150,10 +140,9 @@ export function filterEmptyNodesForHerbDisable(nodes: Node[]): Node[] {
150140
let pendingWhitespace: Node | null = null
151141

152142
for (const node of nodes) {
153-
const isWhitespace = isNode(node, WhitespaceNode) || (isNode(node, HTMLTextNode) && node.content.trim() === "")
154143
const isHerbDisable = isNode(node, ERBContentNode) && isHerbDisableComment(node)
155144

156-
if (isWhitespace) {
145+
if (isPureWhitespaceNode(node)) {
157146
if (!pendingWhitespace) {
158147
pendingWhitespace = node
159148
}

javascript/packages/formatter/src/format-printer.ts

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import {
2828
isERBControlFlowNode,
2929
isERBCommentNode,
3030
isHTMLOpenTagNode,
31+
isPureWhitespaceNode,
3132
filterNodes,
3233
} from "@herb-tools/core"
3334

@@ -44,7 +45,6 @@ import {
4445
isHerbDisableComment,
4546
isInlineElement,
4647
isNonWhitespaceNode,
47-
isPureWhitespaceNode,
4848
shouldAppendToLastLine,
4949
shouldPreserveUserSpacing,
5050
} from "./format-helpers.js"
@@ -711,9 +711,7 @@ export class FormatPrinter extends Printer implements TextFlowDelegate, Attribut
711711
const child = body[index]
712712

713713
if (isNode(child, HTMLTextNode)) {
714-
const isWhitespaceOnly = child.content.trim() === ""
715-
716-
if (isWhitespaceOnly) {
714+
if (isPureWhitespaceNode(child)) {
717715
const hasPreviousNonWhitespace = index > 0 && isNonWhitespaceNode(body[index - 1])
718716
const hasNextNonWhitespace = index < body.length - 1 && isNonWhitespaceNode(body[index + 1])
719717
const hasMultipleNewlines = child.content.includes('\n\n')
@@ -1550,9 +1548,7 @@ export class FormatPrinter extends Printer implements TextFlowDelegate, Attribut
15501548
}
15511549
}
15521550

1553-
const isWhitespace = isNode(child, WhitespaceNode) || (isNode(child, HTMLTextNode) && child.content.trim() === "")
1554-
1555-
if (isWhitespace && !result.endsWith(' ')) {
1551+
if (isPureWhitespaceNode(child) && !result.endsWith(' ')) {
15561552
if (!result && hasHerbDisable && !addedLeadingSpace) {
15571553
result += ' '
15581554
addedLeadingSpace = true
@@ -1578,7 +1574,7 @@ export class FormatPrinter extends Printer implements TextFlowDelegate, Attribut
15781574
}
15791575

15801576
result += childInline
1581-
} else if (!isNode(child, HTMLTextNode) && !isWhitespace) {
1577+
} else if (!isNode(child, HTMLTextNode) && !isPureWhitespaceNode(child)) {
15821578
const captured = this.withInlineMode(() => this.capture(() => this.visit(child)).join(""))
15831579
result += captured
15841580
}

javascript/packages/formatter/src/text-flow-analyzer.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { isNode, getTagName, isERBCommentNode } from "@herb-tools/core"
1+
import { isNode, getTagName, isERBCommentNode, isPureWhitespaceNode } from "@herb-tools/core"
22
import { Node, HTMLTextNode, HTMLElementNode, ERBContentNode, WhitespaceNode } from "@herb-tools/core"
33

44
import type { ContentUnitWithNode } from "./format-helpers.js"
@@ -8,7 +8,6 @@ import {
88
isHerbDisableComment,
99
isInlineElement,
1010
isLineBreakingElement,
11-
isPureWhitespaceNode,
1211
} from "./format-helpers.js"
1312

1413
import {

javascript/packages/formatter/src/text-flow-engine.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { isNode, getTagName } from "@herb-tools/core"
1+
import { isNode, getTagName, isPureWhitespaceNode } from "@herb-tools/core"
22
import { Node, HTMLTextNode, HTMLElementNode, ERBContentNode, WhitespaceNode } from "@herb-tools/core"
33

44
import type { ContentUnitWithNode } from "./format-helpers.js"
@@ -10,7 +10,6 @@ import {
1010
isClosingPunctuation,
1111
isInlineElement,
1212
isLineBreakingElement,
13-
isPureWhitespaceNode,
1413
needsSpaceBetween,
1514
} from "./format-helpers.js"
1615

javascript/packages/language-server/src/document_save_service.ts

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,15 @@ export class DocumentSaveService {
1111
private autofixService: AutofixService
1212
private formattingService: FormattingService
1313

14+
/**
15+
* Tracks documents that were recently autofixed via applyFixesAndFormatting
16+
* (triggered by onDocumentFormatting). When editor.formatOnSave is enabled,
17+
* onDocumentFormatting fires BEFORE willSaveWaitUntil. If applyFixesAndFormatting
18+
* already applied autofix, applyFixes must skip to avoid conflicting edits
19+
* (since this.documents hasn't been updated between the two events).
20+
*/
21+
private recentlyAutofixedViaFormatting = new Set<string>()
22+
1423
constructor(connection: Connection, settings: Settings, autofixService: AutofixService, formattingService: FormattingService) {
1524
this.connection = connection
1625
this.settings = settings
@@ -30,6 +39,11 @@ export class DocumentSaveService {
3039

3140
if (!fixOnSave) return []
3241

42+
if (this.recentlyAutofixedViaFormatting.delete(document.uri)) {
43+
this.connection.console.log(`[DocumentSave] applyFixes skipping: already autofixed via formatting`)
44+
return []
45+
}
46+
3347
return this.autofixService.autofix(document)
3448
}
3549

@@ -48,6 +62,10 @@ export class DocumentSaveService {
4862

4963
if (fixOnSave) {
5064
autofixEdits = await this.autofixService.autofix(document)
65+
66+
if (autofixEdits.length > 0) {
67+
this.recentlyAutofixedViaFormatting.add(document.uri)
68+
}
5169
}
5270

5371
if (!formatterEnabled) return autofixEdits
@@ -56,12 +74,6 @@ export class DocumentSaveService {
5674
return this.formattingService.formatOnSave(document, reason)
5775
}
5876

59-
const autofixedDocument: TextDocument = {
60-
...document,
61-
uri: document.uri,
62-
getText: () => autofixEdits[0].newText,
63-
}
64-
65-
return this.formattingService.formatOnSave(autofixedDocument, reason)
77+
return this.formattingService.formatOnSave(document, reason, autofixEdits[0].newText)
6678
}
6779
}

javascript/packages/language-server/src/formatting_service.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,7 @@ export class FormattingService {
186186
}
187187
}
188188

189-
async formatOnSave(document: TextDocument, reason: TextDocumentSaveReason): Promise<TextEdit[]> {
189+
async formatOnSave(document: TextDocument, reason: TextDocumentSaveReason, textOverride?: string): Promise<TextEdit[]> {
190190
this.connection.console.log(`[Formatting] formatOnSave called for ${document.uri}`)
191191

192192
if (reason !== TextDocumentSaveReason.Manual) {
@@ -203,7 +203,7 @@ export class FormattingService {
203203
return []
204204
}
205205

206-
return this.performFormatting({ textDocument: { uri: document.uri }, options: { tabSize: 2, insertSpaces: true } })
206+
return this.performFormatting({ textDocument: { uri: document.uri }, options: { tabSize: 2, insertSpaces: true } }, textOverride)
207207
}
208208

209209
private shouldFormatFile(filePath: string): boolean {
@@ -235,15 +235,15 @@ export class FormattingService {
235235
} as Config
236236
}
237237

238-
private async performFormatting(params: DocumentFormattingParams): Promise<TextEdit[]> {
238+
private async performFormatting(params: DocumentFormattingParams, textOverride?: string): Promise<TextEdit[]> {
239239
const document = this.documents.get(params.textDocument.uri)
240240

241241
if (!document) {
242242
return []
243243
}
244244

245245
try {
246-
const text = document.getText()
246+
const text = textOverride ?? document.getText()
247247
const config = await this.getConfigWithSettings(params.textDocument.uri)
248248

249249
this.connection.console.log(`[Formatting] Creating formatter with ${this.preRewriters.length} pre-rewriters, ${this.postRewriters.length} post-rewriters`)

javascript/packages/language-server/test/document_save_service.test.ts

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -163,12 +163,11 @@ describe('DocumentSaveService', () => {
163163

164164
expect(autofixService.autofix).toHaveBeenCalledWith(document)
165165

166-
expect(formattingService.formatOnSave).toHaveBeenCalled()
167-
168-
const callArgs = vi.mocked(formattingService.formatOnSave).mock.calls[0]
169-
expect(callArgs[0].uri).toBe(document.uri)
170-
expect(callArgs[0].getText()).toBe('<div>fixed</div>\n')
171-
expect(callArgs[1]).toBe(TextDocumentSaveReason.Manual)
166+
expect(formattingService.formatOnSave).toHaveBeenCalledWith(
167+
document,
168+
TextDocumentSaveReason.Manual,
169+
'<div>fixed</div>\n'
170+
)
172171

173172
expect(result).toEqual(formatEdits)
174173
})

0 commit comments

Comments
 (0)