Use safe_parse_expression instead of parse_expression

- Fixed NoMethod error with .peek (using look instead)
  - Add friendlier error message when {% cycle %}

lib/liquid/parser_switching.rb

@@ -2,9 +2,12 @@
 
 module Liquid
   module ParserSwitching
-    # Do not use this. Use parse_with_selected_parser instead.
-    # It's basically doing the same thing, except this will use strict_parse regardless
-    # of the error mode and fallback only if strict throws.
+    # Do not use this.
+    #
+    # It's basically doing the same thing the {#parse_with_selected_parser},
+    # except this will use the strict parser, instead of the rigid parser.
+    #
+    # @deprecated Use {#parse_with_selected_parser} instead.
     def strict_parse_with_error_mode_fallback(markup)
       strict_parse_with_error_context(markup)
     rescue SyntaxError => e
@@ -26,7 +29,7 @@ module Liquid
       when :lax    then lax_parse(markup)
       when :warn
         begin
-          strict_parse_with_error_context(markup)
+          rigid_parse_with_error_context(markup)
         rescue SyntaxError => e
           parse_context.warnings << e
           lax_parse(markup)
@@ -34,10 +37,14 @@ module Liquid
       end
     end
 
+    def rigid_mode?
+      parse_context.error_mode == :rigid
+    end
+
     private
 
     def rigid_parse_with_error_context(markup)
-      respond_to?(:rigid_parse) ? rigid_parse(markup) : strict_parse(markup)
+      rigid_parse(markup)
     rescue SyntaxError => e
       e.line_number    = line_number
       e.markup_context = markup_context(markup)

lib/liquid/tags/case.rb

@@ -31,12 +31,7 @@ module Liquid
     def initialize(tag_name, markup, options)
       super
       @blocks = []
-
-      if markup =~ Syntax
-        @left = parse_expression(Regexp.last_match(1))
-      else
-        raise SyntaxError, options[:locale].t("errors.syntax.case")
-      end
+      parse_with_selected_parser(markup)
     end
 
     def parse(tokens)
@@ -91,9 +86,51 @@ module Liquid
 
     private
 
+    def rigid_parse(markup)
+      parser = @parse_context.new_parser(markup)
+      @left = safe_parse_expression(parser)
+      parser.consume(:end_of_string)
+    end
+
+    def strict_parse(markup)
+      lax_parse(markup)
+    end
+
+    def lax_parse(markup)
+      if markup =~ Syntax
+        @left = parse_expression(Regexp.last_match(1))
+      else
+        raise SyntaxError, options[:locale].t("errors.syntax.case")
+      end
+    end
+
     def record_when_condition(markup)
       body = new_body
 
+      if rigid_mode?
+        parse_rigid_when(markup, body)
+      else
+        parse_lax_when(markup, body)
+      end
+    end
+
+    def parse_rigid_when(markup, body)
+      parser = @parse_context.new_parser(markup)
+
+      loop do
+        expr = safe_parse_expression(parser)
+        block = Condition.new(@left, '==', expr)
+        block.attach(body)
+        @blocks << block
+
+        # Temporarily until support :or lexeme.
+        break unless parser.id?('or') || parser.consume?(:comma)
+      end
+
+      parser.consume(:end_of_string)
+    end
+
+    def parse_lax_when(markup, body)
       while markup
         unless markup =~ WhenSyntax
           raise SyntaxError, options[:locale].t("errors.syntax.case_invalid_when")

lib/liquid/tags/cycle.rb

@@ -58,19 +58,27 @@ module Liquid
     def rigid_parse(markup)
       p = @parse_context.new_parser(markup)
 
-      if p.look(:id) && p.peek(1) == :colon
+      if p.look(:id) && p.look(:colon, 1)
         @name = p.consume(:id)
         @is_named = true
         p.consume(:colon)
       end
 
       @variables = []
-      while (var = p.expression)
+
+      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)
       end
 
-      raise_syntax_error(options) if @variables.empty?
+      p.consume(:end_of_string)
+
+      unless @is_named
+        @name = @variables.to_s
+        @is_named = !@name.match?(/\w+:0x\h{8}/)
+      end
     end
 
     # Temporarily until we migrate

lib/liquid/tags/for.rb

@@ -111,6 +111,10 @@ module Liquid
 
     private
 
+    def rigid_parse(markup)
+      strict_parse(markup)
+    end
+
     def collection_segment(context)
       offsets = context.registers[:for] ||= {}
 

lib/liquid/tags/if.rb

