Linter: Support more cases for erb-no-duplicate-branch-elements (#1415)

Resolves #1395

Author: marcoroth

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

@@ -186,6 +186,9 @@ export class Server {
 
       if (!document) return []
 
+      const parseResult = this.service.parserService.parseDocument(document)
+      if (parseResult.diagnostics.length > 0) return []
+
       const diagnostics = params.context.diagnostics
       const documentText = document.getText()
 

javascript/packages/linter/src/rules/erb-no-duplicate-branch-elements.ts

@@ -4,6 +4,7 @@ import { IdentityPrinter } from "@herb-tools/printer"
 
 import {
   isHTMLElementNode,
+  isHTMLOpenTagNode,
   isERBIfNode,
   isERBElseNode,
   isERBUnlessNode,
@@ -27,12 +28,21 @@ type ConditionalNode = ERBIfNode | ERBUnlessNode | ERBCaseNode
 
 interface DuplicateBranchAutofixContext extends BaseAutofixContext {
   node: Mutable<ConditionalNode>
+  allIdentical?: boolean
 }
 
 function getSignificantNodes(statements: Node[]): Node[] {
   return statements.filter(node => !isPureWhitespaceNode(node))
 }
 
+function trimWhitespaceNodes(nodes: Node[]): Node[] {
+  let start = 0
+  let end = nodes.length
+  while (start < end && isPureWhitespaceNode(nodes[start])) start++
+  while (end > start && isPureWhitespaceNode(nodes[end - 1])) end--
+  return nodes.slice(start, end)
+}
+
 function allEquivalentElements(nodes: Node[]): nodes is HTMLElementNode[] {
   if (nodes.length < 2) return false
   if (!nodes.every(node => isHTMLElementNode(node))) return false
@@ -172,10 +182,31 @@ class ERBNoDuplicateBranchElementsVisitor extends BaseRuleVisitor<DuplicateBranc
       this.markSubsequentIfNodesAsProcessed(node)
     }
 
+    if (this.allBranchesIdentical(branches)) {
+      this.addOffense(
+        "All branches of this conditional have identical content. The conditional can be removed.",
+        node.location,
+        { node: node as Mutable<ConditionalNode>, allIdentical: true },
+        "warning",
+      )
+
+      return
+    }
+
     const state = { isFirstOffense: true }
     this.checkBranches(branches, node, state)
   }
 
+  private allBranchesIdentical(branches: Node[][]): boolean {
+    if (branches.length < 2) return false
+
+    const first = branches[0].map(node => IdentityPrinter.print(node)).join("")
+
+    return branches.slice(1).every(branch =>
+      branch.map(node => IdentityPrinter.print(node)).join("") === first
+    )
+  }
+
   private markSubsequentIfNodesAsProcessed(node: ERBIfNode): void {
     let current: ERBIfNode | ERBElseNode | null = node.subsequent
 
@@ -214,17 +245,37 @@ class ERBNoDuplicateBranchElementsVisitor extends BaseRuleVisitor<DuplicateBranc
 
     for (const element of elements) {
       const printed = IdentityPrinter.print(element.open_tag)
-      const autofixContext = state.isFirstOffense
-        ? { node: conditionalNode as Mutable<ConditionalNode> }
-        : undefined
 
-      this.addOffense(
-        `The \`${printed}\` element is duplicated across all branches of this conditional and can be moved outside.`,
-        bodiesMatch ? element.location : (element?.open_tag?.location || element.location),
-        autofixContext,
-      )
+      if (bodiesMatch) {
+        const autofixContext = state.isFirstOffense
+          ? { node: conditionalNode as Mutable<ConditionalNode> }
+          : undefined
+
+        this.addOffense(
+          `The \`${printed}\` element is duplicated across all branches of this conditional and can be moved outside.`,
+          element.location,
+          autofixContext,
+        )
 
-      state.isFirstOffense = false
+        state.isFirstOffense = false
+      } else {
+        const autofixContext = state.isFirstOffense
+          ? { node: conditionalNode as Mutable<ConditionalNode> }
+          : undefined
+
+        const tagNameLocation = isHTMLOpenTagNode(element.open_tag) && element.open_tag.tag_name?.location
+          ? element.open_tag.tag_name.location
+          : element?.open_tag?.location || element.location
+
+        this.addOffense(
+          `The \`${printed}\` tag is repeated across all branches with different content. Consider extracting the shared tag outside the conditional.`,
+          tagNameLocation,
+          autofixContext,
+          "hint",
+        )
+
+        state.isFirstOffense = false
+      }
     }
 
     if (!bodiesMatch && bodies.every(body => body.length > 0)) {
@@ -260,6 +311,18 @@ export class ERBNoDuplicateBranchElementsRule extends ParserRule<DuplicateBranch
     const branches = collectBranches(conditionalNode as ConditionalNode)
     if (!branches) return null
 
+    if (offense.autofixContext.allIdentical) {
+      const parentInfo = findParentArray(result.value, conditionalNode as unknown as Node)
+      if (!parentInfo) return null
+
+      const { array: parentArray, index: conditionalIndex } = parentInfo
+      const firstBranchContent = trimWhitespaceNodes(branches[0])
+
+      parentArray.splice(conditionalIndex, 1, ...firstBranchContent)
+
+      return result
+    }
+
     const significantBranches = branches.map(getSignificantNodes)
     if (significantBranches.some(branch => branch.length === 0)) return null
 
@@ -274,24 +337,57 @@ export class ERBNoDuplicateBranchElementsRule extends ParserRule<DuplicateBranch
 
     let { array: parentArray, index: conditionalIndex } = parentInfo
     let hasWrapped = false
+    let didMutate = false
+    let failedToHoistPrefix = false
+    let hoistedBefore = false
 
     const hoistElement = (elements: HTMLElementNode[], position: "before" | "after"): void => {
+      const actualPosition = (position === "before" && failedToHoistPrefix) ? "after" : position
       const bodiesMatch = elements.every(element => IdentityPrinter.print(element) === IdentityPrinter.print(elements[0]))
 
       if (bodiesMatch) {
+        if (actualPosition === "after") {
+          const currentLengths = branches.map(b => getSignificantNodes(b as Node[]).length)
+          if (currentLengths.some(l => l !== currentLengths[0])) return
+        }
+
+        if (actualPosition === "after" && position === "before") {
+          const isAtEnd = branches.every((branch, index) => {
+            const nodes = getSignificantNodes(branch as Node[])
+
+            return nodes.length > 0 && nodes[nodes.length - 1] === elements[index]
+          })
+
+          if (!isAtEnd) return
+        }
+
         for (let i = 0; i < branches.length; i++) {
           removeNodeFromArray(branches[i] as Node[], elements[i])
         }
 
-        if (position === "before") {
-          parentArray.splice(conditionalIndex, 0, elements[0])
-          conditionalIndex++
+        if (actualPosition === "before") {
+          parentArray.splice(conditionalIndex, 0, elements[0], createLiteral("\n"))
+          conditionalIndex += 2
+          hoistedBefore = true
         } else {
-          parentArray.splice(conditionalIndex + 1, 0, elements[0])
+          parentArray.splice(conditionalIndex + 1, 0, createLiteral("\n"), elements[0])
         }
+
+        didMutate = true
       } else {
         if (hasWrapped) return
 
+        const canWrap = branches.every((branch, index) => {
+          const remaining = getSignificantNodes(branch)
+
+          return remaining.length === 1 && remaining[0] === elements[index]
+        })
+
+        if (!canWrap) {
+          if (position === "before") failedToHoistPrefix = true
+          return
+        }
+
         for (let i = 0; i < branches.length; i++) {
           replaceNodeWithBody(branches[i] as Node[], elements[i])
         }
@@ -302,6 +398,7 @@ export class ERBNoDuplicateBranchElementsRule extends ParserRule<DuplicateBranch
         parentArray = wrapper.body as Node[]
         conditionalIndex = 1
         hasWrapped = true
+        didMutate = true
       }
     }
 
@@ -315,6 +412,25 @@ export class ERBNoDuplicateBranchElementsRule extends ParserRule<DuplicateBranch
       hoistElement(elements, "after")
     }
 
-    return result
+    if (!hasWrapped && hoistedBefore) {
+      const remaining = branches.map(branch => getSignificantNodes(branch as Node[]))
+
+      if (remaining.every(branch => branch.length === 1) && allEquivalentElements(remaining.map(b => b[0]))) {
+        const elements = remaining.map(b => b[0] as HTMLElementNode)
+        const bodiesMatch = elements.every(el => IdentityPrinter.print(el) === IdentityPrinter.print(elements[0]))
+
+        if (!bodiesMatch && elements.every(el => el.body.length > 0)) {
+          for (let i = 0; i < branches.length; i++) {
+            replaceNodeWithBody(branches[i] as Node[], elements[i])
+          }
+
+          const wrapper = createWrapper(elements[0], [createLiteral("\n"), conditionalNode as unknown as Node, createLiteral("\n")])
+          parentArray[conditionalIndex] = wrapper
+          didMutate = true
+        }
+      }
+    }
+
+    return didMutate ? result : null
   }
 }

javascript/packages/linter/src/rules/rule-utils.ts

@@ -27,6 +27,7 @@ import type {
 import { DEFAULT_LINT_CONTEXT } from "../types.js"
 
 import type * as Nodes from "@herb-tools/core"
+import type { DiagnosticTag } from "@herb-tools/core"
 import type { UnboundLintOffense, LintContext, LintSeverity, BaseAutofixContext } from "../types.js"
 
 export enum ControlFlowType {
@@ -53,7 +54,7 @@ export abstract class BaseRuleVisitor<TAutofixContext extends BaseAutofixContext
    * Helper method to create an unbound lint offense (without severity).
    * The Linter will bind severity based on the rule's config.
    */
-  protected createOffense(message: string, location: Location, autofixContext?: TAutofixContext, severity?: LintSeverity): UnboundLintOffense<TAutofixContext> {
+  protected createOffense(message: string, location: Location, autofixContext?: TAutofixContext, severity?: LintSeverity, tags?: DiagnosticTag[]): UnboundLintOffense<TAutofixContext> {
     return {
       rule: this.ruleName,
       code: this.ruleName,
@@ -62,14 +63,15 @@ export abstract class BaseRuleVisitor<TAutofixContext extends BaseAutofixContext
       location,
       autofixContext,
       severity,
+      tags,
     }
   }
 
   /**
    * Helper method to add an offense to the offenses array
    */
-  protected addOffense(message: string, location: Location, autofixContext?: TAutofixContext, severity?: LintSeverity): void {
-    this.offenses.push(this.createOffense(message, location, autofixContext, severity))
+  protected addOffense(message: string, location: Location, autofixContext?: TAutofixContext, severity?: LintSeverity, tags?: DiagnosticTag[]): void {
+    this.offenses.push(this.createOffense(message, location, autofixContext, severity, tags))
   }
 }
 
@@ -515,7 +517,7 @@ export abstract class BaseLexerRuleVisitor<TAutofixContext extends BaseAutofixCo
    * Helper method to create an unbound lint offense (without severity).
    * The Linter will bind severity based on the rule's config.
    */
-  protected createOffense(message: string, location: Location, autofixContext?: TAutofixContext, severity?: LintSeverity): UnboundLintOffense<TAutofixContext> {
+  protected createOffense(message: string, location: Location, autofixContext?: TAutofixContext, severity?: LintSeverity, tags?: DiagnosticTag[]): UnboundLintOffense<TAutofixContext> {
     return {
       rule: this.ruleName,
       code: this.ruleName,
@@ -524,14 +526,15 @@ export abstract class BaseLexerRuleVisitor<TAutofixContext extends BaseAutofixCo
       location,
       autofixContext,
       severity,
+      tags,
     }
   }
 
   /**
    * Helper method to add an offense to the offenses array
    */
-  protected addOffense(message: string, location: Location, autofixContext?: TAutofixContext, severity?: LintSeverity): void {
-    this.offenses.push(this.createOffense(message, location, autofixContext, severity))
+  protected addOffense(message: string, location: Location, autofixContext?: TAutofixContext, severity?: LintSeverity, tags?: DiagnosticTag[]): void {
+    this.offenses.push(this.createOffense(message, location, autofixContext, severity, tags))
   }
 
   /**
@@ -578,7 +581,7 @@ export abstract class BaseSourceRuleVisitor<TAutofixContext extends BaseAutofixC
    * Helper method to create an unbound lint offense (without severity).
    * The Linter will bind severity based on the rule's config.
    */
-  protected createOffense(message: string, location: Location, autofixContext?: TAutofixContext, severity?: LintSeverity): UnboundLintOffense<TAutofixContext> {
+  protected createOffense(message: string, location: Location, autofixContext?: TAutofixContext, severity?: LintSeverity, tags?: DiagnosticTag[]): UnboundLintOffense<TAutofixContext> {
     return {
       rule: this.ruleName,
       code: this.ruleName,
@@ -587,14 +590,15 @@ export abstract class BaseSourceRuleVisitor<TAutofixContext extends BaseAutofixC
       location,
       autofixContext,
       severity,
+      tags,
     }
   }
 
   /**
    * Helper method to add an offense to the offenses array
    */
-  protected addOffense(message: string, location: Location, autofixContext?: TAutofixContext, severity?: LintSeverity): void {
-    this.offenses.push(this.createOffense(message, location, autofixContext, severity))
+  protected addOffense(message: string, location: Location, autofixContext?: TAutofixContext, severity?: LintSeverity, tags?: DiagnosticTag[]): void {
+    this.offenses.push(this.createOffense(message, location, autofixContext, severity, tags))
   }
 
   /**

javascript/packages/linter/src/types.ts

@@ -1,5 +1,6 @@
 import { Diagnostic, LexResult, ParseResult, Location } from "@herb-tools/core"
 
+import type { DiagnosticTag } from "@herb-tools/core"
 import type { rules } from "./rules.js"
 import type { Node, ParserOptions } from "@herb-tools/core"
 import type { RuleConfig } from "@herb-tools/config"
@@ -111,7 +112,7 @@ export abstract class ParserRule<TAutofixContext extends BaseAutofixContext = Ba
     return DEFAULT_LINTER_PARSER_OPTIONS
   }
 
-  protected createOffense(message: string, location: Location, autofixContext?: TAutofixContext, severity?: LintSeverity): UnboundLintOffense<TAutofixContext> {
+  protected createOffense(message: string, location: Location, autofixContext?: TAutofixContext, severity?: LintSeverity, tags?: DiagnosticTag[]): UnboundLintOffense<TAutofixContext> {
     return {
       rule: this.ruleName,
       code: this.ruleName,
@@ -120,6 +121,7 @@ export abstract class ParserRule<TAutofixContext extends BaseAutofixContext = Ba
       location,
       autofixContext,
       severity,
+      tags,
     }
   }
 
@@ -164,7 +166,7 @@ export abstract class LexerRule<TAutofixContext extends BaseAutofixContext = Bas
     return DEFAULT_RULE_CONFIG
   }
 
-  protected createOffense(message: string, location: Location, autofixContext?: TAutofixContext, severity?: LintSeverity): UnboundLintOffense<TAutofixContext> {
+  protected createOffense(message: string, location: Location, autofixContext?: TAutofixContext, severity?: LintSeverity, tags?: DiagnosticTag[]): UnboundLintOffense<TAutofixContext> {
     return {
       rule: this.ruleName,
       code: this.ruleName,
@@ -173,6 +175,7 @@ export abstract class LexerRule<TAutofixContext extends BaseAutofixContext = Bas
       location,
       autofixContext,
       severity,
+      tags,
     }
   }
 
@@ -241,7 +244,7 @@ export abstract class SourceRule<TAutofixContext extends BaseAutofixContext = Ba
     return DEFAULT_RULE_CONFIG
   }
 
-  protected createOffense(message: string, location: Location, autofixContext?: TAutofixContext, severity?: LintSeverity): UnboundLintOffense<TAutofixContext> {
+  protected createOffense(message: string, location: Location, autofixContext?: TAutofixContext, severity?: LintSeverity, tags?: DiagnosticTag[]): UnboundLintOffense<TAutofixContext> {
     return {
       rule: this.ruleName,
       code: this.ruleName,
@@ -250,6 +253,7 @@ export abstract class SourceRule<TAutofixContext extends BaseAutofixContext = Ba
       location,
       autofixContext,
       severity,
+      tags,
     }
   }