Stricter 1:1 refactor of strict_parse for Variable

Author: charlespwd

lib/liquid/parser_switching.rb

@@ -5,11 +5,17 @@ module ParserSwitching
     # 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.
+    # except this will try the strict parser regardless of the error mode,
+    # and fall back to the lax parser if the error mode is lax or warn.
     #
     # @deprecated Use {#parse_with_selected_parser} instead.
     def strict_parse_with_error_mode_fallback(markup)
-      strict_parse_with_error_context(markup)
+      case parse_context.error_mode
+      when :rigid
+        rigid_parse_with_error_context(markup)
+      else
+        strict_parse_with_error_context(markup)
+      end
     rescue SyntaxError => e
       case parse_context.error_mode
       when :rigid

lib/liquid/variable.rb

@@ -54,7 +54,7 @@ def lax_parse(markup)
           next unless f =~ /\w+/
           filtername = Regexp.last_match(0)
           filterargs = f.scan(FilterArgsRegex).flatten
-          @filters << parse_filter_expressions(filtername, filterargs)
+          @filters << lax_parse_filter_expressions(filtername, filterargs)
         end
       end
     end
@@ -66,14 +66,14 @@ def strict_parse(markup)
       return if p.look(:end_of_string)
 
       @name = parse_context.safe_parse_expression(p)
-      while p.consume?(:pipe)
-        filtername = p.consume(:id)
-        filterargs = p.consume?(:colon) ? parse_filterargs(p) : Const::EMPTY_ARRAY
-        @filters << parse_filter_expressions(filtername, filterargs, safe: true)
-      end
+      @filters << strict_parse_filter_expressions(p) while p.consume?(:pipe)
       p.consume(:end_of_string)
     end
 
+    def rigid_parse(markup)
+      strict_parse(markup)
+    end
+
     def parse_filterargs(p)
       # first argument
       filterargs = [p.argument]
@@ -122,22 +122,62 @@ def disabled_tags
 
     private
 
-    def parse_filter_expressions(filter_name, unparsed_args, safe: false)
+    def lax_parse_filter_expressions(filter_name, unparsed_args)
       filter_args  = []
       keyword_args = nil
       unparsed_args.each do |a|
-        if (matches = a.match(JustTagAttributes)) # we'll need to fix this
+        if (matches = a.match(JustTagAttributes))
           keyword_args           ||= {}
-          keyword_args[matches[1]] = parse_context.parse_expression(matches[2], safe: false)
+          keyword_args[matches[1]] = parse_context.parse_expression(matches[2])
         else
-          filter_args << parse_context.parse_expression(a, safe: safe)
+          filter_args << parse_context.parse_expression(a)
         end
       end
       result = [filter_name, filter_args]
       result << keyword_args if keyword_args
       result
     end
 
+    # Surprisingly, positional and keyword arguments can be mixed.
+    #
+    # filter = filtername [":" filterargs?]
+    # filterargs = argument ("," argument)*
+    # argument = (positional_argument | keyword_argument)
+    # positional_argument = expression
+    # keyword_argument = id ":" expression
+    def strict_parse_filter_expressions(p)
+      filtername = p.consume(:id)
+      filter_args = []
+      keyword_args = {}
+
+      if p.consume?(:colon)
+        # Parse first argument (no leading comma)
+        argument(p, filter_args, keyword_args) unless end_of_arguments?(p)
+
+        # Parse remaining arguments (with leading commas) and optional trailing comma
+        argument(p, filter_args, keyword_args) while p.consume?(:comma) && !end_of_arguments?(p)
+      end
+
+      result = [filtername, filter_args]
+      result << keyword_args unless keyword_args.empty?
+      result
+    end
+
+    def argument(p, positional_arguments, keyword_arguments)
+      if p.look(:id) && p.look(:colon, 1)
+        key = p.consume(:id)
+        p.consume(:colon)
+        value = parse_context.safe_parse_expression(p)
+        keyword_arguments[key] = value
+      else
+        positional_arguments << parse_context.safe_parse_expression(p)
+      end
+    end
+
+    def end_of_arguments?(p)
+      p.look(:pipe) || p.look(:end_of_string)
+    end
+
     def evaluate_filter_expressions(context, filter_args, filter_kwargs)
       parsed_args = filter_args.map { |expr| context.evaluate(expr) }
       if filter_kwargs

test/unit/variable_unit_test.rb

@@ -135,14 +135,52 @@ def test_lax_filter_argument_parsing
     var = create_variable(%( number_of_comments | pluralize: 'comment': 'comments' ), error_mode: :lax)
     assert_equal(VariableLookup.new('number_of_comments'), var.name)
     assert_equal([['pluralize', ['comment', 'comments']]], var.filters)
+
+    # missing does not throws error
+    create_variable(%(n | f1: ,), error_mode: :lax)
+    create_variable(%(n | f1: ,| f2), error_mode: :lax)
+
+    # arg does not require colon, but ignores args :O, also ignores first kwarg since it splits on ':'
+    var = create_variable(%(n | f1 1 | f2 k1: v1), error_mode: :lax)
+    assert_equal([['f1', []], ['f2', [VariableLookup.new('v1')]]], var.filters)
+
+    # positional and kwargs parsing
+    var = create_variable(%(n | filter: 1, 2, 3 | filter2: k1: 1, k2: 2), error_mode: :lax)
+    assert_equal([['filter', [1, 2, 3]], ['filter2', [], { "k1" => 1, "k2" => 2 }]], var.filters)
+
+    # positional and kwargs intermixed (pos1, key1: val1, pos2)
+    var = create_variable(%(n | link_to: class: "black", "https://example.com", title: "title"), error_mode: :lax)
+    assert_equal([['link_to', ["https://example.com"], { "class" => "black", "title" => "title" }]], var.filters)
   end
 
   def test_strict_filter_argument_parsing
-    with_error_mode(:strict) do
-      assert_raises(SyntaxError) do
-        create_variable(%( number_of_comments | pluralize: 'comment': 'comments' ))
-      end
-    end
+    # optional colon
+    var = create_variable(%(n | f1 | f2:), error_mode: :strict)
+    assert_equal([['f1', []], ['f2', []]], var.filters)
+
+    # missing argument throws error
+    assert_raises(SyntaxError) { create_variable(%(n | f1: ,), error_mode: :strict) }
+    assert_raises(SyntaxError) { create_variable(%(n | f1: ,| f2), error_mode: :strict) }
+
+    # arg requires colon
+    assert_raises(SyntaxError) { create_variable(%(n | f1 1), error_mode: :strict) }
+
+    # trailing comma doesn't throw
+    create_variable(%(n | f1: 1, 2, 3, | f2:), error_mode: :strict)
+
+    # missing comma throws error
+    assert_raises(SyntaxError) { create_variable(%(n | filter: 1 2, 3), error_mode: :strict) }
+
+    # positional and kwargs parsing
+    var = create_variable(%(n | filter: 1, 2, 3 | filter2: k1: 1, k2: 2), error_mode: :strict)
+    assert_equal([['filter', [1, 2, 3]], ['filter2', [], { "k1" => 1, "k2" => 2 }]], var.filters)
+
+    # positional and kwargs intermixed (pos1, key1: val1, pos2)
+    var = create_variable(%(n | link_to: class: "black", "https://example.com", title: "title"), error_mode: :strict)
+    assert_equal([['link_to', ["https://example.com"], { "class" => "black", "title" => "title" }]], var.filters)
+
+    # string key throws
+    assert_raises(SyntaxError) { create_variable(%(n | pluralize: 'comment': 'comments'), error_mode: :strict) }
   end
 
   def test_output_raw_source_of_variable