Skip to content

Commit ff09afe

Browse files
marcorothmarcoroth
authored
Linter: Fix false positive for dynamic IDs across separate ERB blocks (#1299)
This pull request updates the `html-no-duplicate-ids` linter rule by fixing a false positive that occurred when the same dynamic ID pattern appeared in separate ERB block contexts. Previously, an ID like `id="option--<%= opt[:id] %>"` used in two independent `.each` blocks would be flagged as a duplicate, even though each block iterates over a different collection and the variable is scoped to its own block. This happened because ERB blocks are parsed as `ERBBlockNode` and treated as conditional control flow rather than loops, so dynamic IDs were promoted to document scope after exiting the first block, causing the second block to trigger a false positive. The fix introduces an `isDynamic` flag that prevents IDs containing ERB output from being tracked at document scope when inside conditional or block control flow. Dynamic IDs are still tracked within their own branch, so genuine duplicates like using the same dynamic expression twice in the same `.each` block are still correctly flagged. This applies to all ERB block types including `.each`, `.map`, `.times`, and other iterator patterns that produce `ERBBlockNode` in the AST. Resolves #388
1 parent 2cd79cf commit ff09afe

File tree

2 files changed

+146
-14
lines changed

2 files changed

+146
-14
lines changed

‎javascript/packages/linter/src/rules/html-no-duplicate-ids.ts‎

Lines changed: 14 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -93,40 +93,41 @@ class NoDuplicateIdsVisitor extends ControlFlowTrackingVisitor<BaseAutofixContex
9393
return getStaticAttributeName(attributeNode.name) === "id"
9494
}
9595

