From cf3d84531561b5a0568bdd470334f6b4c34c84c4 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Tue, 2 Dec 2025 11:03:26 -0500 Subject: [PATCH] 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 | 2 ++ lib/liquid/parse_context.rb | 6 +--- lib/liquid/parser.rb | 12 +++++-- lib/liquid/tag.rb | 4 --- lib/liquid/tags/case.rb | 4 +-- lib/liquid/tags/cycle.rb | 6 ++-- lib/liquid/tags/include.rb | 6 ++-- lib/liquid/tags/render.rb | 4 +-- lib/liquid/tags/table_row.rb | 4 +-- lib/liquid/variable.rb | 6 ++-- test/unit/condition_unit_test.rb | 49 +++++++++++++++++++--------- test/unit/parse_context_unit_test.rb | 12 +++---- 12 files changed, 66 insertions(+), 49 deletions(-) diff --git a/History.md b/History.md index 8fd6ef37..cf6382eb 100644 --- a/History.md +++ b/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] diff --git a/lib/liquid/parse_context.rb b/lib/liquid/parse_context.rb index 96413eb6..e6279d76 100644 --- a/lib/liquid/parse_context.rb +++ b/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 diff --git a/lib/liquid/parser.rb b/lib/liquid/parser.rb index 645dfa3a..86de2eab 100644 --- a/lib/liquid/parser.rb +++ b/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) diff --git a/lib/liquid/tag.rb b/lib/liquid/tag.rb index 374ee511..501eec61 100644 --- a/lib/liquid/tag.rb +++ b/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 diff --git a/lib/liquid/tags/case.rb b/lib/liquid/tags/case.rb index ca1d329c..6fd8586d 100644 --- a/lib/liquid/tags/case.rb +++ b/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 diff --git a/lib/liquid/tags/cycle.rb b/lib/liquid/tags/cycle.rb index a98d2539..4847824e 100644 --- a/lib/liquid/tags/cycle.rb +++ b/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) diff --git a/lib/liquid/tags/include.rb b/lib/liquid/tags/include.rb index b50c68a6..02d8bf4e 100644 --- a/lib/liquid/tags/include.rb +++ b/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 diff --git a/lib/liquid/tags/render.rb b/lib/liquid/tags/render.rb index 5c071c80..5a2ef067 100644 --- a/lib/liquid/tags/render.rb +++ b/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 diff --git a/lib/liquid/tags/table_row.rb b/lib/liquid/tags/table_row.rb index c9114754..3b4cc586 100644 --- a/lib/liquid/tags/table_row.rb +++ b/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 diff --git a/lib/liquid/variable.rb b/lib/liquid/variable.rb index fcc72351..c7352358 100644 --- a/lib/liquid/variable.rb +++ b/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 diff --git a/test/unit/condition_unit_test.rb b/test/unit/condition_unit_test.rb index b04e0bca..194cbf0e 100644 --- a/test/unit/condition_unit_test.rb +++ b/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 diff --git a/test/unit/parse_context_unit_test.rb b/test/unit/parse_context_unit_test.rb index f75a4804..89923a75 100644 --- a/test/unit/parse_context_unit_test.rb +++ b/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)