Merge pull request github#78 from github/render_local_literal_keys

Enforce local keys being literals

lib/rubocop/cop/github/rails_controller_render_literal.rb

@@ -1,36 +1,15 @@
 # frozen_string_literal: true
 
 require "rubocop"
+require "rubocop/cop/github/render_literal_helpers"
 
 module RuboCop
   module Cop
     module GitHub
       class RailsControllerRenderLiteral < Cop
-        MSG = "render must be used with a string literal or an instance of a Class"
-
-        def_node_matcher :literal?, <<-PATTERN
-          ({str sym true false nil?} ...)
-        PATTERN
-
-        def_node_matcher :render?, <<-PATTERN
-          (send nil? {:render :render_to_string} ...)
-        PATTERN
-
-        def_node_matcher :render_literal?, <<-PATTERN
-          (send nil? {:render :render_to_string} ({str sym} $_) $...)
-        PATTERN
-
-        def_node_matcher :render_const?, <<-PATTERN
-          (send nil? {:render :render_to_string} (const _ _) ...)
-        PATTERN
-
-        def_node_matcher :render_inst?, <<-PATTERN
-          (send nil? {:render :render_to_string} (send _ :new ...) ...)
-        PATTERN
+        include RenderLiteralHelpers
 
-        def_node_matcher :render_with_options?, <<-PATTERN
-          (send nil? {:render :render_to_string} (hash $...))
-        PATTERN
+        MSG = "render must be used with a string literal or an instance of a Class"
 
         def_node_matcher :ignore_key?, <<-PATTERN
           (pair (sym {
@@ -68,10 +47,16 @@ class RailsControllerRenderLiteral < Cop
           }) ...)
         PATTERN
 
+        def_node_matcher :render_const?, <<-PATTERN
+          (send nil? {:render :render_to_string} (const _ _) ...)
+        PATTERN
+
         def on_send(node)
           return unless render?(node)
 
-          if render_literal?(node) || render_inst?(node) || render_const?(node)
+          return if render_view_component?(node) || render_const?(node)
+
+          if render_literal?(node)
           elsif option_pairs = render_with_options?(node)
             option_pairs = option_pairs.reject { |pair| options_key?(pair) }
 
@@ -82,18 +67,40 @@ def on_send(node)
             if template_node = option_pairs.map { |pair| template_key?(pair) }.compact.first
               if !literal?(template_node)
                 add_offense(node, location: :expression)
+                return
               end
             else
               add_offense(node, location: :expression)
+              return
             end
 
             if layout_node = option_pairs.map { |pair| layout_key?(pair) }.compact.first
               if !literal?(layout_node)
                 add_offense(node, location: :expression)
+                return
               end
             end
           else
             add_offense(node, location: :expression)
+            return
+          end
+
+          if render_literal?(node)
+            option_hash = node.arguments[1]
+            if option_hash && !option_hash.hash_type?
+              add_offense(node, location: :expression)
+              return
+            end
+            option_pairs = option_hash && option_hash.pairs
+          else
+            option_pairs = node.arguments[0].pairs
+          end
+
+          if option_pairs
+            locals = option_pairs.map { |pair| locals_key?(pair) }.compact.first
+            if locals && (!locals.hash_type? || !hash_with_literal_keys?(locals))
+              add_offense(node, location: :expression)
+            end
           end
         end
       end

lib/rubocop/cop/github/rails_view_render_literal.rb

@@ -1,36 +1,15 @@
 # frozen_string_literal: true
 
 require "rubocop"
+require "rubocop/cop/github/render_literal_helpers"
 
 module RuboCop
   module Cop
     module GitHub
       class RailsViewRenderLiteral < Cop
-        MSG = "render must be used with a string literal or an instance of a Class"
+        include RenderLiteralHelpers
 
