Fix handling of {% render block %} in UnusedSnippet

lib/theme_check/checks/unused_snippet.rb

@@ -1,4 +1,5 @@
 # frozen_string_literal: true
+
 require "set"
 
 module ThemeCheck
@@ -11,16 +12,22 @@ def initialize
       @used_snippets = Set.new
     end
 
-    def on_include(node)
+    def on_render(node)
       if node.value.template_name_expr.is_a?(String)
         @used_snippets << "snippets/#{node.value.template_name_expr}"
+
+      elsif might_have_a_block_as_variable_lookup?(node)
+        # We ignore this case, because that's a "proper" use case for
+        # the render tag with OS 2.0
+        # {% render block %} shouldn't turn off the UnusedSnippet check
+
       else
         # Can't reliably track unused snippets if an expression is used, ignore this check
         @used_snippets.clear
         ignore!
       end
     end
-    alias_method :on_render, :on_include
+    alias_method :on_include, :on_render
 
     def on_end
       missing_snippets.each do |theme_file|
@@ -33,5 +40,46 @@ def on_end
     def missing_snippets
       theme.snippets.reject { |t| @used_snippets.include?(t.name) }
     end
+
+    private
+
+    # This function returns true when the render node passed might have a
+    # variable lookup that refers to a block as template_name_expr.
+    #
+    # e.g.
+    #
+    # {% for block in col %}
+    #   {% render block %}
+    # {% endfor %}
+    #
+    # In this case, the `block` variable_lookup in the render tag might be
+    # a Block because col might be an array of blocks.
+    #
+    # @param node [Node]
+    def might_have_a_block_as_variable_lookup?(node)
+      return false unless node.type_name == :render
+
+      return false unless node.value.template_name_expr.is_a?(Liquid::VariableLookup)
+
+      name = node.value.template_name_expr.name
+      return false unless name.is_a?(String)
+
+      # We're going through all the parents of the nodes until we find
+      # a For node with variable_name === to the template_name_expr's name
+      find_parent(node.parent) do |parent_node|
+        next false unless parent_node.type_name == :for
+
+        parent_node.value.variable_name == name
+      end
+    end
+
+    # @param node [Node]
+    def find_parent(node, &pred)
+      return nil unless node
+
+      return node if yield node
+
+      find_parent(node.parent, &pred)
+    end
   end
 end

lib/theme_check/liquid_node.rb

@@ -7,6 +7,7 @@ class LiquidNode < Node
 
     def initialize(value, parent, theme_file)
       raise ArgumentError, "Expected a Liquid AST Node" if value.is_a?(LiquidNode)
+
       @value = value
       @parent = parent
       @theme_file = theme_file
@@ -37,7 +38,9 @@ def children
             node
           end
         end
-        nodes.map { |node| LiquidNode.new(node, self, @theme_file) }
+        nodes
+          .reject(&:nil?) # We don't want nil nodes, and they can happen
+          .map { |node| LiquidNode.new(node, self, @theme_file) }
       end
     end
 
@@ -75,11 +78,13 @@ def outer_markup
 
     def inner_markup
       return '' unless block?
+
       @inner_markup ||= source[block_start_end_index...block_end_start_index]
     end
 
     def inner_json
       return nil unless schema?
+
       @inner_json ||= JSON.parse(inner_markup)
     rescue JSON::ParserError
       # Handled by ValidSchema
@@ -186,13 +191,33 @@ def type_name
 
     def filters
       raise TypeError, "Attempting to lookup filters of #{type_name}. Only variables have filters." unless variable?
+
       @value.filters
     end
 
     def source
       theme_file&.source
     end
 
+    # For debugging purposes, this might be easier for the eyes.
+    def to_h
+      if literal?
+        return @value
+      elsif variable_lookup?
+        return {
+          type_name: type_name,
+          name: value.name.to_s,
+          lookups: children.map(&:to_h),
+        }
+      end
+
+      {
+        type_name: type_name,
+        markup: outer_markup,
+        children: children.map(&:to_h),
+      }
+    end
+
     def block_start_markup
       source[block_start_start_index...block_start_end_index]
     end
@@ -217,11 +242,13 @@ def block_end_markup
 
     def block_end_start_index
       return block_start_end_index unless tag? && block?
+
       @block_end_start_index ||= block_end_match&.begin(0) || block_start_end_index
     end
 
     def block_end_end_index
       return block_end_start_index unless tag? && block?
+
       @block_end_end_index ||= block_end_match&.end(0) || block_start_end_index
     end
 

test/checks/unused_snippet_test.rb

@@ -37,6 +37,38 @@ def test_ignores_dynamic_includes
     assert_offenses("", offenses)
   end
 
+  def test_does_not_turn_off_the_check_because_of_potential_render_block
+    offenses = analyze_theme(
+      ThemeCheck::UnusedSnippet.new,
+      "templates/index.liquid" => <<~END,
+        {% for name in section.blocks %}
+          {% render name %}
+        {% endfor %}
+      END
+      "snippets/unused.liquid" => <<~END,
+        This is not used
+      END
+    )
+    assert_offenses(<<~END, offenses)
+      This snippet is not used at snippets/unused.liquid
+    END
+  end
+
+  def test_does_turn_off_the_check_because_of_dynamic_include_in_for_loop
+    offenses = analyze_theme(
+      ThemeCheck::UnusedSnippet.new,
+      "templates/index.liquid" => <<~END,
+        {% for name in includes %}
+          {% include name %}
+        {% endfor %}
+      END
+      "snippets/unused.liquid" => <<~END,
+        This is not used
+      END
+    )
+    assert_offenses("", offenses)
+  end
+
   def test_removes_unused_snippets
     theme = make_theme(
       "templates/index.liquid" => <<~END,