Move safe_parse_expression into Parser.expression_node

- Make `Parser` accept the expression cache
- Remove `safe_parse_expression` from `ParseContext`
- Replace all usage of `safe_parse_expression` with `parser.expression_node`

History.md

@@ -28,10 +28,12 @@
   * `:strict` and `strict_parse` is no longer supported
   * `strict2_parse` is renamed to `parse_markup`
 * The `warnings` system has been removed.
+* `safe_parse_expression` has been moved to `Parser.expression_node`
 
 ### Migrating from `^5.11.0`
 - In custom tags that include `ParserSwitching`, rename `strict2_parse` to `parse_markup`
 - Remove code depending on `:error_mode`
+- Replace `safe_parse_expression` calls with `Parser.expression_node`
 
 ## 5.11.0
 * Revert the Inline Snippets tag (#2001), treat its inclusion in the latest Liquid release as a bug, and allow for feedback on RFC#1916 to better support Liquid developers [Guilherme Carreiro]

lib/liquid/parse_context.rb

@@ -37,7 +37,7 @@ module Liquid
 
     def new_parser(input)
       @string_scanner.string = input
-      Parser.new(@string_scanner)
+      Parser.new(@string_scanner, @expression_cache)
     end
 
     def new_tokenizer(source, start_line_number: nil, for_liquid_tag: false)
@@ -49,10 +49,6 @@ module Liquid
       )
     end
 
-    def safe_parse_expression(parser)
-      Expression.safe_parse(parser, @string_scanner, @expression_cache)
-    end
-
     def parse_expression(markup, safe: false)
       # markup MUST come from a string returned by the parser
       # (e.g., parser.expression). We're not calling the parser here to

lib/liquid/parser.rb

@@ -2,9 +2,10 @@
 
 module Liquid
   class Parser
-    def initialize(input)
-      ss = input.is_a?(StringScanner) ? input : StringScanner.new(input)
-      @tokens = Lexer.tokenize(ss)
+    def initialize(input, expression_cache = nil)
+      @ss = input.is_a?(StringScanner) ? input : StringScanner.new(input)
+      @cache = expression_cache
+      @tokens = Lexer.tokenize(@ss)
       @p      = 0 # pointer to current location
     end
 
@@ -71,6 +72,11 @@ module Liquid
       end
     end
 
+    def expression_node
+      expr = expression
+      Expression.parse(expr, @ss, @cache)
+    end
+
     def argument
       str = +""
       # might be a keyword argument (identifier: expression)

lib/liquid/tag.rb

@@ -68,10 +68,6 @@ module Liquid
 
     private
 
-    def safe_parse_expression(parser)
-      parse_context.safe_parse_expression(parser)
-    end
-
     def parse_expression(markup, safe: false)
       parse_context.parse_expression(markup, safe: safe)
     end

lib/liquid/tags/case.rb

@@ -85,7 +85,7 @@ module Liquid
 
     def parse_markup(markup)
       parser = @parse_context.new_parser(markup)
-      @left = safe_parse_expression(parser)
+      @left = parser.expression_node
       parser.consume(:end_of_string)
     end
 
@@ -99,7 +99,7 @@ module Liquid
       parser = @parse_context.new_parser(markup)
 
       loop do
-        expr = Condition.parse_expression(parse_context, parser.expression, safe: true)
+        expr = parser.expression_node
         block = Condition.new(@left, '==', expr)
         block.attach(body)
         @blocks << block

lib/liquid/tags/cycle.rb

@@ -61,14 +61,14 @@ module Liquid
 
       raise SyntaxError, options[:locale].t("errors.syntax.cycle") if p.look(:end_of_string)
 
-      first_expression = safe_parse_expression(p)
+      first_expression = p.expression_node
       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))
+        @variables << maybe_dup_lookup(p.expression_node)
       else
         # cycle expr1, expr2, ...
         @variables << maybe_dup_lookup(first_expression)
@@ -78,7 +78,7 @@ module Liquid
       while p.consume?(:comma)
         break if p.look(:end_of_string)
 
-        @variables << maybe_dup_lookup(safe_parse_expression(p))
+        @variables << maybe_dup_lookup(p.expression_node)
       end
 
       p.consume(:end_of_string)

