Linter: Implement erb-no-silent-statement rule (#1403)

This pull request implements a new `erb-no-silent-statement` linter rule
that flags all `<%` tags that aren't used for control flow or any form
of assignments.

Since this is quite a radical rule, I decided to disable this one by
default for now.

Author: marcoroth

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

@@ -21,6 +21,7 @@ This page contains documentation for all Herb Linter rules.
 - [`erb-no-output-in-attribute-name`](./erb-no-output-in-attribute-name.md) - Disallow ERB output in attribute names
 - [`erb-no-output-in-attribute-position`](./erb-no-output-in-attribute-position.md) - Disallow ERB output in attribute position
 - [`erb-no-raw-output-in-attribute-value`](./erb-no-raw-output-in-attribute-value.md) - Disallow `<%==` in attribute values
+- [`erb-no-silent-statement`](./erb-no-silent-statement.md) - Disallow silent ERB statements
 - [`erb-no-silent-tag-in-attribute-name`](./erb-no-silent-tag-in-attribute-name.md) - Disallow ERB silent tags in HTML attribute names
 - [`erb-no-statement-in-script`](./erb-no-statement-in-script.md) - Disallow ERB statements inside `<script>` tags
 - [`erb-no-then-in-control-flow`](./erb-no-then-in-control-flow.md) - Disallow `then` in ERB control flow expressions

javascript/packages/linter/docs/rules/erb-no-silent-statement.md

@@ -0,0 +1,53 @@
+# Linter Rule: Disallow silent ERB statements
+
+**Rule:** `erb-no-silent-statement`
+
+## Description
+
+Disallow silent ERB tags (`<% %>`) that execute statements whose return value is discarded. Logic like method calls should live in controllers, helpers, or presenters, not in views. Assignments are allowed since they are pragmatic for DRYing up templates.
+
+## Rationale
+
+Silent ERB tags that aren't control flow or assignments are a code smell. They execute Ruby code whose return value is silently discarded, which usually means the logic belongs in a controller, helper, or presenter rather than the view.
+
+## Examples
+
+### ✅ Good
+
+```erb
+<%= title %>
+<%= render "partial" %>
+```
+
+```erb
+<% x = 1 %>
+<% @title = "Hello" %>
+<% x ||= default_value %>
+<% x += 1 %>
+```
+
+```erb
+<% if user.admin? %>
+  Admin tools
+<% end %>
+```
+
+```erb
+<% users.each do |user| %>
+  <p><%= user.name %></p>
+<% end %>
+```
+
+### 🚫 Bad
+
+```erb
+<% some_method %>
+```
+
+```erb
+<% helper_call(arg) %>
+```
+
+## References
+
+\-

javascript/packages/linter/src/rules.ts

@@ -19,6 +19,7 @@ import { ERBNoOutputControlFlowRule } from "./rules/erb-no-output-control-flow.j
 import { ERBNoOutputInAttributeNameRule } from "./rules/erb-no-output-in-attribute-name.js"
 import { ERBNoOutputInAttributePositionRule } from "./rules/erb-no-output-in-attribute-position.js"
 import { ERBNoRawOutputInAttributeValueRule } from "./rules/erb-no-raw-output-in-attribute-value.js"
+import { ERBNoSilentStatementRule } from "./rules/erb-no-silent-statement.js"
 import { ERBNoSilentTagInAttributeNameRule } from "./rules/erb-no-silent-tag-in-attribute-name.js"
 import { ERBNoStatementInScriptRule } from "./rules/erb-no-statement-in-script.js"
 import { ERBNoThenInControlFlowRule } from "./rules/erb-no-then-in-control-flow.js"
@@ -103,6 +104,7 @@ export const rules: RuleClass[] = [
   ERBNoOutputInAttributeNameRule,
   ERBNoOutputInAttributePositionRule,
   ERBNoRawOutputInAttributeValueRule,
+  ERBNoSilentStatementRule,
   ERBNoSilentTagInAttributeNameRule,
   ERBNoStatementInScriptRule,
   ERBNoThenInControlFlowRule,

javascript/packages/linter/src/rules/erb-no-silent-statement.ts

@@ -0,0 +1,58 @@
+import { BaseRuleVisitor } from "./rule-utils.js"
+import { ParserRule } from "../types.js"
+
+import { isERBOutputNode, PrismNode } from "@herb-tools/core"
+
+import type { UnboundLintOffense, LintContext, FullRuleConfig } from "../types.js"
+import type { ParseResult, ERBContentNode, ParserOptions } from "@herb-tools/core"
+
+function isAssignmentNode(prismNode: PrismNode): boolean {
+  const type: string = prismNode?.constructor?.name
+  if (!type) return false
+
+  return type.endsWith("WriteNode")
+}
+
+class ERBNoSilentStatementVisitor extends BaseRuleVisitor {
+  visitERBContentNode(node: ERBContentNode): void {
+    if (isERBOutputNode(node)) return
+
+    const prismNode = node.prismNode
+    if (!prismNode) return
+
+    if (isAssignmentNode(prismNode)) return
+
+    const content = node.content?.value?.trim()
+    if (!content) return
+
+    this.addOffense(
+      `Avoid using silent ERB tags for statements. Move \`${content}\` to a controller, helper, or presenter.`,
+      node.location,
+    )
+  }
+}
+
+export class ERBNoSilentStatementRule extends ParserRule {
+  static ruleName = "erb-no-silent-statement"
+
+  get defaultConfig(): FullRuleConfig {
+    return {
+      enabled: false,
+      severity: "warning"
+    }
+  }
+
+  get parserOptions(): Partial<ParserOptions> {
+    return {
+      prism_nodes: true,
+    }
+  }
+
+  check(result: ParseResult, context?: Partial<LintContext>): UnboundLintOffense[] {
+    const visitor = new ERBNoSilentStatementVisitor(this.ruleName, context)
+
+    visitor.visit(result.value)
+
+    return visitor.offenses
+  }
+}

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

@@ -20,6 +20,7 @@ export * from "./erb-no-output-control-flow.js"
 export * from "./erb-no-output-in-attribute-name.js"
 export * from "./erb-no-output-in-attribute-position.js"
 export * from "./erb-no-raw-output-in-attribute-value.js"
+export * from "./erb-no-silent-statement.js"
 export * from "./erb-no-silent-tag-in-attribute-name.js"
 export * from "./erb-no-statement-in-script.js"
 export * from "./erb-no-then-in-control-flow.js"

javascript/packages/linter/test/rules/erb-no-silent-statement.test.ts

@@ -0,0 +1,119 @@
+import dedent from "dedent"
+import { describe, test } from "vitest"
+import { ERBNoSilentStatementRule } from "../../src/rules/erb-no-silent-statement.js"
+import { createLinterTest } from "../helpers/linter-test-helper.js"
+
+const { expectNoOffenses, expectWarning, assertOffenses } = createLinterTest(ERBNoSilentStatementRule)
+
+describe("ERBNoSilentStatementRule", () => {
+  test("should not report for output tags", () => {
+    expectNoOffenses(dedent`
+      <div>
+        <%= title %>
+        <%= render "partial" %>
+        <%== raw_html %>
+      </div>
+    `)
+  })
+
+  test("should not report for local variable assignments", () => {
+    expectNoOffenses(dedent`
+      <div>
+        <% x = 1 %>
+        <% name = "hello" %>
+      </div>
+    `)
+  })
+
+  test("should not report for instance variable assignments", () => {
+    expectNoOffenses(dedent`
+      <div>
+        <% @title = "Hello" %>
+      </div>
+    `)
+  })
+
+  test("should not report for or-assignment operators", () => {
+    expectNoOffenses(dedent`
+      <div>
+        <% x ||= default_value %>
+        <% @count ||= 0 %>
+      </div>
+    `)
+  })
+
+  test("should not report for and-assignment operators", () => {
+    expectNoOffenses(dedent`
+      <div>
+        <% x &&= other_value %>
+      </div>
+    `)
+  })
+
+  test("should not report for operator assignments", () => {
+    expectNoOffenses(dedent`
+      <div>
+        <% x += 1 %>
+        <% @count -= 1 %>
+      </div>
+    `)
+  })
+
+  test("should not report for control flow statements", () => {
+    expectNoOffenses(dedent`
+      <% if user.admin? %>
+        Admin tools
+      <% end %>
+
+      <% unless user.banned? %>
+        Content
+      <% end %>
+
+      <% user.posts.each do |post| %>
+        <p><%= post.title %></p>
+      <% end %>
+    `)
+  })
+
+  test("should report for method calls", () => {
+    expectWarning(
+      'Avoid using silent ERB tags for statements. Move `some_method` to a controller, helper, or presenter.',
+      { line: 2, column: 2 },
+    )
+
+    assertOffenses(dedent`
+      <div>
+        <% some_method %>
+      </div>
+    `)
+  })
+
+  test("should report for method calls with arguments", () => {
+    expectWarning(
+      'Avoid using silent ERB tags for statements. Move `helper_call(arg)` to a controller, helper, or presenter.',
+      { line: 2, column: 2 },
+    )
+
+    assertOffenses(dedent`
+      <div>
+        <% helper_call(arg) %>
+      </div>
+    `)
+  })
+
+  test("should not report for constant assignments", () => {
+    expectNoOffenses(dedent`
+      <div>
+        <% CONSTANT = "value" %>
+      </div>
+    `)
+  })
+
+  test("should not report for multi-assignment", () => {
+    expectNoOffenses(dedent`
+      <div>
+        <% a, b = 1, 2 %>
+      </div>
+    `)
+  })
+})