Fix cycle tags with variable arguments

lib/liquid/tags/cycle.rb

@@ -17,7 +17,6 @@ module Liquid
   class Cycle < Tag
     SimpleSyntax = /\A#{QuotedFragment}+/o
     NamedSyntax  = /\A(#{QuotedFragment})\s*\:\s*(.*)/om
-    UNNAMED_CYCLE_PATTERN = /\w+:0x\h{8}/
 
     attr_reader :variables
 
@@ -86,8 +85,8 @@ def strict2_parse(markup)
       p.consume(:end_of_string)
 
       unless @is_named
-        @name = @variables.to_s
-        @is_named = !@name.match?(UNNAMED_CYCLE_PATTERN)
+        @name = cycle_key
+        @is_named = @variables.none? { |variable| variable.is_a?(VariableLookup) }
       end
     end
 
@@ -103,8 +102,8 @@ def lax_parse(markup)
         @is_named = true
       when SimpleSyntax
         @variables = variables_from_string(markup)
-        @name      = @variables.to_s
-        @is_named = !@name.match?(UNNAMED_CYCLE_PATTERN)
+        @name      = cycle_key
+        @is_named = @variables.none? { |variable| variable.is_a?(VariableLookup) }
       else
         raise SyntaxError, options[:locale].t("errors.syntax.cycle")
       end
@@ -120,13 +119,14 @@ def variables_from_string(markup)
       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.
+    # Build an unnamed cycle key from lookup paths instead of object identity so
+    # repeated cycle tags using the same variables share their counter.
+    def cycle_key
+      @variables.map do |variable|
+        variable.is_a?(VariableLookup) ? [variable.name, variable.lookups] : variable
+      end.to_s
+    end
+
     def maybe_dup_lookup(var)
       var.is_a?(VariableLookup) ? var.dup : var
     end

test/integration/tags/cycle_tag_test.rb

@@ -50,7 +50,7 @@ def test_cycle_named_groups_vlookup
     assert_template_result("112233", template)
   end
 
-  def test_unnamed_cycle_have_independent_counters_when_used_with_lookups
+  def test_unnamed_cycle_share_counters_when_used_with_lookups
     template = <<~LIQUID
       {%- assign a = "1" -%}
       {%- for i in (1..3) -%}
@@ -59,7 +59,12 @@ def test_unnamed_cycle_have_independent_counters_when_used_with_lookups
       {%- endfor -%}
     LIQUID
 
-    assert_template_result("112211", template)
+    assert_template_result("121212", template)
+  end
+
+  def test_unnamed_cycle_advances_when_first_value_is_a_lookup
+    assert_template_result("b", "{% cycle a, 'b' %}{% cycle a, 'b' %}")
+    assert_template_result("ab", "{% assign a = 'a' %}{% cycle a, 'b' %}{% cycle a, 'b' %}")
   end
 
   def test_unnamed_cycle_dependent_counter_when_used_with_literal_values