lib/liquid/tags/include.rb

@@ -84,8 +84,8 @@ module Liquid
     def parse_markup(markup)
       p = @parse_context.new_parser(markup)
 
-      @template_name_expr = safe_parse_expression(p)
-      @variable_name_expr = safe_parse_expression(p) if p.id?("for") || p.id?("with")
+      @template_name_expr = p.expression_node
+      @variable_name_expr = p.expression_node if p.id?("for") || p.id?("with")
       @alias_name         = p.consume(:id) if p.id?("as")
 
       p.consume?(:comma)
@@ -94,7 +94,7 @@ module Liquid
       while p.look(:id)
         key = p.consume
         p.consume(:colon)
-        @attributes[key] = safe_parse_expression(p)
+        @attributes[key] = p.expression_node
         p.consume?(:comma)
       end
 

lib/liquid/tags/render.rb

@@ -89,7 +89,7 @@ module Liquid
 
       @template_name_expr = parse_expression(template_name(p), safe: true)
       with_or_for         = p.id?("for") || p.id?("with")
-      @variable_name_expr = safe_parse_expression(p) if with_or_for
+      @variable_name_expr = p.expression_node if with_or_for
       @alias_name         = p.consume(:id) if p.id?("as")
       @is_for_loop        = (with_or_for == FOR)
 
@@ -99,7 +99,7 @@ module Liquid
       while p.look(:id)
         key = p.consume
         p.consume(:colon)
-        @attributes[key] = safe_parse_expression(p)
+        @attributes[key] = p.expression_node
         p.consume?(:comma)
       end
 

lib/liquid/tags/table_row.rb

@@ -42,7 +42,7 @@ module Liquid
         raise SyntaxError, options[:locale].t("errors.syntax.for_invalid_in")
       end
 
-      @collection_name = safe_parse_expression(p)
+      @collection_name = p.expression_node
 
       p.consume?(:comma)
 
@@ -54,7 +54,7 @@ module Liquid
         end
 
         p.consume(:colon)
-        @attributes[key] = safe_parse_expression(p)
+        @attributes[key] = p.expression_node
         p.consume?(:comma)
       end
 

lib/liquid/variable.rb

@@ -47,7 +47,7 @@ module Liquid
 
       return if p.look(:end_of_string)
 
-      @name = parse_context.safe_parse_expression(p)
+      @name = p.expression_node
       @filters << parse_filter_expressions(p) while p.consume?(:pipe)
       p.consume(:end_of_string)
     end
@@ -121,10 +121,10 @@ module Liquid
       if p.look(:id) && p.look(:colon, 1)
         key = p.consume(:id)
         p.consume(:colon)
-        value = parse_context.safe_parse_expression(p)
+        value = p.expression_node
         keyword_arguments[key] = value
       else
-        positional_arguments << parse_context.safe_parse_expression(p)
+        positional_arguments << p.expression_node
       end
     end
 

test/unit/condition_unit_test.rb

@@ -206,7 +206,7 @@ class ConditionUnitTest < Minitest::Test
     @context['whitespace'] = '   '
     blank_literal = Condition.class_variable_get(:@@method_literals)['blank']
 
-    assert_evaluates_true(VariableLookup.new('whitespace'), '==', blank_literal)
+    assert_evaluates_true(VariableLookup.parse('whitespace'), '==', blank_literal)
   end
 
   def test_blank_with_empty_string
@@ -215,7 +215,7 @@ class ConditionUnitTest < Minitest::Test
     @context['empty_string'] = ''
     blank_literal = Condition.class_variable_get(:@@method_literals)['blank']
 
-    assert_evaluates_true(VariableLookup.new('empty_string'), '==', blank_literal)
+    assert_evaluates_true(VariableLookup.parse('empty_string'), '==', blank_literal)
   end
 
   def test_blank_with_empty_array
@@ -224,7 +224,7 @@ class ConditionUnitTest < Minitest::Test
     @context['empty_array'] = []
     blank_literal = Condition.class_variable_get(:@@method_literals)['blank']
 
