Fixup cycle rigid parsing to be backwards compatible

Author: charlespwd

lib/liquid/tags/cycle.rb

@@ -58,19 +58,27 @@ def render_to_output_buffer(context, output)
     def rigid_parse(markup)
       p = @parse_context.new_parser(markup)
 
-      if p.look(:id) && p.look(:colon, 1)
-        @name = p.consume(:id)
-        @is_named = true
-        p.consume(:colon)
-      end
-
       @variables = []
 
       raise SyntaxError, options[:locale].t("errors.syntax.cycle") if p.look(:end_of_string)
 
-      while (var = safe_parse_expression(p))
-        @variables << var
-        break unless p.consume?(:comma)
+      first_expression = safe_parse_expression(p)
+      if p.look(:colon)
+        # cycle name: expr1, expr2, ...
+        @name = first_expression
+        @is_named = true
+        p.consume(:colon)
+        # After the colon, parse the first variable (required for named cycles)
+        @variables << maybe_dup_lookup(safe_parse_expression(p))
+      else
+        # cycle expr1, expr2, ...
+        @variables << maybe_dup_lookup(first_expression)
+      end
+
+      # Parse remaining comma-separated expressions
+      while p.consume?(:comma)
+        break if p.look(:end_of_string)
+        @variables << maybe_dup_lookup(safe_parse_expression(p))
       end
 
       p.consume(:end_of_string)
@@ -106,14 +114,22 @@ def variables_from_string(markup)
         var =~ /\s*(#{QuotedFragment})\s*/o
         next unless Regexp.last_match(1)
 
-        # Expression Parser returns cached objects, and we need to dup them to
-        # start the cycle over for each new cycle call.
-        # Liquid-C does not have a cache, so we don't need to dup the object.
         var = parse_expression(Regexp.last_match(1))
-        var.is_a?(VariableLookup) ? var.dup : var
+        maybe_dup_lookup(var)
       end.compact
     end
 
+    # For backwards compatibility, whenever a lookup is used in an unnamed cycle,
+    # we make it so that the @variables.to_s produces different strings for cycles
+    # called with the same arguments (since @variables.to_s is used as the cycle counter key)
+    # This makes it so {% cycle a, b %} and {% cycle a, b %} have independent counters even if a and b share value.
+    # This is not true for literal values, {% cycle "a", "b" %} and {% cycle "a", "b" %} share the same counter.
+    # I was really scratching my head about this one, but migrating away from this would be more headache
+    # than it's worth. So we're keeping this quirk for now.
+    def maybe_dup_lookup(var)
+      var.is_a?(VariableLookup) ? var.dup : var
+    end
+
     class ParseTreeVisitor < Liquid::ParseTreeVisitor
       def children
         Array(@node.variables)

test/integration/tags/cycle_tag_test.rb

@@ -3,20 +3,11 @@
 require 'test_helper'
 
 class CycleTagTest < Minitest::Test
-  def test_simple_cycle
-    template = <<~LIQUID
-      {%- cycle '1', '2', '3' -%}
-      {%- cycle '1', '2', '3' -%}
-      {%- cycle '1', '2', '3' -%}
-    LIQUID
-
-    assert_template_result("123", template)
-  end
 
   def test_simple_cycle_inside_for_loop
     template = <<~LIQUID
       {%- for i in (1..3) -%}
-        {% cycle '1', '2', '3' %}
+        {%- cycle '1', '2', '3' -%}
       {%- endfor -%}
     LIQUID
 
@@ -36,14 +27,61 @@ def test_cycle_with_variables_inside_for_loop
     assert_template_result("123", template)
   end
 
-  def test_cycle_tag_always_resets_cycle
+  def test_cycle_named_groups_string
+    template = <<~LIQUID
+      {%- for i in (1..3) -%}
+        {%- cycle 'placeholder1': 1, 2, 3 -%}
+        {%- cycle 'placeholder2': 1, 2, 3 -%}
+      {%- endfor -%}
+    LIQUID
+
+    assert_template_result("112233", template)
+  end
+
+  def test_cycle_named_groups_vlookup
+    template = <<~LIQUID
+      {%- assign placeholder1 = 'placeholder1' -%}
+      {%- assign placeholder2 = 'placeholder2' -%}
+      {%- for i in (1..3) -%}
+        {%- cycle placeholder1: 1, 2, 3 -%}
+        {%- cycle placeholder2: 1, 2, 3 -%}
+      {%- endfor -%}
+    LIQUID
+
+    assert_template_result("112233", template)
+  end
+
+  def test_unnamed_cycle_have_independent_counters_when_used_with_lookups
     template = <<~LIQUID
       {%- assign a = "1" -%}
-      {%- cycle a, "2" -%}
-      {%- cycle a, "2" -%}
+      {%- for i in (1..3) -%}
+        {%- cycle a, "2" -%}
+        {%- cycle a, "2" -%}
+      {%- endfor -%}
+    LIQUID
+
+    assert_template_result("112211", template)
+  end
+
+  def test_unnamed_cycle_dependent_counter_when_used_with_literal_values
+    template = <<~LIQUID
+      {%- cycle "1", "2" -%}
+      {%- cycle "1", "2" -%}
+      {%- cycle "1", "2" -%}
     LIQUID
 
-    assert_template_result("11", template)
+    assert_template_result("121", template)
+  end
+
+  def test_optional_trailing_comma
+    template = <<~LIQUID
+      {%- cycle "1", "2", -%}
+      {%- cycle "1", "2", -%}
+      {%- cycle "1", "2", -%}
+      {%- cycle "1", -%}
+    LIQUID
+
+    assert_template_result("1211", template)
   end
 
   def test_cycle_tag_without_arguments
@@ -56,19 +94,19 @@ def test_cycle_tag_without_arguments
 
   def test_cycle_tag_with_error_mode
     # QuotedFragment is more permissive than what Parser#expression allows.
-    temlate1 = "{% assign 5 = 'b' %}{% cycle .5, .4 %}"
-    temlate2 = "{% cycle .5: 'a', 'b' %}"
+    template1 = "{% assign 5 = 'b' %}{% cycle .5, .4 %}"
+    template2 = "{% cycle .5: 'a', 'b' %}"
 
     [:lax, :strict].each do |mode|
       with_error_mode(mode) do
-        assert_template_result("b", temlate1)
-        assert_template_result("a", temlate2)
+        assert_template_result("b", template1)
+        assert_template_result("a", template2)
       end
     end
 
     with_error_mode(:rigid) do
-      error1 = assert_raises(Liquid::SyntaxError) { Template.parse(temlate1) }
-      error2 = assert_raises(Liquid::SyntaxError) { Template.parse(temlate2) }
+      error1 = assert_raises(Liquid::SyntaxError) { Template.parse(template1) }
+      error2 = assert_raises(Liquid::SyntaxError) { Template.parse(template2) }
 
       expected_error = /Liquid syntax error: \[:dot, "."\] is not a valid expression/