Linter: Implement actionview-no-redundant-local-assigns rule

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

@@ -19,6 +19,7 @@ This page contains documentation for all Herb Linter rules.
 #### Action View
 
 - [`actionview-no-helper-shadowing`](./actionview-no-helper-shadowing.md) - Disallow shadowing Action View helpers with block variables
+- [`actionview-no-redundant-local-assigns`](./actionview-no-redundant-local-assigns.md) - Disallow `local_assigns` reads that the strict locals declaration already answers
 - [`actionview-no-silent-helper`](./actionview-no-silent-helper.md) - Disallow silent ERB tags for Action View helpers
 - [`actionview-no-silent-render`](./actionview-no-silent-render.md) - Disallow calling `render` without outputting the result
 - [`actionview-no-unnecessary-html-safe`](./actionview-no-unnecessary-html-safe.md) - Disallow calling `.html_safe` on String literals

javascript/packages/linter/docs/rules/actionview-no-redundant-local-assigns.md

@@ -0,0 +1,70 @@
+# Linter Rule: Disallow `local_assigns` reads that the strict locals declaration already answers
+
+**Rule:** `actionview-no-redundant-local-assigns`
+
+## Description
+
+Detects `local_assigns` lookups in a partial that already has a `<%# locals: (...) %>` declaration, where the declaration makes the lookup either redundant or dead.
+
+## Rationale
+
+`local_assigns` is the documented way to read locals in a partial that has no strict locals declaration, and it stays useful in a partial that has one. Once a declaration exists, though, some of those lookups are answered by the declaration itself and only obscure what the template does.
+
+A required local is already a local variable, so reading it back out of the hash is a longer way to write the name. A required local is also always present, so asking `local_assigns.key?` about it is a condition that can only take one branch. And a name the declaration does not mention can never arrive, because Rails raises `ActionView::StrictLocalsError` for callers that pass an undeclared local, so a lookup for it is dead code.
+
+Optional locals are left alone. `local_assigns.key?(:size)` is the only way to tell "not passed" apart from "passed as `nil`", which a default value cannot express, so that check is a legitimate pattern rather than an offense. Partials with a `**` keyword rest in the declaration are skipped as well, since undeclared locals can legitimately arrive there.
+
+Partials without a strict locals declaration are not checked at all.
+
+## Examples
+
+### ✅ Good
+
+```erb
+<%# locals: (user:) %>
+
+<%= user.name %>
+```
+
+```erb
+<%# locals: (user:, size: nil) %>
+
+<%= user.name %>
+<% if local_assigns.key?(:size) %>
+  <span><%= size %></span>
+<% end %>
+```
+
+```erb
+<%# locals: (user:, **) %>
+
+<%= render "row", **local_assigns %>
+```
+
+### 🚫 Bad
+
+```erb
+<%# locals: (user:) %>
+
+<%= local_assigns[:user].name %>
+```
+
+```erb
+<%# locals: (user:) %>
+
+<% if local_assigns.key?(:user) %>
+  <%= user.name %>
+<% end %>
+```
+
+```erb
+<%# locals: (user:) %>
+
+<%= user.name %>
+<%= local_assigns.fetch(:size, "large") %>
+```
+
+## References
+
+- [Action View - Strict Locals](https://guides.rubyonrails.org/action_view_overview.html#strict-locals)
+- [Action View - Using `local_assigns`](https://guides.rubyonrails.org/action_view_overview.html#using-local-assigns)

javascript/packages/linter/src/rules.ts

@@ -11,6 +11,7 @@ import { A11yNoRedundantImageAltRule } from "./rules/a11y-no-redundant-image-alt
 import { A11ySVGHasAccessibleTextRule } from "./rules/a11y-svg-has-accessible-text.js"
 
 import { ActionViewNoHelperShadowingRule } from "./rules/actionview-no-helper-shadowing.js"
+import { ActionViewNoRedundantLocalAssignsRule } from "./rules/actionview-no-redundant-local-assigns.js"
 import { ActionViewNoSilentHelperRule } from "./rules/actionview-no-silent-helper.js"
 import { ActionViewNoSilentRenderRule } from "./rules/actionview-no-silent-render.js"
 import { ActionViewNoUnnecessaryHTMLSafeRule } from "./rules/actionview-no-unnecessary-html-safe.js"
@@ -132,6 +133,7 @@ export const rules: RuleClass[] = [
   A11ySVGHasAccessibleTextRule,
 
   ActionViewNoHelperShadowingRule,
+  ActionViewNoRedundantLocalAssignsRule,
   ActionViewNoSilentHelperRule,
   ActionViewNoSilentRenderRule,
   ActionViewNoUnnecessaryHTMLSafeRule,

javascript/packages/linter/src/rules/actionview-no-redundant-local-assigns.ts

@@ -0,0 +1,179 @@
+import { ParserRule } from "../types.js"
+import { PrismVisitor, Visitor } from "@herb-tools/core"
+
+import { isPrismNodeType, isRubyParameterNode, locationFromByteOffset } from "@herb-tools/core"
+import { isPartialFile } from "./file-utils.js"
+
+import type { DocumentNode, ERBStrictLocalsNode, ParseResult, ParserOptions, PrismNodes, RubyParameterNode } from "@herb-tools/core"
+import type { FullRuleConfig, LintContext, UnboundLintOffense } from "../types.js"
+
+const KEYWORD_KIND = "keyword"
+const KEYWORD_REST_KIND = "keyword_rest"
+const LOCAL_ASSIGNS = "local_assigns"
+
+const VALUE_LOOKUPS = ["[]", "fetch", "dig"]
+const PRESENCE_LOOKUPS = ["key?", "has_key?", "include?", "member?"]
+
+interface Lookup {
+  name: string
+  method: string
+  presence: boolean
+  extraArguments: boolean
+  startOffset: number
+  length: number
+}
+
+class StrictLocalsCollector extends Visitor {
+  hasDeclaration = false
+  hasKeywordRest = false
+
+  readonly locals = new Map<string, RubyParameterNode>()
+
+  visitERBStrictLocalsNode(node: ERBStrictLocalsNode): void {
+    this.hasDeclaration = true
+
+    for (const local of node.locals) {
+      if (!isRubyParameterNode(local)) continue
+
+      if (local.kind === KEYWORD_REST_KIND) {
+        this.hasKeywordRest = true
+        continue
+      }
+
+      if (local.kind !== KEYWORD_KIND) continue
+
+      const name = local.name?.value
+
+      if (name) this.locals.set(name, local)
+    }
+  }
+}
+
+class LocalAssignsLookupCollector extends PrismVisitor {
+  readonly lookups: Lookup[] = []
+
+  override visitCallNode(node: PrismNodes.CallNode): void {
+    const lookup = this.lookupFor(node)
+
+    if (lookup) this.lookups.push(lookup)
+
+    this.visitChildNodes(node)
+  }
+
+  private lookupFor(node: PrismNodes.CallNode): Lookup | null {
+    const presence = PRESENCE_LOOKUPS.includes(node.name)
+
+    if (!presence && !VALUE_LOOKUPS.includes(node.name)) return null
+    if (!this.isLocalAssignsRead(node.receiver)) return null
+
+    const argumentNodes = node.arguments_?.arguments_ ?? []
+    const [first, ...rest] = argumentNodes
+
+    if (!isPrismNodeType(first, "SymbolNode")) return null
+    if (rest.length > 0 && node.name !== "fetch") return null
+    if (rest.length > 1) return null
+
+    const name = first.unescaped?.value
+
+    if (!name) return null
+
+    return {
+      name,
+      method: node.name,
+      presence,
+      extraArguments: rest.length > 0,
+      startOffset: node.location.startOffset,
+      length: node.location.length,
+    }
+  }
+
+  private isLocalAssignsRead(node: PrismNodes.Node | null): boolean {
+    if (!isPrismNodeType(node, "CallNode")) return false
+    if (node.receiver !== null) return false
+
+    return node.name === LOCAL_ASSIGNS
+  }
+}
+
+export class ActionViewNoRedundantLocalAssignsRule extends ParserRule {
+  static ruleName = "actionview-no-redundant-local-assigns"
+  static introducedIn = this.version("unreleased")
+
+  get defaultConfig(): FullRuleConfig {
+    return {
+      enabled: true,
+      severity: {
+        cli: "error",
+        editor: "info",
+      },
+    }
+  }
+
+  get parserOptions(): Partial<ParserOptions> {
+    return {
+      strict_locals: true,
+      prism_program: true,
+    }
+  }
+
+  check(result: ParseResult, context?: Partial<LintContext>): UnboundLintOffense[] {
+    if (isPartialFile(context?.fileName) === false) return []
+
+    const document = result.value
+    const source = document.source
+    const program = document.prismNode
+
+    if (!source || !program) return []
+
+    const declaration = this.declarationFor(document)
+
+    if (!declaration.hasDeclaration) return []
+
+    const collector = new LocalAssignsLookupCollector()
+
+    collector.visit(program)
+
+    return collector.lookups.flatMap(lookup => {
+      const message = this.messageFor(lookup, declaration)
+
+      if (!message) return []
+
+      const location = locationFromByteOffset(source, lookup.startOffset, lookup.length)
+
+      return [this.createOffense(message, location, undefined, undefined, ["unnecessary"])]
+    })
+  }
+
+  private declarationFor(document: DocumentNode): StrictLocalsCollector {
+    const collector = new StrictLocalsCollector()
+
+    collector.visit(document)
+
+    return collector
+  }
+
+  private messageFor(lookup: Lookup, declaration: StrictLocalsCollector): string | null {
+    const local = declaration.locals.get(lookup.name)
+    const source = this.lookupSource(lookup)
+
+    if (!local) {
+      if (declaration.hasKeywordRest) return null
+
+      return `\`${lookup.name}\` is not declared in the \`locals:\` declaration, so Rails raises if a caller passes it and \`${source}\` can never find it. Declare \`${lookup.name}:\` in the declaration, or remove the lookup.`
+    }
+
+    if (!local.required) return null
+
+    if (lookup.presence) {
+      return `Strict local \`${lookup.name}\` is required, so \`${source}\` is always \`true\`. Remove the condition, or give \`${lookup.name}\` a default value to make it optional.`
+    }
+
+    return `Strict local \`${lookup.name}\` is already a local variable in this partial, so \`${source}\` reads back a value that is already in scope. Use \`${lookup.name}\` instead.`
+  }
+
+  private lookupSource(lookup: Lookup): string {
+    const argument = lookup.extraArguments ? `:${lookup.name}, ...` : `:${lookup.name}`
+
+    return lookup.method === "[]" ? `${LOCAL_ASSIGNS}[${argument}]` : `${LOCAL_ASSIGNS}.${lookup.method}(${argument})`
+  }
+}

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

@@ -16,6 +16,7 @@ export * from "./action-view-utils.js"
 export * from "./herb-disable-comment-base.js"
 
 export * from "./actionview-no-helper-shadowing.js"
+export * from "./actionview-no-redundant-local-assigns.js"
 export * from "./actionview-no-silent-helper.js"
 export * from "./actionview-no-silent-render.js"
 export * from "./actionview-no-unnecessary-html-safe.js"