Linter: Implement erb-no-instance-variables-in-partials linter rule

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,45 @@
+# 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.
+
+This rule detects all forms of instance variable usage including reads (`@var`), writes (`@var = value`), compound assignments (`@var &&=`, `@var ||=`, `@var +=`), and multiple assignment targets.
+
+## Examples
+
+### ✅ Good
+
+```erb [app/views/posts/index.html.erb]
+<%= render "posts/card", post: @post %>
+```
+
+```erb [app/views/posts/_card.html.erb]
+<div>
+  <%= post.title %>
+</div>
+```
+
+### 🚫 Bad
+
+```erb [app/views/posts/index.html.erb]
+<%= render "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,
+      prismProgram: 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>[]
 
   /**

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

@@ -0,0 +1,120 @@
+import dedent from "dedent"
+import { describe, test } from "vitest"
+import { ERBNoInstanceVariablesInPartialsRule } from "../../src/rules/erb-no-instance-variables-in-partials.js"
+import { createLinterTest } from "../helpers/linter-test-helper.js"
+
+const { expectNoOffenses, expectError, assertOffenses } = createLinterTest(ERBNoInstanceVariablesInPartialsRule)
+
+describe("ERBNoInstanceVariablesInPartialsRule", () => {
+  test("allows local variables in partials", () => {
+    expectNoOffenses(dedent`
+      <div>
+        <%= post.title %>
+      </div>
+    `, { fileName: "_card.html.erb" })
+  })
+
+  test("allows local variables passed to partials", () => {
+    expectNoOffenses(dedent`
+      <div>
+        <%= user.name %>
+        <%= title %>
+      </div>
+    `, { fileName: "_user_card.html.erb" })
+  })
+
+  test("flags instance variables in partials", () => {
+    expectError("Avoid using instance variables in partials. Pass `@post` as a local variable instead.", [2, 6])
+
+    assertOffenses(dedent`
+      <div>
+        <%= @post.title %>
+      </div>
+    `, { fileName: "_card.html.erb" })
+  })
+
+  test("flags multiple instance variables in partials", () => {
+    expectError("Avoid using instance variables in partials. Pass `@post` as a local variable instead.", [2, 6])
+    expectError("Avoid using instance variables in partials. Pass `@user` as a local variable instead.", [3, 6])
+
+    assertOffenses(dedent`
+      <div>
+        <%= @post.title %>
+        <%= @user.name %>
+      </div>
+    `, { fileName: "_card.html.erb" })
+  })
+
+  test("flags instance variables in silent ERB tags in partials", () => {
+    expectError("Avoid using instance variables in partials. Pass `@posts` as a local variable instead.", [1, 3])
+
+    assertOffenses(dedent`
+      <% @posts.each do |post| %>
+        <div><%= post.title %></div>
+      <% end %>
+    `, { fileName: "_list.html.erb" })
+  })
+
+  test("does not flag instance variables in non-partial files", () => {
+    expectNoOffenses(dedent`
+      <div>
+        <%= @post.title %>
+      </div>
+    `, { fileName: "show.html.erb" })
+  })
+
+  test("does not flag instance variables in layout files", () => {
+    expectNoOffenses(dedent`
+      <!DOCTYPE html>
+      <html>
+        <body><%= @content %></body>
+      </html>
+    `, { fileName: "application.html.erb" })
+  })
+
+  test("does not flag when filename is not provided", () => {
+    expectNoOffenses(dedent`
+      <div>
+        <%= @post.title %>
+      </div>
+    `, { fileName: undefined })
+  })
+
+  test("flags instance variables in deeply nested partials path", () => {
+    expectError("Avoid using instance variables in partials. Pass `@user` as a local variable instead.", [1, 4])
+
+    assertOffenses(dedent`
+      <%= @user.name %>
+    `, { fileName: "app/views/users/_profile.html.erb" })
+  })
+
+  test("does not flag class variables", () => {
+    expectNoOffenses(dedent`
+      <%= @@counter %>
+    `, { fileName: "_card.html.erb" })
+  })
+
+  test("does not flag email addresses in text", () => {
+    expectNoOffenses(dedent`
+      <p>Contact us</p>
+    `, { fileName: "_footer.html.erb" })
+  })
+
+  test("flags instance variable writes in partials", () => {
+    expectError("Avoid setting instance variables in partials. Use a local variable instead of `@record`.", [1, 3])
+
+    assertOffenses(dedent`
+      <% @record = "hello" %>
+    `, { fileName: "_form.html.erb" })
+  })
+
+  test("flags instance variables in if conditions", () => {
+    expectError("Avoid using instance variables in partials. Pass `@show` as a local variable instead.", [1, 6])
+
+    assertOffenses(dedent`
+      <% if @show %>
+        <p>Visible</p>
+      <% end %>
+    `, { fileName: "_toggle.html.erb" })
+  })
+})