Merge pull request #395 from Shopify/strict-locals-linter

Add strict locals linter

README.md

@@ -111,7 +111,6 @@ linters:
 | [FinalNewline](#finalnewline)                    | Yes      | warns about missing newline at the end of a ERB template |
 | [NoJavascriptTagHelper](#nojavascripttaghelper)  | Yes      | prevents the usage of Rails' `javascript_tag` |
 | ParserErrors                                     | Yes      |             |
-| PartialInstanceVariable                          | No       | detects instance variables in partials |
 | [RequireInputAutocomplete](#requireinputautocomplete)        | Yes       | warns about missing autocomplete attributes in input tags |
 | [RightTrim](#righttrim)                          | Yes      | enforces trimming at the right of an ERB tag |
 | [SelfClosingTag](#selfclosingtag)                | Yes      | enforces self closing tag styles for void elements |
@@ -121,9 +120,11 @@ linters:
 | TrailingWhitespace                               | Yes      |             |
 | [DeprecatedClasses](#deprecatedclasses)          | No       | warns about deprecated css classes |
 | [ErbSafety](#erbsafety)                          | No       | detects unsafe interpolation of ruby data into various javascript contexts and enforce usage of safe helpers like `.to_json`. |
+| [HardCodedString](#hardcodedstring)              | No       | warns if there is a visible hardcoded string in the DOM (does not check for a hardcoded string nested inside a JavaScript tag)            |
+| PartialInstanceVariable                          | No       | detects instance variables in partials |
 | [Rubocop](#rubocop)                              | No       | runs RuboCop rules on ruby statements found in ERB templates |
 | [RequireScriptNonce](#requirescriptnonce)        | No       | warns about missing [Content Security Policy nonces](https://guides.rubyonrails.org/security.html#content-security-policy) in script tags |
-| [HardCodedString](#hardcodedstring)              | No       | warns if there is a visible hardcoded string in the DOM (does not check for a hardcoded string nested inside a JavaScript tag)            |
+| [StrictLocals](#strictlocals)                    | No       | enforces the use of strict locals in Rails view partial templates |
 
 ### DeprecatedClasses
 
@@ -544,6 +545,27 @@ class I18nCorrector
 end
 ```
 
+### StrictLocals
+This linter enforces the use of [strict locals](https://guides.rubyonrails.org/layouts_and_rendering.html#strict-locals) in Rails view partial templates.
+
+```
+# app/views/foo/_bar.html.erb
+
+Bad ❌
+<div>
+  My name is <%= @name %>
+</div>
+
+Good ✅
+<%# locals: (name:) %>
+<div>
+  My name is <%= name %>
+</div>
+```
+
+**Note**: This linter does not enforce the use of strict locals in view templates (files that don't start with `_`).
+**Note**: This linter does not prevent the use of instance variables. It merely enforces that a strict locals declaration is present. It's recommended to use this linter in conjunction with the `PartialInstanceVariable` linter to enforce the use of locals.
+
 ## CommentSyntax
 
 This linter enforces the use of the correct ERB comment syntax, since Ruby comments (`<% # comment %>` with a space) are not technically valid ERB comments.

lib/erb_lint/linters/strict_locals.rb

@@ -0,0 +1,50 @@
+# frozen_string_literal: true
+
+module ERBLint
+  module Linters
+    # Enforces the use of strict locals in Rails view partial templates.
+    class StrictLocals < Linter
+      include LinterRegistry
+
+      STRICT_LOCALS_REGEX = /\s+locals:\s+\((.*)\)/
+
+      def initialize(file_loader, config)
+        super
+      end
+
+      def run(processed_source)
+        return unless processed_source.filename.match?(%r{(\A|.*/)_[^/\s]*\.html\.erb\z})
+
+        file_content = processed_source.file_content
+        return if file_content.empty?
+
+        strict_locals_node = processed_source.ast.descendants(:erb).find do |erb_node|
+          indicator_node, _, code_node, _ = *erb_node
+
+          indicator_node_str = indicator_node&.deconstruct&.last
+          next unless indicator_node_str == "#"
+
+          code_node_str = code_node&.deconstruct&.last
+
+          code_node_str.match(STRICT_LOCALS_REGEX)
+        end
+
+        unless strict_locals_node
+          add_offense(
+            processed_source.to_source_range(0...processed_source.file_content.size),
+            <<~EOF.chomp,
+              Missing strict locals declaration.
+              Add <%# locals: () %> at the top of the file to enforce strict locals.
+            EOF
+          )
+        end
+      end
+
+      def autocorrect(_processed_source, offense)
+        lambda do |corrector|
+          corrector.insert_before(offense.source_range, "<%# locals: () %>\n")
+        end
+      end
+    end
+  end
+end

spec/erb_lint/linters/strict_locals_spec.rb

@@ -0,0 +1,113 @@
+# frozen_string_literal: true
+
+require "spec_helper"
+
+describe ERBLint::Linters::StrictLocals do
+  let(:linter_config) { described_class.config_schema.new }
+  let(:file_loader) { ERBLint::FileLoader.new(".") }
+  let(:linter) { described_class.new(file_loader, linter_config) }
+
+  subject { linter.offenses }
+  before { linter.run(processed_source) }
+
+  context "when the ERB is not a view partial" do
+    let(:file) { <<~FILE }
+      <div>
+        <%= foo %>
+      </div>
+    FILE
+
+    context "when the ERB is a simple file" do
+      let(:processed_source) { ERBLint::ProcessedSource.new("file.html.erb", file) }
+
+      it "does not report any offenses" do
+        expect(subject).to(be_empty)
+      end
+    end
+
+    context "when the ERB is a nested file" do
+      let(:processed_source) { ERBLint::ProcessedSource.new("foo/bar/baz/my_file.html.erb", file) }
+
+      it "does not report any offenses" do
+        expect(subject).to(be_empty)
+      end
+    end
+  end
+
+  context "when the ERB is empty" do
+    let(:file) { "" }
+    let(:processed_source) { ERBLint::ProcessedSource.new("_file.html.erb", file) }
+
+    it "does not report any offenses" do
+      expect(subject).to(be_empty)
+    end
+  end
+
+  context "when the ERB is a view partial" do
+    let(:processed_source) { ERBLint::ProcessedSource.new("_file.html.erb", file) }
+
+    context "when the ERB contains a strict locals declaration at the top of the file" do
+      let(:file) { <<~FILE }
+        <%# locals: (foo: "bar") %>
+        <div>
+          <%= foo %>
+        </div>
+      FILE
+
+      it "does not report any offenses" do
+        expect(subject).to(be_empty)
+      end
+    end
+
+    context "when the ERB contains a strict locals declaration anywhere else in the file" do
+      let(:file) { <<~FILE }
+        <div>
+          <%= foo %>
+        </div>
+        <%# locals: (foo: "bar") %>
+      FILE
+
+      it "does not report any offenses" do
+        expect(subject).to(be_empty)
+      end
+    end
+
+    context "when the ERB contains an empty strict locals declaration" do
+      let(:file) { <<~FILE }
+        <%# locals: () %>
+        <div>
+          <%= foo %>
+        </div>
+      FILE
+
+      it "does not report any offenses" do
+        expect(subject).to(be_empty)
+      end
+    end
+
+    context "when the ERB does not contain a strict locals declaration" do
+      let(:file) { <<~FILE }
+        <div>
+          <%= foo %>
+        </div>
+      FILE
+      let(:corrector) { ERBLint::Corrector.new(processed_source, subject) }
+      let(:corrected_content) { corrector.corrected_content }
+
+      it "reports an offense" do
+        expect(subject.size).to(eq(1))
+      end
+
+      it "reports the suggested fix" do
+        expect(subject.first.message).to(include(
+          "Missing strict locals declaration.\n",
+          "Add <%# locals: () %> at the top of the file to enforce strict locals.",
+        ))
+      end
+
+      it "corrects the file" do
+        expect(corrected_content).to(eq("<%# locals: () %>\n<div>\n  <%= foo %>\n</div>\n"))
+      end
+    end
+  end
+end