Linter: Implement erb-no-instance-variables-in-partials linter rule (#1355)

Resolves #390
Resolves #544

Author: marcoroth

javascript/packages/linter/docs/rules/README.md

@@ -12,6 +12,7 @@ This page contains documentation for all Herb Linter rules.
 - [`erb-no-extra-newline`](./erb-no-extra-newline.md) - Disallow extra newlines.
 - [`erb-no-extra-whitespace-inside-tags`](./erb-no-extra-whitespace-inside-tags.md) - Disallow multiple consecutive spaces inside ERB tags
 - [`erb-no-inline-case-conditions`](./erb-no-inline-case-conditions.md) - Disallow inline `case`/`when` and `case`/`in` conditions in a single ERB tag
+- [`erb-no-instance-variables-in-partials`](./erb-no-instance-variables-in-partials.md) - Disallow instance variables in partials
 - [`erb-no-interpolated-class-names`](./erb-no-interpolated-class-names.md) - Disallow ERB interpolation inside CSS class names
 - [`erb-no-javascript-tag-helper`](./erb-no-javascript-tag-helper.md) - Disallow `javascript_tag` helper
 - [`erb-no-output-control-flow`](./erb-no-output-control-flow.md) - Prevents outputting control flow blocks

javascript/packages/linter/docs/rules/erb-no-instance-variables-in-partials.md

@@ -0,0 +1,43 @@
+# Linter Rule: Disallow instance variables in partials
+
+**Rule:** `erb-no-instance-variables-in-partials`
+
+## Description
+
+Prevent usage of instance variables inside ERB partials. Using instance variables inside partials can cause issues as their dependency is defined outside of the partial itself. This makes partials more fragile and less reusable. Local variables should be passed directly to partial renders.
+
+A partial is any template whose filename begins with an underscore (e.g. `_card.html.erb`).
+
+Instance variables in partials create implicit dependencies on the controller or parent view, making partials harder to reuse, test, and reason about. Passing data as local variables makes the partial's interface explicit and self-documenting.
+
+## Examples
+
+### ✅ Good
+
+```erb [app/views/posts/index.html.erb]
+<%= render partial: "posts/card", locals: { post: @post } %>
+```
+
+```erb [app/views/posts/_card.html.erb]
+<div>
+  <%= post.title %>
+</div>
+```
+
+### 🚫 Bad
+
+```erb [app/views/posts/index.html.erb]
+<%= render partial: "posts/card" %>
+```
+
+```erb [app/views/posts/_card.html.erb]
+<div>
+  <%= @post.title %>
+</div>
+```
+
+## References
+
+- [Rails Guide: Using Partials](https://guides.rubyonrails.org/layouts_and_rendering.html#using-partials)
+- [Rails Guide: Passing Local Variables](https://guides.rubyonrails.org/layouts_and_rendering.html#passing-local-variables)
+- [Inspiration: ERB Lint `PartialInstanceVariable` rule](https://github.com/Shopify/erb_lint?tab=readme-ov-file#partialinstancevariable)

javascript/packages/linter/src/rules.ts

@@ -27,6 +27,7 @@ import { ERBRightTrimRule } from "./rules/erb-right-trim.js"
 import { ERBNoOutputInAttributePositionRule } from "./rules/erb-no-output-in-attribute-position.js"
 import { ERBNoOutputInAttributeNameRule } from "./rules/erb-no-output-in-attribute-name.js"
 import { ERBStrictLocalsCommentSyntaxRule } from "./rules/erb-strict-locals-comment-syntax.js"
+import { ERBNoInstanceVariablesInPartialsRule } from "./rules/erb-no-instance-variables-in-partials.js"
 import { ERBStrictLocalsRequiredRule } from "./rules/erb-strict-locals-required.js"
 
 import { HerbDisableCommentValidRuleNameRule } from "./rules/herb-disable-comment-valid-rule-name.js"
@@ -104,6 +105,7 @@ export const rules: RuleClass[] = [
   ERBRightTrimRule,
   ERBNoOutputInAttributePositionRule,
   ERBNoOutputInAttributeNameRule,
+  ERBNoInstanceVariablesInPartialsRule,
   ERBStrictLocalsCommentSyntaxRule,
   ERBStrictLocalsRequiredRule,
 

javascript/packages/linter/src/rules/erb-no-instance-variables-in-partials.ts

@@ -0,0 +1,101 @@
+import { PrismVisitor, PrismNodes } from "@herb-tools/core"
+import { ParserRule } from "../types.js"
+
+import { isPartialFile } from "./file-utils.js"
+import { locationFromOffset } from "./rule-utils.js"
+
+import type { ParseResult, ParserOptions, PrismLocation } from "@herb-tools/core"
+import type { UnboundLintOffense, LintContext, FullRuleConfig } from "../types.js"
+
+interface InstanceVariableNode {
+  name: string
+  location: PrismLocation
+}
+
+type InstanceVariableUsage = "read" | "write"
+
+interface InstanceVariableReference {
+  name: string
+  usage: InstanceVariableUsage
+  startOffset: number
+  length: number
+}
+
+class InstanceVariableCollector extends PrismVisitor {
+  public readonly instanceVariables: InstanceVariableReference[] = []
+
+  visitInstanceVariableReadNode(node: PrismNodes.InstanceVariableReadNode): void {
+    this.collect(node, "read")
+  }
+
+  visitInstanceVariableWriteNode(node: PrismNodes.InstanceVariableWriteNode): void {
+    this.collect(node, "write")
+  }
+
+  visitInstanceVariableAndWriteNode(node: PrismNodes.InstanceVariableAndWriteNode): void {
+    this.collect(node, "write")
+  }
+
+  visitInstanceVariableOrWriteNode(node: PrismNodes.InstanceVariableOrWriteNode): void {
+    this.collect(node, "write")
+  }
+
+  visitInstanceVariableOperatorWriteNode(node: PrismNodes.InstanceVariableOperatorWriteNode): void {
+    this.collect(node, "write")
+  }
+
+  visitInstanceVariableTargetNode(node: PrismNodes.InstanceVariableTargetNode): void {
+    this.collect(node, "write")
+  }
+
+  private collect(node: InstanceVariableNode, usage: InstanceVariableUsage): void {
+    this.instanceVariables.push({
+      name: node.name,
+      usage,
+      startOffset: node.location.startOffset,
+      length: node.location.length
+    })
+  }
+}
+
+export class ERBNoInstanceVariablesInPartialsRule extends ParserRule {
+  static ruleName = "erb-no-instance-variables-in-partials"
+
+  get defaultConfig(): FullRuleConfig {
+    return {
+      enabled: true,
+      severity: "error",
+    }
+  }
+
+  get parserOptions(): Partial<ParserOptions> {
+    return {
+      track_whitespace: true,
+      prism_program: true,
+    }
+  }
+
+  isEnabled(_result: ParseResult, context?: Partial<LintContext>): boolean {
+    return isPartialFile(context?.fileName) === true
+  }
+
+  check(result: ParseResult, _context?: Partial<LintContext>): UnboundLintOffense[] {
+    const source = result.value.source
+    const prismNode = result.value.prismNode
+
+    if (!prismNode || !source) return []
+
+    const collector = new InstanceVariableCollector()
+
+    collector.visit(prismNode)
+
+    return collector.instanceVariables.map(ivar => {
+      const location = locationFromOffset(source, ivar.startOffset, ivar.length)
+      const message = ivar.usage === "read"
+        ? `Avoid using instance variables in partials. Pass \`${ivar.name}\` as a local variable instead.`
+        : `Avoid setting instance variables in partials. Use a local variable instead of \`${ivar.name}\`.`
+
+      return this.createOffense(message, location)
+    })
+  }
+}

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

@@ -27,6 +27,7 @@ export * from "./erb-no-unsafe-script-interpolation.js"
 export * from "./erb-right-trim.js"
 export * from "./erb-no-output-in-attribute-position.js"
 export * from "./erb-no-output-in-attribute-name.js"
+export * from "./erb-no-instance-variables-in-partials.js"
 export * from "./erb-strict-locals-comment-syntax.js"
 export * from "./erb-strict-locals-required.js"
 

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

@@ -853,6 +853,15 @@ export function positionFromOffset(source: string, offset: number): Position {
   return new Position(line, column)
 }
 
+/**
+ * Creates a Location from a source string, a start offset, and a length.
+ */
+export function locationFromOffset(source: string, startOffset: number, length: number): Location {
+  const start = positionFromOffset(source, startOffset)
+  const end = positionFromOffset(source, startOffset + length)
+  return Location.from(start.line, start.column, end.line, end.column)
+}
+
 /**
  * Checks if a position (line, column) is within a node's location range.
  * @param node - The node to check

javascript/packages/linter/src/types.ts

@@ -1,4 +1,4 @@
-import { Diagnostic, LexResult, ParseResult } from "@herb-tools/core"
+import { Diagnostic, LexResult, ParseResult, Location } from "@herb-tools/core"
 
 import type { rules } from "./rules.js"
 import type { Node, ParserOptions } from "@herb-tools/core"
@@ -111,6 +111,18 @@ 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> {
+    return {
+      rule: this.ruleName,
+      code: this.ruleName,
+      source: "Herb Linter",
+      message,
+      location,
+      autofixContext,
+      severity,
+    }
+  }
+
   abstract check(result: ParseResult, context?: Partial<LintContext>): UnboundLintOffense<TAutofixContext>[]
 
   /**
@@ -151,6 +163,19 @@ export abstract class LexerRule<TAutofixContext extends BaseAutofixContext = Bas
   get defaultConfig(): FullRuleConfig {
     return DEFAULT_RULE_CONFIG
   }
+
+  protected createOffense(message: string, location: Location, autofixContext?: TAutofixContext, severity?: LintSeverity): UnboundLintOffense<TAutofixContext> {
+    return {
+      rule: this.ruleName,
+      code: this.ruleName,
+      source: "Herb Linter",
+      message,
+      location,
+      autofixContext,
+      severity,
+    }
+  }
+
   abstract check(lexResult: LexResult, context?: Partial<LintContext>): UnboundLintOffense<TAutofixContext>[]
 
   /**
@@ -215,6 +240,19 @@ export abstract class SourceRule<TAutofixContext extends BaseAutofixContext = Ba
   get defaultConfig(): FullRuleConfig {
     return DEFAULT_RULE_CONFIG
   }
+
+  protected createOffense(message: string, location: Location, autofixContext?: TAutofixContext, severity?: LintSeverity): UnboundLintOffense<TAutofixContext> {
+    return {
+      rule: this.ruleName,
+      code: this.ruleName,
+      source: "Herb Linter",
+      message,
+      location,
+      autofixContext,
+      severity,
+    }
+  }
+
   abstract check(source: string, context?: Partial<LintContext>): UnboundLintOffense<TAutofixContext>[]
 
   /**