@@ -66,6 +66,10 @@ module Liquid
 
     private
 
+    def rigid_parse(markup)
+      strict_parse(markup)
+    end
+
     def push_block(tag, markup)
       block = if tag == 'else'
         ElseCondition.new

test/integration/tags/cycle_tag_test.rb

@@ -46,19 +46,70 @@ class CycleTagTest < Minitest::Test
     assert_template_result("11", template)
   end
 
+  def test_cycle_tag_without_arguments
+    error = assert_raises(Liquid::SyntaxError) do
+      Template.parse("{% cycle %}")
+    end
+
+    assert_match(/Syntax Error in 'cycle' - Valid syntax: cycle \[name :\] var/, error.message)
+  end
+
   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' %}"
+
     [:lax, :strict].each do |mode|
       with_error_mode(mode) do
-        assert_template_result("a", "{% cycle .5: 'a', 'b' %}")
-        assert_template_result("b", "{% assign 5 = 'b' %}{% cycle .5, .4 %}")
+        assert_template_result("b", temlate1)
+        assert_template_result("a", temlate2)
       end
     end
 
-    skip("todo(guilherme): parse_context.safe_parse_expression in progress...")
-    # with_error_mode(:rigid) do
-    #   assert_raises(Liquid::SyntaxError) { Template.parse("{% cycle .5: 'a', 'b' %}") }
-    #   assert_raises(Liquid::SyntaxError) { Template.parse("{% cycle .5, .4 %}") }
-    # end
+    with_error_mode(:rigid) do
+      error1 = assert_raises(Liquid::SyntaxError) { Template.parse(temlate1) }
+      error2 = assert_raises(Liquid::SyntaxError) { Template.parse(temlate2) }
+
+      expected_error = /Liquid syntax error: \[:dot, "."\] is not a valid expression/
+
+      assert_match(expected_error, error1.message)
+      assert_match(expected_error, error2.message)
+    end
+  end
+
+  def test_cycle_with_trailing_elements
+    assignments = "{% assign a = 'A' %}{% assign n = 'N' %}"
+
+    template1 = "#{assignments}{% cycle       'a'  'b', 'c' %}"
+    template2 = "#{assignments}{% cycle name: 'a'  'b', 'c' %}"
+    template3 = "#{assignments}{% cycle name: 'a', 'b'  'c' %}"
+    template4 = "#{assignments}{% cycle n  e: 'a', 'b', 'c' %}"
+    template5 = "#{assignments}{% cycle n  e  'a', 'b', 'c' %}"
+
+    [:lax, :strict].each do |mode|
+      with_error_mode(mode) do
+        assert_template_result("a", template1)
+        assert_template_result("a", template2)
+        assert_template_result("a", template3)
+        assert_template_result("N", template4)
+        assert_template_result("N", template5)
+      end
+    end
+
+    with_error_mode(:rigid) do
+      error1 = assert_raises(Liquid::SyntaxError) { Template.parse(template1) }
+      error2 = assert_raises(Liquid::SyntaxError) { Template.parse(template2) }
+      error3 = assert_raises(Liquid::SyntaxError) { Template.parse(template3) }
+      error4 = assert_raises(Liquid::SyntaxError) { Template.parse(template4) }
+      error5 = assert_raises(Liquid::SyntaxError) { Template.parse(template5) }
+
+      expected_error = /Expected end_of_string but found/
+
+      assert_match(expected_error, error1.message)
+      assert_match(expected_error, error2.message)
+      assert_match(expected_error, error3.message)
+      assert_match(expected_error, error4.message)
+      assert_match(expected_error, error5.message)
+    end
   end
 end

test/unit/rigid_mode_unit_test.rb

@@ -5,11 +5,9 @@ require 'test_helper'
 class RigidModeUnitTest < Minitest::Test
   include Liquid
 
-  def setup
-    skip("todo(guilherme): parse_context.safe_parse_expression in progress...")
-  end
-
   def test_direct_parse_expression_comparison