-    assert_evaluates_true(VariableLookup.new('empty_array'), '==', blank_literal)
+    assert_evaluates_true(VariableLookup.parse('empty_array'), '==', blank_literal)
   end
 
   def test_blank_with_empty_hash
@@ -233,7 +233,7 @@ class ConditionUnitTest < Minitest::Test
     @context['empty_hash'] = {}
     blank_literal = Condition.class_variable_get(:@@method_literals)['blank']
 
-    assert_evaluates_true(VariableLookup.new('empty_hash'), '==', blank_literal)
+    assert_evaluates_true(VariableLookup.parse('empty_hash'), '==', blank_literal)
   end
 
   def test_blank_with_nil
@@ -242,7 +242,7 @@ class ConditionUnitTest < Minitest::Test
     @context['nil_value'] = nil
     blank_literal = Condition.class_variable_get(:@@method_literals)['blank']
 
-    assert_evaluates_true(VariableLookup.new('nil_value'), '==', blank_literal)
+    assert_evaluates_true(VariableLookup.parse('nil_value'), '==', blank_literal)
   end
 
   def test_blank_with_false
@@ -251,7 +251,7 @@ class ConditionUnitTest < Minitest::Test
     @context['false_value'] = false
     blank_literal = Condition.class_variable_get(:@@method_literals)['blank']
 
-    assert_evaluates_true(VariableLookup.new('false_value'), '==', blank_literal)
+    assert_evaluates_true(VariableLookup.parse('false_value'), '==', blank_literal)
   end
 
   def test_not_blank_with_true
@@ -260,7 +260,7 @@ class ConditionUnitTest < Minitest::Test
     @context['true_value'] = true
     blank_literal = Condition.class_variable_get(:@@method_literals)['blank']
 
-    assert_evaluates_false(VariableLookup.new('true_value'), '==', blank_literal)
+    assert_evaluates_false(VariableLookup.parse('true_value'), '==', blank_literal)
   end
 
   def test_not_blank_with_number
@@ -269,7 +269,7 @@ class ConditionUnitTest < Minitest::Test
     @context['number'] = 42
     blank_literal = Condition.class_variable_get(:@@method_literals)['blank']
 
-    assert_evaluates_false(VariableLookup.new('number'), '==', blank_literal)
+    assert_evaluates_false(VariableLookup.parse('number'), '==', blank_literal)
   end
 
   def test_not_blank_with_string_content
@@ -278,7 +278,7 @@ class ConditionUnitTest < Minitest::Test
     @context['string'] = 'hello'
     blank_literal = Condition.class_variable_get(:@@method_literals)['blank']
 
-    assert_evaluates_false(VariableLookup.new('string'), '==', blank_literal)
+    assert_evaluates_false(VariableLookup.parse('string'), '==', blank_literal)
   end
 
   def test_not_blank_with_non_empty_array
@@ -287,7 +287,7 @@ class ConditionUnitTest < Minitest::Test
     @context['array'] = [1, 2, 3]
     blank_literal = Condition.class_variable_get(:@@method_literals)['blank']
 
-    assert_evaluates_false(VariableLookup.new('array'), '==', blank_literal)
+    assert_evaluates_false(VariableLookup.parse('array'), '==', blank_literal)
   end
 
   def test_not_blank_with_non_empty_hash
@@ -296,7 +296,7 @@ class ConditionUnitTest < Minitest::Test
     @context['hash'] = { 'a' => 1 }
     blank_literal = Condition.class_variable_get(:@@method_literals)['blank']
 
-    assert_evaluates_false(VariableLookup.new('hash'), '==', blank_literal)
+    assert_evaluates_false(VariableLookup.parse('hash'), '==', blank_literal)
   end
 
   # Tests for empty? comparison without ActiveSupport
@@ -312,7 +312,7 @@ class ConditionUnitTest < Minitest::Test
     @context['empty_string'] = ''
     empty_literal = Condition.class_variable_get(:@@method_literals)['empty']
 
-    assert_evaluates_true(VariableLookup.new('empty_string'), '==', empty_literal)
+    assert_evaluates_true(VariableLookup.parse('empty_string'), '==', empty_literal)
   end
 
   def test_empty_with_whitespace_string_not_empty
