From 5f318d6a689aca2288669584b83631e8e4f74d84 Mon Sep 17 00:00:00 2001
From: Emil Ong <emil@binti.com>
Date: Thu, 11 Nov 2021 14:09:20 -0800
Subject: [PATCH 1/2] Add InstanceVariable linter

Adds the InstanceVariable linter, which can be limited to partials only
(thus deprecating PartialInstanceVariable), but considers an instance
variable in any template to be an offense by default.

Also changed range behavior in both linters to highlight the instance
variables themselves. The previous behavior highlighted the start of the
variable to the end of the file.

Finally, added support for detecting class instance variables.
---
 README.md                                     |  78 ++++++++++++-
 lib/erb_lint/linters/instance_variable.rb     |  45 ++++++++
 .../linters/partial_instance_variable.rb      |  28 ++---
 .../linters/instance_variable_spec.rb         | 109 ++++++++++++++++++
 .../linters/partial_instance_variable_spec.rb |  21 +++-
 5 files changed, 263 insertions(+), 18 deletions(-)
 create mode 100644 lib/erb_lint/linters/instance_variable.rb
 create mode 100644 spec/erb_lint/linters/instance_variable_spec.rb

diff --git a/README.md b/README.md
index 191a4828..7ab86cee 100644
--- a/README.md
+++ b/README.md
@@ -98,7 +98,8 @@ 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 |
+| [InstanceVariable](#InstanceVariable)            | No       | detects instance variables |
+| PartialInstanceVariable                          | No       | detects instance variables in partials (deprecated in favor of [InstanceVariable](#InstanceVariable))|
 | [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 |
@@ -355,15 +356,15 @@ Linter-Specific Option | Description
 `correction_style`     | When configured with `cdata`, adds CDATA markers. When configured with `plain`, don't add makers. Defaults to `cdata`.
 
 ### RequireScriptNonce
-This linter prevents the usage of HTML `<script>`, Rails `javascript_tag`, `javascript_include_tag` and `javascript_pack_tag` without a `nonce` argument. The purpose of such a check is to ensure that when [content securty policy](https://edgeguides.rubyonrails.org/security.html#content-security-policy) is implemented in an application, there is a means of discovering tags that need to be updated with a `nonce` argument to enable script execution at application runtime. 
+This linter prevents the usage of HTML `<script>`, Rails `javascript_tag`, `javascript_include_tag` and `javascript_pack_tag` without a `nonce` argument. The purpose of such a check is to ensure that when [content securty policy](https://edgeguides.rubyonrails.org/security.html#content-security-policy) is implemented in an application, there is a means of discovering tags that need to be updated with a `nonce` argument to enable script execution at application runtime.
 
 ```
 Bad ❌
-<script> 
+<script>
     alert(1)
 </script>
 Good ✅
-<script nonce="<%= request.content_security_policy_nonce %>" > 
+<script nonce="<%= request.content_security_policy_nonce %>" >
     alert(1)
 </script>
 ```
@@ -486,6 +487,75 @@ Linter-Specific Option    | Description
 `allow_blank`             | True or false, depending on whether or not the `type` attribute may be omitted entirely from a `<script>` tag. Defaults to `true`.
 `disallow_inline_scripts` | Do not allow inline `<script>` tags anywhere in ERB templates. Defaults to `false`.
 
+### InstanceVariables
+
+This linter prevents instance variables from being referenced in templates.
+
+```erb
+Bad ❌
+<%= @instance_variable %>
+
+Good ✅
+<%= local_variable %>
+```
+
+Instance variables in templates, if not declared (for example because of
+changes or even misspellings), will evaluate to `nil`. Using locals offers better
+protection against inadvertently referencing an undefined instance variable.
+
+Locals also offer more explicit specification of the dependencies of a template,
+which you may prefer for your project.
+
+While locals may be used in any template (see below), they have often been
+limited to use in partials. To restrict this linter to checking partials,
+specify the option `partials_only` to `true`. This configuration can replace the
+usage of the deprecated `PartialInstanceVariable` linter.
+
+To use locals in non-partials in Rails controller actions, you can use an
+explicit `render` method call, e.g.
+
+```ruby
+class MyController < ApplicationController
+  def my_action
+    render locals: { local1: 5, local2: "local 2" }
+  end
+end
+```
+
+To use locals in non-partials in Rails mailers, you can use an explicit `render`
+method call in a `format` block, e.g.
+
+```ruby
+class MyMailer < ApplicationMailer
+  def send_my_mail
+    mail(to: "example@example.com", subject: "Partials in mailer") do |format|
+      format.html do
+        render locals: { local1: 5, local2: "local 2" }
+      end
+    end
+  end
+end
+```
+
+Locals need not be required in templates either. Either of the following techniques
+will allow use of optional locals, including setting defaults within the template:
+
+```erb
+<%
+  optional_variable ||= nil
+  optional_variable_with_default ||= "some default"
+%>
+
+<% if defined?(other_optional_variable) %>
+  <span><%= other_optional_variable %></span>
+<% end %>
+%>
+```
+
+Linter-Specific Option    | Description
+--------------------------|---------------------------------------------------------
+`partials_only`           | Boolean to limit linting to partial templates only.
+
 ## Custom Linters
 
 `erb-lint` allows you to create custom linters specific to your project. It will load linters from the `.erb-linters` directory in the root of your
diff --git a/lib/erb_lint/linters/instance_variable.rb b/lib/erb_lint/linters/instance_variable.rb
new file mode 100644
index 00000000..4f90491c
--- /dev/null
+++ b/lib/erb_lint/linters/instance_variable.rb
@@ -0,0 +1,45 @@
+# frozen_string_literal: true
+
+module ERBLint
+  module Linters
+    # Checks for instance variables in partials.
+    class InstanceVariable < Linter
+      include LinterRegistry
+
+      class ConfigSchema < LinterConfig
+        property :partials_only, accepts: [true, false], default: false, reader: :partials_only?
+      end
+      self.config_schema = ConfigSchema
+
+      INSTANCE_VARIABLE_REGEX = /[^@]?(@?@[a-z_][a-zA-Z_0-9]*)/.freeze
+      PARTIAL_FILE_REGEX = %r{(\A|.*/)_[^/\s]*\.html\.erb\z}.freeze
+
+      def run(processed_source)
+        return unless process_file?(processed_source)
+
+        matches = processed_source
+          .file_content
+          .to_enum(:scan, INSTANCE_VARIABLE_REGEX)
+          .map { Regexp.last_match }
+        return if matches.empty?
+
+        matches.each do |match|
+          range = match.begin(1)...match.end(1)
+          add_offense(processed_source.to_source_range(range), offense_message)
+        end
+      end
+
+      private
+
+      def process_file?(processed_source)
+        return true unless @config.partials_only?
+
+        processed_source.filename.match?(PARTIAL_FILE_REGEX)
+      end
+
+      def offense_message
+        "Instance variable detected."
+      end
+    end
+  end
+end
diff --git a/lib/erb_lint/linters/partial_instance_variable.rb b/lib/erb_lint/linters/partial_instance_variable.rb
index 50c5ff27..18432813 100644
--- a/lib/erb_lint/linters/partial_instance_variable.rb
+++ b/lib/erb_lint/linters/partial_instance_variable.rb
@@ -2,21 +2,23 @@
 
 module ERBLint
   module Linters
-    # Checks for instance variables in partials.
-    class PartialInstanceVariable < Linter
-      include LinterRegistry
+    # Checks for instance variables in partials only
+    class PartialInstanceVariable < InstanceVariable
+      self.config_schema = ConfigSchema
 
-      def run(processed_source)
-        instance_variable_regex = /\s@\w+/
-        return unless processed_source.filename.match?(%r{(\A|.*/)_[^/\s]*\.html\.erb\z}) &&
-          processed_source.file_content.match?(instance_variable_regex)
-
-        add_offense(
-          processed_source.to_source_range(
-            processed_source.file_content =~ instance_variable_regex..processed_source.file_content.size
-          ),
-          "Instance variable detected in partial."
+      def initialize(file_loader, config)
+        warn(
+          "PartialInstanceVariable is deprecated. "\
+          "Please use InstanceVariable with partials_only=true."
         )
+        config[:partials_only] = true
+        super(file_loader, config)
+      end
+
+      private
+
+      def offense_message
+        "Instance variable detected in partial."
       end
     end
   end
diff --git a/spec/erb_lint/linters/instance_variable_spec.rb b/spec/erb_lint/linters/instance_variable_spec.rb
new file mode 100644
index 00000000..eaaaa276
--- /dev/null
+++ b/spec/erb_lint/linters/instance_variable_spec.rb
@@ -0,0 +1,109 @@
+# frozen_string_literal: true
+
+require "spec_helper"
+
+describe ERBLint::Linters::InstanceVariable do
+  let(:linter_config) { described_class.config_schema.new }
+  let(:file_loader) { ERBLint::FileLoader.new(".") }
+  let(:linter) { described_class.new(file_loader, linter_config) }
+  let(:processed_source_one) { ERBLint::ProcessedSource.new("_file.html.erb", file) }
+  let(:processed_source_two) { ERBLint::ProcessedSource.new("app/views/_a_model/a_view.html.erb", file) }
+  let(:offenses) { linter.offenses }
+  before do
+    linter_config[:partials_only] = partials_only
+    linter.run(processed_source_one)
+    linter.run(processed_source_two)
+  end
+
+  describe "offenses" do
+    subject { offenses }
+
+    context "with partials_only=true" do
+      let(:partials_only) { true }
+
+      context "when an instance variable is not present" do
+        let(:file) { "<%= user.first_name %>" }
+        it { expect(subject).to(eq([])) }
+      end
+
+      context "when an instance variable is present" do
+        let(:file) { "<h2><%= @user.first_name %></h2>" }
+        it do
+          expect(subject).to(eq([
+            build_offense(processed_source_one, 8...13, "Instance variable detected."),
+          ]))
+        end
+      end
+
+      context "when a class instance variable is present" do
+        let(:file) { "<h2><%= @@user.first_name %></h2>" }
+        it do
+          expect(subject).to(eq([
+            build_offense(processed_source_one, 8...14, "Instance variable detected."),
+          ]))
+        end
+      end
+
+      context "when multiple instance variables are present" do
+        let(:file) { "<h2><%= @user.first_name %> <%= @user.last_name %></h2>" }
+        it do
+          expect(subject).to(eq([
+            build_offense(processed_source_one, 8...13, "Instance variable detected."),
+            build_offense(processed_source_one, 32...37, "Instance variable detected."),
+          ]))
+        end
+      end
+    end
+
+    context "with partials_only=true" do
+      let(:partials_only) { false }
+
+      context "when an instance variable is not present" do
+        let(:file) { "<%= user.first_name %>" }
+        it { expect(subject).to(eq([])) }
+      end
+
+      context "when an instance variable is present" do
+        let(:file) { "<h2><%= @user.first_name %></h2>" }
+        it do
+          expect(subject).to(eq([
+            build_offense(processed_source_one, 8...13, "Instance variable detected."),
+            build_offense(processed_source_two, 8...13, "Instance variable detected."),
+          ]))
+        end
+      end
+
+      context "when a class instance variable is present" do
+        let(:file) { "<h2><%= @@user.first_name %></h2>" }
+        it do
+          expect(subject).to(eq([
+            build_offense(processed_source_one, 8...14, "Instance variable detected."),
+            build_offense(processed_source_two, 8...14, "Instance variable detected."),
+          ]))
+        end
+      end
+
+      context "when multiple instance variables are present" do
+        let(:file) { "<h2><%= @user.first_name %> <%= @user.last_name %></h2>" }
+        it do
+          expect(subject).to(eq([
+            build_offense(processed_source_one, 8...13, "Instance variable detected."),
+            build_offense(processed_source_one, 32...37, "Instance variable detected."),
+            build_offense(processed_source_two, 8...13, "Instance variable detected."),
+            build_offense(processed_source_two, 32...37, "Instance variable detected."),
+          ]))
+        end
+      end
+    end
+  end
+
+  private
+
+  def build_offense(processed_source, range, message)
+    ERBLint::Offense.new(
+      linter,
+      processed_source.to_source_range(range),
+      message
+    )
+  end
+end
diff --git a/spec/erb_lint/linters/partial_instance_variable_spec.rb b/spec/erb_lint/linters/partial_instance_variable_spec.rb
index 49359e97..116abd55 100644
--- a/spec/erb_lint/linters/partial_instance_variable_spec.rb
+++ b/spec/erb_lint/linters/partial_instance_variable_spec.rb
@@ -26,7 +26,26 @@
       let(:file) { "<h2><%= @user.first_name %></h2>" }
       it do
         expect(subject).to(eq([
-          build_offense(processed_source_one, 7..32, "Instance variable detected in partial."),
+          build_offense(processed_source_one, 8...13, "Instance variable detected in partial."),
+        ]))
+      end
+    end
+
+    context "when a class instance variable is present" do
+      let(:file) { "<h2><%= @@user.first_name %></h2>" }
+      it do
+        expect(subject).to(eq([
+          build_offense(processed_source_one, 8...14, "Instance variable detected in partial."),
+        ]))
+      end
+    end
+
+    context "when multiple instance variables are present" do
+      let(:file) { "<h2><%= @user.first_name %> <%= @user.last_name %></h2>" }
+      it do
+        expect(subject).to(eq([
+          build_offense(processed_source_one, 8...13, "Instance variable detected in partial."),
+          build_offense(processed_source_one, 32...37, "Instance variable detected in partial."),
         ]))
       end
     end

From 422a839d8f6512c19009f25a251a53ea1016a732 Mon Sep 17 00:00:00 2001
From: Emil Ong <emil@binti.com>
Date: Mon, 29 Nov 2021 08:26:01 -0800
Subject: [PATCH 2/2] Change default partial_only to true, fix dependency
 loading issue

---
 lib/erb_lint/linters/instance_variable.rb         | 2 +-
 lib/erb_lint/linters/partial_instance_variable.rb | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/erb_lint/linters/instance_variable.rb b/lib/erb_lint/linters/instance_variable.rb
index 4f90491c..56142eeb 100644
--- a/lib/erb_lint/linters/instance_variable.rb
+++ b/lib/erb_lint/linters/instance_variable.rb
@@ -7,7 +7,7 @@ class InstanceVariable < Linter
       include LinterRegistry
 
       class ConfigSchema < LinterConfig
-        property :partials_only, accepts: [true, false], default: false, reader: :partials_only?
+        property :partials_only, accepts: [true, false], default: true, reader: :partials_only?
       end
       self.config_schema = ConfigSchema
 
diff --git a/lib/erb_lint/linters/partial_instance_variable.rb b/lib/erb_lint/linters/partial_instance_variable.rb
index 18432813..c826f8b8 100644
--- a/lib/erb_lint/linters/partial_instance_variable.rb
+++ b/lib/erb_lint/linters/partial_instance_variable.rb
@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require_relative "instance_variable"
+
 module ERBLint
   module Linters
     # Checks for instance variables in partials only