+    skip("todo(guilherme): parse_context.safe_parse_expression in progress...")
+
     test_cases = [
       'foo bar',
       'user.name first',
@@ -32,6 +30,8 @@ class RigidModeUnitTest < Minitest::Test
   end
 
   def test_comparison_strict_vs_rigid_with_space_separated_lookups
+    skip("todo(guilherme): parse_context.safe_parse_expression in progress...")
+
     expr = 'product title'
 
     ctx_lax = ParseContext.new(environment: lax_env)
@@ -51,6 +51,8 @@ class RigidModeUnitTest < Minitest::Test
   end
 
   def test_tablerow_limit_with_invalid_expression
+    skip("todo(guilherme): parse_context.safe_parse_expression in progress...")
+
     template = <<~LIQUID
       {% tablerow i in (1..10) limit: foo=>bar %}{{ i }}{% endtablerow %}
     LIQUID
@@ -64,6 +66,8 @@ class RigidModeUnitTest < Minitest::Test
   end
 
   def test_tablerow_offset_with_invalid_expression
+    skip("todo(guilherme): parse_context.safe_parse_expression in progress...")
+
     template = <<~LIQUID
       {% tablerow i in (1..10) offset: foo=>bar %}{{ i }}{% endtablerow %}
     LIQUID

test/unit/tags/case_tag_unit_test.rb

@@ -9,4 +9,124 @@ class CaseTagUnitTest < Minitest::Test
     template = Liquid::Template.parse('{% case var %}{% when true %}WHEN{% else %}ELSE{% endcase %}')
     assert_equal(['WHEN', 'ELSE'], template.root.nodelist[0].nodelist.map(&:nodelist).flatten)
   end
+
+  def test_case_with_trailing_element
+    template = <<~LIQUID
+      {%- case 1 bar -%}
+        {%- when 1 -%}
+          one
+        {%- else -%}
+          two
+      {%- endcase -%}
+    LIQUID
+
+    [:lax, :strict].each do |mode|
+      with_error_mode(mode) { assert_template_result("one", template) }
+    end
+
+    with_error_mode(:rigid) do
+      error = assert_raises(Liquid::SyntaxError) { Template.parse(template) }
+
+      assert_match(/Expected end_of_string but found/, error.message)
+    end
+  end
+
+  def test_case_when_trailing_element
+    template = <<~LIQUID
+      {%- case 1 -%}
+        {%- when 1 bar -%}
+          one
+        {%- else -%}
+          two
+      {%- endcase -%}
+    LIQUID
+
+    [:lax, :strict].each do |mode|
+      with_error_mode(mode) { assert_template_result("one", template) }
+    end
+
+    with_error_mode(:rigid) do
+      error = assert_raises(Liquid::SyntaxError) { Template.parse(template) }
+
+      assert_match(/Expected end_of_string but found/, error.message)
+    end
+  end
+
+  def test_case_when_with_comma
+    template = <<~LIQUID
+      {%- case 1 -%}
+        {%- when 2, 1 -%}
+          one
+        {%- else -%}
+          two
+      {%- endcase -%}
+    LIQUID
+
+    [:lax, :strict, :rigid].each do |mode|
+      with_error_mode(mode) { assert_template_result("one", template) }
+    end
+  end
+
+  def test_case_when_with_or
+    template = <<~LIQUID
+      {%- case 1 -%}
+        {%- when 2 or 1 -%}
+          one
+        {%- else -%}
+          two
+      {%- endcase -%}
+    LIQUID
+
+    [:lax, :strict, :rigid].each do |mode|
+      with_error_mode(mode) { assert_template_result("one", template) }
+    end
+  end
+
+  def test_case_with_invalid_expression
+    template = <<~LIQUID
+      {%- case foo=>bar -%}
+        {%- when 'baz' -%}
+          one
+        {%- else -%}
+          two
+      {%- endcase -%}
+    LIQUID
+    assigns = { 'foo' => { 'bar' => 'baz' } }
+
+    [:lax, :strict].each do |mode|
+      with_error_mode(mode) do
+        assert_template_result("one", template, assigns)
+      end
+    end
+
+    with_error_mode(:rigid) do
+      error = assert_raises(Liquid::SyntaxError) { Template.parse(template) }
+
+      assert_match(/Unexpected character =/, error.message)
+    end
+  end
+
+  def test_case_when_with_invalid_expression
+    template = <<~LIQUID
+      {%- case 'baz' -%}
+        {%- when foo=>bar -%}
+          one
+        {%- else -%}
+          two
+      {%- endcase -%}
+    LIQUID
+    assigns = { 'foo' => { 'bar' => 'baz' } }
+
+    [:lax, :strict].each do |mode|
+      with_error_mode(mode) do
+        assert_template_result("one", template, assigns)
+      end
+    end
+
+    with_error_mode(:rigid) do
+      error = assert_raises(Liquid::SyntaxError) { Template.parse(template) }
+
+      assert_match(/Unexpected character =/, error.message)
+    end
+  end
 end