@@ -322,7 +322,7 @@ class ConditionUnitTest < Minitest::Test
     @context['whitespace'] = '   '
     empty_literal = Condition.class_variable_get(:@@method_literals)['empty']
 
-    assert_evaluates_false(VariableLookup.new('whitespace'), '==', empty_literal)
+    assert_evaluates_false(VariableLookup.parse('whitespace'), '==', empty_literal)
   end
 
   def test_empty_with_empty_array
@@ -331,7 +331,7 @@ class ConditionUnitTest < Minitest::Test
     @context['empty_array'] = []
     empty_literal = Condition.class_variable_get(:@@method_literals)['empty']
 
-    assert_evaluates_true(VariableLookup.new('empty_array'), '==', empty_literal)
+    assert_evaluates_true(VariableLookup.parse('empty_array'), '==', empty_literal)
   end
 
   def test_empty_with_empty_hash
@@ -340,7 +340,7 @@ class ConditionUnitTest < Minitest::Test
     @context['empty_hash'] = {}
     empty_literal = Condition.class_variable_get(:@@method_literals)['empty']
 
-    assert_evaluates_true(VariableLookup.new('empty_hash'), '==', empty_literal)
+    assert_evaluates_true(VariableLookup.parse('empty_hash'), '==', empty_literal)
   end
 
   def test_nil_is_not_empty
@@ -350,7 +350,7 @@ class ConditionUnitTest < Minitest::Test
     @context['nil_value'] = nil
     empty_literal = Condition.class_variable_get(:@@method_literals)['empty']
 
-    assert_evaluates_false(VariableLookup.new('nil_value'), '==', empty_literal)
+    assert_evaluates_false(VariableLookup.parse('nil_value'), '==', empty_literal)
   end
 
   private
@@ -358,3 +358,20 @@ class ConditionUnitTest < Minitest::Test
   def assert_evaluates_true(left, op, right)
     assert(
       Condition.new(left, op, right).evaluate(@context),
+      "Evaluated false: #{left.inspect} #{op} #{right.inspect}",
+    )
+  end
+
+  def assert_evaluates_false(left, op, right)
+    assert(
+      !Condition.new(left, op, right).evaluate(@context),
+      "Evaluated true: #{left.inspect} #{op} #{right.inspect}",
+    )
+  end
+
+  def assert_evaluates_argument_error(left, op, right)
+    assert_raises(Liquid::ArgumentError) do
+      Condition.new(left, op, right).evaluate(@context)
+    end
+  end
+end # ConditionTest

test/unit/parse_context_unit_test.rb

@@ -7,7 +7,7 @@ class ParseContextUnitTest < Minitest::Test
 
   def test_safe_parse_expression_with_variable_lookup
     parser = parse_context.new_parser('product.title')
-    result = parse_context.safe_parse_expression(parser)
+    result = parser.expression_node
 
     assert_instance_of(VariableLookup, result)
     assert_equal('product', result.name)
@@ -18,7 +18,7 @@ class ParseContextUnitTest < Minitest::Test
     parser = parse_context.new_parser('')
 
     error = assert_raises(Liquid::SyntaxError) do
-      parse_context.safe_parse_expression(parser)
+      parser.expression_node
     end
 
     assert_match(/is not a valid expression/, error.message)
@@ -56,15 +56,15 @@ class ParseContextUnitTest < Minitest::Test
   def test_safe_parse_expression_advances_parser_pointer
     parser = parse_context.new_parser('foo, bar')
 
-    # safe_parse_expression consumes "foo"
-    first_result = parse_context.safe_parse_expression(parser)
+    # parser.expression_node consumes "foo"
+    first_result = parser.expression_node
     assert_instance_of(VariableLookup, first_result)
     assert_equal('foo', first_result.name)
 
     parser.consume(:comma)
 
-    # safe_parse_expression consumes "bar"
-    second_result = parse_context.safe_parse_expression(parser)
+    # parser.expression_node consumes "bar"
+    second_result = parser.expression_node
     assert_instance_of(VariableLookup, second_result)
     assert_equal('bar', second_result.name)