-        def_node_matcher :literal?, <<-PATTERN
-          ({str sym true false nil?} ...)
-        PATTERN
-
-        def_node_matcher :render?, <<-PATTERN
-          (send nil? {:render :render_to_string} $...)
-        PATTERN
-
-        def_node_matcher :render_literal?, <<-PATTERN
-          (send nil? {:render :render_to_string} ({str sym} $_) $...)
-        PATTERN
-
-        def_node_matcher :render_inst?, <<-PATTERN
-          (send nil? {:render :render_to_string} (send _ :new ...) ...)
-        PATTERN
-
-        def_node_matcher :render_collection?, <<-PATTERN
-          (send nil? {:render :render_to_string} (send _ :with_collection ...) ...)
-        PATTERN
-
-        def_node_matcher :render_with_options?, <<-PATTERN
-          (send nil? {:render :render_to_string} (hash $...) ...)
-        PATTERN
+        MSG = "render must be used with a literal template and use literals for locals keys"
 
         def_node_matcher :ignore_key?, <<-PATTERN
           (pair (sym {
@@ -50,7 +29,10 @@ class RailsViewRenderLiteral < Cop
         def on_send(node)
           return unless render?(node)
 
-          if render_literal?(node) || render_inst?(node) || render_collection?(node)
+          # Ignore "component"-style renders
+          return if render_view_component?(node)
+
+          if render_literal?(node)
           elsif option_pairs = render_with_options?(node)
             if option_pairs.any? { |pair| ignore_key?(pair) }
               return
@@ -59,12 +41,31 @@ def on_send(node)
             if partial_node = option_pairs.map { |pair| partial_key?(pair) }.compact.first
               if !literal?(partial_node)
                 add_offense(node, location: :expression)
+                return
               end
             else
               add_offense(node, location: :expression)
+              return
             end
           else
             add_offense(node, location: :expression)
+            return
+          end
+
+          if render_literal?(node) && node.arguments.count > 1
+            locals = node.arguments[1]
+          elsif options_pairs = render_with_options?(node)
+            locals = option_pairs.map { |pair| locals_key?(pair) }.compact.first
+          end
+
+          if locals
+            if locals.hash_type?
+              if !hash_with_literal_keys?(locals)
+                add_offense(node, location: :expression)
+              end
+            else
+              add_offense(node, location: :expression)
+            end
           end
         end
       end

lib/rubocop/cop/github/render_literal_helpers.rb

@@ -0,0 +1,54 @@
+# frozen_string_literal: true
+#
+require "rubocop"
+
+module RuboCop
+  module Cop
+    module GitHub
+      module RenderLiteralHelpers
+        extend NodePattern::Macros
+
+        def_node_matcher :literal?, <<-PATTERN
+          ({str sym true false nil?} ...)
+        PATTERN
+
+        def_node_matcher :render?, <<-PATTERN
+          (send nil? {:render :render_to_string} ...)
+        PATTERN
+
+        def_node_matcher :render_literal?, <<-PATTERN
+          (send nil? {:render :render_to_string} ({str sym} $_) $...)
+        PATTERN
+
+        def_node_matcher :render_inst?, <<-PATTERN
+          (send nil? {:render :render_to_string} (send _ :new ...) ...)
+        PATTERN
+
+        def_node_matcher :render_with_options?, <<-PATTERN
+          (send nil? {:render :render_to_string} (hash $...) ...)
+        PATTERN
+
+        def_node_matcher :render_view_component_instance?, <<-PATTERN
+          (send nil? {:render :render_to_string} (send _ :new ...) ...)
+        PATTERN
+
+        def_node_matcher :render_view_component_collection?, <<-PATTERN
+          (send nil? {:render :render_to_string} (send _ :with_collection ...) ...)
+        PATTERN
+
+        def_node_matcher :locals_key?, <<-PATTERN
+          (pair (sym :locals) $_)
+        PATTERN
+
+        def hash_with_literal_keys?(hash)
+          hash.pairs.all? { |pair| literal?(pair.key) }
+        end
+
+        def render_view_component?(node)
+          render_view_component_instance?(node) ||
+            render_view_component_collection?(node)
+        end
+      end
+    end
+  end
+end

test/test_rails_controller_render_literal.rb

@@ -393,4 +393,65 @@ def index
     assert_equal 1, cop.offenses.count
     assert_equal "render must be used with a string literal or an instance of a Class", cop.offenses[0].message
   end
+
+  def test_render_shorthand_static_locals_no_offsense
+    investigate cop, <<-RUBY, "app/controllers/products_controller.rb"
+      class ProductsController < ActionController::Base
+        def index
+          render "products/index", locals: { product: product }
+        end
+      end
+    RUBY
+
+    assert_equal 0, cop.offenses.count
+  end
+
+  def test_render_partial_static_locals_no_offsense
+    investigate cop, <<-RUBY, "app/controllers/products_controller.rb"
+      class ProductsController < ActionController::Base
+        def index
+          render partial: "products/index", locals: { product: product }
+        end
+      end
+    RUBY
+
+    assert_equal 0, cop.offenses.count
+  end
+
+  def test_render_literal_dynamic_options_offense
+    investigate cop, <<-RUBY, "app/controllers/products_controller.rb"
+      class ProductsController < ActionController::Base
+        def index
+          render "products/product", options
+        end
+      end
+    RUBY
+
+    assert_equal 1, cop.offenses.count
+  end
+
+  def test_render_literal_dynamic_locals_offense
+    investigate cop, <<-RUBY, "app/controllers/products_controller.rb"
+      class ProductsController < ActionController::Base
+        def index
+          render "products/product", locals: locals
+        end
+      end
+    RUBY
+
+    assert_equal 1, cop.offenses.count
+  end
+
+
+  def test_render_literal_dynamic_local_key_offense
+    investigate cop, <<-RUBY, "app/controllers/products_controller.rb"
+      class ProductsController < ActionController::Base
+        def index
+          render "products/product", locals: { product_key => product }
+        end
+      end
+    RUBY
+
+    assert_equal 1, cop.offenses.count
+  end
 end

test/test_rails_view_render_literal.rb

@@ -43,7 +43,7 @@ def test_render_variable_offense
     ERB
 
     assert_equal 2, cop.offenses.count
-    assert_equal "render must be used with a string literal or an instance of a Class", cop.offenses[0].message
+    assert_equal "render must be used with a literal template and use literals for locals keys", cop.offenses[0].message
   end
 
   def test_render_component_no_offense
@@ -94,7 +94,7 @@ def test_render_layout_variable_literal_no_offense
     ERB
 
     assert_equal 1, cop.offenses.count
-    assert_equal "render must be used with a string literal or an instance of a Class", cop.offenses[0].message
+    assert_equal "render must be used with a literal template and use literals for locals keys", cop.offenses[0].message
   end
 
   def test_render_inline_no_offense
@@ -112,4 +112,52 @@ def test_render_template_literal_no_offense
 
     assert_equal 0, cop.offenses.count
   end
+
+  def test_render_literal_static_locals_no_offense
+    erb_investigate cop, <<-ERB, "app/views/products/index.html.erb"
+      <%= render "products/product", product: product %>
+    ERB
+
+    assert_equal 0, cop.offenses.count
+  end
+
+  def test_render_literal_dynamic_locals_offense
+    erb_investigate cop, <<-ERB, "app/views/products/index.html.erb"
+      <%= render "products/product", locals %>
+    ERB
+
+    assert_equal 1, cop.offenses.count
+  end
+
+  def test_render_literal_dynamic_local_key_offense
+    erb_investigate cop, <<-ERB, "app/views/products/index.html.erb"
+      <%= render "products/product", { product_key => product } %>
+    ERB
+
+    assert_equal 1, cop.offenses.count
+  end
+
+  def test_render_options_static_locals_no_offense
+    erb_investigate cop, <<-ERB, "app/views/products/index.html.erb"
+      <%= render partial: "products/product", locals: { product: product } %>
+    ERB
+
+    assert_equal 0, cop.offenses.count
+  end
+
+  def test_render_options_dynamic_locals_offense
+    erb_investigate cop, <<-ERB, "app/views/products/index.html.erb"
+      <%= render partial: "products/product", locals: locals %>
+    ERB
+
+    assert_equal 1, cop.offenses.count
+  end
+
+  def test_render_options_dynamic_local_key_offense
+    erb_investigate cop, <<-ERB, "app/views/products/index.html.erb"
+      <%= render partial: "products/product", locals: { product_key => product } %>
+    ERB
+
+    assert_equal 1, cop.offenses.count
+  end
 end