Ignore implicit locals if not declared by templates with strict locals

Fix: rails#49780
Fix: rails#49761

`CollectionRenderer` implictly inject `<name>_counter` and `<name>_iteration`
locals, but in the overwhelming majority of cases they aren't used.

So when the rendered template has strict locals we shouldn't require
these to be declared, and if they aren't we should simply not pass them.

Co-Authored-By: HolyWalley <[email protected]>

Author: byroot

actionview/CHANGELOG.md

@@ -1,3 +1,11 @@
+*   Automatically discard the implicit locals injected by collection rendering for template that can't accept them
+
+    When rendering a collection, two implicit variables are injected, which breaks templates with strict locals.
+
+    Now they are only passed if the template will actually accept them.
+
+    *Yasha Krasnou*, *Jean Boussier*
+
 *   Fix `@rails/ujs` calling `start()` an extra time when using bundlers
 
     *Hartley McGuire*, *Ryunosuke Sato*

actionview/lib/action_view/renderer/collection_renderer.rb

@@ -194,7 +194,7 @@ def collection_with_template(view, template, layout, collection)
 
           _template = (cache[path] ||= (template || find_template(path, @locals.keys + [as, counter, iteration])))
 
-          content = _template.render(view, locals)
+          content = _template.render(view, locals, implicit_locals: [counter, iteration])
           content = layout.render(view, locals) { content } if layout
           partial_iteration.iterate!
           build_rendered_template(content, _template)

actionview/lib/action_view/template.rb

@@ -201,6 +201,7 @@ def initialize(source, identifier, handler, locals:, format: nil, variant: nil,
       @variant           = variant
       @compile_mutex     = Mutex.new
       @strict_locals     = NONE
+      @strict_local_keys = nil
       @type              = nil
     end
 
@@ -244,9 +245,15 @@ def supports_streaming?
     # This method is instrumented as "!render_template.action_view". Notice that
     # we use a bang in this instrumentation because you don't want to
     # consume this in production. This is only slow if it's being listened to.
-    def render(view, locals, buffer = nil, add_to_stack: true, &block)
+    def render(view, locals, buffer = nil, implicit_locals: [], add_to_stack: true, &block)
       instrument_render_template do
         compile!(view)
+
+        if strict_locals? && @strict_local_keys && !implicit_locals.empty?
+          locals_to_ignore = implicit_locals - @strict_local_keys
+          locals.except!(*locals_to_ignore)
+        end
+
         if buffer
           view._run(method_name, self, locals, buffer, add_to_stack: add_to_stack, has_strict_locals: strict_locals?, &block)
           nil
@@ -474,23 +481,30 @@ def compile(mod)
 
         return unless strict_locals?
 
+        parameters = mod.instance_method(method_name).parameters - [[:req, :output_buffer]]
         # Check compiled method parameters to ensure that only kwargs
         # were provided as strict locals, preventing `locals: (foo, *foo)` etc
         # and allowing `locals: (foo:)`.
 
-        non_kwarg_parameters =
-          (mod.instance_method(method_name).parameters - [[:req, :output_buffer]]).
-            select { |parameter| ![:keyreq, :key, :keyrest, :nokey].include?(parameter[0]) }
+        non_kwarg_parameters = parameters.select do |parameter|
+          ![:keyreq, :key, :keyrest, :nokey].include?(parameter[0])
+        end
 
-        return unless non_kwarg_parameters.any?
+        unless non_kwarg_parameters.empty?
+          mod.undef_method(method_name)
 
-        mod.undef_method(method_name)
+          raise ArgumentError.new(
+            "#{non_kwarg_parameters.map { |_, name| "`#{name}`" }.to_sentence} set as non-keyword " \
+            "#{'argument'.pluralize(non_kwarg_parameters.length)} for #{short_identifier}. " \
+            "Locals can only be set as keyword arguments."
+          )
+        end
 
-        raise ArgumentError.new(
-          "#{non_kwarg_parameters.map { |_, name| "`#{name}`" }.to_sentence} set as non-keyword " \
-          "#{'argument'.pluralize(non_kwarg_parameters.length)} for #{short_identifier}. " \
-          "Locals can only be set as keyword arguments."
-        )
+        unless parameters.any? { |type, _| type == :keyrest }
+          parameters.map!(&:first)
+          parameters.sort!
+          @strict_local_keys = parameters.freeze
+        end
       end
 
       def offset

actionview/test/template/template_test.rb

@@ -62,8 +62,8 @@ def new_template(body = "<%= hello %>", details = {})
     ActionView::Template.new(body.dup, "hello template", details.delete(:handler) || ERBHandler, virtual_path: "hello", **details)
   end
 
-  def render(locals = {})
-    @template.render(@context, locals)
+  def render(implicit_locals: [], **locals)
+    @template.render(@context, locals, implicit_locals: implicit_locals)
   end
 
   def setup
@@ -200,6 +200,21 @@ def test_extra_locals_raises_error
     assert_match(/unknown local: :foo/, error.message)
   end
 
+  def test_rails_injected_locals_does_not_raise_error_if_not_passed
+    @template = new_template("<%# locals: (message:) -%>")
+    render(message: "Hi", message_counter: 1, message_iteration: 1, implicit_locals: %i[message_counter message_iteration])
+  end
+
+  def test_rails_injected_locals_can_be_specified
+    @template = new_template("<%# locals: (message: 'Hello') -%>\n<%= message %>")
+    assert_equal "Hello", render(message: "Hello", implicit_locals: %i[message])
+  end
+
+  def test_rails_injected_locals_can_be_specified_as_kwargs
+    @template = new_template("<%# locals: (message: 'Hello', **kwargs) -%>\n<%= kwargs[:message_counter] %>-<%= kwargs[:message_iteration] %>")
+    assert_equal "1-2", render(message: "Hello", message_counter: 1, message_iteration: 2, implicit_locals: %i[message_counter message_iteration])
+  end
+
   # TODO: This is currently handled inside ERB. The case of explicitly
   # lying about encodings via the normal Rails API should be handled
   # inside Rails.