96-
private extractIdValue(attributeNode: HTMLAttributeNode): { identifier: string; shouldTrackDuplicates: boolean } | null {
96+
private extractIdValue(attributeNode: HTMLAttributeNode): { identifier: string; shouldTrackDuplicates: boolean; isDynamic: boolean } | null {
9797
const valueNodes = attributeNode.value?.children || []
98+
const isDynamic = hasERBOutput(valueNodes)
9899

99-
if (hasERBOutput(valueNodes) && this.isInControlFlow && this.currentControlFlowType === ControlFlowType.LOOP) {
100+
if (isDynamic && this.isInControlFlow && this.currentControlFlowType === ControlFlowType.LOOP) {
100101
return null
101102
}
102103

103104
const identifier = isEffectivelyStatic(valueNodes) ? getValidatableStaticContent(valueNodes) : OutputPrinter.print(valueNodes)
104105
if (!identifier) return null
105106

106-
return { identifier, shouldTrackDuplicates: true }
107+
return { identifier, shouldTrackDuplicates: true, isDynamic }
107108
}
108109

109110
private isWhitespaceOnlyId(identifier: string): boolean {
110111
return identifier !== '' && identifier.trim() === ''
111112
}
112113

113-
private processIdDuplicate(idValue: { identifier: string; shouldTrackDuplicates: boolean }, attributeNode: HTMLAttributeNode): void {
114-
const { identifier, shouldTrackDuplicates } = idValue
114+
private processIdDuplicate(idValue: { identifier: string; shouldTrackDuplicates: boolean; isDynamic: boolean }, attributeNode: HTMLAttributeNode): void {
115+
const { identifier, shouldTrackDuplicates, isDynamic } = idValue
115116

116117
if (!shouldTrackDuplicates) return
117118

118119
if (this.isInControlFlow) {
119-
this.handleControlFlowId(identifier, attributeNode)
120+
this.handleControlFlowId(identifier, attributeNode, isDynamic)
120121
} else {
121122
this.handleGlobalId(identifier, attributeNode)
122123
}
123124
}
124125

125-
private handleControlFlowId(identifier: string, attributeNode: HTMLAttributeNode): void {
126+
private handleControlFlowId(identifier: string, attributeNode: HTMLAttributeNode, isDynamic: boolean): void {
126127
if (this.currentControlFlowType === ControlFlowType.LOOP) {
127128
this.handleLoopId(identifier, attributeNode)
128129
} else {
129-
this.handleConditionalId(identifier, attributeNode)
130+
this.handleConditionalId(identifier, attributeNode, isDynamic)
130131
}
131132

132133
this.currentBranchIds.add(identifier)
@@ -145,18 +146,20 @@ class NoDuplicateIdsVisitor extends ControlFlowTrackingVisitor<BaseAutofixContex
145146
}
146147
}
147148

148-
private handleConditionalId(identifier: string, attributeNode: HTMLAttributeNode): void {
149+
private handleConditionalId(identifier: string, attributeNode: HTMLAttributeNode, isDynamic: boolean): void {
149150
if (this.currentBranchIds.has(identifier)) {
150151
this.addSameBranchOffense(identifier, attributeNode.location)
151152
return
152153
}
153154

154-
if (this.documentIds.has(identifier)) {
155+
if (!isDynamic && this.documentIds.has(identifier)) {
155156
this.addDuplicateIdOffense(identifier, attributeNode.location)
156157
return
157158
}
158159

159-
this.controlFlowIds.add(identifier)
160+
if (!isDynamic) {
161+
this.controlFlowIds.add(identifier)
162+
}
160163
}
161164

162165
private handleGlobalId(identifier: string, attributeNode: HTMLAttributeNode): void {

‎javascript/packages/linter/test/rules/html-no-duplicate-ids.test.ts‎

Lines changed: 132 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -209,9 +209,8 @@ describe("html-no-duplicate-ids", () => {
209209
`)
210210
})
211211

212-
test("passes for output ERB IDs in loops (unique per iteration)", () => {
213-
expectError('Duplicate ID `user-<%= user.id %>` found. IDs must be unique within a document.')
214-
assertOffenses(dedent`
212+
test("passes for output ERB IDs in separate block contexts (unique per iteration)", () => {
213+
expectNoOffenses(dedent`
215214
<% users.each do |user| %>
216215
<div id="user-<%= user.id %>">User</div>
217216
<% end %>
@@ -321,4 +320,134 @@ describe("html-no-duplicate-ids", () => {
321320
<% end %>
322321
`)
323322
})
323+
324+
test("passes for dynamic IDs with same pattern in separate each blocks (issue #388)", () => {
325+
expectNoOffenses(dedent`
326+
<% first_options.each do |opt| %>
327+
<div id="option--<%= opt[:id] %>"></div>
328+
<% end %>
329+
330+
<% second_options.each do |opt| %>
331+
<div id="option--<%= opt[:id] %>"></div>
332+
<% end %>
333+
`)
334+
})
335+
336+
test("fails for duplicate dynamic IDs within same block", () => {
337+
expectError('Duplicate ID `item-<%= item.id %>` found within the same control flow branch. IDs must be unique within the same control flow branch.')
338+
339+
assertOffenses(dedent`
340+
<% items.each do |item| %>
341+
<div id="item-<%= item.id %>"></div>
342+
<span id="item-<%= item.id %>"></span>
343+
<% end %>
344+
`)
345+
})
346+
347+
test("passes for three separate each blocks with same dynamic ID pattern", () => {
348+
expectNoOffenses(dedent`
349+
<% first.each do |item| %>
350+
<div id="item-<%= item.id %>"></div>
351+
<% end %>
352+
353+
<% second.each do |item| %>
354+
<div id="item-<%= item.id %>"></div>
355+
<% end %>
356+
357+
<% third.each do |item| %>
358+
<div id="item-<%= item.id %>"></div>
359+
<% end %>
360+
`)
361+
})
362+
363+
test("passes for dynamic ID in global scope and same dynamic ID in a block", () => {
364+
expectNoOffenses(dedent`
365+
<div id="user-<%= user.id %>"></div>
366+
367+
<% items.each do |item| %>
368+
<div id="user-<%= user.id %>"></div>
369+
<% end %>
370+
`)
371+
})
372+
373+
test("passes for dynamic ID in a block and same dynamic ID in global scope", () => {
374+
expectNoOffenses(dedent`
375+
<% items.each do |item| %>
376+
<div id="user-<%= user.id %>"></div>
377+
<% end %>
378+
379+
<div id="user-<%= user.id %>"></div>
380+
`)
381+
})
382+
383+
test("passes for dynamic ID in an if branch and same dynamic ID in global scope", () => {
384+
expectNoOffenses(dedent`
385+
<div id="user-<%= user.id %>"></div>
386+
387+
<% if condition %>
388+
<div id="user-<%= user.id %>"></div>
389+
<% end %>
390+
`)
391+
})
392+
393+
test("passes for nested each blocks with same dynamic ID pattern", () => {
394+
expectNoOffenses(dedent`
395+
<% groups.each do |group| %>
396+
<% group.items.each do |item| %>
397+
<div id="item-<%= item.id %>"></div>
398+
<% end %>
399+
<% end %>
400+
`)
401+
})
402+
403+
test("passes for different dynamic ID patterns in separate blocks", () => {
404+
expectNoOffenses(dedent`
405+
<% users.each do |user| %>
406+
<div id="user-<%= user.id %>"></div>
407+
<% end %>
408+
409+
<% posts.each do |post| %>
410+
<div id="post-<%= post.id %>"></div>
411+
<% end %>
412+
`)
413+
})
414+
415+
test("passes for dynamic IDs across map and each blocks", () => {
416+
expectNoOffenses(dedent`
417+
<% items.map do |item| %>
418+
<div id="item-<%= item.id %>"></div>
419+
<% end %>
420+
421+
<% items.each do |item| %>
422+
<div id="item-<%= item.id %>"></div>
423+
<% end %>
424+
`)
425+
})
426+
427+
test("fails for static IDs in separate each blocks", () => {
428+
expectError('Duplicate ID `static-id` found. IDs must be unique within a document.')
429+
assertOffenses(dedent`
430+
<% first.each do |item| %>
431+
<div id="static-id"></div>
432+
<% end %>
433+
434+
<% second.each do |item| %>
435+
<div id="static-id"></div>
436+
<% end %>
437+
`)
438+
})
439+
440+
test("passes for dynamic ID in each block nested inside if/else", () => {
441+
expectNoOffenses(dedent`
442+
<% if condition %>
443+
<% items.each do |item| %>
444+
<div id="item-<%= item.id %>"></div>
445+
<% end %>
446+
<% else %>
447+
<% items.each do |item| %>
448+
<div id="item-<%= item.id %>"></div>
449+
<% end %>
450+
<% end %>
451+
`)
452+
})
324453
})

0 commit comments

Comments
 (0)