Linter: Implement html-require-closing-tags rule (#1199)

Co-authored-by: Marco Roth <marco.roth@intergga.ch>

Author: markokajzer

javascript/packages/linter/docs/rules/html-require-closing-tags.md

@@ -0,0 +1,142 @@
+# Linter Rule: Require explicit closing tags
+
+**Rule:** `html-require-closing-tags`
+
+## Description
+
+Disallow the omission of optional closing tags for HTML elements where the closing tag is technically optional according to the HTML specification. This rule flags elements that have an `HTMLOmittedCloseTagNode` as their close tag.
+
+## Rationale
+
+While HTML allows certain closing tags to be omitted (implicitly closed by sibling elements or parent closing), explicit closing tags improve code readability and maintainability. They make the document structure clear at a glance and reduce ambiguity about where elements end.
+
+Explicit closing tags also:
+
+- Make templates easier to understand for developers unfamiliar with HTML's implicit closing rules
+- Reduce potential for subtle bugs when refactoring or moving code
+- Improve consistency across the codebase
+- Make diffs cleaner when adding content to elements
+
+## Elements with Optional Closing Tags
+
+This rule would flag elements that have omitted closing tags:
+
+- `<li>` - list items
+- `<dt>`, `<dd>` - definition list terms and descriptions
+- `<p>` - paragraphs
+- `<option>`, `<optgroup>` - select options and groups
+- `<thead>`, `<tbody>`, `<tfoot>` - table sections
+- `<tr>` - table rows
+- `<td>`, `<th>` - table cells
+- `<colgroup>` - table column groups
+- `<rt>`, `<rp>` - ruby annotations
+
+## Examples
+
+### ✅ Good
+
+Explicit closing tags:
+
+```erb
+<ul>
+  <li>Item 1</li>
+  <li>Item 2</li>
+  <li>Item 3</li>
+</ul>
+```
+
+```erb
+<dl>
+  <dt>Term 1</dt>
+  <dd>Definition 1</dd>
+  <dt>Term 2</dt>
+  <dd>Definition 2</dd>
+</dl>
+```
+
+```erb
+<table>
+  <thead>
+    <tr>
+      <th>Header 1</th>
+      <th>Header 2</th>
+    </tr>
+  </thead>
+  <tbody>
+    <tr>
+      <td>Cell 1</td>
+      <td>Cell 2</td>
+    </tr>
+  </tbody>
+</table>
+```
+
+```erb
+<select>
+  <option>Option 1</option>
+  <option>Option 2</option>
+  <option>Option 3</option>
+</select>
+```
+
+```erb
+<div>
+  <p>Paragraph 1</p>
+  <p>Paragraph 2</p>
+</div>
+```
+
+### 🚫 Bad
+
+Omitted closing tags (implicitly closed):
+
+```erb
+<ul>
+  <li>Item 1
+  <li>Item 2
+  <li>Item 3
+</ul>
+```
+
+```erb
+<dl>
+  <dt>Term 1
+  <dd>Definition 1
+  <dt>Term 2
+  <dd>Definition 2
+</dl>
+```
+
+```erb
+<table>
+  <thead>
+    <tr>
+      <th>Header 1
+      <th>Header 2
+  <tbody>
+    <tr>
+      <td>Cell 1
+      <td>Cell 2
+</table>
+```
+
+```erb
+<select>
+  <option>Option 1
+  <option>Option 2
+  <option>Option 3
+</select>
+```
+
+```erb
+<div>
+  <p>Paragraph 1
+  <p>Paragraph 2
+</div>
+```
+
+## References
+
+- [HTML Spec: Optional Tags](https://html.spec.whatwg.org/multipage/syntax.html#optional-tags)
+- [HTML Spec: Tag Omission](https://html.spec.whatwg.org/multipage/dom.html#concept-element-tag-omission)
+- [CSS-Tricks: Fighting the Space Between Inline Block Elements](https://css-tricks.com/fighting-the-space-between-inline-block-elements/)

javascript/packages/linter/src/linter.ts

@@ -1,13 +1,16 @@
+import picomatch from "picomatch"
+
 import { Location } from "@herb-tools/core"
 import { IdentityPrinter } from "@herb-tools/printer"
-import picomatch from "picomatch"
 
 import { rules } from "./rules.js"
 import { findNodeByLocation } from "./rules/rule-utils.js"
 import { parseHerbDisableLine } from "./herb-disable-comment-utils.js"
 import { hasLinterIgnoreDirective } from "./linter-ignore.js"
+import { ParseCache } from "./parse-cache.js"
 
 import { ParserNoErrorsRule } from "./rules/parser-no-errors.js"
+
 import { DEFAULT_RULE_CONFIG } from "./types.js"
 
 import type { RuleClass, Rule, ParserRule, LexerRule, SourceRule, LintResult, LintOffense, UnboundLintOffense, LintContext, AutofixResult } from "./types.js"
@@ -48,6 +51,7 @@ export class Linter {
   protected rules: RuleClass[]
   protected allAvailableRules: RuleClass[]
   protected herb: HerbBackend
+  protected parseCache: ParseCache
   protected offenses: LintOffense[]
   protected config?: Config
 
@@ -81,6 +85,7 @@ export class Linter {
    */
   constructor(herb: HerbBackend, rules?: RuleClass[], config?: Config, allAvailableRules?: RuleClass[]) {
     this.herb = herb
+    this.parseCache = new ParseCache(herb)
     this.config = config
     this.rules = rules !== undefined ? rules : this.getDefaultRules()
     this.allAvailableRules = allAvailableRules !== undefined ? allAvailableRules : this.rules
@@ -170,6 +175,13 @@ export class Linter {
     return (rule.constructor as any).type === "source"
   }
 
+  /**
+   * Type guard to check if a rule is a ParserRule
+   */
+  protected isParserRule(rule: Rule): rule is ParserRule {
+    return (rule.constructor as any).type === "parser"
+  }
+
   /**
    * Execute a single rule and return its unbound offenses.
    * Handles rule type checking (Lexer/Parser/Source) and isEnabled checks.
@@ -303,7 +315,7 @@ export class Linter {
     let ignoredCount = 0
     let wouldBeIgnoredCount = 0
 
-    const parseResult = this.herb.parse(source, { track_whitespace: true })
+    const parseResult = this.parseCache.get(source)
 
     // Check for file-level ignore directive using visitor
     if (hasLinterIgnoreDirective(parseResult)) {
@@ -330,15 +342,6 @@ export class Linter {
         const offenses = rule.check(parseResult)
         this.offenses.push(...offenses)
       }
-
-      return {
-        offenses: this.offenses,
-        errors: this.offenses.filter(o => o.severity === "error").length,
-        warnings: this.offenses.filter(o => o.severity === "warning").length,
-        info: this.offenses.filter(o => o.severity === "info").length,
-        hints: this.offenses.filter(o => o.severity === "hint").length,
-        ignored: 0
-      }
     }
 
     for (let i = 0; i < sourceLines.length; i++) {
@@ -364,6 +367,17 @@ export class Linter {
 
     for (const RuleClass of regularRules) {
       const rule = new RuleClass()
+      const parserOptions = this.isParserRule(rule) ? rule.parserOptions : {}
+      const parseResult = this.parseCache.get(source, parserOptions)
+
+      // Skip parser rules whose parse result has errors (parser-no-errors handled above)
+      // Skip lexer/source rules when the default parse has errors
+      if (this.isParserRule(rule)) {
+        if (parseResult.recursiveErrors().length > 0) continue
+      } else if (hasParserErrors) {
+        continue
+      }
+
       const unboundOffenses = this.executeRule(rule, parseResult, lexResult, source, context)
       const boundOffenses = this.bindSeverity(unboundOffenses, rule.name)
 
@@ -388,6 +402,7 @@ export class Linter {
 
     if (unnecessaryRuleClass) {
       const unnecessaryRule = new unnecessaryRuleClass() as ParserRule
+      const parseResult = this.parseCache.get(source, unnecessaryRule.parserOptions)
       const unboundOffenses = unnecessaryRule.check(parseResult, context)
       const boundOffenses = this.bindSeverity(unboundOffenses, unnecessaryRule.name)
 
@@ -494,7 +509,7 @@ export class Linter {
     const unfixed: LintOffense[] = []
 
     if (parserOffenses.length > 0) {
-      const parseResult = this.herb.parse(currentSource, { track_whitespace: true })
+      const parseResult = this.parseCache.get(currentSource)
 
       for (const offense of parserOffenses) {
         const RuleClass = this.rules.find(rule => new rule().name === offense.rule)
@@ -602,4 +617,3 @@ export class Linter {
     }
   }
 }
-

javascript/packages/linter/src/parse-cache.ts

@@ -0,0 +1,39 @@
+import { DEFAULT_PARSER_OPTIONS } from "@herb-tools/core"
+import { DEFAULT_LINTER_PARSER_OPTIONS } from "./types.js"
+
+import type { ParseResult, HerbBackend, ParserOptions } from "@herb-tools/core"
+
+export class ParseCache {
+  private herb: HerbBackend
+  private cache = new Map<string, ParseResult>()
+
+  constructor(herb: HerbBackend) {
+    this.herb = herb
+  }
+
+  get(source: string, parserOptions: Partial<ParserOptions> = {}): ParseResult {
+    const effectiveOptions = this.resolveOptions(parserOptions)
+    const key = source + JSON.stringify(effectiveOptions)
+
+    let result = this.cache.get(key)
+
+    if (!result) {
+      result = this.herb.parse(source, effectiveOptions)
+      this.cache.set(key, result)
+    }
+
+    return result
+  }
+
+  clear(): void {
+    this.cache.clear()
+  }
+
+  resolveOptions(parserOptions: Partial<ParserOptions>): ParserOptions {
+    return {
+      ...DEFAULT_PARSER_OPTIONS,
+      ...DEFAULT_LINTER_PARSER_OPTIONS,
+      ...parserOptions
+    }
+  }
+}

javascript/packages/linter/src/rules.ts

@@ -57,6 +57,7 @@ import { HTMLNoSelfClosingRule } from "./rules/html-no-self-closing.js"
 import { HTMLNoSpaceInTagRule } from "./rules/html-no-space-in-tag.js"
 import { HTMLNoTitleAttributeRule } from "./rules/html-no-title-attribute.js"
 import { HTMLNoUnderscoresInAttributeNamesRule } from "./rules/html-no-underscores-in-attribute-names.js"
+import { HTMLRequireClosingTagsRule } from "./rules/html-require-closing-tags.js"
 import { HTMLTagNameLowercaseRule } from "./rules/html-tag-name-lowercase.js"
 
 import { SVGTagNameCapitalizationRule } from "./rules/svg-tag-name-capitalization.js"
@@ -121,6 +122,7 @@ export const rules: RuleClass[] = [
   HTMLNoSpaceInTagRule,
   HTMLNoTitleAttributeRule,
   HTMLNoUnderscoresInAttributeNamesRule,
+  HTMLRequireClosingTagsRule,
   HTMLTagNameLowercaseRule,
 
   SVGTagNameCapitalizationRule,

javascript/packages/linter/src/rules/html-require-closing-tags.ts

@@ -0,0 +1,41 @@
+import { ParserRule } from "../types.js"
+import { BaseRuleVisitor } from "./rule-utils.js"
+
+import type { UnboundLintOffense, LintContext, FullRuleConfig } from "../types.js"
+import type { HTMLOmittedCloseTagNode, ParseResult, ParserOptions } from "@herb-tools/core"
+
+class RequireClosingTagsVisitor extends BaseRuleVisitor {
+  visitHTMLOmittedCloseTagNode(node: HTMLOmittedCloseTagNode): void {
+    const tagName = node.tag_name?.value
+    if (!tagName) return
+
+    this.addOffense(
+      `Missing explicit closing tag for \`<${tagName}>\`. Use \`</${tagName}>\` instead of relying on implicit tag closing.`,
+      node.location
+    )
+  }
+}
+
+export class HTMLRequireClosingTagsRule extends ParserRule {
+  static autocorrectable = false
+  name = "html-require-closing-tags"
+
+  get defaultConfig(): FullRuleConfig {
+    return {
+      enabled: true,
+      severity: "error",
+    }
+  }
+
+  get parserOptions(): Partial<ParserOptions> {
+    return { strict: false }
+  }
+
+  check(result: ParseResult, context?: Partial<LintContext>): UnboundLintOffense[] {
+    const visitor = new RequireClosingTagsVisitor(this.name, context)
+
+    visitor.visit(result.value)
+
+    return visitor.offenses
+  }
+}

javascript/packages/linter/src/rules/index.ts

@@ -58,6 +58,7 @@ export * from "./html-no-self-closing.js"
 export * from "./html-no-space-in-tag.js"
 export * from "./html-no-title-attribute.js"
 export * from "./html-no-underscores-in-attribute-names.js"
+export * from "./html-require-closing-tags.js"
 export * from "./html-tag-name-lowercase.js"
 
 export * from "./svg-tag-name-capitalization.js"

javascript/packages/linter/src/types.ts

@@ -1,14 +1,18 @@
 import { Diagnostic, LexResult, ParseResult } from "@herb-tools/core"
 
 import type { rules } from "./rules.js"
-import type { Node } from "@herb-tools/core"
+import type { Node, ParserOptions } from "@herb-tools/core"
 import type { RuleConfig } from "@herb-tools/config"
 import type { Mutable } from "@herb-tools/rewriter"
 
 export type { Mutable } from "@herb-tools/rewriter"
 
 export type LintSeverity = "error" | "warning" | "info" | "hint"
 
+export const DEFAULT_LINTER_PARSER_OPTIONS: Partial<ParserOptions> = {
+  track_whitespace: true,
+}
+
 export type FullRuleConfig = Required<Pick<RuleConfig, 'enabled' | 'severity'>> & Omit<RuleConfig, 'enabled' | 'severity'>
 
 /**
@@ -92,6 +96,11 @@ export abstract class ParserRule<TAutofixContext extends BaseAutofixContext = Ba
   get defaultConfig(): FullRuleConfig {
     return DEFAULT_RULE_CONFIG
   }
+
+  get parserOptions(): Partial<ParserOptions> {
+    return DEFAULT_LINTER_PARSER_OPTIONS
+  }
+
   abstract check(result: ParseResult, context?: Partial<LintContext>): UnboundLintOffense<TAutofixContext>[]
 
   /**