From aa11d345b7df4058c4507de4c3867a9ac89b39f4 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Thu, 6 Nov 2025 14:58:31 -0500 Subject: [PATCH 01/32] Remove :lax, :strict and :warn error modes --- README.md | 25 ------- Rakefile | 37 ++-------- lib/liquid/environment.rb | 4 +- lib/liquid/expression.rb | 3 - lib/liquid/parser_switching.rb | 57 +--------------- lib/liquid/tags/case.rb | 29 -------- lib/liquid/tags/cycle.rb | 31 --------- lib/liquid/tags/for.rb | 23 +------ lib/liquid/tags/if.rb | 30 +------- lib/liquid/tags/include.rb | 26 ------- lib/liquid/tags/render.rb | 23 ------- lib/liquid/tags/table_row.rb | 18 ----- lib/liquid/template.rb | 3 - lib/liquid/variable.rb | 59 +--------------- test/integration/assign_test.rb | 5 +- test/integration/context_test.rb | 2 +- test/integration/error_handling_test.rb | 52 ++------------ test/integration/expression_test.rb | 18 ----- test/integration/parsing_quirks_test.rb | 83 ++--------------------- test/integration/tags/cycle_tag_test.rb | 21 ------ test/integration/tags/include_tag_test.rb | 30 +------- test/integration/tags/render_tag_test.rb | 16 ----- test/integration/tags/table_row_test.rb | 35 ---------- test/integration/template_test.rb | 2 +- test/integration/variable_test.rb | 59 ---------------- test/test_helper.rb | 2 +- test/unit/condition_unit_test.rb | 4 +- test/unit/parse_context_unit_test.rb | 18 ----- test/unit/partial_cache_unit_test.rb | 4 +- test/unit/tags/case_tag_unit_test.rb | 20 +----- test/unit/variable_unit_test.rb | 38 +---------- 31 files changed, 35 insertions(+), 742 deletions(-) diff --git a/README.md b/README.md index 5066dc659..b3745e024 100644 --- a/README.md +++ b/README.md @@ -93,31 +93,6 @@ LIQUID By using Environments, you ensure that custom tags and filters are only available in the contexts where they are needed, making your Liquid templates more robust and easier to manage. For smaller projects, a global environment is available via `Liquid::Environment.default`. -### Error Modes - -Setting the error mode of Liquid lets you specify how strictly you want your templates to be interpreted. -Normally the parser is very lax and will accept almost anything without error. Unfortunately this can make -it very hard to debug and can lead to unexpected behaviour. - -Liquid also comes with different parsers that can be used when editing templates to give better error messages -when templates are invalid. You can enable this new parser like this: - -```ruby -Liquid::Environment.default.error_mode = :strict2 # Raises a SyntaxError when invalid syntax is used in all tags -Liquid::Environment.default.error_mode = :strict # Raises a SyntaxError when invalid syntax is used in some tags -Liquid::Environment.default.error_mode = :warn # Adds strict errors to template.errors but continues as normal -Liquid::Environment.default.error_mode = :lax # The default mode, accepts almost anything. -``` - -If you want to set the error mode only on specific templates you can pass `:error_mode` as an option to `parse`: -```ruby -Liquid::Template.parse(source, error_mode: :strict) -``` -This is useful for doing things like enabling strict mode only in the theme editor. - -It is recommended that you enable `:strict` or `:warn` mode on new apps to stop invalid templates from being created. -It is also recommended that you use it in the template editors of existing apps to give editors better error messages. - ### Undefined variables and filters By default, the renderer doesn't raise or in any other way notify you if some variables or filters are missing, i.e. not passed to the `render` method. diff --git a/Rakefile b/Rakefile index 83afcbfa0..b8939316b 100755 --- a/Rakefile +++ b/Rakefile @@ -33,28 +33,13 @@ task :rubocop do end end -desc('runs test suite with lax, strict, and strict2 parsers') +desc('runs test suite with strict2 parser') task :test do - ENV['LIQUID_PARSER_MODE'] = 'lax' - Rake::Task['base_test'].invoke - - ENV['LIQUID_PARSER_MODE'] = 'strict' - Rake::Task['base_test'].reenable - Rake::Task['base_test'].invoke - ENV['LIQUID_PARSER_MODE'] = 'strict2' Rake::Task['base_test'].reenable Rake::Task['base_test'].invoke if RUBY_ENGINE == 'ruby' || RUBY_ENGINE == 'truffleruby' - ENV['LIQUID_PARSER_MODE'] = 'lax' - Rake::Task['integration_test'].reenable - Rake::Task['integration_test'].invoke - - ENV['LIQUID_PARSER_MODE'] = 'strict' - Rake::Task['integration_test'].reenable - Rake::Task['integration_test'].invoke - ENV['LIQUID_PARSER_MODE'] = 'strict2' Rake::Task['integration_test'].reenable Rake::Task['integration_test'].invoke @@ -78,23 +63,13 @@ task release: :build do end namespace :benchmark do - desc "Run the liquid benchmark with lax parsing" - task :lax do - ruby "./performance/benchmark.rb lax" - end - - desc "Run the liquid benchmark with strict parsing" - task :strict do - ruby "./performance/benchmark.rb strict" - end - desc "Run the liquid benchmark with strict2 parsing" task :strict2 do ruby "./performance/benchmark.rb strict2" end - desc "Run the liquid benchmark with lax, strict, and strict2 parsing" - task run: [:lax, :strict, :strict2] + desc "Run the liquid benchmark" + task run: [:strict2] desc "Run unit benchmarks" namespace :unit do @@ -127,9 +102,9 @@ namespace :profile do ruby "./performance/profile.rb" end - desc "Run the liquid profile/performance coverage with strict parsing" - task :strict do - ruby "./performance/profile.rb strict" + desc "Run the liquid profile/performance coverage with strict2 parsing" + task :strict2 do + ruby "./performance/profile.rb strict2" end end diff --git a/lib/liquid/environment.rb b/lib/liquid/environment.rb index 100bd0bbd..095b2b091 100644 --- a/lib/liquid/environment.rb +++ b/lib/liquid/environment.rb @@ -34,7 +34,7 @@ class << self # @param file_system The default file system that is used # to load templates from. # @param error_mode [Symbol] The default error mode for all templates - # (either :strict2, :strict, :warn, or :lax). + # (:strict2). # @param exception_renderer [Proc] The exception renderer that is used to # render exceptions. # @yieldparam environment [Environment] The environment instance that is being built. @@ -75,7 +75,7 @@ def dangerously_override(environment) # @api private def initialize @tags = Tags::STANDARD_TAGS.dup - @error_mode = :lax + @error_mode = :strict2 @strainer_template = Class.new(StrainerTemplate).tap do |klass| klass.add_filter(StandardFilters) end diff --git a/lib/liquid/expression.rb b/lib/liquid/expression.rb index 2605c5576..3f9f08837 100644 --- a/lib/liquid/expression.rb +++ b/lib/liquid/expression.rb @@ -11,9 +11,6 @@ class Expression 'false' => false, 'blank' => '', 'empty' => '', - # in lax mode, minus sign can be a VariableLookup - # For simplicity and performace, we treat it like a literal - '-' => VariableLookup.parse("-", nil).freeze, }.freeze DOT = ".".ord diff --git a/lib/liquid/parser_switching.rb b/lib/liquid/parser_switching.rb index e419dc999..4b35e2139 100644 --- a/lib/liquid/parser_switching.rb +++ b/lib/liquid/parser_switching.rb @@ -2,59 +2,12 @@ module Liquid module ParserSwitching - # Do not use this. - # - # It's basically doing the same thing the {#parse_with_selected_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, - # except when in strict2 mode where it uses the strict2 parser. - # - # @deprecated Use {#parse_with_selected_parser} instead. - def strict_parse_with_error_mode_fallback(markup) - return strict2_parse_with_error_context(markup) if strict2_mode? - - strict_parse_with_error_context(markup) - rescue SyntaxError => e - case parse_context.error_mode - when :rigid - rigid_warn - raise - when :strict2 - raise - when :strict - raise - when :warn - parse_context.warnings << e - end - lax_parse(markup) - end - def parse_with_selected_parser(markup) - case parse_context.error_mode - when :rigid then rigid_warn && strict2_parse_with_error_context(markup) - when :strict2 then strict2_parse_with_error_context(markup) - when :strict then strict_parse_with_error_context(markup) - when :lax then lax_parse(markup) - when :warn - begin - strict2_parse_with_error_context(markup) - rescue SyntaxError => e - parse_context.warnings << e - lax_parse(markup) - end - end - end - - def strict2_mode? - parse_context.error_mode == :strict2 || parse_context.error_mode == :rigid + strict2_parse_with_error_context(markup) end private - def rigid_warn - Deprecations.warn(':rigid', ':strict2') - end - def strict2_parse_with_error_context(markup) strict2_parse(markup) rescue SyntaxError => e @@ -63,14 +16,6 @@ def strict2_parse_with_error_context(markup) raise e end - def strict_parse_with_error_context(markup) - strict_parse(markup) - rescue SyntaxError => e - e.line_number = line_number - e.markup_context = markup_context(markup) - raise e - end - def markup_context(markup) "in \"#{markup.strip}\"" end diff --git a/lib/liquid/tags/case.rb b/lib/liquid/tags/case.rb index b32ea1ae7..e24cf5a1b 100644 --- a/lib/liquid/tags/case.rb +++ b/lib/liquid/tags/case.rb @@ -23,9 +23,6 @@ module Liquid # @liquid_syntax_keyword second_expression An expression to be rendered when the variable's value matches `second_value`. # @liquid_syntax_keyword third_expression An expression to be rendered when the variable's value has no match. class Case < Block - Syntax = /(#{QuotedFragment})/o - WhenSyntax = /(#{QuotedFragment})(?:(?:\s+or\s+|\s*\,\s*)(#{QuotedFragment}.*))?/om - attr_reader :blocks, :left def initialize(tag_name, markup, options) @@ -92,18 +89,6 @@ def strict2_parse(markup) 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 @@ -129,20 +114,6 @@ def parse_strict2_when(markup, body) 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") - end - - markup = Regexp.last_match(2) - - block = Condition.new(@left, '==', Condition.parse_expression(parse_context, Regexp.last_match(1))) - block.attach(body) - @blocks << block - end - end - def record_else_condition(markup) unless markup.strip.empty? raise SyntaxError, options[:locale].t("errors.syntax.case_invalid_else") diff --git a/lib/liquid/tags/cycle.rb b/lib/liquid/tags/cycle.rb index b7d3069c8..2d372cee5 100644 --- a/lib/liquid/tags/cycle.rb +++ b/lib/liquid/tags/cycle.rb @@ -15,8 +15,6 @@ module Liquid # @liquid_syntax # {% cycle string, string, ... %} class Cycle < Tag - SimpleSyntax = /\A#{QuotedFragment}+/o - NamedSyntax = /\A(#{QuotedFragment})\s*\:\s*(.*)/om UNNAMED_CYCLE_PATTERN = /\w+:0x\h{8}/ attr_reader :variables @@ -91,35 +89,6 @@ def strict2_parse(markup) end end - def strict_parse(markup) - lax_parse(markup) - end - - def lax_parse(markup) - case markup - when NamedSyntax - @variables = variables_from_string(Regexp.last_match(2)) - @name = parse_expression(Regexp.last_match(1)) - @is_named = true - when SimpleSyntax - @variables = variables_from_string(markup) - @name = @variables.to_s - @is_named = !@name.match?(UNNAMED_CYCLE_PATTERN) - else - raise SyntaxError, options[:locale].t("errors.syntax.cycle") - end - end - - def variables_from_string(markup) - markup.split(',').collect do |var| - var =~ /\s*(#{QuotedFragment})\s*/o - next unless Regexp.last_match(1) - - var = parse_expression(Regexp.last_match(1)) - maybe_dup_lookup(var) - 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) diff --git a/lib/liquid/tags/for.rb b/lib/liquid/tags/for.rb index cbea85bcb..4b89b7972 100644 --- a/lib/liquid/tags/for.rb +++ b/lib/liquid/tags/for.rb @@ -25,8 +25,6 @@ module Liquid # @liquid_optional_param range [untyped] A custom numeric range to iterate over. # @liquid_optional_param reversed [untyped] Iterate in reverse order. class For < Block - Syntax = /\A(#{VariableSegment}+)\s+in\s+(#{QuotedFragment}+)\s*(reversed)?/o - attr_reader :collection_name, :variable_name, :limit, :from def initialize(tag_name, markup, options) @@ -72,22 +70,7 @@ def render_to_output_buffer(context, output) protected - def lax_parse(markup) - if markup =~ Syntax - @variable_name = Regexp.last_match(1) - collection_name = Regexp.last_match(2) - @reversed = !!Regexp.last_match(3) - @name = "#{@variable_name}-#{collection_name}" - @collection_name = parse_expression(collection_name) - markup.scan(TagAttributes) do |key, value| - set_attribute(key, value) - end - else - raise SyntaxError, options[:locale].t("errors.syntax.for") - end - end - - def strict_parse(markup) + def strict2_parse(markup) p = @parse_context.new_parser(markup) @variable_name = p.consume(:id) raise SyntaxError, options[:locale].t("errors.syntax.for_invalid_in") unless p.id?('in') @@ -111,10 +94,6 @@ def strict_parse(markup) private - def strict2_parse(markup) - strict_parse(markup) - end - def collection_segment(context) offsets = context.registers[:for] ||= {} diff --git a/lib/liquid/tags/if.rb b/lib/liquid/tags/if.rb index c423c1e84..287fe78ae 100644 --- a/lib/liquid/tags/if.rb +++ b/lib/liquid/tags/if.rb @@ -14,10 +14,6 @@ module Liquid # @liquid_syntax_keyword condition The condition to evaluate. # @liquid_syntax_keyword expression The expression to render if the condition is met. class If < Block - Syntax = /(#{QuotedFragment})\s*([=!<>a-z_]+)?\s*(#{QuotedFragment})?/o - ExpressionsAndOperators = /(?:\b(?:\s?and\s?|\s?or\s?)\b|(?:\s*(?!\b(?:\s?and\s?|\s?or\s?)\b)(?:#{QuotedFragment}|\S+)\s*)+)/o - BOOLEAN_OPERATORS = %w(and or).freeze - attr_reader :blocks def initialize(tag_name, markup, options) @@ -66,10 +62,6 @@ def render_to_output_buffer(context, output) private - def strict2_parse(markup) - strict_parse(markup) - end - def push_block(tag, markup) block = if tag == 'else' ElseCondition.new @@ -85,27 +77,7 @@ def parse_expression(markup, safe: false) Condition.parse_expression(parse_context, markup, safe: safe) end - def lax_parse(markup) - expressions = markup.scan(ExpressionsAndOperators) - raise SyntaxError, options[:locale].t("errors.syntax.if") unless expressions.pop =~ Syntax - - condition = Condition.new(parse_expression(Regexp.last_match(1)), Regexp.last_match(2), parse_expression(Regexp.last_match(3))) - - until expressions.empty? - operator = expressions.pop.to_s.strip - - raise SyntaxError, options[:locale].t("errors.syntax.if") unless expressions.pop.to_s =~ Syntax - - new_condition = Condition.new(parse_expression(Regexp.last_match(1)), Regexp.last_match(2), parse_expression(Regexp.last_match(3))) - raise SyntaxError, options[:locale].t("errors.syntax.if") unless BOOLEAN_OPERATORS.include?(operator) - new_condition.send(operator, condition) - condition = new_condition - end - - condition - end - - def strict_parse(markup) + def strict2_parse(markup) p = @parse_context.new_parser(markup) condition = parse_binary_comparisons(p) p.consume(:end_of_string) diff --git a/lib/liquid/tags/include.rb b/lib/liquid/tags/include.rb index 969482d49..49bc032f6 100644 --- a/lib/liquid/tags/include.rb +++ b/lib/liquid/tags/include.rb @@ -20,9 +20,6 @@ module Liquid class Include < Tag prepend Tag::Disableable - SYNTAX = /(#{QuotedFragment}+)(\s+(?:with|for)\s+(#{QuotedFragment}+))?(\s+(?:as)\s+(#{VariableSegment}+))?/o - Syntax = SYNTAX - attr_reader :template_name_expr, :variable_name_expr, :attributes def initialize(tag_name, markup, options) @@ -104,29 +101,6 @@ def strict2_parse(markup) p.consume(:end_of_string) end - def strict_parse(markup) - lax_parse(markup) - end - - def lax_parse(markup) - if markup =~ SYNTAX - template_name = Regexp.last_match(1) - variable_name = Regexp.last_match(3) - - @alias_name = Regexp.last_match(5) - @variable_name_expr = variable_name ? parse_expression(variable_name) : nil - @template_name_expr = parse_expression(template_name) - @attributes = {} - - markup.scan(TagAttributes) do |key, value| - @attributes[key] = parse_expression(value) - end - - else - raise SyntaxError, options[:locale].t("errors.syntax.include") - end - end - class ParseTreeVisitor < Liquid::ParseTreeVisitor def children [ diff --git a/lib/liquid/tags/render.rb b/lib/liquid/tags/render.rb index 6e1559cc2..43d0bca82 100644 --- a/lib/liquid/tags/render.rb +++ b/lib/liquid/tags/render.rb @@ -27,7 +27,6 @@ module Liquid # @liquid_syntax_keyword filename The name of the snippet to render, without the `.liquid` extension. class Render < Tag FOR = 'for' - SYNTAX = /(#{QuotedString}+)(\s+(with|#{FOR})\s+(#{QuotedFragment}+))?(\s+(?:as)\s+(#{VariableSegment}+))?/o disable_tags "include" @@ -111,28 +110,6 @@ def strict2_template_name(p) p.consume(:string) end - def strict_parse(markup) - lax_parse(markup) - end - - def lax_parse(markup) - raise SyntaxError, options[:locale].t("errors.syntax.render") unless markup =~ SYNTAX - - template_name = Regexp.last_match(1) - with_or_for = Regexp.last_match(3) - variable_name = Regexp.last_match(4) - - @alias_name = Regexp.last_match(6) - @variable_name_expr = variable_name ? parse_expression(variable_name) : nil - @template_name_expr = parse_expression(template_name) - @is_for_loop = (with_or_for == FOR) - - @attributes = {} - markup.scan(TagAttributes) do |key, value| - @attributes[key] = parse_expression(value) - end - end - class ParseTreeVisitor < Liquid::ParseTreeVisitor def children [ diff --git a/lib/liquid/tags/table_row.rb b/lib/liquid/tags/table_row.rb index b69f91486..07c6da23a 100644 --- a/lib/liquid/tags/table_row.rb +++ b/lib/liquid/tags/table_row.rb @@ -24,7 +24,6 @@ module Liquid # @liquid_optional_param offset: [number] The 1-based index to start iterating at. # @liquid_optional_param range [untyped] A custom numeric range to iterate over. class TableRow < Block - Syntax = /(\w+)\s+in\s+(#{QuotedFragment}+)/o ALLOWED_ATTRIBUTES = ['cols', 'limit', 'offset', 'range'].freeze attr_reader :variable_name, :collection_name, :attributes @@ -62,23 +61,6 @@ def strict2_parse(markup) p.consume(:end_of_string) end - def strict_parse(markup) - lax_parse(markup) - end - - def lax_parse(markup) - if markup =~ Syntax - @variable_name = Regexp.last_match(1) - @collection_name = parse_expression(Regexp.last_match(2)) - @attributes = {} - markup.scan(TagAttributes) do |key, value| - @attributes[key] = parse_expression(value) - end - else - raise SyntaxError, options[:locale].t("errors.syntax.table_row") - end - end - def render_to_output_buffer(context, output) (collection = context.evaluate(@collection_name)) || (return '') diff --git a/lib/liquid/template.rb b/lib/liquid/template.rb index b007765ce..6bb03dfc3 100644 --- a/lib/liquid/template.rb +++ b/lib/liquid/template.rb @@ -22,9 +22,6 @@ class Template class << self # Sets how strict the parser should be. - # :lax acts like liquid 2.5 and silently ignores malformed tags in most cases. - # :warn is the default and will give deprecation warnings when invalid syntax is used. - # :strict enforces correct syntax for most tags # :strict2 enforces correct syntax for all tags def error_mode=(mode) Deprecations.warn("Template.error_mode=", "Environment#error_mode=") diff --git a/lib/liquid/variable.rb b/lib/liquid/variable.rb index 6b5fb412b..0a0ad21ec 100644 --- a/lib/liquid/variable.rb +++ b/lib/liquid/variable.rb @@ -30,7 +30,7 @@ def initialize(markup, parse_context) @parse_context = parse_context @line_number = parse_context.line_number - strict_parse_with_error_mode_fallback(markup) + parse_with_selected_parser(markup) end def raw @@ -41,39 +41,6 @@ def markup_context(markup) "in \"{{#{markup}}}\"" end - def lax_parse(markup) - @filters = [] - return unless markup =~ MarkupWithQuotedFragment - - name_markup = Regexp.last_match(1) - filter_markup = Regexp.last_match(2) - @name = parse_context.parse_expression(name_markup) - if filter_markup =~ FilterMarkupRegex - filters = Regexp.last_match(1).scan(FilterParser) - filters.each do |f| - next unless f =~ /\w+/ - filtername = Regexp.last_match(0) - filterargs = f.scan(FilterArgsRegex).flatten - @filters << lax_parse_filter_expressions(filtername, filterargs) - end - end - end - - def strict_parse(markup) - @filters = [] - p = @parse_context.new_parser(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 << lax_parse_filter_expressions(filtername, filterargs) - end - p.consume(:end_of_string) - end - def strict2_parse(markup) @filters = [] p = @parse_context.new_parser(markup) @@ -85,14 +52,6 @@ def strict2_parse(markup) p.consume(:end_of_string) end - def parse_filterargs(p) - # first argument - filterargs = [p.argument] - # followed by comma separated others - filterargs << p.argument while p.consume?(:comma) - filterargs - end - def render(context) obj = context.evaluate(@name) @@ -133,22 +92,6 @@ def disabled_tags private - def lax_parse_filter_expressions(filter_name, unparsed_args) - filter_args = [] - keyword_args = nil - unparsed_args.each do |a| - if (matches = a.match(JustTagAttributes)) - keyword_args ||= {} - keyword_args[matches[1]] = parse_context.parse_expression(matches[2]) - else - 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?] diff --git a/test/integration/assign_test.rb b/test/integration/assign_test.rb index fdb6c99ca..a88941ae2 100644 --- a/test/integration/assign_test.rb +++ b/test/integration/assign_test.rb @@ -41,11 +41,10 @@ def test_assign_syntax_error def test_assign_uses_error_mode assert_match_syntax_error( - "Expected dotdot but found pipe in ", + "Expected dotdot but found pipe", "{% assign foo = ('X' | downcase) %}", - error_mode: :strict, + error_mode: :rigid, ) - assert_template_result("", "{% assign foo = ('X' | downcase) %}", error_mode: :lax) end def test_expression_with_whitespace_in_square_brackets diff --git a/test/integration/context_test.rb b/test/integration/context_test.rb index d230734f6..db68da45f 100644 --- a/test/integration/context_test.rb +++ b/test/integration/context_test.rb @@ -632,7 +632,7 @@ def notice(output) end def test_has_key_will_not_add_an_error_for_missing_keys - with_error_modes(:strict) do + with_error_modes(:rigid) do context = Context.new context.key?('unknown') assert_empty(context.errors) diff --git a/test/integration/error_handling_test.rb b/test/integration/error_handling_test.rb index 0fda83ca5..9f07b1f87 100644 --- a/test/integration/error_handling_test.rb +++ b/test/integration/error_handling_test.rb @@ -67,20 +67,13 @@ def test_missing_endtag_parse_time_error end def test_unrecognized_operator - with_error_modes(:strict) do + with_error_modes(:rigid) do assert_raises(SyntaxError) do Liquid::Template.parse(' {% if 1 =! 2 %}ok{% endif %} ') end end end - def test_lax_unrecognized_operator - template = Liquid::Template.parse(' {% if 1 =! 2 %}ok{% endif %} ', error_mode: :lax) - assert_equal(' Liquid error: Unknown operator =! ', template.render) - assert_equal(1, template.errors.size) - assert_equal(Liquid::ArgumentError, template.errors.first.class) - end - def test_with_line_numbers_adds_numbers_to_parser_errors source = <<~LIQUID foobar @@ -104,25 +97,6 @@ def test_with_line_numbers_adds_numbers_to_parser_errors_with_whitespace_trim assert_match_syntax_error(/Liquid syntax error \(line 3\)/, source) end - def test_parsing_warn_with_line_numbers_adds_numbers_to_lexer_errors - template = Liquid::Template.parse( - ' - foobar - - {% if 1 =! 2 %}ok{% endif %} - - bla - ', - error_mode: :warn, - line_numbers: true, - ) - - assert_equal( - ['Liquid syntax error (line 4): Unexpected character = in "1 =! 2"'], - template.warnings.map(&:message), - ) - end - def test_parsing_strict_with_line_numbers_adds_numbers_to_lexer_errors err = assert_raises(SyntaxError) do Liquid::Template.parse( @@ -133,7 +107,7 @@ def test_parsing_strict_with_line_numbers_adds_numbers_to_lexer_errors bla ', - error_mode: :strict, + error_mode: :rigid, line_numbers: true, ) end @@ -157,34 +131,16 @@ def test_syntax_errors_in_nested_blocks_have_correct_line_number def test_strict_error_messages err = assert_raises(SyntaxError) do - Liquid::Template.parse(' {% if 1 =! 2 %}ok{% endif %} ', error_mode: :strict) + Liquid::Template.parse(' {% if 1 =! 2 %}ok{% endif %} ', error_mode: :rigid) end assert_equal('Liquid syntax error: Unexpected character = in "1 =! 2"', err.message) err = assert_raises(SyntaxError) do - Liquid::Template.parse('{{%%%}}', error_mode: :strict) + Liquid::Template.parse('{{%%%}}', error_mode: :rigid) end assert_equal('Liquid syntax error: Unexpected character % in "{{%%%}}"', err.message) end - def test_warnings - template = Liquid::Template.parse('{% if ~~~ %}{{%%%}}{% else %}{{ hello. }}{% endif %}', error_mode: :warn) - assert_equal(3, template.warnings.size) - assert_equal('Unexpected character ~ in "~~~"', template.warnings[0].to_s(false)) - assert_equal('Unexpected character % in "{{%%%}}"', template.warnings[1].to_s(false)) - assert_equal('Expected id but found end_of_string in "{{ hello. }}"', template.warnings[2].to_s(false)) - assert_equal('', template.render) - end - - def test_warning_line_numbers - template = Liquid::Template.parse("{% if ~~~ %}\n{{%%%}}{% else %}\n{{ hello. }}{% endif %}", error_mode: :warn, line_numbers: true) - assert_equal('Liquid syntax error (line 1): Unexpected character ~ in "~~~"', template.warnings[0].message) - assert_equal('Liquid syntax error (line 2): Unexpected character % in "{{%%%}}"', template.warnings[1].message) - assert_equal('Liquid syntax error (line 3): Expected id but found end_of_string in "{{ hello. }}"', template.warnings[2].message) - assert_equal(3, template.warnings.size) - assert_equal([1, 2, 3], template.warnings.map(&:line_number)) - end - # Liquid should not catch Exceptions that are not subclasses of StandardError, like Interrupt and NoMemoryError def test_exceptions_propagate assert_raises(Exception) do diff --git a/test/integration/expression_test.rb b/test/integration/expression_test.rb index ae84fa36d..988f1d122 100644 --- a/test/integration/expression_test.rb +++ b/test/integration/expression_test.rb @@ -27,11 +27,6 @@ def test_float assert_template_result("-17.42", "{{ -17.42 }}") assert_template_result("2.5", "{{ 2.5 }}") - with_error_modes(:lax) do - assert_expression_result(0.0, "0.....5") - assert_expression_result(0.0, "-0..1") - end - assert_expression_result(1.5, "1.5") # this is a unfortunate quirky behavior of Liquid @@ -56,19 +51,6 @@ def test_range ) end - def test_quirky_negative_sign_expression_markup - result = Expression.parse("-", nil) - assert(result.is_a?(VariableLookup)) - assert_equal("-", result.name) - - # for this template, the expression markup is "-" - assert_template_result( - "", - "{{ - 'theme.css' - }}", - error_mode: :lax, - ) - end - def test_expression_cache skip("Liquid-C does not support Expression caching") if defined?(Liquid::C) && Liquid::C.enabled diff --git a/test/integration/parsing_quirks_test.rb b/test/integration/parsing_quirks_test.rb index b82ce86c5..523512098 100644 --- a/test/integration/parsing_quirks_test.rb +++ b/test/integration/parsing_quirks_test.rb @@ -31,18 +31,14 @@ def test_raise_on_label_and_no_close_bracets_percent def test_error_on_empty_filter assert(Template.parse("{{test}}")) - with_error_modes(:lax) do - assert(Template.parse("{{|test}}")) - end - - with_error_modes(:strict) do - assert_raises(SyntaxError) { Template.parse("{{|test}}") } - assert_raises(SyntaxError) { Template.parse("{{test |a|b|}}") } + with_error_modes(:rigid) do + assert_raises(Liquid::SyntaxError) { Template.parse("{{|test}}") } + assert_raises(Liquid::SyntaxError) { Template.parse("{{test |a|b|}}") } end end def test_meaningless_parens_error - with_error_modes(:strict) do + with_error_modes(:rigid) do assert_raises(SyntaxError) do markup = "a == 'foo' or (b == 'bar' and c == 'baz') or false" Template.parse("{% if #{markup} %} YES {% endif %}") @@ -51,7 +47,7 @@ def test_meaningless_parens_error end def test_unexpected_characters_syntax_error - with_error_modes(:strict) do + with_error_modes(:rigid) do assert_raises(SyntaxError) do markup = "true && false" Template.parse("{% if #{markup} %} YES {% endif %}") @@ -63,61 +59,12 @@ def test_unexpected_characters_syntax_error end end - def test_no_error_on_lax_empty_filter - assert(Template.parse("{{test |a|b|}}", error_mode: :lax)) - assert(Template.parse("{{test}}", error_mode: :lax)) - assert(Template.parse("{{|test|}}", error_mode: :lax)) - end - - def test_meaningless_parens_lax - with_error_modes(:lax) do - assigns = { 'b' => 'bar', 'c' => 'baz' } - markup = "a == 'foo' or (b == 'bar' and c == 'baz') or false" - assert_template_result(' YES ', "{% if #{markup} %} YES {% endif %}", assigns) - end - end - - def test_unexpected_characters_silently_eat_logic_lax - with_error_modes(:lax) do - markup = "true && false" - assert_template_result(' YES ', "{% if #{markup} %} YES {% endif %}") - markup = "false || true" - assert_template_result('', "{% if #{markup} %} YES {% endif %}") - end - end - def test_raise_on_invalid_tag_delimiter assert_raises(Liquid::SyntaxError) do Template.new.parse('{% end %}') end end - def test_unanchored_filter_arguments - with_error_modes(:lax) do - assert_template_result('hi', "{{ 'hi there' | split$$$:' ' | first }}") - - assert_template_result('x', "{{ 'X' | downcase) }}") - - # After the messed up quotes a filter without parameters (reverse) should work - # but one with parameters (remove) shouldn't be detected. - assert_template_result('here', "{{ 'hi there' | split:\"t\"\" | reverse | first}}") - assert_template_result('hi ', "{{ 'hi there' | split:\"t\"\" | remove:\"i\" | first}}") - end - end - - def test_invalid_variables_work - with_error_modes(:lax) do - assert_template_result('bar', "{% assign 123foo = 'bar' %}{{ 123foo }}") - assert_template_result('123', "{% assign 123 = 'bar' %}{{ 123 }}") - end - end - - def test_extra_dots_in_ranges - with_error_modes(:lax) do - assert_template_result('12345', "{% for i in (1...5) %}{{ i }}{% endfor %}") - end - end - def test_blank_variable_markup assert_template_result('', "{{}}") end @@ -131,24 +78,4 @@ def test_lookup_on_var_with_literal_name def test_contains_in_id assert_template_result(' YES ', '{% if containsallshipments == true %} YES {% endif %}', { 'containsallshipments' => true }) end - - def test_incomplete_expression - with_error_modes(:lax) do - assert_template_result("false", "{{ false - }}") - assert_template_result("false", "{{ false > }}") - assert_template_result("false", "{{ false < }}") - assert_template_result("false", "{{ false = }}") - assert_template_result("false", "{{ false ! }}") - assert_template_result("false", "{{ false 1 }}") - assert_template_result("false", "{{ false a }}") - - assert_template_result("false", "{% liquid assign foo = false -\n%}{{ foo }}") - assert_template_result("false", "{% liquid assign foo = false >\n%}{{ foo }}") - assert_template_result("false", "{% liquid assign foo = false <\n%}{{ foo }}") - assert_template_result("false", "{% liquid assign foo = false =\n%}{{ foo }}") - assert_template_result("false", "{% liquid assign foo = false !\n%}{{ foo }}") - assert_template_result("false", "{% liquid assign foo = false 1\n%}{{ foo }}") - assert_template_result("false", "{% liquid assign foo = false a\n%}{{ foo }}") - end - end end # ParsingQuirksTest diff --git a/test/integration/tags/cycle_tag_test.rb b/test/integration/tags/cycle_tag_test.rb index dfb5984d8..cf3fdea18 100644 --- a/test/integration/tags/cycle_tag_test.rb +++ b/test/integration/tags/cycle_tag_test.rb @@ -96,11 +96,6 @@ def test_cycle_tag_with_error_mode template1 = "{% assign 5 = 'b' %}{% cycle .5, .4 %}" template2 = "{% cycle .5: 'a', 'b' %}" - with_error_modes(:lax, :strict) do - assert_template_result("b", template1) - assert_template_result("a", template2) - end - with_error_modes(:strict2) do error1 = assert_raises(Liquid::SyntaxError) { Template.parse(template1) } error2 = assert_raises(Liquid::SyntaxError) { Template.parse(template2) } @@ -121,14 +116,6 @@ def test_cycle_with_trailing_elements template4 = "#{assignments}{% cycle n e: 'a', 'b', 'c' %}" template5 = "#{assignments}{% cycle n e 'a', 'b', 'c' %}" - with_error_modes(:lax, :strict) 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 - with_error_modes(:strict2) do error1 = assert_raises(Liquid::SyntaxError) { Template.parse(template1) } error2 = assert_raises(Liquid::SyntaxError) { Template.parse(template2) } @@ -153,10 +140,6 @@ def test_cycle_name_with_invalid_expression {% endfor %} LIQUID - with_error_modes(:lax, :strict) do - refute_nil(Template.parse(template)) - end - with_error_modes(:strict2) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } assert_match(/Unexpected character =/, error.message) @@ -170,10 +153,6 @@ def test_cycle_variable_with_invalid_expression {% endfor %} LIQUID - with_error_modes(:lax, :strict) do - refute_nil(Template.parse(template)) - end - with_error_modes(:strict2) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } assert_match(/Unexpected character =/, error.message) diff --git a/test/integration/tags/include_tag_test.rb b/test/integration/tags/include_tag_test.rb index 44c0dcda7..e35addcd9 100644 --- a/test/integration/tags/include_tag_test.rb +++ b/test/integration/tags/include_tag_test.rb @@ -205,14 +205,6 @@ def test_dynamically_choosen_template end def test_strict2_parsing_errors - with_error_modes(:lax, :strict) do - assert_template_result( - 'hello value1 value2', - '{% include "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', - partials: { 'snippet' => 'hello {{ arg1 }} {{ arg2 }}' }, - ) - end - with_error_modes(:strict2) do assert_syntax_error( '{% include "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', @@ -301,16 +293,10 @@ def test_passing_options_to_included_templates env = Liquid::Environment.build(file_system: TestFileSystem.new) assert_raises(Liquid::SyntaxError) do - Template.parse("{% include template %}", error_mode: :strict, environment: env).render!("template" => '{{ "X" || downcase }}') - end - with_error_modes(:lax) do - assert_equal('x', Template.parse("{% include template %}", error_mode: :strict, include_options_blacklist: true, environment: env).render!("template" => '{{ "X" || downcase }}')) + Template.parse("{% include template %}", error_mode: :rigid, environment: env).render!("template" => '{{ "X" || downcase }}') end assert_raises(Liquid::SyntaxError) do - Template.parse("{% include template %}", error_mode: :strict, include_options_blacklist: [:locale], environment: env).render!("template" => '{{ "X" || downcase }}') - end - with_error_modes(:lax) do - assert_equal('x', Template.parse("{% include template %}", error_mode: :strict, include_options_blacklist: [:error_mode], environment: env).render!("template" => '{{ "X" || downcase }}')) + Template.parse("{% include template %}", error_mode: :rigid, include_options_blacklist: [:locale], environment: env).render!("template" => '{{ "X" || downcase }}') end end @@ -404,10 +390,6 @@ def test_render_tag_renders_error_with_template_name_from_template_factory def test_include_template_with_invalid_expression template = "{% include foo=>bar %}" - with_error_modes(:lax, :strict) do - refute_nil(Template.parse(template)) - end - with_error_modes(:strict2) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } assert_match(/Unexpected character =/, error.message) @@ -417,10 +399,6 @@ def test_include_template_with_invalid_expression def test_include_with_invalid_expression template = '{% include "snippet" with foo=>bar %}' - with_error_modes(:lax, :strict) do - refute_nil(Template.parse(template)) - end - with_error_modes(:strict2) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } assert_match(/Unexpected character =/, error.message) @@ -430,10 +408,6 @@ def test_include_with_invalid_expression def test_include_attribute_with_invalid_expression template = '{% include "snippet", key: foo=>bar %}' - with_error_modes(:lax, :strict) do - refute_nil(Template.parse(template)) - end - with_error_modes(:strict2) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } assert_match(/Unexpected character =/, error.message) diff --git a/test/integration/tags/render_tag_test.rb b/test/integration/tags/render_tag_test.rb index 0bd09d088..440342c33 100644 --- a/test/integration/tags/render_tag_test.rb +++ b/test/integration/tags/render_tag_test.rb @@ -106,14 +106,6 @@ def test_dynamically_choosen_templates_are_not_allowed end def test_strict2_parsing_errors - with_error_modes(:lax, :strict) do - assert_template_result( - 'hello value1 value2', - '{% render "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', - partials: { 'snippet' => 'hello {{ arg1 }} {{ arg2 }}' }, - ) - end - with_error_modes(:strict2) do assert_syntax_error( '{% render "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', @@ -318,10 +310,6 @@ def test_render_tag_renders_error_with_template_name_from_template_factory def test_render_with_invalid_expression template = '{% render "snippet" with foo=>bar %}' - with_error_modes(:lax, :strict) do - refute_nil(Template.parse(template)) - end - with_error_modes(:strict2) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } assert_match(/Unexpected character =/, error.message) @@ -331,10 +319,6 @@ def test_render_with_invalid_expression def test_render_attribute_with_invalid_expression template = '{% render "snippet", key: foo=>bar %}' - with_error_modes(:lax, :strict) do - refute_nil(Template.parse(template)) - end - with_error_modes(:strict2) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } assert_match(/Unexpected character =/, error.message) diff --git a/test/integration/tags/table_row_test.rb b/test/integration/tags/table_row_test.rb index 45628ce9d..e8f7adb4f 100644 --- a/test/integration/tags/table_row_test.rb +++ b/test/integration/tags/table_row_test.rb @@ -188,29 +188,6 @@ def test_tablerow_loop_drop_attributes assert_template_result(expected_output, template) end - def test_table_row_renders_correct_error_message_for_invalid_parameters - assert_template_result( - "Liquid error (line 1): invalid integer", - '{% tablerow n in (1...10) limit:true %} {{n}} {% endtablerow %}', - error_mode: :warn, - render_errors: true, - ) - - assert_template_result( - "Liquid error (line 1): invalid integer", - '{% tablerow n in (1...10) offset:true %} {{n}} {% endtablerow %}', - error_mode: :warn, - render_errors: true, - ) - - assert_template_result( - "Liquid error (line 1): invalid integer", - '{% tablerow n in (1...10) cols:true %} {{n}} {% endtablerow %}', - render_errors: true, - error_mode: :warn, - ) - end - def test_table_row_handles_interrupts assert_template_result( "\n 1 \n", @@ -439,10 +416,6 @@ def test_tablerow_with_invalid_attribute_strict_vs_strict2 12345 OUTPUT - with_error_modes(:lax, :strict) do - assert_template_result(expected, template) - end - with_error_modes(:strict2) do error = assert_raises(SyntaxError) { Template.parse(template) } assert_match(/Invalid attribute 'invalid_attr'/, error.message) @@ -452,14 +425,6 @@ def test_tablerow_with_invalid_attribute_strict_vs_strict2 def test_tablerow_with_invalid_expression_strict_vs_strict2 template = '{% tablerow i in (1..5) limit: foo=>bar %}{{ i }}{% endtablerow %}' - with_error_modes(:lax, :strict) do - expected = <<~OUTPUT - - - OUTPUT - assert_template_result(expected, template) - end - with_error_modes(:strict2) do error = assert_raises(SyntaxError) { Template.parse(template) } assert_match(/Unexpected character =/, error.message) diff --git a/test/integration/template_test.rb b/test/integration/template_test.rb index 92ece017c..2a90d34f7 100644 --- a/test/integration/template_test.rb +++ b/test/integration/template_test.rb @@ -259,7 +259,7 @@ def test_undefined_variables end def test_nil_value_does_not_raise - t = Template.parse("some{{x}}thing", error_mode: :strict) + t = Template.parse("some{{x}}thing", error_mode: :rigid) result = t.render!({ 'x' => nil }, strict_variables: true) assert_equal(0, t.errors.count) diff --git a/test/integration/variable_test.rb b/test/integration/variable_test.rb index e272b96b9..82e7ac3e2 100644 --- a/test/integration/variable_test.rb +++ b/test/integration/variable_test.rb @@ -176,48 +176,9 @@ def test_double_nested_variable_lookup ) end - def test_variable_lookup_should_not_hang_with_invalid_syntax - Timeout.timeout(1) do - assert_template_result( - 'bar', - "{{['foo'}}", - { - 'foo' => 'bar', - }, - error_mode: :lax, - ) - end - - very_long_key = "1234567890" * 100 - - template_list = [ - "{{['#{very_long_key}']}}", # valid - "{{['#{very_long_key}'}}", # missing closing bracket - "{{[['#{very_long_key}']}}", # extra open bracket - ] - - template_list.each do |template| - Timeout.timeout(1) do - assert_template_result( - 'bar', - template, - { - very_long_key => 'bar', - }, - error_mode: :lax, - ) - end - end - end - def test_filter_with_single_trailing_comma template = '{{ "hello" | append: "world", }}' - with_error_modes(:strict) do - error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } - assert_match(/is not a valid expression/, error.message) - end - with_error_modes(:strict2) do assert_template_result('helloworld', template) end @@ -226,11 +187,6 @@ def test_filter_with_single_trailing_comma def test_multiple_filters_with_trailing_commas template = '{{ "hello" | append: "1", | append: "2", }}' - with_error_modes(:strict) do - error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } - assert_match(/is not a valid expression/, error.message) - end - with_error_modes(:strict2) do assert_template_result('hello12', template) end @@ -239,11 +195,6 @@ def test_multiple_filters_with_trailing_commas def test_filter_with_colon_but_no_arguments template = '{{ "test" | upcase: }}' - with_error_modes(:strict) do - error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } - assert_match(/is not a valid expression/, error.message) - end - with_error_modes(:strict2) do assert_template_result('TEST', template) end @@ -252,11 +203,6 @@ def test_filter_with_colon_but_no_arguments def test_filter_chain_with_colon_no_args template = '{{ "test" | append: "x" | upcase: }}' - with_error_modes(:strict) do - error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } - assert_match(/is not a valid expression/, error.message) - end - with_error_modes(:strict2) do assert_template_result('TESTX', template) end @@ -265,11 +211,6 @@ def test_filter_chain_with_colon_no_args def test_combining_trailing_comma_and_empty_args template = '{{ "test" | append: "x", | upcase: }}' - with_error_modes(:strict) do - error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } - assert_match(/is not a valid expression/, error.message) - end - with_error_modes(:strict2) do assert_template_result('TESTX', template) end diff --git a/test/test_helper.rb b/test/test_helper.rb index 293ea4766..70ba9b694 100755 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -8,7 +8,7 @@ require 'liquid.rb' require 'liquid/profiler' -mode = :strict +mode = :rigid if (env_mode = ENV['LIQUID_PARSER_MODE']) puts "-- #{env_mode.upcase} ERROR MODE" mode = env_mode.to_sym diff --git a/test/unit/condition_unit_test.rb b/test/unit/condition_unit_test.rb index eb466f7a9..b77eed9c4 100644 --- a/test/unit/condition_unit_test.rb +++ b/test/unit/condition_unit_test.rb @@ -167,9 +167,9 @@ def test_default_context_is_deprecated end def test_parse_expression_in_strict_mode - environment = Environment.build(error_mode: :strict) + environment = Environment.build(error_mode: :rigid) parse_context = ParseContext.new(environment: environment) - result = Condition.parse_expression(parse_context, 'product.title') + result = Condition.parse_expression(parse_context, 'product.title', safe: true) assert_instance_of(VariableLookup, result) assert_equal('product', result.name) diff --git a/test/unit/parse_context_unit_test.rb b/test/unit/parse_context_unit_test.rb index f53cd1148..d1b32efcd 100644 --- a/test/unit/parse_context_unit_test.rb +++ b/test/unit/parse_context_unit_test.rb @@ -38,12 +38,6 @@ def test_safe_parse_expression_raises_syntax_error_for_invalid_expression end def test_parse_expression_with_variable_lookup - result_strict = strict_parse_context.parse_expression('product.title') - - assert_instance_of(VariableLookup, result_strict) - assert_equal('product', result_strict.name) - assert_equal(['title'], result_strict.lookups) - error = assert_raises(Liquid::InternalError) do strict2_parse_context.parse_expression('product.title') end @@ -66,9 +60,6 @@ def test_parse_expression_with_safe_true end def test_parse_expression_with_empty_string - result_strict = strict_parse_context.parse_expression('') - assert_nil(result_strict) - error = assert_raises(Liquid::InternalError) do strict2_parse_context.parse_expression('') end @@ -77,9 +68,6 @@ def test_parse_expression_with_empty_string end def test_parse_expression_with_empty_string_and_safe_true - result_strict = strict_parse_context.parse_expression('', safe: true) - assert_nil(result_strict) - result_strict2 = strict2_parse_context.parse_expression('', safe: true) assert_nil(result_strict2) end @@ -109,12 +97,6 @@ def test_parse_expression_with_whitespace_in_strict2_mode private - def strict_parse_context - @strict_parse_context ||= ParseContext.new( - environment: Environment.build(error_mode: :strict), - ) - end - def strict2_parse_context @strict2_parse_context ||= ParseContext.new( environment: Environment.build(error_mode: :strict2), diff --git a/test/unit/partial_cache_unit_test.rb b/test/unit/partial_cache_unit_test.rb index 623241b40..72555d52b 100644 --- a/test/unit/partial_cache_unit_test.rb +++ b/test/unit/partial_cache_unit_test.rb @@ -184,7 +184,7 @@ def test_includes_error_mode_into_template_cache }, ) - [:lax, :warn, :strict, :strict2].each do |error_mode| + [:strict2].each do |error_mode| Liquid::PartialCache.load( 'my_partial', context: context, @@ -193,7 +193,7 @@ def test_includes_error_mode_into_template_cache end assert_equal( - ["my_partial:lax", "my_partial:warn", "my_partial:strict", "my_partial:strict2"], + ["my_partial:strict2"], context.registers[:cached_partials].keys, ) end diff --git a/test/unit/tags/case_tag_unit_test.rb b/test/unit/tags/case_tag_unit_test.rb index 9d7d34a39..183071c49 100644 --- a/test/unit/tags/case_tag_unit_test.rb +++ b/test/unit/tags/case_tag_unit_test.rb @@ -20,10 +20,6 @@ def test_case_with_trailing_element {%- endcase -%} LIQUID - with_error_modes(:lax, :strict) do - assert_template_result("one", template) - end - with_error_modes(:strict2) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } @@ -41,10 +37,6 @@ def test_case_when_with_trailing_element {%- endcase -%} LIQUID - with_error_modes(:lax, :strict) do - assert_template_result("one", template) - end - with_error_modes(:strict2) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } @@ -62,7 +54,7 @@ def test_case_when_with_comma {%- endcase -%} LIQUID - with_error_modes(:lax, :strict, :strict2) do + with_error_modes(:strict2) do assert_template_result("one", template) end end @@ -77,7 +69,7 @@ def test_case_when_with_or {%- endcase -%} LIQUID - with_error_modes(:lax, :strict, :strict2) do + with_error_modes(:strict2) do assert_template_result("one", template) end end @@ -93,10 +85,6 @@ def test_case_with_invalid_expression LIQUID assigns = { 'foo' => { 'bar' => 'baz' } } - with_error_modes(:lax, :strict) do - assert_template_result("one", template, assigns) - end - with_error_modes(:strict2) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } @@ -115,10 +103,6 @@ def test_case_when_with_invalid_expression LIQUID assigns = { 'foo' => { 'bar' => 'baz' } } - with_error_modes(:lax, :strict) do - assert_template_result("one", template, assigns) - end - with_error_modes(:strict2) do error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } diff --git a/test/unit/variable_unit_test.rb b/test/unit/variable_unit_test.rb index 03f5862aa..590aa3f38 100644 --- a/test/unit/variable_unit_test.rb +++ b/test/unit/variable_unit_test.rb @@ -72,12 +72,6 @@ def test_filters_without_whitespace assert_equal([['replace', ['foo', 'bar']], ['textileze', []]], var.filters) end - def test_symbol - var = create_variable("http://disney.com/logo.gif | image: 'med' ", error_mode: :lax) - assert_equal(VariableLookup.new('http://disney.com/logo.gif'), var.name) - assert_equal([['image', ['med']]], var.filters) - end - def test_string_to_filter var = create_variable("'http://disney.com/logo.gif' | image: 'med' ") assert_equal('http://disney.com/logo.gif', var.name) @@ -108,7 +102,7 @@ def test_dashes assert_equal(VariableLookup.new('foo-bar'), create_variable('foo-bar').name) assert_equal(VariableLookup.new('foo-bar-2'), create_variable('foo-bar-2').name) - with_error_modes(:strict) do + with_error_modes(:strict2) do assert_raises(Liquid::SyntaxError) { create_variable('foo - bar') } assert_raises(Liquid::SyntaxError) { create_variable('-foo') } assert_raises(Liquid::SyntaxError) { create_variable('2foo') } @@ -131,36 +125,6 @@ def test_filter_with_keyword_arguments assert_equal([['things', [], { 'greeting' => 'world', 'farewell' => 'goodbye' }]], var.filters) end - 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_modes(:strict) do - assert_raises(SyntaxError) do - create_variable(%( number_of_comments | pluralize: 'comment': 'comments' )) - end - end - end - def test_strict2_filter_argument_parsing with_error_modes(:strict2) do # optional colon From db61632e0f88c8fc967f10002d6a87dc6eb9a8f0 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Thu, 6 Nov 2025 15:14:05 -0500 Subject: [PATCH 02/32] Rename strict2_parse to parse_markup --- lib/liquid/parser_switching.rb | 10 +++------- lib/liquid/tags/case.rb | 10 +++------- lib/liquid/tags/cycle.rb | 2 +- lib/liquid/tags/for.rb | 2 +- lib/liquid/tags/if.rb | 2 +- lib/liquid/tags/include.rb | 2 +- lib/liquid/tags/render.rb | 6 +++--- lib/liquid/tags/table_row.rb | 2 +- lib/liquid/variable.rb | 6 +++--- 9 files changed, 17 insertions(+), 25 deletions(-) diff --git a/lib/liquid/parser_switching.rb b/lib/liquid/parser_switching.rb index 4b35e2139..ca4267668 100644 --- a/lib/liquid/parser_switching.rb +++ b/lib/liquid/parser_switching.rb @@ -3,19 +3,15 @@ module Liquid module ParserSwitching def parse_with_selected_parser(markup) - strict2_parse_with_error_context(markup) - end - - private - - def strict2_parse_with_error_context(markup) - strict2_parse(markup) + parse_markup(markup) rescue SyntaxError => e e.line_number = line_number e.markup_context = markup_context(markup) raise e end + private + def markup_context(markup) "in \"#{markup.strip}\"" end diff --git a/lib/liquid/tags/case.rb b/lib/liquid/tags/case.rb index e24cf5a1b..0a466c44c 100644 --- a/lib/liquid/tags/case.rb +++ b/lib/liquid/tags/case.rb @@ -83,7 +83,7 @@ def render_to_output_buffer(context, output) private - def strict2_parse(markup) + def parse_markup(markup) parser = @parse_context.new_parser(markup) @left = safe_parse_expression(parser) parser.consume(:end_of_string) @@ -92,14 +92,10 @@ def strict2_parse(markup) def record_when_condition(markup) body = new_body - if strict2_mode? - parse_strict2_when(markup, body) - else - parse_lax_when(markup, body) - end + parse_when(markup, body) end - def parse_strict2_when(markup, body) + def parse_when(markup, body) parser = @parse_context.new_parser(markup) loop do diff --git a/lib/liquid/tags/cycle.rb b/lib/liquid/tags/cycle.rb index 2d372cee5..a98d25392 100644 --- a/lib/liquid/tags/cycle.rb +++ b/lib/liquid/tags/cycle.rb @@ -54,7 +54,7 @@ def render_to_output_buffer(context, output) private # cycle [name:] expression(, expression)* - def strict2_parse(markup) + def parse_markup(markup) p = @parse_context.new_parser(markup) @variables = [] diff --git a/lib/liquid/tags/for.rb b/lib/liquid/tags/for.rb index 4b89b7972..cac9d2393 100644 --- a/lib/liquid/tags/for.rb +++ b/lib/liquid/tags/for.rb @@ -70,7 +70,7 @@ def render_to_output_buffer(context, output) protected - def strict2_parse(markup) + def parse_markup(markup) p = @parse_context.new_parser(markup) @variable_name = p.consume(:id) raise SyntaxError, options[:locale].t("errors.syntax.for_invalid_in") unless p.id?('in') diff --git a/lib/liquid/tags/if.rb b/lib/liquid/tags/if.rb index 287fe78ae..06c752da7 100644 --- a/lib/liquid/tags/if.rb +++ b/lib/liquid/tags/if.rb @@ -77,7 +77,7 @@ def parse_expression(markup, safe: false) Condition.parse_expression(parse_context, markup, safe: safe) end - def strict2_parse(markup) + def parse_markup(markup) p = @parse_context.new_parser(markup) condition = parse_binary_comparisons(p) p.consume(:end_of_string) diff --git a/lib/liquid/tags/include.rb b/lib/liquid/tags/include.rb index 49bc032f6..b50c68a66 100644 --- a/lib/liquid/tags/include.rb +++ b/lib/liquid/tags/include.rb @@ -81,7 +81,7 @@ def render_to_output_buffer(context, output) alias_method :parse_context, :options private :parse_context - def strict2_parse(markup) + def parse_markup(markup) p = @parse_context.new_parser(markup) @template_name_expr = safe_parse_expression(p) diff --git a/lib/liquid/tags/render.rb b/lib/liquid/tags/render.rb index 43d0bca82..5c071c803 100644 --- a/lib/liquid/tags/render.rb +++ b/lib/liquid/tags/render.rb @@ -84,10 +84,10 @@ def render_tag(context, output) end # render (string) (with|for expression)? (as id)? (key: value)* - def strict2_parse(markup) + def parse_markup(markup) p = @parse_context.new_parser(markup) - @template_name_expr = parse_expression(strict2_template_name(p), safe: true) + @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 @alias_name = p.consume(:id) if p.id?("as") @@ -106,7 +106,7 @@ def strict2_parse(markup) p.consume(:end_of_string) end - def strict2_template_name(p) + def template_name(p) p.consume(:string) end diff --git a/lib/liquid/tags/table_row.rb b/lib/liquid/tags/table_row.rb index 07c6da23a..c91147541 100644 --- a/lib/liquid/tags/table_row.rb +++ b/lib/liquid/tags/table_row.rb @@ -33,7 +33,7 @@ def initialize(tag_name, markup, options) parse_with_selected_parser(markup) end - def strict2_parse(markup) + def parse_markup(markup) p = @parse_context.new_parser(markup) @variable_name = p.consume(:id) diff --git a/lib/liquid/variable.rb b/lib/liquid/variable.rb index 0a0ad21ec..fcc723515 100644 --- a/lib/liquid/variable.rb +++ b/lib/liquid/variable.rb @@ -41,14 +41,14 @@ def markup_context(markup) "in \"{{#{markup}}}\"" end - def strict2_parse(markup) + def parse_markup(markup) @filters = [] p = @parse_context.new_parser(markup) return if p.look(:end_of_string) @name = parse_context.safe_parse_expression(p) - @filters << strict2_parse_filter_expressions(p) while p.consume?(:pipe) + @filters << parse_filter_expressions(p) while p.consume?(:pipe) p.consume(:end_of_string) end @@ -99,7 +99,7 @@ def disabled_tags # argument = (positional_argument | keyword_argument) # positional_argument = expression # keyword_argument = id ":" expression - def strict2_parse_filter_expressions(p) + def parse_filter_expressions(p) filtername = p.consume(:id) filter_args = [] keyword_args = {} From f0f63d44c026288cf941bf6c95ff0270c13c64d7 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Tue, 2 Dec 2025 09:20:03 -0500 Subject: [PATCH 03/32] Remove :error_mode --- Rakefile | 18 +---- lib/liquid/environment.rb | 10 +-- lib/liquid/parse_context.rb | 20 ++--- lib/liquid/partial_cache.rb | 2 +- lib/liquid/template.rb | 11 --- performance/benchmark.rb | 1 - performance/memory_profile.rb | 2 - performance/profile.rb | 1 - test/integration/assign_test.rb | 3 +- test/integration/context_test.rb | 8 +- test/integration/error_handling_test.rb | 11 +-- test/integration/parsing_quirks_test.rb | 30 +++---- test/integration/tags/cycle_tag_test.rb | 52 +++++------- test/integration/tags/include_tag_test.rb | 40 ++++----- test/integration/tags/render_tag_test.rb | 30 +++---- test/integration/tags/table_row_test.rb | 99 ++++++++--------------- test/integration/template_test.rb | 2 +- test/integration/variable_test.rb | 20 ++--- test/test_helper.rb | 29 ++----- test/unit/condition_unit_test.rb | 20 ++--- test/unit/parse_context_unit_test.rb | 73 +++++++---------- test/unit/partial_cache_unit_test.rb | 16 ++-- test/unit/tags/case_tag_unit_test.rb | 32 +++----- test/unit/variable_unit_test.rb | 58 +++++++------ 24 files changed, 202 insertions(+), 386 deletions(-) diff --git a/Rakefile b/Rakefile index b8939316b..086a5a81e 100755 --- a/Rakefile +++ b/Rakefile @@ -33,14 +33,12 @@ task :rubocop do end end -desc('runs test suite with strict2 parser') +desc('runs test suite') task :test do - ENV['LIQUID_PARSER_MODE'] = 'strict2' Rake::Task['base_test'].reenable Rake::Task['base_test'].invoke if RUBY_ENGINE == 'ruby' || RUBY_ENGINE == 'truffleruby' - ENV['LIQUID_PARSER_MODE'] = 'strict2' Rake::Task['integration_test'].reenable Rake::Task['integration_test'].invoke end @@ -63,13 +61,10 @@ task release: :build do end namespace :benchmark do - desc "Run the liquid benchmark with strict2 parsing" - task :strict2 do - ruby "./performance/benchmark.rb strict2" - end - desc "Run the liquid benchmark" - task run: [:strict2] + task :run do + ruby "./performance/benchmark.rb" + end desc "Run unit benchmarks" namespace :unit do @@ -101,11 +96,6 @@ namespace :profile do task :run do ruby "./performance/profile.rb" end - - desc "Run the liquid profile/performance coverage with strict2 parsing" - task :strict2 do - ruby "./performance/profile.rb strict2" - end end namespace :memory_profile do diff --git a/lib/liquid/environment.rb b/lib/liquid/environment.rb index 095b2b091..7834f49c3 100644 --- a/lib/liquid/environment.rb +++ b/lib/liquid/environment.rb @@ -4,10 +4,6 @@ module Liquid # The Environment is the container for all configuration options of Liquid, such as # the registered tags, filters, and the default error mode. class Environment - # The default error mode for all templates. This can be overridden on a - # per-template basis. - attr_accessor :error_mode - # The tags that are available to use in the template. attr_accessor :tags @@ -33,17 +29,14 @@ class << self # the template. # @param file_system The default file system that is used # to load templates from. - # @param error_mode [Symbol] The default error mode for all templates - # (:strict2). # @param exception_renderer [Proc] The exception renderer that is used to # render exceptions. # @yieldparam environment [Environment] The environment instance that is being built. # @return [Environment] The new environment instance. - def build(tags: nil, file_system: nil, error_mode: nil, exception_renderer: nil) + def build(tags: nil, file_system: nil, exception_renderer: nil) ret = new ret.tags = tags if tags ret.file_system = file_system if file_system - ret.error_mode = error_mode if error_mode ret.exception_renderer = exception_renderer if exception_renderer yield ret if block_given? ret.freeze @@ -75,7 +68,6 @@ def dangerously_override(environment) # @api private def initialize @tags = Tags::STANDARD_TAGS.dup - @error_mode = :strict2 @strainer_template = Class.new(StrainerTemplate).tap do |klass| klass.add_filter(StandardFilters) end diff --git a/lib/liquid/parse_context.rb b/lib/liquid/parse_context.rb index 855acc64e..8161a0369 100644 --- a/lib/liquid/parse_context.rb +++ b/lib/liquid/parse_context.rb @@ -3,7 +3,7 @@ module Liquid class ParseContext attr_accessor :locale, :line_number, :trim_whitespace, :depth - attr_reader :partial, :warnings, :error_mode, :environment + attr_reader :partial, :warnings, :environment def initialize(options = Const::EMPTY_HASH) @environment = options.fetch(:environment, Environment.default) @@ -55,16 +55,12 @@ def safe_parse_expression(parser) end def parse_expression(markup, safe: false) - if !safe && @error_mode == :strict2 - # parse_expression is a widely used API. To maintain backward - # compatibility while raising awareness about strict2 parser standards, - # the safe flag supports API users make a deliberate decision. - # - # In strict2 mode, markup MUST come from a string returned by the parser - # (e.g., parser.expression). We're not calling the parser here to - # prevent redundant parser overhead. - raise Liquid::InternalError, "unsafe parse_expression cannot be used in strict2 mode" - end + # markup MUST come from a string returned by the parser + # (e.g., parser.expression). We're not calling the parser here to + # prevent redundant parser overhead. The `safe` opt-in + # exists to ensure it is not accidentally still called with + # the result of a regex. + raise Liquid::InternalError, "unsafe parse_expression cannot be used" unless safe Expression.parse(markup, @string_scanner, @expression_cache) end @@ -72,8 +68,6 @@ def parse_expression(markup, safe: false) def partial=(value) @partial = value @options = value ? partial_options : @template_options - - @error_mode = @options[:error_mode] || @environment.error_mode end def partial_options diff --git a/lib/liquid/partial_cache.rb b/lib/liquid/partial_cache.rb index f49d14d90..5cca4e04b 100644 --- a/lib/liquid/partial_cache.rb +++ b/lib/liquid/partial_cache.rb @@ -4,7 +4,7 @@ module Liquid class PartialCache def self.load(template_name, context:, parse_context:) cached_partials = context.registers[:cached_partials] - cache_key = "#{template_name}:#{parse_context.error_mode}" + cache_key = template_name.to_s cached = cached_partials[cache_key] return cached if cached diff --git a/lib/liquid/template.rb b/lib/liquid/template.rb index 6bb03dfc3..e638a01d7 100644 --- a/lib/liquid/template.rb +++ b/lib/liquid/template.rb @@ -21,17 +21,6 @@ class Template attr_reader :profiler class << self - # Sets how strict the parser should be. - # :strict2 enforces correct syntax for all tags - def error_mode=(mode) - Deprecations.warn("Template.error_mode=", "Environment#error_mode=") - Environment.default.error_mode = mode - end - - def error_mode - Environment.default.error_mode - end - def default_exception_renderer=(renderer) Deprecations.warn("Template.default_exception_renderer=", "Environment#exception_renderer=") Environment.default.exception_renderer = renderer diff --git a/performance/benchmark.rb b/performance/benchmark.rb index b61e9057c..c8aa6769d 100644 --- a/performance/benchmark.rb +++ b/performance/benchmark.rb @@ -4,7 +4,6 @@ require_relative 'theme_runner' RubyVM::YJIT.enable if defined?(RubyVM::YJIT) -Liquid::Environment.default.error_mode = ARGV.first.to_sym if ARGV.first profiler = ThemeRunner.new diff --git a/performance/memory_profile.rb b/performance/memory_profile.rb index e2934297a..fb7312d55 100644 --- a/performance/memory_profile.rb +++ b/performance/memory_profile.rb @@ -53,8 +53,6 @@ def sanitize(string) end end -Liquid::Template.error_mode = ARGV.first.to_sym if ARGV.first - runner = ThemeRunner.new Profiler.run do |x| x.profile('parse') { runner.compile } diff --git a/performance/profile.rb b/performance/profile.rb index 70740778d..f756fb202 100644 --- a/performance/profile.rb +++ b/performance/profile.rb @@ -3,7 +3,6 @@ require 'stackprof' require_relative 'theme_runner' -Liquid::Template.error_mode = ARGV.first.to_sym if ARGV.first profiler = ThemeRunner.new profiler.run diff --git a/test/integration/assign_test.rb b/test/integration/assign_test.rb index a88941ae2..69163ab96 100644 --- a/test/integration/assign_test.rb +++ b/test/integration/assign_test.rb @@ -39,11 +39,10 @@ def test_assign_syntax_error assert_match_syntax_error(/assign/, '{% assign foo not values %}.') end - def test_assign_uses_error_mode + def test_assign_throws_on_unsupported_syntax assert_match_syntax_error( "Expected dotdot but found pipe", "{% assign foo = ('X' | downcase) %}", - error_mode: :rigid, ) end diff --git a/test/integration/context_test.rb b/test/integration/context_test.rb index db68da45f..e1952e634 100644 --- a/test/integration/context_test.rb +++ b/test/integration/context_test.rb @@ -632,11 +632,9 @@ def notice(output) end def test_has_key_will_not_add_an_error_for_missing_keys - with_error_modes(:rigid) do - context = Context.new - context.key?('unknown') - assert_empty(context.errors) - end + context = Context.new + context.key?('unknown') + assert_empty(context.errors) end def test_key_lookup_will_raise_for_missing_keys_when_strict_variables_is_enabled diff --git a/test/integration/error_handling_test.rb b/test/integration/error_handling_test.rb index 9f07b1f87..9de179f2d 100644 --- a/test/integration/error_handling_test.rb +++ b/test/integration/error_handling_test.rb @@ -67,10 +67,8 @@ def test_missing_endtag_parse_time_error end def test_unrecognized_operator - with_error_modes(:rigid) do - assert_raises(SyntaxError) do - Liquid::Template.parse(' {% if 1 =! 2 %}ok{% endif %} ') - end + assert_raises(SyntaxError) do + Liquid::Template.parse(' {% if 1 =! 2 %}ok{% endif %} ') end end @@ -107,7 +105,6 @@ def test_parsing_strict_with_line_numbers_adds_numbers_to_lexer_errors bla ', - error_mode: :rigid, line_numbers: true, ) end @@ -131,12 +128,12 @@ def test_syntax_errors_in_nested_blocks_have_correct_line_number def test_strict_error_messages err = assert_raises(SyntaxError) do - Liquid::Template.parse(' {% if 1 =! 2 %}ok{% endif %} ', error_mode: :rigid) + Liquid::Template.parse(' {% if 1 =! 2 %}ok{% endif %} ') end assert_equal('Liquid syntax error: Unexpected character = in "1 =! 2"', err.message) err = assert_raises(SyntaxError) do - Liquid::Template.parse('{{%%%}}', error_mode: :rigid) + Liquid::Template.parse('{{%%%}}') end assert_equal('Liquid syntax error: Unexpected character % in "{{%%%}}"', err.message) end diff --git a/test/integration/parsing_quirks_test.rb b/test/integration/parsing_quirks_test.rb index 523512098..75954f93f 100644 --- a/test/integration/parsing_quirks_test.rb +++ b/test/integration/parsing_quirks_test.rb @@ -31,31 +31,25 @@ def test_raise_on_label_and_no_close_bracets_percent def test_error_on_empty_filter assert(Template.parse("{{test}}")) - with_error_modes(:rigid) do - assert_raises(Liquid::SyntaxError) { Template.parse("{{|test}}") } - assert_raises(Liquid::SyntaxError) { Template.parse("{{test |a|b|}}") } - end + assert_raises(Liquid::SyntaxError) { Template.parse("{{|test}}") } + assert_raises(Liquid::SyntaxError) { Template.parse("{{test |a|b|}}") } end def test_meaningless_parens_error - with_error_modes(:rigid) do - assert_raises(SyntaxError) do - markup = "a == 'foo' or (b == 'bar' and c == 'baz') or false" - Template.parse("{% if #{markup} %} YES {% endif %}") - end + assert_raises(SyntaxError) do + markup = "a == 'foo' or (b == 'bar' and c == 'baz') or false" + Template.parse("{% if #{markup} %} YES {% endif %}") end end def test_unexpected_characters_syntax_error - with_error_modes(:rigid) do - assert_raises(SyntaxError) do - markup = "true && false" - Template.parse("{% if #{markup} %} YES {% endif %}") - end - assert_raises(SyntaxError) do - markup = "false || true" - Template.parse("{% if #{markup} %} YES {% endif %}") - end + assert_raises(SyntaxError) do + markup = "true && false" + Template.parse("{% if #{markup} %} YES {% endif %}") + end + assert_raises(SyntaxError) do + markup = "false || true" + Template.parse("{% if #{markup} %} YES {% endif %}") end end diff --git a/test/integration/tags/cycle_tag_test.rb b/test/integration/tags/cycle_tag_test.rb index cf3fdea18..9edd19958 100644 --- a/test/integration/tags/cycle_tag_test.rb +++ b/test/integration/tags/cycle_tag_test.rb @@ -91,23 +91,21 @@ def test_cycle_tag_without_arguments assert_match(/Syntax Error in 'cycle' - Valid syntax: cycle \[name :\] var/, error.message) end - def test_cycle_tag_with_error_mode + def test_cycle_tag_unsupported_legacy_quirk # QuotedFragment is more permissive than what Parser#expression allows. template1 = "{% assign 5 = 'b' %}{% cycle .5, .4 %}" template2 = "{% cycle .5: 'a', 'b' %}" - with_error_modes(:strict2) do - error1 = assert_raises(Liquid::SyntaxError) { Template.parse(template1) } - error2 = assert_raises(Liquid::SyntaxError) { Template.parse(template2) } + error1 = assert_raises(Liquid::SyntaxError) { Template.parse(template1) } + error2 = assert_raises(Liquid::SyntaxError) { Template.parse(template2) } - expected_error = /Liquid syntax error: \[:dot, "."\] is not a valid expression/ + expected_error = /Liquid syntax error: \[:dot, "."\] is not a valid expression/ - assert_match(expected_error, error1.message) - assert_match(expected_error, error2.message) - end + assert_match(expected_error, error1.message) + assert_match(expected_error, error2.message) end - def test_cycle_with_trailing_elements + def test_cycle_with_trailing_elements_legacy_syntax assignments = "{% assign a = 'A' %}{% assign n = 'N' %}" template1 = "#{assignments}{% cycle 'a' 'b', 'c' %}" @@ -116,21 +114,19 @@ def test_cycle_with_trailing_elements template4 = "#{assignments}{% cycle n e: 'a', 'b', 'c' %}" template5 = "#{assignments}{% cycle n e 'a', 'b', 'c' %}" - with_error_modes(:strict2) 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) } + 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/ + 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 + 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 def test_cycle_name_with_invalid_expression @@ -140,10 +136,8 @@ def test_cycle_name_with_invalid_expression {% endfor %} LIQUID - with_error_modes(:strict2) do - error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } - assert_match(/Unexpected character =/, error.message) - end + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/Unexpected character =/, error.message) end def test_cycle_variable_with_invalid_expression @@ -153,9 +147,7 @@ def test_cycle_variable_with_invalid_expression {% endfor %} LIQUID - with_error_modes(:strict2) do - error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } - assert_match(/Unexpected character =/, error.message) - end + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/Unexpected character =/, error.message) end end diff --git a/test/integration/tags/include_tag_test.rb b/test/integration/tags/include_tag_test.rb index e35addcd9..b911ace16 100644 --- a/test/integration/tags/include_tag_test.rb +++ b/test/integration/tags/include_tag_test.rb @@ -204,15 +204,13 @@ def test_dynamically_choosen_template ) end - def test_strict2_parsing_errors - with_error_modes(:strict2) do - assert_syntax_error( - '{% include "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', - ) - assert_syntax_error( - '{% include "snippet" | filter %}', - ) - end + def test_parsing_errors_for_legacy_quirk + assert_syntax_error( + '{% include "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', + ) + assert_syntax_error( + '{% include "snippet" | filter %}', + ) end def test_optional_commas @@ -293,10 +291,10 @@ def test_passing_options_to_included_templates env = Liquid::Environment.build(file_system: TestFileSystem.new) assert_raises(Liquid::SyntaxError) do - Template.parse("{% include template %}", error_mode: :rigid, environment: env).render!("template" => '{{ "X" || downcase }}') + Template.parse("{% include template %}", environment: env).render!("template" => '{{ "X" || downcase }}') end assert_raises(Liquid::SyntaxError) do - Template.parse("{% include template %}", error_mode: :rigid, include_options_blacklist: [:locale], environment: env).render!("template" => '{{ "X" || downcase }}') + Template.parse("{% include template %}", include_options_blacklist: [:locale], environment: env).render!("template" => '{{ "X" || downcase }}') end end @@ -351,7 +349,7 @@ def test_including_with_strict_variables file_system: StubFileSystem.new('simple' => 'simple'), ) - template = Liquid::Template.parse("{% include 'simple' %}", error_mode: :warn, environment: env) + template = Liquid::Template.parse("{% include 'simple' %}", environment: env) template.render(nil, strict_variables: true) assert_equal([], template.errors) @@ -390,27 +388,21 @@ def test_render_tag_renders_error_with_template_name_from_template_factory def test_include_template_with_invalid_expression template = "{% include foo=>bar %}" - with_error_modes(:strict2) do - error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } - assert_match(/Unexpected character =/, error.message) - end + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/Unexpected character =/, error.message) end def test_include_with_invalid_expression template = '{% include "snippet" with foo=>bar %}' - with_error_modes(:strict2) do - error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } - assert_match(/Unexpected character =/, error.message) - end + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/Unexpected character =/, error.message) end def test_include_attribute_with_invalid_expression template = '{% include "snippet", key: foo=>bar %}' - with_error_modes(:strict2) do - error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } - assert_match(/Unexpected character =/, error.message) - end + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/Unexpected character =/, error.message) end end # IncludeTagTest diff --git a/test/integration/tags/render_tag_test.rb b/test/integration/tags/render_tag_test.rb index 440342c33..2c69ba547 100644 --- a/test/integration/tags/render_tag_test.rb +++ b/test/integration/tags/render_tag_test.rb @@ -105,15 +105,13 @@ def test_dynamically_choosen_templates_are_not_allowed assert_syntax_error("{% assign name = 'snippet' %}{% render name %}") end - def test_strict2_parsing_errors - with_error_modes(:strict2) do - assert_syntax_error( - '{% render "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', - ) - assert_syntax_error( - '{% render "snippet" | filter %}', - ) - end + def test_parsing_errors_legacy_syntax + assert_syntax_error( + '{% render "snippet" !!! arg1: "value1" ~~~ arg2: "value2" %}', + ) + assert_syntax_error( + '{% render "snippet" | filter %}', + ) end def test_optional_commas @@ -309,19 +307,13 @@ def test_render_tag_renders_error_with_template_name_from_template_factory def test_render_with_invalid_expression template = '{% render "snippet" with foo=>bar %}' - - with_error_modes(:strict2) do - error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } - assert_match(/Unexpected character =/, error.message) - end + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/Unexpected character =/, error.message) end def test_render_attribute_with_invalid_expression template = '{% render "snippet", key: foo=>bar %}' - - with_error_modes(:strict2) do - error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } - assert_match(/Unexpected character =/, error.message) - end + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + assert_match(/Unexpected character =/, error.message) end end diff --git a/test/integration/tags/table_row_test.rb b/test/integration/tags/table_row_test.rb index e8f7adb4f..59b4cc25e 100644 --- a/test/integration/tags/table_row_test.rb +++ b/test/integration/tags/table_row_test.rb @@ -236,7 +236,7 @@ def test_table_row_does_not_leak_interrupts ) end - def test_tablerow_with_cols_attribute_in_strict2_mode + def test_tablerow_with_cols_attribute template = <<~LIQUID.chomp {% tablerow i in (1..6) cols: 3 %}{{ i }}{% endtablerow %} LIQUID @@ -247,12 +247,10 @@ def test_tablerow_with_cols_attribute_in_strict2_mode 456 OUTPUT - with_error_modes(:strict2) do - assert_template_result(expected, template) - end + assert_template_result(expected, template) end - def test_tablerow_with_limit_attribute_in_strict2_mode + def test_tablerow_with_limit_attribute template = <<~LIQUID.chomp {% tablerow i in (1..10) limit: 3 %}{{ i }}{% endtablerow %} LIQUID @@ -262,12 +260,10 @@ def test_tablerow_with_limit_attribute_in_strict2_mode 123 OUTPUT - with_error_modes(:strict2) do - assert_template_result(expected, template) - end + assert_template_result(expected, template) end - def test_tablerow_with_offset_attribute_in_strict2_mode + def test_tablerow_with_offset_attribute template = <<~LIQUID.chomp {% tablerow i in (1..5) offset: 2 %}{{ i }}{% endtablerow %} LIQUID @@ -277,12 +273,10 @@ def test_tablerow_with_offset_attribute_in_strict2_mode 345 OUTPUT - with_error_modes(:strict2) do - assert_template_result(expected, template) - end + assert_template_result(expected, template) end - def test_tablerow_with_range_attribute_in_strict2_mode + def test_tablerow_with_range_attribute template = <<~LIQUID.chomp {% tablerow i in (1..3) range: (1..10) %}{{ i }}{% endtablerow %} LIQUID @@ -292,12 +286,10 @@ def test_tablerow_with_range_attribute_in_strict2_mode 123 OUTPUT - with_error_modes(:strict2) do - assert_template_result(expected, template) - end + assert_template_result(expected, template) end - def test_tablerow_with_multiple_attributes_in_strict2_mode + def test_tablerow_with_multiple_attributes template = <<~LIQUID.chomp {% tablerow i in (1..10) cols: 2, limit: 4, offset: 1 %}{{ i }}{% endtablerow %} LIQUID @@ -308,12 +300,10 @@ def test_tablerow_with_multiple_attributes_in_strict2_mode 45 OUTPUT - with_error_modes(:strict2) do - assert_template_result(expected, template) - end + assert_template_result(expected, template) end - def test_tablerow_with_variable_collection_in_strict2_mode + def test_tablerow_with_variable_collection template = <<~LIQUID.chomp {% tablerow n in numbers cols: 2 %}{{ n }}{% endtablerow %} LIQUID @@ -324,12 +314,10 @@ def test_tablerow_with_variable_collection_in_strict2_mode 34 OUTPUT - with_error_modes(:strict2) do - assert_template_result(expected, template, { 'numbers' => [1, 2, 3, 4] }) - end + assert_template_result(expected, template, { 'numbers' => [1, 2, 3, 4] }) end - def test_tablerow_with_dotted_access_in_strict2_mode + def test_tablerow_with_dotted_access template = <<~LIQUID.chomp {% tablerow n in obj.numbers cols: 2 %}{{ n }}{% endtablerow %} LIQUID @@ -340,12 +328,10 @@ def test_tablerow_with_dotted_access_in_strict2_mode 34 OUTPUT - with_error_modes(:strict2) do - assert_template_result(expected, template, { 'obj' => { 'numbers' => [1, 2, 3, 4] } }) - end + assert_template_result(expected, template, { 'obj' => { 'numbers' => [1, 2, 3, 4] } }) end - def test_tablerow_with_bracketed_access_in_strict2_mode + def test_tablerow_with_bracketed_access template = <<~LIQUID.chomp {% tablerow n in obj["numbers"] cols: 2 %}{{ n }}{% endtablerow %} LIQUID @@ -355,12 +341,10 @@ def test_tablerow_with_bracketed_access_in_strict2_mode 1020 OUTPUT - with_error_modes(:strict2) do - assert_template_result(expected, template, { 'obj' => { 'numbers' => [10, 20] } }) - end + assert_template_result(expected, template, { 'obj' => { 'numbers' => [10, 20] } }) end - def test_tablerow_without_attributes_in_strict2_mode + def test_tablerow_without_attributes template = <<~LIQUID.chomp {% tablerow i in (1..3) %}{{ i }}{% endtablerow %} LIQUID @@ -370,30 +354,24 @@ def test_tablerow_without_attributes_in_strict2_mode 123 OUTPUT - with_error_modes(:strict2) do - assert_template_result(expected, template) - end + assert_template_result(expected, template) end - def test_tablerow_without_in_keyword_in_strict2_mode + def test_tablerow_without_in_keyword template = '{% tablerow i (1..10) %}{{ i }}{% endtablerow %}' - with_error_modes(:strict2) do - error = assert_raises(SyntaxError) { Template.parse(template) } - assert_equal("Liquid syntax error: For loops require an 'in' clause in \"i (1..10)\"", error.message) - end + error = assert_raises(SyntaxError) { Template.parse(template) } + assert_equal("Liquid syntax error: For loops require an 'in' clause in \"i (1..10)\"", error.message) end - def test_tablerow_with_multiple_invalid_attributes_reports_first_in_strict2_mode + def test_tablerow_with_multiple_invalid_attributes_reports_first template = '{% tablerow i in (1..10) invalid1: 5, invalid2: 10 %}{{ i }}{% endtablerow %}' - with_error_modes(:strict2) do - error = assert_raises(SyntaxError) { Template.parse(template) } - assert_equal("Liquid syntax error: Invalid attribute 'invalid1' in tablerow loop. Valid attributes are cols, limit, offset, and range in \"i in (1..10) invalid1: 5, invalid2: 10\"", error.message) - end + error = assert_raises(SyntaxError) { Template.parse(template) } + assert_equal("Liquid syntax error: Invalid attribute 'invalid1' in tablerow loop. Valid attributes are cols, limit, offset, and range in \"i in (1..10) invalid1: 5, invalid2: 10\"", error.message) end - def test_tablerow_with_empty_collection_in_strict2_mode + def test_tablerow_with_empty_collection template = <<~LIQUID.chomp {% tablerow i in empty_array cols: 2 %}{{ i }}{% endtablerow %} LIQUID @@ -403,31 +381,18 @@ def test_tablerow_with_empty_collection_in_strict2_mode OUTPUT - with_error_modes(:strict2) do - assert_template_result(expected, template, { 'empty_array' => [] }) - end + assert_template_result(expected, template, { 'empty_array' => [] }) end - def test_tablerow_with_invalid_attribute_strict_vs_strict2 + def test_tablerow_with_invalid_attribute template = '{% tablerow i in (1..5) invalid_attr: 10 %}{{ i }}{% endtablerow %}' - - expected = <<~OUTPUT - - 12345 - OUTPUT - - with_error_modes(:strict2) do - error = assert_raises(SyntaxError) { Template.parse(template) } - assert_match(/Invalid attribute 'invalid_attr'/, error.message) - end + error = assert_raises(SyntaxError) { Template.parse(template) } + assert_match(/Invalid attribute 'invalid_attr'/, error.message) end - def test_tablerow_with_invalid_expression_strict_vs_strict2 + def test_tablerow_with_invalid_expression template = '{% tablerow i in (1..5) limit: foo=>bar %}{{ i }}{% endtablerow %}' - - with_error_modes(:strict2) do - error = assert_raises(SyntaxError) { Template.parse(template) } - assert_match(/Unexpected character =/, error.message) - end + error = assert_raises(SyntaxError) { Template.parse(template) } + assert_match(/Unexpected character =/, error.message) end end diff --git a/test/integration/template_test.rb b/test/integration/template_test.rb index 2a90d34f7..b09f2584a 100644 --- a/test/integration/template_test.rb +++ b/test/integration/template_test.rb @@ -259,7 +259,7 @@ def test_undefined_variables end def test_nil_value_does_not_raise - t = Template.parse("some{{x}}thing", error_mode: :rigid) + t = Template.parse("some{{x}}thing") result = t.render!({ 'x' => nil }, strict_variables: true) assert_equal(0, t.errors.count) diff --git a/test/integration/variable_test.rb b/test/integration/variable_test.rb index 82e7ac3e2..38096e41d 100644 --- a/test/integration/variable_test.rb +++ b/test/integration/variable_test.rb @@ -179,40 +179,30 @@ def test_double_nested_variable_lookup def test_filter_with_single_trailing_comma template = '{{ "hello" | append: "world", }}' - with_error_modes(:strict2) do - assert_template_result('helloworld', template) - end + assert_template_result('helloworld', template) end def test_multiple_filters_with_trailing_commas template = '{{ "hello" | append: "1", | append: "2", }}' - with_error_modes(:strict2) do - assert_template_result('hello12', template) - end + assert_template_result('hello12', template) end def test_filter_with_colon_but_no_arguments template = '{{ "test" | upcase: }}' - with_error_modes(:strict2) do - assert_template_result('TEST', template) - end + assert_template_result('TEST', template) end def test_filter_chain_with_colon_no_args template = '{{ "test" | append: "x" | upcase: }}' - with_error_modes(:strict2) do - assert_template_result('TESTX', template) - end + assert_template_result('TESTX', template) end def test_combining_trailing_comma_and_empty_args template = '{{ "test" | append: "x", | upcase: }}' - with_error_modes(:strict2) do - assert_template_result('TESTX', template) - end + assert_template_result('TESTX', template) end end diff --git a/test/test_helper.rb b/test/test_helper.rb index 70ba9b694..0c9596f4e 100755 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -8,13 +8,6 @@ require 'liquid.rb' require 'liquid/profiler' -mode = :rigid -if (env_mode = ENV['LIQUID_PARSER_MODE']) - puts "-- #{env_mode.upcase} ERROR MODE" - mode = env_mode.to_sym -end -Liquid::Environment.default.error_mode = mode - if Minitest.const_defined?('Test') # We're on Minitest 5+. Nothing to do here. else @@ -34,27 +27,27 @@ module Assertions def assert_template_result( expected, template, assigns = {}, - message: nil, partials: nil, error_mode: Liquid::Environment.default.error_mode, render_errors: false, + message: nil, partials: nil, render_errors: false, template_factory: nil ) file_system = StubFileSystem.new(partials || {}) environment = Liquid::Environment.build(file_system: file_system) - template = Liquid::Template.parse(template, line_numbers: true, error_mode: error_mode&.to_sym, environment: environment) + template = Liquid::Template.parse(template, line_numbers: true, environment: environment) registers = Liquid::Registers.new(file_system: file_system, template_factory: template_factory) context = Liquid::Context.build(static_environments: assigns, rethrow_errors: !render_errors, registers: registers, environment: environment) output = template.render(context) assert_equal(expected, output, message) end - def assert_match_syntax_error(match, template, error_mode: nil) + def assert_match_syntax_error(match, template) exception = assert_raises(Liquid::SyntaxError) do - Template.parse(template, line_numbers: true, error_mode: error_mode&.to_sym).render + Template.parse(template, line_numbers: true).render end assert_match(match, exception.message) end - def assert_syntax_error(template, error_mode: nil) - assert_match_syntax_error("", template, error_mode: error_mode) + def assert_syntax_error(template) + assert_match_syntax_error("", template) end def assert_usage_increment(name, times: 1) @@ -82,16 +75,6 @@ def with_global_filter(*globals, &blk) Environment.dangerously_override(environment, &blk) end - def with_error_modes(*modes) - old_mode = Liquid::Environment.default.error_mode - modes.each do |mode| - Liquid::Environment.default.error_mode = mode - yield - end - ensure - Liquid::Environment.default.error_mode = old_mode - end - def with_custom_tag(tag_name, tag_class, &block) environment = Liquid::Environment.default.dup environment.register_tag(tag_name, tag_class) diff --git a/test/unit/condition_unit_test.rb b/test/unit/condition_unit_test.rb index b77eed9c4..584951e21 100644 --- a/test/unit/condition_unit_test.rb +++ b/test/unit/condition_unit_test.rb @@ -166,8 +166,8 @@ def test_default_context_is_deprecated assert_includes(err.lines.map(&:strip), expected) end - def test_parse_expression_in_strict_mode - environment = Environment.build(error_mode: :rigid) + def test_parse_expression_with_safe_true + environment = Environment.build parse_context = ParseContext.new(environment: environment) result = Condition.parse_expression(parse_context, 'product.title', safe: true) @@ -176,25 +176,15 @@ def test_parse_expression_in_strict_mode assert_equal(['title'], result.lookups) end - def test_parse_expression_in_strict2_mode_raises_internal_error - environment = Environment.build(error_mode: :strict2) + def test_parse_expression_raises_internal_error_if_not_safe + environment = Environment.build parse_context = ParseContext.new(environment: environment) error = assert_raises(Liquid::InternalError) do Condition.parse_expression(parse_context, 'product.title') end - assert_match(/unsafe parse_expression cannot be used in strict2 mode/, error.message) - end - - def test_parse_expression_with_safe_true_in_strict2_mode - environment = Environment.build(error_mode: :strict2) - parse_context = ParseContext.new(environment: environment) - result = Condition.parse_expression(parse_context, 'product.title', safe: true) - - assert_instance_of(VariableLookup, result) - assert_equal('product', result.name) - assert_equal(['title'], result.lookups) + assert_match(/unsafe parse_expression cannot be used/, error.message) end private diff --git a/test/unit/parse_context_unit_test.rb b/test/unit/parse_context_unit_test.rb index d1b32efcd..f75a48045 100644 --- a/test/unit/parse_context_unit_test.rb +++ b/test/unit/parse_context_unit_test.rb @@ -6,100 +6,81 @@ class ParseContextUnitTest < Minitest::Test include Liquid def test_safe_parse_expression_with_variable_lookup - parser_strict = strict_parse_context.new_parser('product.title') - result_strict = strict_parse_context.safe_parse_expression(parser_strict) + parser = parse_context.new_parser('product.title') + result = parse_context.safe_parse_expression(parser) - parser_strict2 = strict2_parse_context.new_parser('product.title') - result_strict2 = strict2_parse_context.safe_parse_expression(parser_strict2) - - assert_instance_of(VariableLookup, result_strict) - assert_equal('product', result_strict.name) - assert_equal(['title'], result_strict.lookups) - - assert_instance_of(VariableLookup, result_strict2) - assert_equal('product', result_strict2.name) - assert_equal(['title'], result_strict2.lookups) + assert_instance_of(VariableLookup, result) + assert_equal('product', result.name) + assert_equal(['title'], result.lookups) end def test_safe_parse_expression_raises_syntax_error_for_invalid_expression - parser_strict = strict_parse_context.new_parser('') - parser_strict2 = strict2_parse_context.new_parser('') - - error_strict = assert_raises(Liquid::SyntaxError) do - strict_parse_context.safe_parse_expression(parser_strict) - end - assert_match(/is not a valid expression/, error_strict.message) + parser = parse_context.new_parser('') - error_strict2 = assert_raises(Liquid::SyntaxError) do - strict2_parse_context.safe_parse_expression(parser_strict2) + error = assert_raises(Liquid::SyntaxError) do + parse_context.safe_parse_expression(parser) end - assert_match(/is not a valid expression/, error_strict2.message) + assert_match(/is not a valid expression/, error.message) end def test_parse_expression_with_variable_lookup error = assert_raises(Liquid::InternalError) do - strict2_parse_context.parse_expression('product.title') + parse_context.parse_expression('product.title') end - assert_match(/unsafe parse_expression cannot be used in strict2 mode/, error.message) + assert_match(/unsafe parse_expression cannot be used/, error.message) end def test_parse_expression_with_safe_true - result_strict = strict_parse_context.parse_expression('product.title', safe: true) + result = parse_context.parse_expression('product.title', safe: true) - assert_instance_of(VariableLookup, result_strict) - assert_equal('product', result_strict.name) - assert_equal(['title'], result_strict.lookups) - - result_strict2 = strict2_parse_context.parse_expression('product.title', safe: true) - - assert_instance_of(VariableLookup, result_strict2) - assert_equal('product', result_strict2.name) - assert_equal(['title'], result_strict2.lookups) + assert_instance_of(VariableLookup, result) + assert_equal('product', result.name) + assert_equal(['title'], result.lookups) end def test_parse_expression_with_empty_string error = assert_raises(Liquid::InternalError) do - strict2_parse_context.parse_expression('') + parse_context.parse_expression('') end - assert_match(/unsafe parse_expression cannot be used in strict2 mode/, error.message) + assert_match(/unsafe parse_expression cannot be used/, error.message) end def test_parse_expression_with_empty_string_and_safe_true - result_strict2 = strict2_parse_context.parse_expression('', safe: true) - assert_nil(result_strict2) + result = parse_context.parse_expression('', safe: true) + assert_nil(result) end def test_safe_parse_expression_advances_parser_pointer - parser = strict2_parse_context.new_parser('foo, bar') + parser = parse_context.new_parser('foo, bar') # safe_parse_expression consumes "foo" - first_result = strict2_parse_context.safe_parse_expression(parser) + first_result = parse_context.safe_parse_expression(parser) assert_instance_of(VariableLookup, first_result) assert_equal('foo', first_result.name) parser.consume(:comma) # safe_parse_expression consumes "bar" - second_result = strict2_parse_context.safe_parse_expression(parser) + second_result = parse_context.safe_parse_expression(parser) assert_instance_of(VariableLookup, second_result) assert_equal('bar', second_result.name) parser.consume(:end_of_string) end - def test_parse_expression_with_whitespace_in_strict2_mode - result = strict2_parse_context.parse_expression(' ', safe: true) + def test_parse_expression_with_whitespace + result = parse_context.parse_expression(' ', safe: true) assert_nil(result) end private - def strict2_parse_context - @strict2_parse_context ||= ParseContext.new( - environment: Environment.build(error_mode: :strict2), + def parse_context + @parse_context ||= ParseContext.new( + environment: Environment.build, ) end end diff --git a/test/unit/partial_cache_unit_test.rb b/test/unit/partial_cache_unit_test.rb index 72555d52b..cb8e2d354 100644 --- a/test/unit/partial_cache_unit_test.rb +++ b/test/unit/partial_cache_unit_test.rb @@ -175,7 +175,7 @@ def test_uses_template_name_from_template_factory assert_equal('some/path/my_partial', partial.name) end - def test_includes_error_mode_into_template_cache + def test_cache_key template_factory = StubTemplateFactory.new context = Liquid::Context.build( registers: { @@ -184,16 +184,14 @@ def test_includes_error_mode_into_template_cache }, ) - [:strict2].each do |error_mode| - Liquid::PartialCache.load( - 'my_partial', - context: context, - parse_context: Liquid::ParseContext.new(error_mode: error_mode), - ) - end + Liquid::PartialCache.load( + 'my_partial', + context: context, + parse_context: Liquid::ParseContext.new, + ) assert_equal( - ["my_partial:strict2"], + ["my_partial"], context.registers[:cached_partials].keys, ) end diff --git a/test/unit/tags/case_tag_unit_test.rb b/test/unit/tags/case_tag_unit_test.rb index 183071c49..0d70c6c0a 100644 --- a/test/unit/tags/case_tag_unit_test.rb +++ b/test/unit/tags/case_tag_unit_test.rb @@ -20,11 +20,9 @@ def test_case_with_trailing_element {%- endcase -%} LIQUID - with_error_modes(:strict2) do - error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } - assert_match(/Expected end_of_string but found/, error.message) - end + assert_match(/Expected end_of_string but found/, error.message) end def test_case_when_with_trailing_element @@ -37,11 +35,9 @@ def test_case_when_with_trailing_element {%- endcase -%} LIQUID - with_error_modes(:strict2) do - error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } - assert_match(/Expected end_of_string but found/, error.message) - end + assert_match(/Expected end_of_string but found/, error.message) end def test_case_when_with_comma @@ -54,9 +50,7 @@ def test_case_when_with_comma {%- endcase -%} LIQUID - with_error_modes(:strict2) do - assert_template_result("one", template) - end + assert_template_result("one", template) end def test_case_when_with_or @@ -69,9 +63,7 @@ def test_case_when_with_or {%- endcase -%} LIQUID - with_error_modes(:strict2) do - assert_template_result("one", template) - end + assert_template_result("one", template) end def test_case_with_invalid_expression @@ -85,11 +77,9 @@ def test_case_with_invalid_expression LIQUID assigns = { 'foo' => { 'bar' => 'baz' } } - with_error_modes(:strict2) do - error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + error = assert_raises(Liquid::SyntaxError) { Template.parse(template, assigns) } - assert_match(/Unexpected character =/, error.message) - end + assert_match(/Unexpected character =/, error.message) end def test_case_when_with_invalid_expression @@ -103,10 +93,8 @@ def test_case_when_with_invalid_expression LIQUID assigns = { 'foo' => { 'bar' => 'baz' } } - with_error_modes(:strict2) do - error = assert_raises(Liquid::SyntaxError) { Template.parse(template) } + error = assert_raises(Liquid::SyntaxError) { Template.parse(template, assigns) } - assert_match(/Unexpected character =/, error.message) - end + assert_match(/Unexpected character =/, error.message) end end diff --git a/test/unit/variable_unit_test.rb b/test/unit/variable_unit_test.rb index 590aa3f38..6379c6ece 100644 --- a/test/unit/variable_unit_test.rb +++ b/test/unit/variable_unit_test.rb @@ -102,11 +102,9 @@ def test_dashes assert_equal(VariableLookup.new('foo-bar'), create_variable('foo-bar').name) assert_equal(VariableLookup.new('foo-bar-2'), create_variable('foo-bar-2').name) - with_error_modes(:strict2) do - assert_raises(Liquid::SyntaxError) { create_variable('foo - bar') } - assert_raises(Liquid::SyntaxError) { create_variable('-foo') } - assert_raises(Liquid::SyntaxError) { create_variable('2foo') } - end + assert_raises(Liquid::SyntaxError) { create_variable('foo - bar') } + assert_raises(Liquid::SyntaxError) { create_variable('-foo') } + assert_raises(Liquid::SyntaxError) { create_variable('2foo') } end def test_string_with_special_chars @@ -125,40 +123,38 @@ def test_filter_with_keyword_arguments assert_equal([['things', [], { 'greeting' => 'world', 'farewell' => 'goodbye' }]], var.filters) end - def test_strict2_filter_argument_parsing - with_error_modes(:strict2) do - # optional colon - var = create_variable(%(n | f1 | f2:)) - assert_equal([['f1', []], ['f2', []]], var.filters) + def test_filter_argument_parsing + # optional colon + var = create_variable(%(n | f1 | f2:)) + assert_equal([['f1', []], ['f2', []]], var.filters) - # missing argument throws error - assert_raises(SyntaxError) { create_variable(%(n | f1: ,)) } - assert_raises(SyntaxError) { create_variable(%(n | f1: ,| f2)) } + # missing argument throws error + assert_raises(SyntaxError) { create_variable(%(n | f1: ,)) } + assert_raises(SyntaxError) { create_variable(%(n | f1: ,| f2)) } - # arg requires colon - assert_raises(SyntaxError) { create_variable(%(n | f1 1)) } + # arg requires colon + assert_raises(SyntaxError) { create_variable(%(n | f1 1)) } - # trailing comma doesn't throw - create_variable(%(n | f1: 1, 2, 3, | f2:)) + # trailing comma doesn't throw + create_variable(%(n | f1: 1, 2, 3, | f2:)) - # missing comma throws error - assert_raises(SyntaxError) { create_variable(%(n | filter: 1 2, 3)) } + # missing comma throws error + assert_raises(SyntaxError) { create_variable(%(n | filter: 1 2, 3)) } - # positional and kwargs parsing - var = create_variable(%(n | filter: 1, 2, 3 | filter2: k1: 1, k2: 2)) - assert_equal([['filter', [1, 2, 3]], ['filter2', [], { "k1" => 1, "k2" => 2 }]], var.filters) + # positional and kwargs parsing + var = create_variable(%(n | filter: 1, 2, 3 | filter2: k1: 1, k2: 2)) + assert_equal([['filter', [1, 2, 3]], ['filter2', [], { "k1" => 1, "k2" => 2 }]], var.filters) - # positional and kwargs mixed - var = create_variable(%(n | filter: 'a', 'b', key1: 1, key2: 2, 'c')) - assert_equal([["filter", ["a", "b", "c"], { "key1" => 1, "key2" => 2 }]], var.filters) + # positional and kwargs mixed + var = create_variable(%(n | filter: 'a', 'b', key1: 1, key2: 2, 'c')) + assert_equal([["filter", ["a", "b", "c"], { "key1" => 1, "key2" => 2 }]], var.filters) - # positional and kwargs intermixed (pos1, key1: val1, pos2) - var = create_variable(%(n | link_to: class: "black", "https://example.com", title: "title")) - assert_equal([['link_to', ["https://example.com"], { "class" => "black", "title" => "title" }]], var.filters) + # positional and kwargs intermixed (pos1, key1: val1, pos2) + var = create_variable(%(n | link_to: class: "black", "https://example.com", title: "title")) + 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')) } - end + # string key throws + assert_raises(SyntaxError) { create_variable(%(n | pluralize: 'comment': 'comments')) } end def test_output_raw_source_of_variable From c12fc1b1bc97a4313cfa6bb0dbef9b0c83b38bb4 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Mon, 1 Dec 2025 10:19:34 -0500 Subject: [PATCH 04/32] Update changelog with planned changes for 6.0.0 --- History.md | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/History.md b/History.md index 94feaabfd..2bb2bbb3b 100644 --- a/History.md +++ b/History.md @@ -1,5 +1,37 @@ # Liquid Change Log +## 6.0.0 + +### Architectural changes + +### Features +* (TODO) Add support for boolean expressions everywhere + * As variable output `{{ a or b }}` + * As filter argument `{{ collection | where: 'prop', a or b }}` + * As tag argument `{% render 'snip', enabled: a or b %}` + * As conditional tag argument `{% if cond %}` (extending previous behaviour) +* (TODO) Add support for subexpression prioritization and associativity + * In ascending order of priority: + * Logical: `and`, `or` (right to left) + * Equality: `==`, `!=`, `<>` (left to right) + * Comparison: `>`, `>=`, `<`, `<=`, `contains` (left to right) + - For example, this is now supported + * `{{ a > b == c < d or e == f }}` which is equivalent to + * `{{ ((a > b) == (c < d)) or (e == f) }}` +- (TODO) Add support for parenthesized expressions + * e.g. `(a or b) and c` + +### Breaking changes +* We are removing the Environment's `error_mode` option. + * `:warn` is no longer supported + * `:lax` and `lax_parse` is no longer supported + * `:strict` and `strict_parse` is no longer supported + * `strict2_parse` is renamed to `parse_markup` + +### Migrating from `^5.11.0` +- In custom tags that include `ParserSwitching`, rename `strict2_parse` to `parse_markup` +- Remove code depending on `:error_mode` + ## 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] * Rename the `:rigid` error mode to `:strict2` and display a warning when users attempt to use the `:rigid` mode [Guilherme Carreiro] From d22ead1dae90db1ada8ff243125797694ee4adfa Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Wed, 3 Dec 2025 10:04:10 -0500 Subject: [PATCH 05/32] Remove warnings system --- History.md | 3 ++- lib/liquid/context.rb | 7 +------ lib/liquid/parse_context.rb | 5 ++--- lib/liquid/template.rb | 3 +-- test/integration/template_test.rb | 10 ---------- 5 files changed, 6 insertions(+), 22 deletions(-) diff --git a/History.md b/History.md index 2bb2bbb3b..8fd6ef372 100644 --- a/History.md +++ b/History.md @@ -22,11 +22,12 @@ * e.g. `(a or b) and c` ### Breaking changes -* We are removing the Environment's `error_mode` option. +* The Environment's `error_mode` option has been removed. * `:warn` is no longer supported * `:lax` and `lax_parse` is no longer supported * `:strict` and `strict_parse` is no longer supported * `strict2_parse` is renamed to `parse_markup` +* The `warnings` system has been removed. ### Migrating from `^5.11.0` - In custom tags that include `ParserSwitching`, rename `strict2_parse` to `parse_markup` diff --git a/lib/liquid/context.rb b/lib/liquid/context.rb index 433b6d003..1db6bb663 100644 --- a/lib/liquid/context.rb +++ b/lib/liquid/context.rb @@ -60,10 +60,6 @@ def initialize(environments = {}, outer_scope = {}, registers = {}, rethrow_erro end # rubocop:enable Metrics/ParameterLists - def warnings - @warnings ||= [] - end - def strainer @strainer ||= @environment.create_strainer(self, @filters) end @@ -157,7 +153,6 @@ def new_isolated_subcontext subcontext.filters = @filters subcontext.strainer = nil subcontext.errors = errors - subcontext.warnings = warnings subcontext.disabled_tags = @disabled_tags end end @@ -244,7 +239,7 @@ def tag_disabled?(tag_name) protected - attr_writer :base_scope_depth, :warnings, :errors, :strainer, :filters, :disabled_tags + attr_writer :base_scope_depth, :errors, :strainer, :filters, :disabled_tags private diff --git a/lib/liquid/parse_context.rb b/lib/liquid/parse_context.rb index 8161a0369..96413eb61 100644 --- a/lib/liquid/parse_context.rb +++ b/lib/liquid/parse_context.rb @@ -3,14 +3,13 @@ module Liquid class ParseContext attr_accessor :locale, :line_number, :trim_whitespace, :depth - attr_reader :partial, :warnings, :environment + attr_reader :partial, :environment def initialize(options = Const::EMPTY_HASH) @environment = options.fetch(:environment, Environment.default) @template_options = options ? options.dup : {} - @locale = @template_options[:locale] ||= I18n.new - @warnings = [] + @locale = @template_options[:locale] ||= I18n.new # constructing new StringScanner in Lexer, Tokenizer, etc is expensive # This StringScanner will be shared by all of them diff --git a/lib/liquid/template.rb b/lib/liquid/template.rb index e638a01d7..a81fec423 100644 --- a/lib/liquid/template.rb +++ b/lib/liquid/template.rb @@ -16,7 +16,7 @@ module Liquid # class Template attr_accessor :root, :name - attr_reader :resource_limits, :warnings + attr_reader :resource_limits attr_reader :profiler @@ -209,7 +209,6 @@ def configure_options(options) ParseContext.new(opts) end - @warnings = parse_context.warnings parse_context end diff --git a/test/integration/template_test.rb b/test/integration/template_test.rb index b09f2584a..e29ca3b6b 100644 --- a/test/integration/template_test.rb +++ b/test/integration/template_test.rb @@ -44,16 +44,6 @@ def test_instance_assigns_persist_on_same_template_object_between_parses assert_equal('from instance assigns', t.parse("{{ foo }}").render!) end - def test_warnings_is_not_exponential_time - str = "false" - 100.times do - str = "{% if true %}true{% else %}#{str}{% endif %}" - end - - t = Template.parse(str) - assert_equal([], Timeout.timeout(1) { t.warnings }) - end - def test_instance_assigns_persist_on_same_template_parsing_between_renders t = Template.new.parse("{{ foo }}{% assign foo = 'foo' %}{{ foo }}") assert_equal('foo', t.render!) From c688f705281bc26615a39d9312aa1e689e523ecd Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Tue, 2 Dec 2025 11:03:26 -0500 Subject: [PATCH 06/32] 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/parse_context_unit_test.rb | 12 ++++++------ 11 files changed, 33 insertions(+), 33 deletions(-) diff --git a/History.md b/History.md index 8fd6ef372..cf6382eb8 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 96413eb61..e6279d767 100644 --- a/lib/liquid/parse_context.rb +++ b/lib/liquid/parse_context.rb @@ -37,7 +37,7 @@ def new_block_body 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 @@ def new_tokenizer(source, start_line_number: nil, for_liquid_tag: false) ) 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 645dfa3a1..86de2eab3 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 @@ def expression 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 374ee511e..501eec611 100644 --- a/lib/liquid/tag.rb +++ b/lib/liquid/tag.rb @@ -68,10 +68,6 @@ def blank? 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 0a466c44c..6fd8586dc 100644 --- a/lib/liquid/tags/case.rb +++ b/lib/liquid/tags/case.rb @@ -85,7 +85,7 @@ def render_to_output_buffer(context, output) 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 @@ def parse_when(markup, body) parser = @parse_context.new_parser(markup) loop do - expr = safe_parse_expression(parser) + 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 a98d25392..4847824eb 100644 --- a/lib/liquid/tags/cycle.rb +++ b/lib/liquid/tags/cycle.rb @@ -61,14 +61,14 @@ def parse_markup(markup) 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 @@ def parse_markup(markup) 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 b50c68a66..02d8bf4e1 100644 --- a/lib/liquid/tags/include.rb +++ b/lib/liquid/tags/include.rb @@ -84,8 +84,8 @@ def render_to_output_buffer(context, output) 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 @@ def parse_markup(markup) 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 5c071c803..5a2ef0675 100644 --- a/lib/liquid/tags/render.rb +++ b/lib/liquid/tags/render.rb @@ -89,7 +89,7 @@ def parse_markup(markup) @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 @@ def parse_markup(markup) 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 c91147541..3b4cc5866 100644 --- a/lib/liquid/tags/table_row.rb +++ b/lib/liquid/tags/table_row.rb @@ -42,7 +42,7 @@ def parse_markup(markup) 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 @@ def parse_markup(markup) 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 fcc723515..c73523587 100644 --- a/lib/liquid/variable.rb +++ b/lib/liquid/variable.rb @@ -47,7 +47,7 @@ def parse_markup(markup) 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 @@ 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) + 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/parse_context_unit_test.rb b/test/unit/parse_context_unit_test.rb index f75a48045..89923a751 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 @@ def test_safe_parse_expression_raises_syntax_error_for_invalid_expression 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 @@ def test_parse_expression_with_empty_string_and_safe_true 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) From 158feb0d01367c074bf9bbe83a1a969d7e4f62d6 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Tue, 2 Dec 2025 11:36:44 -0500 Subject: [PATCH 07/32] Move parse_expression to Parser.unsafe_parse_expression - Add Parser#string - Add Parser#unsafe_parse_expression - Add private Parser#parse_expression - Remove ParseContext.parse_expression - Remove Tag.parse_expression - Condition.parse_expression now takes a parser as argument --- History.md | 2 + lib/liquid/condition.rb | 5 +- lib/liquid/parse_context.rb | 11 -- lib/liquid/parser.rb | 16 ++- lib/liquid/parser.rb.orig | 165 +++++++++++++++++++++++++++ lib/liquid/tag.rb | 6 - lib/liquid/tags/for.rb | 11 +- lib/liquid/tags/if.rb | 8 +- lib/liquid/tags/render.rb | 4 +- test/unit/condition_unit_test.rb | 15 ++- test/unit/parse_context_unit_test.rb | 34 +----- 11 files changed, 207 insertions(+), 70 deletions(-) create mode 100644 lib/liquid/parser.rb.orig diff --git a/History.md b/History.md index cf6382eb8..cf93d993b 100644 --- a/History.md +++ b/History.md @@ -29,6 +29,8 @@ * `strict2_parse` is renamed to `parse_markup` * The `warnings` system has been removed. * `safe_parse_expression` has been moved to `Parser.expression_node` +* `parse_expression` methods have been moved to `Parser#unsafe_parse_expression` + * Use `Parser#expression_node`, `Parser#string`, etc. instead ### Migrating from `^5.11.0` - In custom tags that include `ParserSwitching`, rename `strict2_parse` to `parse_markup` diff --git a/lib/liquid/condition.rb b/lib/liquid/condition.rb index 9ab350f07..ac1908773 100644 --- a/lib/liquid/condition.rb +++ b/lib/liquid/condition.rb @@ -48,8 +48,9 @@ def self.operators @@operators end - def self.parse_expression(parse_context, markup, safe: false) - @@method_literals[markup] || parse_context.parse_expression(markup, safe: safe) + def self.parse_expression(parser) + markup = parser.expression + @@method_literals[markup] || parser.unsafe_parse_expression(markup) end attr_reader :attachment, :child_condition diff --git a/lib/liquid/parse_context.rb b/lib/liquid/parse_context.rb index e6279d767..c8cc35e38 100644 --- a/lib/liquid/parse_context.rb +++ b/lib/liquid/parse_context.rb @@ -49,17 +49,6 @@ def new_tokenizer(source, start_line_number: nil, for_liquid_tag: false) ) 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 - # prevent redundant parser overhead. The `safe` opt-in - # exists to ensure it is not accidentally still called with - # the result of a regex. - raise Liquid::InternalError, "unsafe parse_expression cannot be used" unless safe - - Expression.parse(markup, @string_scanner, @expression_cache) - end - def partial=(value) @partial = value @options = value ? partial_options : @template_options diff --git a/lib/liquid/parser.rb b/lib/liquid/parser.rb index 86de2eab3..606644071 100644 --- a/lib/liquid/parser.rb +++ b/lib/liquid/parser.rb @@ -72,9 +72,9 @@ def expression end end + def expression_node - expr = expression - Expression.parse(expr, @ss, @cache) + parse_expression(expression) end def argument @@ -104,5 +104,17 @@ def variable_lookups end str end + + # Assumes safe input. For cases where you need the string. + # Don't use this unless you're sure about what you're doing. + def unsafe_parse_expression(markup) + parse_expression(markup) + end + + private + + def parse_expression(markup) + Expression.parse(markup, @ss, @cache) + end end end diff --git a/lib/liquid/parser.rb.orig b/lib/liquid/parser.rb.orig new file mode 100644 index 000000000..3e177e8ef --- /dev/null +++ b/lib/liquid/parser.rb.orig @@ -0,0 +1,165 @@ +# frozen_string_literal: true + +module Liquid + class Parser + 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 + + def jump(point) + @p = point + end + + def consume(type = nil) + token = @tokens[@p] + if type && token[0] != type + raise SyntaxError, "Expected #{type} but found #{@tokens[@p].first}" + end + @p += 1 + token[1] + end + + # Only consumes the token if it matches the type + # Returns the token's contents if it was consumed + # or false otherwise. + def consume?(type) + token = @tokens[@p] + return false unless token && token[0] == type + @p += 1 + token[1] + end + + # Like consume? Except for an :id token of a certain name + def id?(str) + token = @tokens[@p] + return false unless token && token[0] == :id + return false unless token[1] == str + @p += 1 + token[1] + end + + def look(type, ahead = 0) + tok = @tokens[@p + ahead] + return false unless tok + tok[0] == type + end + + def expression + token = @tokens[@p] + case token[0] + when :id + str = consume + str << variable_lookups + when :open_square + str = consume.dup + str << expression + str << consume(:close_square) + str << variable_lookups + when :string, :number + consume + when :open_round + consume + first = expression + consume(:dotdot) + last = expression + consume(:close_round) + "(#{first}..#{last})" + else + raise SyntaxError, "#{token} is not a valid expression" + end + end + + def string + parse_expression(consume(:string)) + end + + def expression_node + parse_expression(expression) + end + +<<<<<<< HEAD + # Assumes safe input. For cases where you need the string. + # Don't use this unless you're sure about what you're doing. + def unsafe_parse_expression(markup) + parse_expression(markup) + end + + def argument +======= + def string + consume(:string)[1..-2] + end + + def argument_string +>>>>>>> 68476f39 (Move unsafe_parse_expression to the end) + str = +"" + # might be a keyword argument (identifier: expression) + if look(:id) && look(:colon, 1) + str << consume << consume << ' ' + end + + str << expression + str + end + + def variable_lookups + str = +"" + loop do + if look(:open_square) + str << consume + str << expression + str << consume(:close_square) + elsif look(:dot) + str << consume + str << consume(:id) + else + break + end + end + str + end + +<<<<<<< HEAD +======= + def variable_lookup + name = consume(:id) + lookups, command_flags = variable_lookups + if Expression::LITERALS.key?(name) && lookups.empty? + Expression::LITERALS[name] + else + VariableLookup.new(name, lookups, command_flags) + end + end + + def unnamed_variable_lookup + name = indexed_lookup + lookups, command_flags = variable_lookups + VariableLookup.new(name, lookups, command_flags) + end + + def range_lookup + consume(:open_round) + first = expression + consume(:dotdot) + last = expression + consume(:close_round) + RangeLookup.create(first, last) + end + + # Assumes safe input. For cases where you need the string. + # Don't use this unless you're sure about what you're doing. + def unsafe_parse_expression(markup) + parse_expression(markup) + end + +>>>>>>> 68476f39 (Move unsafe_parse_expression to the end) + private + + def parse_expression(markup) + Expression.parse(markup, @ss, @cache) + end + end +end diff --git a/lib/liquid/tag.rb b/lib/liquid/tag.rb index 501eec611..216a80bd5 100644 --- a/lib/liquid/tag.rb +++ b/lib/liquid/tag.rb @@ -65,11 +65,5 @@ def render_to_output_buffer(context, output) def blank? false end - - private - - def parse_expression(markup, safe: false) - parse_context.parse_expression(markup, safe: safe) - end end end diff --git a/lib/liquid/tags/for.rb b/lib/liquid/tags/for.rb index cac9d2393..ae507c25f 100644 --- a/lib/liquid/tags/for.rb +++ b/lib/liquid/tags/for.rb @@ -76,7 +76,7 @@ def parse_markup(markup) raise SyntaxError, options[:locale].t("errors.syntax.for_invalid_in") unless p.id?('in') collection_name = p.expression - @collection_name = parse_expression(collection_name, safe: true) + @collection_name = p.unsafe_parse_expression(collection_name) @name = "#{@variable_name}-#{collection_name}" @reversed = p.id?('reversed') @@ -87,7 +87,7 @@ def parse_markup(markup) raise SyntaxError, options[:locale].t("errors.syntax.for_invalid_attribute") end p.consume(:colon) - set_attribute(attribute, p.expression, safe: true) + set_attribute(attribute, p) end p.consume(:end_of_string) end @@ -157,16 +157,17 @@ def render_segment(context, output, segment) output end - def set_attribute(key, expr, safe: false) + def set_attribute(key, p) + expr = p.expression case key when 'offset' @from = if expr == 'continue' :continue else - parse_expression(expr, safe: safe) + p.unsafe_parse_expression(expr) end when 'limit' - @limit = parse_expression(expr, safe: safe) + @limit = p.unsafe_parse_expression(expr) end end diff --git a/lib/liquid/tags/if.rb b/lib/liquid/tags/if.rb index 06c752da7..2081c788f 100644 --- a/lib/liquid/tags/if.rb +++ b/lib/liquid/tags/if.rb @@ -73,8 +73,8 @@ def push_block(tag, markup) block.attach(new_body) end - def parse_expression(markup, safe: false) - Condition.parse_expression(parse_context, markup, safe: safe) + def parse_expression(parser) + Condition.parse_expression(parser) end def parse_markup(markup) @@ -96,9 +96,9 @@ def parse_binary_comparisons(p) end def parse_comparison(p) - a = parse_expression(p.expression, safe: true) + a = parse_expression(p) if (op = p.consume?(:comparison)) - b = parse_expression(p.expression, safe: true) + b = parse_expression(p) Condition.new(a, op, b) else Condition.new(a) diff --git a/lib/liquid/tags/render.rb b/lib/liquid/tags/render.rb index 5a2ef0675..fab99487e 100644 --- a/lib/liquid/tags/render.rb +++ b/lib/liquid/tags/render.rb @@ -87,7 +87,7 @@ def render_tag(context, output) def parse_markup(markup) p = @parse_context.new_parser(markup) - @template_name_expr = parse_expression(template_name(p), safe: true) + @template_name_expr = template_name(p) with_or_for = p.id?("for") || p.id?("with") @variable_name_expr = p.expression_node if with_or_for @alias_name = p.consume(:id) if p.id?("as") @@ -107,7 +107,7 @@ def parse_markup(markup) end def template_name(p) - p.consume(:string) + p.string end class ParseTreeVisitor < Liquid::ParseTreeVisitor diff --git a/test/unit/condition_unit_test.rb b/test/unit/condition_unit_test.rb index 584951e21..7245e3b2d 100644 --- a/test/unit/condition_unit_test.rb +++ b/test/unit/condition_unit_test.rb @@ -166,25 +166,24 @@ def test_default_context_is_deprecated assert_includes(err.lines.map(&:strip), expected) end - def test_parse_expression_with_safe_true + def test_parse_expression environment = Environment.build parse_context = ParseContext.new(environment: environment) - result = Condition.parse_expression(parse_context, 'product.title', safe: true) + parser = parse_context.new_parser('product.title') + result = Condition.parse_expression(parser) assert_instance_of(VariableLookup, result) assert_equal('product', result.name) assert_equal(['title'], result.lookups) end - def test_parse_expression_raises_internal_error_if_not_safe + def test_parse_expression_returns_method_literal_for_blank_and_empty environment = Environment.build parse_context = ParseContext.new(environment: environment) + parser = parse_context.new_parser('blank') + result = Condition.parse_expression(parser) - error = assert_raises(Liquid::InternalError) do - Condition.parse_expression(parse_context, 'product.title') - end - - assert_match(/unsafe parse_expression cannot be used/, error.message) + assert_instance_of(Condition::MethodLiteral, result) end private diff --git a/test/unit/parse_context_unit_test.rb b/test/unit/parse_context_unit_test.rb index 89923a751..80b8ffc6d 100644 --- a/test/unit/parse_context_unit_test.rb +++ b/test/unit/parse_context_unit_test.rb @@ -5,7 +5,7 @@ class ParseContextUnitTest < Minitest::Test include Liquid - def test_safe_parse_expression_with_variable_lookup + def test_parser_expression_node_with_variable_lookup parser = parse_context.new_parser('product.title') result = parser.expression_node @@ -14,7 +14,7 @@ def test_safe_parse_expression_with_variable_lookup assert_equal(['title'], result.lookups) end - def test_safe_parse_expression_raises_syntax_error_for_invalid_expression + def test_parser_expression_node_raises_syntax_error_for_invalid_expression parser = parse_context.new_parser('') error = assert_raises(Liquid::SyntaxError) do @@ -25,35 +25,14 @@ def test_safe_parse_expression_raises_syntax_error_for_invalid_expression end def test_parse_expression_with_variable_lookup - error = assert_raises(Liquid::InternalError) do - parse_context.parse_expression('product.title') - end - - assert_match(/unsafe parse_expression cannot be used/, error.message) - end - - def test_parse_expression_with_safe_true - result = parse_context.parse_expression('product.title', safe: true) + result = parse_context.new_parser('product.title').expression_node assert_instance_of(VariableLookup, result) assert_equal('product', result.name) assert_equal(['title'], result.lookups) end - def test_parse_expression_with_empty_string - error = assert_raises(Liquid::InternalError) do - parse_context.parse_expression('') - end - - assert_match(/unsafe parse_expression cannot be used/, error.message) - end - - def test_parse_expression_with_empty_string_and_safe_true - result = parse_context.parse_expression('', safe: true) - assert_nil(result) - end - - def test_safe_parse_expression_advances_parser_pointer + def test_parser_expression_node_advances_parser_pointer parser = parse_context.new_parser('foo, bar') # parser.expression_node consumes "foo" @@ -71,11 +50,6 @@ def test_safe_parse_expression_advances_parser_pointer parser.consume(:end_of_string) end - def test_parse_expression_with_whitespace - result = parse_context.parse_expression(' ', safe: true) - assert_nil(result) - end - private def parse_context From 44106dc25023622c670d41e6e70aca1f54ec8875 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Tue, 2 Dec 2025 12:08:06 -0500 Subject: [PATCH 08/32] Rename Parser#expression -> Parser#expression_string --- lib/liquid/condition.rb | 2 +- lib/liquid/expression.rb | 2 +- lib/liquid/parser.rb | 57 +++++++++++++++++------------------ lib/liquid/tags/for.rb | 4 +-- test/unit/parser_unit_test.rb | 24 +++++++-------- 5 files changed, 44 insertions(+), 45 deletions(-) diff --git a/lib/liquid/condition.rb b/lib/liquid/condition.rb index ac1908773..8c4afc980 100644 --- a/lib/liquid/condition.rb +++ b/lib/liquid/condition.rb @@ -49,7 +49,7 @@ def self.operators end def self.parse_expression(parser) - markup = parser.expression + markup = parser.expression_string @@method_literals[markup] || parser.unsafe_parse_expression(markup) end diff --git a/lib/liquid/expression.rb b/lib/liquid/expression.rb index 3f9f08837..919cdf040 100644 --- a/lib/liquid/expression.rb +++ b/lib/liquid/expression.rb @@ -26,7 +26,7 @@ class Expression class << self def safe_parse(parser, ss = StringScanner.new(""), cache = nil) - parse(parser.expression, ss, cache) + parse(parser.expression_string, ss, cache) end def parse(markup, ss = StringScanner.new(""), cache = nil) diff --git a/lib/liquid/parser.rb b/lib/liquid/parser.rb index 606644071..881a24a37 100644 --- a/lib/liquid/parser.rb +++ b/lib/liquid/parser.rb @@ -47,34 +47,8 @@ def look(type, ahead = 0) tok[0] == type end - def expression - token = @tokens[@p] - case token[0] - when :id - str = consume - str << variable_lookups - when :open_square - str = consume.dup - str << expression - str << consume(:close_square) - str << variable_lookups - when :string, :number - consume - when :open_round - consume - first = expression - consume(:dotdot) - last = expression - consume(:close_round) - "(#{first}..#{last})" - else - raise SyntaxError, "#{token} is not a valid expression" - end - end - - def expression_node - parse_expression(expression) + parse_expression(expression_string) end def argument @@ -84,7 +58,7 @@ def argument str << consume << consume << ' ' end - str << expression + str << expression_string str end @@ -93,7 +67,7 @@ def variable_lookups loop do if look(:open_square) str << consume - str << expression + str << expression_string str << consume(:close_square) elsif look(:dot) str << consume @@ -105,6 +79,31 @@ def variable_lookups str end + def expression_string + token = @tokens[@p] + case token[0] + when :id + str = consume + str << variable_lookups + when :open_square + str = consume.dup + str << expression_string + str << consume(:close_square) + str << variable_lookups + when :string, :number + consume + when :open_round + consume + first = expression_string + consume(:dotdot) + last = expression_string + consume(:close_round) + "(#{first}..#{last})" + else + raise SyntaxError, "#{token} is not a valid expression" + end + end + # Assumes safe input. For cases where you need the string. # Don't use this unless you're sure about what you're doing. def unsafe_parse_expression(markup) diff --git a/lib/liquid/tags/for.rb b/lib/liquid/tags/for.rb index ae507c25f..3e23b2a14 100644 --- a/lib/liquid/tags/for.rb +++ b/lib/liquid/tags/for.rb @@ -75,7 +75,7 @@ def parse_markup(markup) @variable_name = p.consume(:id) raise SyntaxError, options[:locale].t("errors.syntax.for_invalid_in") unless p.id?('in') - collection_name = p.expression + collection_name = p.expression_string @collection_name = p.unsafe_parse_expression(collection_name) @name = "#{@variable_name}-#{collection_name}" @@ -158,7 +158,7 @@ def render_segment(context, output, segment) end def set_attribute(key, p) - expr = p.expression + expr = p.expression_string case key when 'offset' @from = if expr == 'continue' diff --git a/test/unit/parser_unit_test.rb b/test/unit/parser_unit_test.rb index 2c6a3594e..453c10f80 100644 --- a/test/unit/parser_unit_test.rb +++ b/test/unit/parser_unit_test.rb @@ -47,23 +47,23 @@ def test_look def test_expressions p = new_parser("hi.there hi?[5].there? hi.there.bob") - assert_equal('hi.there', p.expression) - assert_equal('hi?[5].there?', p.expression) - assert_equal('hi.there.bob', p.expression) + assert_equal('hi.there', p.expression_string) + assert_equal('hi?[5].there?', p.expression_string) + assert_equal('hi.there.bob', p.expression_string) p = new_parser("567 6.0 'lol' \"wut\"") - assert_equal('567', p.expression) - assert_equal('6.0', p.expression) - assert_equal("'lol'", p.expression) - assert_equal('"wut"', p.expression) + assert_equal('567', p.expression_string) + assert_equal('6.0', p.expression_string) + assert_equal("'lol'", p.expression_string) + assert_equal('"wut"', p.expression_string) end def test_ranges p = new_parser("(5..7) (1.5..9.6) (young..old) (hi[5].wat..old)") - assert_equal('(5..7)', p.expression) - assert_equal('(1.5..9.6)', p.expression) - assert_equal('(young..old)', p.expression) - assert_equal('(hi[5].wat..old)', p.expression) + assert_equal('(5..7)', p.expression_string) + assert_equal('(1.5..9.6)', p.expression_string) + assert_equal('(young..old)', p.expression_string) + assert_equal('(hi[5].wat..old)', p.expression_string) end def test_arguments @@ -78,7 +78,7 @@ def test_arguments def test_invalid_expression assert_raises(SyntaxError) do p = new_parser("==") - p.expression + p.expression_string end end From 123c2e3ea8aa8f5cedbbd6d636c3af4970ef0b0c Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Wed, 3 Dec 2025 09:55:15 -0500 Subject: [PATCH 09/32] Rename Parser#argument -> argument_string --- lib/liquid/parser.rb | 2 +- lib/liquid/parser.rb.orig | 165 ---------------------------------- test/unit/parser_unit_test.rb | 6 +- 3 files changed, 4 insertions(+), 169 deletions(-) delete mode 100644 lib/liquid/parser.rb.orig diff --git a/lib/liquid/parser.rb b/lib/liquid/parser.rb index 881a24a37..229a94bbe 100644 --- a/lib/liquid/parser.rb +++ b/lib/liquid/parser.rb @@ -51,7 +51,7 @@ def expression_node parse_expression(expression_string) end - def argument + def argument_string str = +"" # might be a keyword argument (identifier: expression) if look(:id) && look(:colon, 1) diff --git a/lib/liquid/parser.rb.orig b/lib/liquid/parser.rb.orig deleted file mode 100644 index 3e177e8ef..000000000 --- a/lib/liquid/parser.rb.orig +++ /dev/null @@ -1,165 +0,0 @@ -# frozen_string_literal: true - -module Liquid - class Parser - 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 - - def jump(point) - @p = point - end - - def consume(type = nil) - token = @tokens[@p] - if type && token[0] != type - raise SyntaxError, "Expected #{type} but found #{@tokens[@p].first}" - end - @p += 1 - token[1] - end - - # Only consumes the token if it matches the type - # Returns the token's contents if it was consumed - # or false otherwise. - def consume?(type) - token = @tokens[@p] - return false unless token && token[0] == type - @p += 1 - token[1] - end - - # Like consume? Except for an :id token of a certain name - def id?(str) - token = @tokens[@p] - return false unless token && token[0] == :id - return false unless token[1] == str - @p += 1 - token[1] - end - - def look(type, ahead = 0) - tok = @tokens[@p + ahead] - return false unless tok - tok[0] == type - end - - def expression - token = @tokens[@p] - case token[0] - when :id - str = consume - str << variable_lookups - when :open_square - str = consume.dup - str << expression - str << consume(:close_square) - str << variable_lookups - when :string, :number - consume - when :open_round - consume - first = expression - consume(:dotdot) - last = expression - consume(:close_round) - "(#{first}..#{last})" - else - raise SyntaxError, "#{token} is not a valid expression" - end - end - - def string - parse_expression(consume(:string)) - end - - def expression_node - parse_expression(expression) - end - -<<<<<<< HEAD - # Assumes safe input. For cases where you need the string. - # Don't use this unless you're sure about what you're doing. - def unsafe_parse_expression(markup) - parse_expression(markup) - end - - def argument -======= - def string - consume(:string)[1..-2] - end - - def argument_string ->>>>>>> 68476f39 (Move unsafe_parse_expression to the end) - str = +"" - # might be a keyword argument (identifier: expression) - if look(:id) && look(:colon, 1) - str << consume << consume << ' ' - end - - str << expression - str - end - - def variable_lookups - str = +"" - loop do - if look(:open_square) - str << consume - str << expression - str << consume(:close_square) - elsif look(:dot) - str << consume - str << consume(:id) - else - break - end - end - str - end - -<<<<<<< HEAD -======= - def variable_lookup - name = consume(:id) - lookups, command_flags = variable_lookups - if Expression::LITERALS.key?(name) && lookups.empty? - Expression::LITERALS[name] - else - VariableLookup.new(name, lookups, command_flags) - end - end - - def unnamed_variable_lookup - name = indexed_lookup - lookups, command_flags = variable_lookups - VariableLookup.new(name, lookups, command_flags) - end - - def range_lookup - consume(:open_round) - first = expression - consume(:dotdot) - last = expression - consume(:close_round) - RangeLookup.create(first, last) - end - - # Assumes safe input. For cases where you need the string. - # Don't use this unless you're sure about what you're doing. - def unsafe_parse_expression(markup) - parse_expression(markup) - end - ->>>>>>> 68476f39 (Move unsafe_parse_expression to the end) - private - - def parse_expression(markup) - Expression.parse(markup, @ss, @cache) - end - end -end diff --git a/test/unit/parser_unit_test.rb b/test/unit/parser_unit_test.rb index 453c10f80..a800f90e3 100644 --- a/test/unit/parser_unit_test.rb +++ b/test/unit/parser_unit_test.rb @@ -66,13 +66,13 @@ def test_ranges assert_equal('(hi[5].wat..old)', p.expression_string) end - def test_arguments + def test_argument_string p = new_parser("filter: hi.there[5], keyarg: 7") assert_equal('filter', p.consume(:id)) assert_equal(':', p.consume(:colon)) - assert_equal('hi.there[5]', p.argument) + assert_equal('hi.there[5]', p.argument_string) assert_equal(',', p.consume(:comma)) - assert_equal('keyarg: 7', p.argument) + assert_equal('keyarg: 7', p.argument_string) end def test_invalid_expression From 8222a1ced60dbe2ebb95c4f027338ec68a411e34 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Tue, 2 Dec 2025 12:13:51 -0500 Subject: [PATCH 10/32] Rename Parser#expression_node -> Parser#expression --- History.md | 17 +++++--- lib/liquid/parser.rb | 2 +- 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 +-- performance/unit/expression_benchmark.rb | 49 +++++++++++++++--------- performance/unit/lexer_benchmark.rb | 7 ++-- test/unit/parse_context_unit_test.rb | 20 +++++----- 11 files changed, 71 insertions(+), 54 deletions(-) diff --git a/History.md b/History.md index cf93d993b..4337aeb98 100644 --- a/History.md +++ b/History.md @@ -2,8 +2,6 @@ ## 6.0.0 -### Architectural changes - ### Features * (TODO) Add support for boolean expressions everywhere * As variable output `{{ a or b }}` @@ -21,6 +19,13 @@ - (TODO) Add support for parenthesized expressions * e.g. `(a or b) and c` +### Architectural changes +* `parse_expression` and `safe_parse_expression` have been removed from `Tag` and `ParseContext` +* `Parser` methods now produce AST nodes instead of strings + * `Parser#expression` produces a value, + * `Parser#string` produces a string, + * etc. + ### Breaking changes * The Environment's `error_mode` option has been removed. * `:warn` is no longer supported @@ -28,14 +33,14 @@ * `: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` -* `parse_expression` methods have been moved to `Parser#unsafe_parse_expression` - * Use `Parser#expression_node`, `Parser#string`, etc. instead +* `Parser#expression` is renamed to `Parser#expression_string` +* `safe_parse_expression` methods are replaced by `Parser#expression` +* `parse_expression` methods are replaced by `Parser#unsafe_parse_expression` ### 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` +- Replace `safe_parse_expression` calls with `Parser#expression` ## 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/parser.rb b/lib/liquid/parser.rb index 229a94bbe..5709e1be1 100644 --- a/lib/liquid/parser.rb +++ b/lib/liquid/parser.rb @@ -47,7 +47,7 @@ def look(type, ahead = 0) tok[0] == type end - def expression_node + def expression parse_expression(expression_string) end diff --git a/lib/liquid/tags/case.rb b/lib/liquid/tags/case.rb index 6fd8586dc..0f23ead1d 100644 --- a/lib/liquid/tags/case.rb +++ b/lib/liquid/tags/case.rb @@ -85,7 +85,7 @@ def render_to_output_buffer(context, output) def parse_markup(markup) parser = @parse_context.new_parser(markup) - @left = parser.expression_node + @left = parser.expression parser.consume(:end_of_string) end @@ -99,7 +99,7 @@ def parse_when(markup, body) parser = @parse_context.new_parser(markup) loop do - expr = parser.expression_node + expr = parser.expression 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 4847824eb..1c166f555 100644 --- a/lib/liquid/tags/cycle.rb +++ b/lib/liquid/tags/cycle.rb @@ -61,14 +61,14 @@ def parse_markup(markup) raise SyntaxError, options[:locale].t("errors.syntax.cycle") if p.look(:end_of_string) - first_expression = p.expression_node + first_expression = p.expression 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(p.expression_node) + @variables << maybe_dup_lookup(p.expression) else # cycle expr1, expr2, ... @variables << maybe_dup_lookup(first_expression) @@ -78,7 +78,7 @@ def parse_markup(markup) while p.consume?(:comma) break if p.look(:end_of_string) - @variables << maybe_dup_lookup(p.expression_node) + @variables << maybe_dup_lookup(p.expression) end p.consume(:end_of_string) diff --git a/lib/liquid/tags/include.rb b/lib/liquid/tags/include.rb index 02d8bf4e1..1f193473b 100644 --- a/lib/liquid/tags/include.rb +++ b/lib/liquid/tags/include.rb @@ -84,8 +84,8 @@ def render_to_output_buffer(context, output) def parse_markup(markup) p = @parse_context.new_parser(markup) - @template_name_expr = p.expression_node - @variable_name_expr = p.expression_node if p.id?("for") || p.id?("with") + @template_name_expr = p.expression + @variable_name_expr = p.expression if p.id?("for") || p.id?("with") @alias_name = p.consume(:id) if p.id?("as") p.consume?(:comma) @@ -94,7 +94,7 @@ def parse_markup(markup) while p.look(:id) key = p.consume p.consume(:colon) - @attributes[key] = p.expression_node + @attributes[key] = p.expression p.consume?(:comma) end diff --git a/lib/liquid/tags/render.rb b/lib/liquid/tags/render.rb index fab99487e..a5babca28 100644 --- a/lib/liquid/tags/render.rb +++ b/lib/liquid/tags/render.rb @@ -89,7 +89,7 @@ def parse_markup(markup) @template_name_expr = template_name(p) with_or_for = p.id?("for") || p.id?("with") - @variable_name_expr = p.expression_node if with_or_for + @variable_name_expr = p.expression if with_or_for @alias_name = p.consume(:id) if p.id?("as") @is_for_loop = (with_or_for == FOR) @@ -99,7 +99,7 @@ def parse_markup(markup) while p.look(:id) key = p.consume p.consume(:colon) - @attributes[key] = p.expression_node + @attributes[key] = p.expression p.consume?(:comma) end diff --git a/lib/liquid/tags/table_row.rb b/lib/liquid/tags/table_row.rb index 3b4cc5866..e6cfff040 100644 --- a/lib/liquid/tags/table_row.rb +++ b/lib/liquid/tags/table_row.rb @@ -42,7 +42,7 @@ def parse_markup(markup) raise SyntaxError, options[:locale].t("errors.syntax.for_invalid_in") end - @collection_name = p.expression_node + @collection_name = p.expression p.consume?(:comma) @@ -54,7 +54,7 @@ def parse_markup(markup) end p.consume(:colon) - @attributes[key] = p.expression_node + @attributes[key] = p.expression p.consume?(:comma) end diff --git a/lib/liquid/variable.rb b/lib/liquid/variable.rb index c73523587..9dd6eec56 100644 --- a/lib/liquid/variable.rb +++ b/lib/liquid/variable.rb @@ -47,7 +47,7 @@ def parse_markup(markup) return if p.look(:end_of_string) - @name = p.expression_node + @name = p.expression @filters << parse_filter_expressions(p) while p.consume?(:pipe) p.consume(:end_of_string) end @@ -121,10 +121,10 @@ def argument(p, positional_arguments, keyword_arguments) if p.look(:id) && p.look(:colon, 1) key = p.consume(:id) p.consume(:colon) - value = p.expression_node + value = p.expression keyword_arguments[key] = value else - positional_arguments << p.expression_node + positional_arguments << p.expression end end diff --git a/performance/unit/expression_benchmark.rb b/performance/unit/expression_benchmark.rb index 8cd552595..eca9e825b 100644 --- a/performance/unit/expression_benchmark.rb +++ b/performance/unit/expression_benchmark.rb @@ -6,7 +6,7 @@ require 'liquid' -RubyVM::YJIT.enable +RubyVM::YJIT.enable if defined?(RubyVM::YJIT) STRING_MARKUPS = [ "\"foo\"", @@ -45,22 +45,14 @@ RANGE_MARKUPS = [ "(1..30)", - "(1...30)", - "(1..30..5)", - "(1.0...30.0)", - "(1.........30)", "(1..foo)", "(foo..30)", "(foo..bar)", - "(foo...bar...100)", - "(foo...bar...100.0)", ] LITERAL_MARKUPS = [ - nil, 'nil', 'null', - '', 'true', 'false', 'blank', @@ -75,20 +67,39 @@ "range" => RANGE_MARKUPS, } -Benchmark.ips do |x| - x.config(time: 5, warmup: 5) +module Liquid + Benchmark.ips do |x| + x.config(time: 5, warmup: 5) - MARKUPS.each do |type, markups| - x.report("Liquid::Expression#parse: #{type}") do - markups.each do |markup| - Liquid::Expression.parse(markup) + ss = StringScanner.new('') + + MARKUPS.each do |type, markups| + x.report("#{type} - Liquid::Expression#parse") do + markups.each do |markup| + ss.string = markup + Expression.parse(markup, ss) + end + end + + x.report("#{type} - Liquid::Parser#expression") do + markups.each do |markup| + ss.string = markup + Parser.new(ss).expression + end + end + + x.report("#{type} - Liquid::Expression.parse(Parser#expression_string)") do + markups.each do |markup| + ss.string = markup + Expression.parse(Parser.new(ss).expression_string, ss) + end end end - end - x.report("Liquid::Expression#parse: all") do - MARKUPS.values.flatten.each do |markup| - Liquid::Expression.parse(markup) + x.report("Liquid::Expression#parse: all") do + MARKUPS.values.flatten.each do |markup| + Expression.parse(markup) + end end end end diff --git a/performance/unit/lexer_benchmark.rb b/performance/unit/lexer_benchmark.rb index fda6ce1f6..261451527 100644 --- a/performance/unit/lexer_benchmark.rb +++ b/performance/unit/lexer_benchmark.rb @@ -6,7 +6,7 @@ require 'liquid' -RubyVM::YJIT.enable +RubyVM::YJIT.enable if defined?(RubyVM::YJIT) EXPRESSIONS = [ "foo[1..2].baz", @@ -31,11 +31,12 @@ Benchmark.ips do |x| x.config(time: 10, warmup: 5) + ss = StringScanner.new('') x.report("Liquid::Lexer#tokenize") do EXPRESSIONS.each do |expr| - l = Liquid::Lexer.new(expr) - l.tokenize + ss.string = expr + Liquid::Lexer.tokenize(ss) end end diff --git a/test/unit/parse_context_unit_test.rb b/test/unit/parse_context_unit_test.rb index 80b8ffc6d..42773ef7c 100644 --- a/test/unit/parse_context_unit_test.rb +++ b/test/unit/parse_context_unit_test.rb @@ -5,45 +5,45 @@ class ParseContextUnitTest < Minitest::Test include Liquid - def test_parser_expression_node_with_variable_lookup + def test_parser_expression_with_variable_lookup parser = parse_context.new_parser('product.title') - result = parser.expression_node + result = parser.expression assert_instance_of(VariableLookup, result) assert_equal('product', result.name) assert_equal(['title'], result.lookups) end - def test_parser_expression_node_raises_syntax_error_for_invalid_expression + def test_parser_expression_raises_syntax_error_for_invalid_expression parser = parse_context.new_parser('') error = assert_raises(Liquid::SyntaxError) do - parser.expression_node + parser.expression end assert_match(/is not a valid expression/, error.message) end def test_parse_expression_with_variable_lookup - result = parse_context.new_parser('product.title').expression_node + result = parse_context.new_parser('product.title').expression assert_instance_of(VariableLookup, result) assert_equal('product', result.name) assert_equal(['title'], result.lookups) end - def test_parser_expression_node_advances_parser_pointer + def test_parser_expression_advances_parser_pointer parser = parse_context.new_parser('foo, bar') - # parser.expression_node consumes "foo" - first_result = parser.expression_node + # parser.expression consumes "foo" + first_result = parser.expression assert_instance_of(VariableLookup, first_result) assert_equal('foo', first_result.name) parser.consume(:comma) - # parser.expression_node consumes "bar" - second_result = parser.expression_node + # parser.expression consumes "bar" + second_result = parser.expression assert_instance_of(VariableLookup, second_result) assert_equal('bar', second_result.name) From 58cb8858331e7a44d9c0898b6def975aafedac76 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Tue, 2 Dec 2025 12:21:26 -0500 Subject: [PATCH 11/32] Remove Expression#safe_parse --- lib/liquid/expression.rb | 4 ---- test/integration/expression_test.rb | 12 ++++++------ 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/lib/liquid/expression.rb b/lib/liquid/expression.rb index 919cdf040..f73fe8fac 100644 --- a/lib/liquid/expression.rb +++ b/lib/liquid/expression.rb @@ -25,10 +25,6 @@ class Expression FLOAT_REGEX = /\A(-?\d+)\.\d+\z/ class << self - def safe_parse(parser, ss = StringScanner.new(""), cache = nil) - parse(parser.expression_string, ss, cache) - end - def parse(markup, ss = StringScanner.new(""), cache = nil) return unless markup diff --git a/test/integration/expression_test.rb b/test/integration/expression_test.rb index 988f1d122..166df2507 100644 --- a/test/integration/expression_test.rb +++ b/test/integration/expression_test.rb @@ -134,30 +134,30 @@ def test_disable_expression_cache assert(parse_context.instance_variable_get(:@expression_cache).nil?) end - def test_safe_parse_with_variable_lookup + def test_parser_expression_with_variable_lookup parse_context = Liquid::ParseContext.new parser = parse_context.new_parser('product.title') - result = Liquid::Expression.safe_parse(parser) + result = parser.expression assert_instance_of(Liquid::VariableLookup, result) assert_equal('product', result.name) assert_equal(['title'], result.lookups) end - def test_safe_parse_with_number + def test_parser_expression_with_number parse_context = Liquid::ParseContext.new parser = parse_context.new_parser('42') - result = Liquid::Expression.safe_parse(parser) + result = parser.expression assert_equal(42, result) end - def test_safe_parse_raises_syntax_error_for_invalid_expression + def test_parser_expression_raises_syntax_error_for_invalid_expression parse_context = Liquid::ParseContext.new parser = parse_context.new_parser('') error = assert_raises(Liquid::SyntaxError) do - Liquid::Expression.safe_parse(parser) + parser.expression end assert_match(/is not a valid expression/, error.message) From 8848065cf971e5bf1a1c1c7d0bb9ca520c734af8 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Tue, 2 Dec 2025 14:39:11 -0500 Subject: [PATCH 12/32] Extract Parser#string out of Expression.parse --- lib/liquid/parser.rb | 12 +++++++++++- test/unit/parser_unit_test.rb | 8 ++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/lib/liquid/parser.rb b/lib/liquid/parser.rb index 5709e1be1..5c2613c89 100644 --- a/lib/liquid/parser.rb +++ b/lib/liquid/parser.rb @@ -48,7 +48,17 @@ def look(type, ahead = 0) end def expression - parse_expression(expression_string) + token = @tokens[@p] + case token[0] + when :string + string + else + parse_expression(expression_string) + end + end + + def string + consume(:string)[1..-2] end def argument_string diff --git a/test/unit/parser_unit_test.rb b/test/unit/parser_unit_test.rb index a800f90e3..9bf737f0a 100644 --- a/test/unit/parser_unit_test.rb +++ b/test/unit/parser_unit_test.rb @@ -58,6 +58,14 @@ def test_expressions assert_equal('"wut"', p.expression_string) end + def test_string + p = new_parser("'s1' \"s2\" 'this \"s3\"' \"that 's4'\"") + assert_equal('s1', p.string) + assert_equal('s2', p.string) + assert_equal('this "s3"', p.string) + assert_equal("that 's4'", p.string) + end + def test_ranges p = new_parser("(5..7) (1.5..9.6) (young..old) (hi[5].wat..old)") assert_equal('(5..7)', p.expression_string) From 96e1595886ca47fd69d5a345a40f39e72470a13a Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Tue, 2 Dec 2025 15:58:42 -0500 Subject: [PATCH 13/32] Extract Parser#number parse out of Expression.parse --- lib/liquid/parser.rb | 7 +++++++ test/integration/expression_test.rb | 6 +++--- test/unit/parser_unit_test.rb | 8 ++++++++ 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/lib/liquid/parser.rb b/lib/liquid/parser.rb index 5c2613c89..ba4bad57c 100644 --- a/lib/liquid/parser.rb +++ b/lib/liquid/parser.rb @@ -52,11 +52,18 @@ def expression case token[0] when :string string + when :number + number else parse_expression(expression_string) end end + def number + num = consume(:number) + Expression.parse_number(num) + end + def string consume(:string)[1..-2] end diff --git a/test/integration/expression_test.rb b/test/integration/expression_test.rb index 166df2507..fd5273fa9 100644 --- a/test/integration/expression_test.rb +++ b/test/integration/expression_test.rb @@ -67,7 +67,7 @@ def test_expression_cache Liquid::Template.parse(template, expression_cache: cache).render assert_equal( - ["1", "2", "x", "y"], + ["x", "y"], cache.to_a.map { _1[0] }.sort, ) end @@ -91,7 +91,7 @@ def test_expression_cache_with_true_boolean cache = parse_context.instance_variable_get(:@expression_cache) assert_equal( - ["1", "2", "x", "y"], + ["x", "y"], cache.to_a.map { _1[0] }.sort, ) end @@ -112,7 +112,7 @@ def test_expression_cache_with_lru_redux Liquid::Template.parse(template, expression_cache: cache).render assert_equal( - ["1", "2", "x", "y"], + ["x", "y"], cache.to_a.map { _1[0] }.sort, ) end diff --git a/test/unit/parser_unit_test.rb b/test/unit/parser_unit_test.rb index 9bf737f0a..117114bf9 100644 --- a/test/unit/parser_unit_test.rb +++ b/test/unit/parser_unit_test.rb @@ -58,6 +58,14 @@ def test_expressions assert_equal('"wut"', p.expression_string) end + def test_number + p = new_parser('-1 0 1 2.0') + assert_equal(-1, p.number) + assert_equal(0, p.number) + assert_equal(1, p.number) + assert_equal(2.0, p.number) + end + def test_string p = new_parser("'s1' \"s2\" 'this \"s3\"' \"that 's4'\"") assert_equal('s1', p.string) From 7b37ba61cbe3b0c715d980c9c48f43dfed2afefe Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Tue, 2 Dec 2025 16:01:09 -0500 Subject: [PATCH 14/32] Simplify parse_number We don't need all the multi-dot logic in a world where number comes out of the Lexer. --- lib/liquid/expression.rb | 51 ++++------------------------------------ 1 file changed, 5 insertions(+), 46 deletions(-) diff --git a/lib/liquid/expression.rb b/lib/liquid/expression.rb index f73fe8fac..8c82b96be 100644 --- a/lib/liquid/expression.rb +++ b/lib/liquid/expression.rb @@ -57,63 +57,22 @@ def inner_parse(markup, ss, cache) ) end - if (num = parse_number(markup, ss)) + if (num = parse_number(markup)) num else VariableLookup.parse(markup, ss, cache) end end - def parse_number(markup, ss) + def parse_number(markup) # check if the markup is simple integer or float case markup when INTEGER_REGEX - return Integer(markup, 10) + Integer(markup, 10) when FLOAT_REGEX - return markup.to_f - end - - ss.string = markup - # the first byte must be a digit or a dash - byte = ss.scan_byte - - return false if byte != DASH && (byte < ZERO || byte > NINE) - - if byte == DASH - peek_byte = ss.peek_byte - - # if it starts with a dash, the next byte must be a digit - return false if peek_byte.nil? || !(peek_byte >= ZERO && peek_byte <= NINE) - end - - # The markup could be a float with multiple dots - first_dot_pos = nil - num_end_pos = nil - - while (byte = ss.scan_byte) - return false if byte != DOT && (byte < ZERO || byte > NINE) - - # we found our number and now we are just scanning the rest of the string - next if num_end_pos - - if byte == DOT - if first_dot_pos.nil? - first_dot_pos = ss.pos - else - # we found another dot, so we know that the number ends here - num_end_pos = ss.pos - 1 - end - end - end - - num_end_pos = markup.length if ss.eos? - - if num_end_pos - # number ends with a number "123.123" - markup.byteslice(0, num_end_pos).to_f + markup.to_f else - # number ends with a dot "123." - markup.byteslice(0, first_dot_pos).to_f + false end end end From 58ce63ebb3b27612c3e43cf5fe125dbc0a78a507 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Tue, 2 Dec 2025 16:14:22 -0500 Subject: [PATCH 15/32] Replace RangeLookup.parse with RangeLookup.create --- lib/liquid/expression.rb | 14 +++++++++----- lib/liquid/range_lookup.rb | 4 +--- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/lib/liquid/expression.rb b/lib/liquid/expression.rb index 8c82b96be..1bf7edcf6 100644 --- a/lib/liquid/expression.rb +++ b/lib/liquid/expression.rb @@ -49,11 +49,15 @@ def parse(markup, ss = StringScanner.new(""), cache = nil) def inner_parse(markup, ss, cache) if (markup.start_with?("(") && markup.end_with?(")")) && markup =~ RANGES_REGEX - return RangeLookup.parse( - Regexp.last_match(1), - Regexp.last_match(2), - ss, - cache, + start_markup = Regexp.last_match(1) + end_markup = Regexp.last_match(2) + start_obj = parse(start_markup, ss, cache) + end_obj = parse(end_markup, ss, cache) + return RangeLookup.create( + start_obj, + end_obj, + start_markup, + end_markup, ) end diff --git a/lib/liquid/range_lookup.rb b/lib/liquid/range_lookup.rb index bc316fe12..e4ce296cd 100644 --- a/lib/liquid/range_lookup.rb +++ b/lib/liquid/range_lookup.rb @@ -2,9 +2,7 @@ module Liquid class RangeLookup - def self.parse(start_markup, end_markup, string_scanner, cache = nil) - start_obj = Expression.parse(start_markup, string_scanner, cache) - end_obj = Expression.parse(end_markup, string_scanner, cache) + def self.create(start_obj, end_obj, start_markup, end_markup) if start_obj.respond_to?(:evaluate) || end_obj.respond_to?(:evaluate) new(start_obj, end_obj) else From 1023f3f41122615ffecac2076415cff4aa0140d2 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Tue, 2 Dec 2025 16:19:41 -0500 Subject: [PATCH 16/32] Move VariableLookup parsing logic to .parse instead of initializer Goal is to get rid of it entirely, but baby steps. --- lib/liquid/variable_lookup.rb | 20 +++++++++++--------- test/unit/condition_unit_test.rb | 8 ++++---- test/unit/variable_unit_test.rb | 28 ++++++++++++++-------------- 3 files changed, 29 insertions(+), 27 deletions(-) diff --git a/lib/liquid/variable_lookup.rb b/lib/liquid/variable_lookup.rb index 340c0b66d..d8c09423a 100644 --- a/lib/liquid/variable_lookup.rb +++ b/lib/liquid/variable_lookup.rb @@ -7,10 +7,6 @@ class VariableLookup attr_reader :name, :lookups def self.parse(markup, string_scanner = StringScanner.new(""), cache = nil) - new(markup, string_scanner, cache) - end - - def initialize(markup, string_scanner = StringScanner.new(""), cache = nil) lookups = markup.scan(VariableParser) name = lookups.shift @@ -21,12 +17,10 @@ def initialize(markup, string_scanner = StringScanner.new(""), cache = nil) cache, ) end - @name = name - @lookups = lookups - @command_flags = 0 + command_flags = 0 - @lookups.each_index do |i| + lookups.each_index do |i| lookup = lookups[i] if lookup&.start_with?('[') && lookup&.end_with?(']') lookups[i] = Expression.parse( @@ -35,9 +29,17 @@ def initialize(markup, string_scanner = StringScanner.new(""), cache = nil) cache, ) elsif COMMAND_METHODS.include?(lookup) - @command_flags |= 1 << i + command_flags |= 1 << i end end + + new(name, lookups, command_flags) + end + + def initialize(name, lookups, command_flags) + @name = name + @lookups = lookups + @command_flags = command_flags end def lookup_command?(lookup_index) diff --git a/test/unit/condition_unit_test.rb b/test/unit/condition_unit_test.rb index 7245e3b2d..9123b5099 100644 --- a/test/unit/condition_unit_test.rb +++ b/test/unit/condition_unit_test.rb @@ -82,7 +82,7 @@ def test_hash_compare_backwards_compatibility def test_contains_works_on_arrays @context = Liquid::Context.new @context['array'] = [1, 2, 3, 4, 5] - array_expr = VariableLookup.new("array") + array_expr = VariableLookup.parse("array") assert_evaluates_false(array_expr, 'contains', 0) assert_evaluates_true(array_expr, 'contains', 1) @@ -96,8 +96,8 @@ def test_contains_works_on_arrays def test_contains_returns_false_for_nil_operands @context = Liquid::Context.new - assert_evaluates_false(VariableLookup.new('not_assigned'), 'contains', '0') - assert_evaluates_false(0, 'contains', VariableLookup.new('not_assigned')) + assert_evaluates_false(VariableLookup.parse('not_assigned'), 'contains', '0') + assert_evaluates_false(0, 'contains', VariableLookup.parse('not_assigned')) end def test_contains_return_false_on_wrong_data_type @@ -149,7 +149,7 @@ def test_left_or_right_may_contain_operators @context = Liquid::Context.new @context['one'] = @context['another'] = "gnomeslab-and-or-liquid" - assert_evaluates_true(VariableLookup.new("one"), '==', VariableLookup.new("another")) + assert_evaluates_true(VariableLookup.parse("one"), '==', VariableLookup.parse("another")) end def test_default_context_is_deprecated diff --git a/test/unit/variable_unit_test.rb b/test/unit/variable_unit_test.rb index 6379c6ece..b6f1c9aae 100644 --- a/test/unit/variable_unit_test.rb +++ b/test/unit/variable_unit_test.rb @@ -7,20 +7,20 @@ class VariableUnitTest < Minitest::Test def test_variable var = create_variable('hello') - assert_equal(VariableLookup.new('hello'), var.name) + assert_equal(VariableLookup.parse('hello'), var.name) end def test_filters var = create_variable('hello | textileze') - assert_equal(VariableLookup.new('hello'), var.name) + assert_equal(VariableLookup.parse('hello'), var.name) assert_equal([['textileze', []]], var.filters) var = create_variable('hello | textileze | paragraph') - assert_equal(VariableLookup.new('hello'), var.name) + assert_equal(VariableLookup.parse('hello'), var.name) assert_equal([['textileze', []], ['paragraph', []]], var.filters) var = create_variable(%( hello | strftime: '%Y')) - assert_equal(VariableLookup.new('hello'), var.name) + assert_equal(VariableLookup.parse('hello'), var.name) assert_equal([['strftime', ['%Y']]], var.filters) var = create_variable(%( 'typo' | link_to: 'Typo', true )) @@ -44,11 +44,11 @@ def test_filters assert_equal([['repeat', [3, 3, 3]]], var.filters) var = create_variable(%( hello | strftime: '%Y, okay?')) - assert_equal(VariableLookup.new('hello'), var.name) + assert_equal(VariableLookup.parse('hello'), var.name) assert_equal([['strftime', ['%Y, okay?']]], var.filters) var = create_variable(%( hello | things: "%Y, okay?", 'the other one')) - assert_equal(VariableLookup.new('hello'), var.name) + assert_equal(VariableLookup.parse('hello'), var.name) assert_equal([['things', ['%Y, okay?', 'the other one']]], var.filters) end @@ -60,15 +60,15 @@ def test_filter_with_date_parameter def test_filters_without_whitespace var = create_variable('hello | textileze | paragraph') - assert_equal(VariableLookup.new('hello'), var.name) + assert_equal(VariableLookup.parse('hello'), var.name) assert_equal([['textileze', []], ['paragraph', []]], var.filters) var = create_variable('hello|textileze|paragraph') - assert_equal(VariableLookup.new('hello'), var.name) + assert_equal(VariableLookup.parse('hello'), var.name) assert_equal([['textileze', []], ['paragraph', []]], var.filters) var = create_variable("hello|replace:'foo','bar'|textileze") - assert_equal(VariableLookup.new('hello'), var.name) + assert_equal(VariableLookup.parse('hello'), var.name) assert_equal([['replace', ['foo', 'bar']], ['textileze', []]], var.filters) end @@ -99,8 +99,8 @@ def test_float end def test_dashes - assert_equal(VariableLookup.new('foo-bar'), create_variable('foo-bar').name) - assert_equal(VariableLookup.new('foo-bar-2'), create_variable('foo-bar-2').name) + assert_equal(VariableLookup.parse('foo-bar'), create_variable('foo-bar').name) + assert_equal(VariableLookup.parse('foo-bar-2'), create_variable('foo-bar-2').name) assert_raises(Liquid::SyntaxError) { create_variable('foo - bar') } assert_raises(Liquid::SyntaxError) { create_variable('-foo') } @@ -114,12 +114,12 @@ def test_string_with_special_chars def test_string_dot var = create_variable(%( test.test )) - assert_equal(VariableLookup.new('test.test'), var.name) + assert_equal(VariableLookup.parse('test.test'), var.name) end def test_filter_with_keyword_arguments var = create_variable(%( hello | things: greeting: "world", farewell: 'goodbye')) - assert_equal(VariableLookup.new('hello'), var.name) + assert_equal(VariableLookup.parse('hello'), var.name) assert_equal([['things', [], { 'greeting' => 'world', 'farewell' => 'goodbye' }]], var.filters) end @@ -163,7 +163,7 @@ def test_output_raw_source_of_variable end def test_variable_lookup_interface - lookup = VariableLookup.new('a.b.c') + lookup = VariableLookup.parse('a.b.c') assert_equal('a', lookup.name) assert_equal(['b', 'c'], lookup.lookups) end From 0d8011729d777dc358c5c284052fad3132f45308 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Tue, 2 Dec 2025 16:37:44 -0500 Subject: [PATCH 17/32] Extract Parser#variable_lookup out of Expression.parse --- lib/liquid/parser.rb | 96 +++++++++++++++++++++-------- test/integration/expression_test.rb | 6 +- test/unit/parser_unit_test.rb | 30 ++++++++- 3 files changed, 103 insertions(+), 29 deletions(-) diff --git a/lib/liquid/parser.rb b/lib/liquid/parser.rb index ba4bad57c..72516c264 100644 --- a/lib/liquid/parser.rb +++ b/lib/liquid/parser.rb @@ -50,6 +50,10 @@ def look(type, ahead = 0) def expression token = @tokens[@p] case token[0] + when :id + variable_lookup + when :open_square + unnamed_variable_lookup when :string string when :number @@ -68,32 +72,20 @@ def string consume(:string)[1..-2] end - def argument_string - str = +"" - # might be a keyword argument (identifier: expression) - if look(:id) && look(:colon, 1) - str << consume << consume << ' ' + def variable_lookup + name = consume(:id) + lookups, command_flags = variable_lookups + if Expression::LITERALS.key?(name) && lookups.empty? + Expression::LITERALS[name] + else + VariableLookup.new(name, lookups, command_flags) end - - str << expression_string - str end - def variable_lookups - str = +"" - loop do - if look(:open_square) - str << consume - str << expression_string - str << consume(:close_square) - elsif look(:dot) - str << consume - str << consume(:id) - else - break - end - end - str + def unnamed_variable_lookup + name = indexed_lookup + lookups, command_flags = variable_lookups + VariableLookup.new(name, lookups, command_flags) end def expression_string @@ -101,12 +93,12 @@ def expression_string case token[0] when :id str = consume - str << variable_lookups + str << variable_lookups_string when :open_square str = consume.dup str << expression_string str << consume(:close_square) - str << variable_lookups + str << variable_lookups_string when :string, :number consume when :open_round @@ -121,6 +113,34 @@ def expression_string end end + def argument_string + str = +"" + # might be a keyword argument (identifier: expression) + if look(:id) && look(:colon, 1) + str << consume << consume << ' ' + end + + str << expression_string + str + end + + def variable_lookups_string + str = +"" + loop do + if look(:open_square) + str << consume + str << expression_string + str << consume(:close_square) + elsif look(:dot) + str << consume + str << consume(:id) + else + break + end + end + str + end + # Assumes safe input. For cases where you need the string. # Don't use this unless you're sure about what you're doing. def unsafe_parse_expression(markup) @@ -132,5 +152,31 @@ def unsafe_parse_expression(markup) def parse_expression(markup) Expression.parse(markup, @ss, @cache) end + + def variable_lookups + lookups = [] + command_flags = 0 + i = -1 + loop do + i += 1 + if look(:open_square) + lookups << indexed_lookup + elsif consume?(:dot) + lookup = consume(:id) + lookups << lookup + command_flags |= 1 << i if VariableLookup::COMMAND_METHODS.include?(lookup) + else + break + end + end + [lookups, command_flags] + end + + def indexed_lookup + consume(:open_square) + expr = expression + consume(:close_square) + expr + end end end diff --git a/test/integration/expression_test.rb b/test/integration/expression_test.rb index fd5273fa9..2f53596d8 100644 --- a/test/integration/expression_test.rb +++ b/test/integration/expression_test.rb @@ -67,7 +67,7 @@ def test_expression_cache Liquid::Template.parse(template, expression_cache: cache).render assert_equal( - ["x", "y"], + [], cache.to_a.map { _1[0] }.sort, ) end @@ -91,7 +91,7 @@ def test_expression_cache_with_true_boolean cache = parse_context.instance_variable_get(:@expression_cache) assert_equal( - ["x", "y"], + [], cache.to_a.map { _1[0] }.sort, ) end @@ -112,7 +112,7 @@ def test_expression_cache_with_lru_redux Liquid::Template.parse(template, expression_cache: cache).render assert_equal( - ["x", "y"], + [], cache.to_a.map { _1[0] }.sort, ) end diff --git a/test/unit/parser_unit_test.rb b/test/unit/parser_unit_test.rb index 117114bf9..1b3e142df 100644 --- a/test/unit/parser_unit_test.rb +++ b/test/unit/parser_unit_test.rb @@ -45,7 +45,7 @@ def test_look assert_equal(false, p.look(:number, 1)) end - def test_expressions + def test_expression_string p = new_parser("hi.there hi?[5].there? hi.there.bob") assert_equal('hi.there', p.expression_string) assert_equal('hi?[5].there?', p.expression_string) @@ -58,6 +58,25 @@ def test_expressions assert_equal('"wut"', p.expression_string) end + def test_expression + p = new_parser("hi.there hi?[5].there? hi.there.bob") + v1 = p.expression + v2 = p.expression + v3 = p.expression + assert(v1.is_a?(VariableLookup) && v1.name == 'hi' && v1.lookups[0] == 'there') + assert(v2.is_a?(VariableLookup) && v2.name == 'hi?' && v2.lookups[0] == 5) + assert(v3.is_a?(VariableLookup) && v3.name == 'hi' && v3.lookups[0] == 'there') + + p = new_parser("567 6.0 'lol' \"wut\" true false (0..5)") + assert_equal(567, p.expression) + assert_equal(6.0, p.expression) + assert_equal('lol', p.expression) + assert_equal('wut', p.expression) + assert_equal(true, p.expression) + assert_equal(false, p.expression) + assert_equal((0..5), p.expression) + end + def test_number p = new_parser('-1 0 1 2.0') assert_equal(-1, p.number) @@ -74,6 +93,15 @@ def test_string assert_equal("that 's4'", p.string) end + def test_unnamed_variable_lookup + p = new_parser('[key].title') + v = p.expression + assert(v.is_a?(VariableLookup)) + assert(v.name.is_a?(VariableLookup)) + assert_equal('key', v.name.name) + assert_equal('title', v.lookups[0]) + end + def test_ranges p = new_parser("(5..7) (1.5..9.6) (young..old) (hi[5].wat..old)") assert_equal('(5..7)', p.expression_string) From ab8928106ed1a66cb8e993eb5e7bcb4258bd63d9 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Wed, 3 Dec 2025 11:40:51 -0500 Subject: [PATCH 18/32] Extract Parser#range_lookup out of Expression.parse --- lib/liquid/parser.rb | 13 ++++++++++++- lib/liquid/range_lookup.rb | 4 +++- test/integration/expression_test.rb | 2 +- test/unit/parser_unit_test.rb | 9 +++++++++ 4 files changed, 25 insertions(+), 3 deletions(-) diff --git a/lib/liquid/parser.rb b/lib/liquid/parser.rb index 72516c264..7003af9be 100644 --- a/lib/liquid/parser.rb +++ b/lib/liquid/parser.rb @@ -58,8 +58,10 @@ def expression string when :number number + when :open_round + range_lookup else - parse_expression(expression_string) + raise SyntaxError, "#{token} is not a valid expression" end end @@ -88,6 +90,15 @@ def unnamed_variable_lookup VariableLookup.new(name, lookups, command_flags) end + def range_lookup + consume(:open_round) + first = expression + consume(:dotdot) + last = expression + consume(:close_round) + RangeLookup.create(first, last) + end + def expression_string token = @tokens[@p] case token[0] diff --git a/lib/liquid/range_lookup.rb b/lib/liquid/range_lookup.rb index e4ce296cd..8e617f733 100644 --- a/lib/liquid/range_lookup.rb +++ b/lib/liquid/range_lookup.rb @@ -2,13 +2,15 @@ module Liquid class RangeLookup - def self.create(start_obj, end_obj, start_markup, end_markup) + def self.create(start_obj, end_obj, start_markup = nil, end_markup = nil) if start_obj.respond_to?(:evaluate) || end_obj.respond_to?(:evaluate) new(start_obj, end_obj) else begin start_obj.to_i..end_obj.to_i rescue NoMethodError + start_markup = start_obj.to_s unless start_markup + end_markup = end_obj.to_s unless end_markup invalid_expr = start_markup unless start_obj.respond_to?(:to_i) invalid_expr ||= end_markup unless end_obj.respond_to?(:to_i) if invalid_expr diff --git a/test/integration/expression_test.rb b/test/integration/expression_test.rb index 2f53596d8..89dc9ab10 100644 --- a/test/integration/expression_test.rb +++ b/test/integration/expression_test.rb @@ -46,7 +46,7 @@ def test_range "{{ (false..true) }}", ) assert_match_syntax_error( - "Liquid syntax error (line 1): Invalid expression type '(1..2)' in range expression", + "Liquid syntax error (line 1): Invalid expression type '1..2' in range expression", "{{ ((1..2)..3) }}", ) end diff --git a/test/unit/parser_unit_test.rb b/test/unit/parser_unit_test.rb index 1b3e142df..cf0d8c7bb 100644 --- a/test/unit/parser_unit_test.rb +++ b/test/unit/parser_unit_test.rb @@ -102,6 +102,15 @@ def test_unnamed_variable_lookup assert_equal('title', v.lookups[0]) end + def test_range_lookup + p = new_parser('(0..5) (a..b)') + assert_equal((0..5), p.expression) + + r2 = p.expression + assert(r2.is_a?(RangeLookup)) + assert_equal((1..4), r2.evaluate(Context.new({ 'a' => 1, 'b' => 4 }))) + end + def test_ranges p = new_parser("(5..7) (1.5..9.6) (young..old) (hi[5].wat..old)") assert_equal('(5..7)', p.expression_string) From 5e3d8f4ad54ef9f23c95e7c3b111d693da87673f Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Wed, 3 Dec 2025 12:00:17 -0500 Subject: [PATCH 19/32] Replace Context[]'s Expression.parse with Parser#expression --- lib/liquid/context.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/liquid/context.rb b/lib/liquid/context.rb index 1db6bb663..750749f1e 100644 --- a/lib/liquid/context.rb +++ b/lib/liquid/context.rb @@ -175,7 +175,8 @@ def []=(key, value) # Example: # products == empty #=> products.empty? def [](expression) - evaluate(Expression.parse(expression, @string_scanner)) + @string_scanner.string = expression + evaluate(Parser.new(@string_scanner).expression) end def key?(key) From 82f10b7ace7e3e39bf2b904428e495abff0c511e Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Wed, 3 Dec 2025 15:03:01 -0500 Subject: [PATCH 20/32] Make Parser.expression parse comparisons --- lib/liquid.rb | 1 + lib/liquid/binary_expression.rb | 46 +++++++++++++++++++++++++++++++++ lib/liquid/parser.rb | 17 ++++++++++++ test/unit/parser_unit_test.rb | 24 +++++++++++++++++ 4 files changed, 88 insertions(+) create mode 100644 lib/liquid/binary_expression.rb diff --git a/lib/liquid.rb b/lib/liquid.rb index 4d0a71a64..d069f6ca0 100644 --- a/lib/liquid.rb +++ b/lib/liquid.rb @@ -62,6 +62,7 @@ module Liquid require 'liquid/tags' require "liquid/environment" require 'liquid/lexer' +require 'liquid/binary_expression' require 'liquid/parser' require 'liquid/i18n' require 'liquid/drop' diff --git a/lib/liquid/binary_expression.rb b/lib/liquid/binary_expression.rb new file mode 100644 index 000000000..944099f7f --- /dev/null +++ b/lib/liquid/binary_expression.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +module Liquid + class BinaryExpression + attr_reader :left, :operator, :right + + def initialize(left, operator, right) + @left = left + @operator = operator + @right = right + end + + def evaluate(context) + left_value = value(left, context) + right_value = value(@right, context) + + case operator + when '>' + left_value > right_value + when '>=' + left_value >= right_value + when '<' + left_value < right_value + when '<=' + left_value <= right_value + when '==' + left_value == right_value + when '!=', '<>' + left_value != right_value + when 'contains' + if left_value && right_value && left_value.respond_to?(:include?) + right_value = right_value.to_s if left_value.is_a?(String) + left_value.include?(right_value) + else + false + end + end + end + + private + + def value(expr, context) + Utils.to_liquid_value(context.evaluate(expr)) + end + end +end diff --git a/lib/liquid/parser.rb b/lib/liquid/parser.rb index 7003af9be..66d7c4233 100644 --- a/lib/liquid/parser.rb +++ b/lib/liquid/parser.rb @@ -47,7 +47,24 @@ def look(type, ahead = 0) tok[0] == type end + # expression := comparison + # comparison := primary ((">=" | ">" | "<" | "<=" | ... ) primary)* + # primary := string | number | variable_lookup | range | boolean def expression + comparison + end + + # comparison := primary ((">=" | ">" | "<" | "<=" | ... ) primary)* + def comparison + expr = primary + while look(:comparison) + operator = consume + expr = BinaryExpression.new(expr, operator, primary) + end + expr + end + + def primary token = @tokens[@p] case token[0] when :id diff --git a/test/unit/parser_unit_test.rb b/test/unit/parser_unit_test.rb index cf0d8c7bb..58531b3c0 100644 --- a/test/unit/parser_unit_test.rb +++ b/test/unit/parser_unit_test.rb @@ -77,6 +77,30 @@ def test_expression assert_equal((0..5), p.expression) end + def test_comparison + p = new_parser("a > b") + expr = p.expression + assert(expr.is_a?(BinaryExpression)) + assert_equal('>', expr.operator) + assert(expr.left.is_a?(VariableLookup)) + assert_equal('a', expr.left.name) + assert(expr.right.is_a?(VariableLookup)) + assert_equal('b', expr.right.name) + + # BinaryExpression(>=) + # left: BinaryExpression(>) + # left: 10 + # right: 5 + # right: 4 + p = new_parser("10 > 5 >= 4") + expr = p.expression + assert(expr.is_a?(BinaryExpression)) + assert_equal('>=', expr.operator) + assert_equal(10, expr.left.left) + assert_equal(5, expr.left.right) + assert_equal(4, expr.right) + end + def test_number p = new_parser('-1 0 1 2.0') assert_equal(-1, p.number) From 1e70fed6389e2ef7e2640ce8713af3d04fcd6786 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Wed, 3 Dec 2025 15:03:42 -0500 Subject: [PATCH 21/32] Make Parser.expression parse equality expressions --- lib/liquid/lexer.rb | 6 +++--- lib/liquid/parser.rb | 14 ++++++++++++-- lib/liquid/tags/if.rb | 2 +- test/unit/lexer_unit_test.rb | 13 ++++++++++--- test/unit/parser_unit_test.rb | 25 +++++++++++++++++++++++++ 5 files changed, 51 insertions(+), 9 deletions(-) diff --git a/lib/liquid/lexer.rb b/lib/liquid/lexer.rb index f1740dbad..c0a6463bb 100644 --- a/lib/liquid/lexer.rb +++ b/lib/liquid/lexer.rb @@ -6,14 +6,14 @@ class Lexer CLOSE_SQUARE = [:close_square, "]"].freeze COLON = [:colon, ":"].freeze COMMA = [:comma, ","].freeze - COMPARISION_NOT_EQUAL = [:comparison, "!="].freeze + COMPARISION_NOT_EQUAL = [:equality, "!="].freeze COMPARISON_CONTAINS = [:comparison, "contains"].freeze - COMPARISON_EQUAL = [:comparison, "=="].freeze + COMPARISON_EQUAL = [:equality, "=="].freeze COMPARISON_GREATER_THAN = [:comparison, ">"].freeze COMPARISON_GREATER_THAN_OR_EQUAL = [:comparison, ">="].freeze COMPARISON_LESS_THAN = [:comparison, "<"].freeze COMPARISON_LESS_THAN_OR_EQUAL = [:comparison, "<="].freeze - COMPARISON_NOT_EQUAL_ALT = [:comparison, "<>"].freeze + COMPARISON_NOT_EQUAL_ALT = [:equality, "<>"].freeze DASH = [:dash, "-"].freeze DOT = [:dot, "."].freeze DOTDOT = [:dotdot, ".."].freeze diff --git a/lib/liquid/parser.rb b/lib/liquid/parser.rb index 66d7c4233..8d5151e7d 100644 --- a/lib/liquid/parser.rb +++ b/lib/liquid/parser.rb @@ -47,11 +47,21 @@ def look(type, ahead = 0) tok[0] == type end - # expression := comparison + # expression := equality + # equality := comparison (("==" | "!=" | "<>") comparison)* # comparison := primary ((">=" | ">" | "<" | "<=" | ... ) primary)* # primary := string | number | variable_lookup | range | boolean def expression - comparison + equality + end + + def equality + expr = comparison + while look(:equality) + operator = consume + expr = BinaryExpression.new(expr, operator, comparison) + end + expr end # comparison := primary ((">=" | ">" | "<" | "<=" | ... ) primary)* diff --git a/lib/liquid/tags/if.rb b/lib/liquid/tags/if.rb index 2081c788f..3471f6778 100644 --- a/lib/liquid/tags/if.rb +++ b/lib/liquid/tags/if.rb @@ -97,7 +97,7 @@ def parse_binary_comparisons(p) def parse_comparison(p) a = parse_expression(p) - if (op = p.consume?(:comparison)) + if (op = p.consume?(:comparison) || p.consume?(:equality)) b = parse_expression(p) Condition.new(a, op, b) else diff --git a/test/unit/lexer_unit_test.rb b/test/unit/lexer_unit_test.rb index 73eeb7398..494769b8b 100644 --- a/test/unit/lexer_unit_test.rb +++ b/test/unit/lexer_unit_test.rb @@ -26,10 +26,17 @@ def test_float ) end + def test_equality + assert_equal( + [[:equality, '=='], [:equality, '<>'], [:equality, '!='], [:end_of_string]], + tokenize('== <> != '), + ) + end + def test_comparison assert_equal( - [[:comparison, '=='], [:comparison, '<>'], [:comparison, 'contains'], [:end_of_string]], - tokenize('== <> contains '), + [[:comparison, '>'], [:comparison, '>='], [:comparison, '<'], [:comparison, '<='], [:comparison, 'contains'], [:end_of_string]], + tokenize('> >= < <= contains'), ) end @@ -81,7 +88,7 @@ def test_fancy_identifiers def test_whitespace assert_equal( - [[:id, 'five'], [:pipe, '|'], [:comparison, '=='], [:end_of_string]], + [[:id, 'five'], [:pipe, '|'], [:equality, '=='], [:end_of_string]], tokenize("five|\n\t =="), ) end diff --git a/test/unit/parser_unit_test.rb b/test/unit/parser_unit_test.rb index 58531b3c0..a0c86c081 100644 --- a/test/unit/parser_unit_test.rb +++ b/test/unit/parser_unit_test.rb @@ -77,6 +77,31 @@ def test_expression assert_equal((0..5), p.expression) end + def test_equality + p = new_parser("a == b") + expr = p.expression + assert(expr.is_a?(BinaryExpression)) + assert_equal('==', expr.operator) + assert_equal('a', expr.left.name) + assert_equal('b', expr.right.name) + + # BinaryExpression(==) + # left: BinaryExpression(<) + # left: 0 + # right: 5 + # right: BinaryExpression(>) + # left: 6 + # right: 1 + p = new_parser("0 < 5 == 6 > 1") + expr = p.expression + assert(expr.is_a?(BinaryExpression)) + assert_equal('==', expr.operator) + assert_equal(0, expr.left.left) + assert_equal(5, expr.left.right) + assert_equal(6, expr.right.left) + assert_equal(1, expr.right.right) + end + def test_comparison p = new_parser("a > b") expr = p.expression From 1d42d9cfe65e1ca755c0354bf02b63d4913f9c2f Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Wed, 3 Dec 2025 15:58:10 -0500 Subject: [PATCH 22/32] Use BinaryExpression instead of Condition for comparisons --- lib/liquid.rb | 1 + lib/liquid/binary_expression.rb | 72 +++++++++++---- lib/liquid/condition.rb | 10 --- lib/liquid/expression.rb | 4 +- lib/liquid/method_literal.rb | 16 ++++ lib/liquid/tags/if.rb | 8 +- test/integration/tags/if_else_tag_test.rb | 22 ----- test/unit/binary_expression_test.rb | 101 ++++++++++++++++++++++ test/unit/condition_unit_test.rb | 2 +- test/unit/parser_unit_test.rb | 46 +++++----- 10 files changed, 200 insertions(+), 82 deletions(-) create mode 100644 lib/liquid/method_literal.rb create mode 100644 test/unit/binary_expression_test.rb diff --git a/lib/liquid.rb b/lib/liquid.rb index d069f6ca0..0ffd07a1d 100644 --- a/lib/liquid.rb +++ b/lib/liquid.rb @@ -62,6 +62,7 @@ module Liquid require 'liquid/tags' require "liquid/environment" require 'liquid/lexer' +require 'liquid/method_literal' require 'liquid/binary_expression' require 'liquid/parser' require 'liquid/i18n' diff --git a/lib/liquid/binary_expression.rb b/lib/liquid/binary_expression.rb index 944099f7f..00e3873d2 100644 --- a/lib/liquid/binary_expression.rb +++ b/lib/liquid/binary_expression.rb @@ -2,39 +2,41 @@ module Liquid class BinaryExpression - attr_reader :left, :operator, :right + attr_reader :operator + attr_accessor :left_node, :right_node def initialize(left, operator, right) - @left = left + @left_node = left @operator = operator - @right = right + @right_node = right end def evaluate(context) - left_value = value(left, context) - right_value = value(@right, context) + left = value(left_node, context) + right = value(right_node, context) case operator when '>' - left_value > right_value + left > right if can_compare?(left, right) when '>=' - left_value >= right_value + left >= right if can_compare?(left, right) when '<' - left_value < right_value + left < right if can_compare?(left, right) when '<=' - left_value <= right_value + left <= right if can_compare?(left, right) when '==' - left_value == right_value + equal_variables(left, right) when '!=', '<>' - left_value != right_value + !equal_variables(left, right) when 'contains' - if left_value && right_value && left_value.respond_to?(:include?) - right_value = right_value.to_s if left_value.is_a?(String) - left_value.include?(right_value) - else - false - end + contains(left, right) end + rescue ::ArgumentError => e + raise Liquid::ArgumentError, e.message + end + + def to_s + "(#{left_node} #{operator} #{right_node})" end private @@ -42,5 +44,41 @@ def evaluate(context) def value(expr, context) Utils.to_liquid_value(context.evaluate(expr)) end + + def can_compare?(left, right) + left.respond_to?(operator) && right.respond_to?(operator) && !left.is_a?(Hash) && !right.is_a?(Hash) + end + + def contains(left, right) + if left && right && left.respond_to?(:include?) + right = right.to_s if left.is_a?(String) + left.include?(right) + else + false + end + rescue Encoding::CompatibilityError + # "✅".b.include?("✅") raises Encoding::CompatibilityError despite being materially equal + left.b.include?(right.b) + end + + def apply_method_literal(node, other) + other.send(node.method_name) if other.respond_to?(node.method_name) + end + + def equal_variables(left, right) + return apply_method_literal(left, right) if left.is_a?(MethodLiteral) + return apply_method_literal(right, left) if right.is_a?(MethodLiteral) + + left == right + end + + class ParseTreeVisitor < Liquid::ParseTreeVisitor + def children + [ + @node.left_node, + @node.right_node, + ] + end + end end end diff --git a/lib/liquid/condition.rb b/lib/liquid/condition.rb index 8c4afc980..bc3927295 100644 --- a/lib/liquid/condition.rb +++ b/lib/liquid/condition.rb @@ -29,16 +29,6 @@ class Condition # :nodoc: left.b.include?(right.b) end, } - - class MethodLiteral - attr_reader :method_name, :to_s - - def initialize(method_name, to_s) - @method_name = method_name - @to_s = to_s - end - end - @@method_literals = { 'blank' => MethodLiteral.new(:blank?, '').freeze, 'empty' => MethodLiteral.new(:empty?, '').freeze, diff --git a/lib/liquid/expression.rb b/lib/liquid/expression.rb index 1bf7edcf6..4f9754b9f 100644 --- a/lib/liquid/expression.rb +++ b/lib/liquid/expression.rb @@ -9,8 +9,8 @@ class Expression '' => nil, 'true' => true, 'false' => false, - 'blank' => '', - 'empty' => '', + 'blank' => MethodLiteral.new(:blank?, '').freeze, + 'empty' => MethodLiteral.new(:empty?, '').freeze, }.freeze DOT = ".".ord diff --git a/lib/liquid/method_literal.rb b/lib/liquid/method_literal.rb new file mode 100644 index 000000000..812c7199f --- /dev/null +++ b/lib/liquid/method_literal.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +module Liquid + class MethodLiteral + attr_reader :method_name, :to_s + + def initialize(method_name, to_s) + @method_name = method_name + @to_s = to_s + end + + def to_liquid + to_s + end + end +end diff --git a/lib/liquid/tags/if.rb b/lib/liquid/tags/if.rb index 3471f6778..b07a91c73 100644 --- a/lib/liquid/tags/if.rb +++ b/lib/liquid/tags/if.rb @@ -96,13 +96,7 @@ def parse_binary_comparisons(p) end def parse_comparison(p) - a = parse_expression(p) - if (op = p.consume?(:comparison) || p.consume?(:equality)) - b = parse_expression(p) - Condition.new(a, op, b) - else - Condition.new(a) - end + Condition.new(p.expression) end class ParseTreeVisitor < Liquid::ParseTreeVisitor diff --git a/test/integration/tags/if_else_tag_test.rb b/test/integration/tags/if_else_tag_test.rb index 3f04f2b8d..b37e4e411 100644 --- a/test/integration/tags/if_else_tag_test.rb +++ b/test/integration/tags/if_else_tag_test.rb @@ -147,28 +147,6 @@ def test_syntax_error_no_expression assert_raises(SyntaxError) { assert_template_result('', '{% if %}') } end - def test_if_with_custom_condition - original_op = Condition.operators['contains'] - Condition.operators['contains'] = :[] - - assert_template_result('yes', %({% if 'bob' contains 'o' %}yes{% endif %})) - assert_template_result('no', %({% if 'bob' contains 'f' %}yes{% else %}no{% endif %})) - ensure - Condition.operators['contains'] = original_op - end - - def test_operators_are_ignored_unless_isolated - original_op = Condition.operators['contains'] - Condition.operators['contains'] = :[] - - assert_template_result( - 'yes', - %({% if 'gnomeslab-and-or-liquid' contains 'gnomeslab-and-or-liquid' %}yes{% endif %}), - ) - ensure - Condition.operators['contains'] = original_op - end - def test_operators_are_whitelisted assert_raises(SyntaxError) do assert_template_result('', %({% if 1 or throw or or 1 %}yes{% endif %})) diff --git a/test/unit/binary_expression_test.rb b/test/unit/binary_expression_test.rb new file mode 100644 index 000000000..19fe33d7f --- /dev/null +++ b/test/unit/binary_expression_test.rb @@ -0,0 +1,101 @@ +# frozen_string_literal: true + +require 'test_helper' + +class BinaryExpressionTest < Minitest::Test + include Liquid + + def test_simple_comparison_evaluation + assert_eval(false, BinaryExpression.new(5, ">", 5)) + assert_eval(true, BinaryExpression.new(5, ">=", 5)) + assert_eval(false, BinaryExpression.new(5, "<", 5)) + assert_eval(true, BinaryExpression.new(5, "<=", 5)) + assert_eval(true, BinaryExpression.new("abcd", "contains", "a")) + end + + def test_complex_evaluation + # 1 > 2 == 2 > 3 + assert_eval(true, BinaryExpression.new( + BinaryExpression.new(1, '>', 2), + '==', + BinaryExpression.new(2, '>', 3), + )) + + # 1 > 2 != 2 > 3 + assert_eval(false, BinaryExpression.new( + BinaryExpression.new(1, '>', 2), + '!=', + BinaryExpression.new(2, '>', 3), + )) + + # a > 0 == b.prop > 0 + assert_eval( + true, + BinaryExpression.new( + BinaryExpression.new(var('a'), '>', 0), + '==', + BinaryExpression.new(var('b.prop'), '>', 0), + ), + { 'a' => 1, 'b' => { 'prop' => 2 } }, + ) + end + + def test_method_literal_equality + empty = MethodLiteral.new(:empty?, '') + + # a == empty, empty == a + assert_eval(false, BinaryExpression.new("123", "==", empty)) + assert_eval(true, BinaryExpression.new("", "==", empty)) + assert_eval(false, BinaryExpression.new(empty, "==", "123")) + assert_eval(true, BinaryExpression.new(empty, "==", "")) + + # a does not have .empty? + assert_eval(nil, BinaryExpression.new(1, "==", empty)) + assert_eval(nil, BinaryExpression.new(true, "==", empty)) + assert_eval(nil, BinaryExpression.new(false, "==", empty)) + assert_eval(nil, BinaryExpression.new(nil, "==", empty)) + + # a != empty + assert_eval(true, BinaryExpression.new("123", "!=", empty)) + assert_eval(false, BinaryExpression.new("", "!=", empty)) + assert_eval(true, BinaryExpression.new(empty, "!=", "123")) + assert_eval(false, BinaryExpression.new(empty, "!=", "")) + + # a does not have .empty? + assert_eval(true, BinaryExpression.new(1, "!=", empty)) + assert_eval(true, BinaryExpression.new(true, "!=", empty)) + assert_eval(true, BinaryExpression.new(false, "!=", empty)) + assert_eval(true, BinaryExpression.new(nil, "!=", empty)) + end + + def test_method_literal_comparison + empty = MethodLiteral.new(:empty?, '') + + ['>', '>='].each do |op| + assert_eval(nil, BinaryExpression.new("123", op, empty)) + assert_eval(nil, BinaryExpression.new("", op, empty)) + assert_eval(nil, BinaryExpression.new(empty, op, "123")) + assert_eval(nil, BinaryExpression.new(empty, op, "")) + end + + # Interesting case, contains on strings does include?(right.to_s) + assert_eval(true, BinaryExpression.new("123", "contains", empty)) + assert_eval(true, BinaryExpression.new("", "contains", empty)) + end + + def assert_eval(expected, expr, assigns = {}) + actual = expr.evaluate(context(assigns)) + message = "Expected '#{expr}' to evaluate to '#{expected}'" + return assert_nil(actual, message) if expected.nil? + + assert_equal(expected, actual, message) + end + + def var(markup) + Parser.new(markup).variable_lookup + end + + def context(assigns = {}) + Context.build(outer_scope: assigns) + end +end diff --git a/test/unit/condition_unit_test.rb b/test/unit/condition_unit_test.rb index 9123b5099..fee8f7263 100644 --- a/test/unit/condition_unit_test.rb +++ b/test/unit/condition_unit_test.rb @@ -183,7 +183,7 @@ def test_parse_expression_returns_method_literal_for_blank_and_empty parser = parse_context.new_parser('blank') result = Condition.parse_expression(parser) - assert_instance_of(Condition::MethodLiteral, result) + assert_instance_of(MethodLiteral, result) end private diff --git a/test/unit/parser_unit_test.rb b/test/unit/parser_unit_test.rb index a0c86c081..49ef252ad 100644 --- a/test/unit/parser_unit_test.rb +++ b/test/unit/parser_unit_test.rb @@ -82,24 +82,24 @@ def test_equality expr = p.expression assert(expr.is_a?(BinaryExpression)) assert_equal('==', expr.operator) - assert_equal('a', expr.left.name) - assert_equal('b', expr.right.name) + assert_equal('a', expr.left_node.name) + assert_equal('b', expr.right_node.name) # BinaryExpression(==) - # left: BinaryExpression(<) - # left: 0 - # right: 5 - # right: BinaryExpression(>) - # left: 6 - # right: 1 + # left_node: BinaryExpression(<) + # left_node: 0 + # right_node: 5 + # right_node: BinaryExpression(>) + # left_node: 6 + # right_node: 1 p = new_parser("0 < 5 == 6 > 1") expr = p.expression assert(expr.is_a?(BinaryExpression)) assert_equal('==', expr.operator) - assert_equal(0, expr.left.left) - assert_equal(5, expr.left.right) - assert_equal(6, expr.right.left) - assert_equal(1, expr.right.right) + assert_equal(0, expr.left_node.left_node) + assert_equal(5, expr.left_node.right_node) + assert_equal(6, expr.right_node.left_node) + assert_equal(1, expr.right_node.right_node) end def test_comparison @@ -107,23 +107,23 @@ def test_comparison expr = p.expression assert(expr.is_a?(BinaryExpression)) assert_equal('>', expr.operator) - assert(expr.left.is_a?(VariableLookup)) - assert_equal('a', expr.left.name) - assert(expr.right.is_a?(VariableLookup)) - assert_equal('b', expr.right.name) + assert(expr.left_node.is_a?(VariableLookup)) + assert_equal('a', expr.left_node.name) + assert(expr.right_node.is_a?(VariableLookup)) + assert_equal('b', expr.right_node.name) # BinaryExpression(>=) - # left: BinaryExpression(>) - # left: 10 - # right: 5 - # right: 4 + # left_node: BinaryExpression(>) + # left_node: 10 + # right_node: 5 + # right_node: 4 p = new_parser("10 > 5 >= 4") expr = p.expression assert(expr.is_a?(BinaryExpression)) assert_equal('>=', expr.operator) - assert_equal(10, expr.left.left) - assert_equal(5, expr.left.right) - assert_equal(4, expr.right) + assert_equal(10, expr.left_node.left_node) + assert_equal(5, expr.left_node.right_node) + assert_equal(4, expr.right_node) end def test_number From f49de89881600cd112dc173c6ca757a54298c5e7 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Wed, 3 Dec 2025 16:05:49 -0500 Subject: [PATCH 23/32] Add boolean expressions as variable unit tests --- test/integration/variable_test.rb | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/integration/variable_test.rb b/test/integration/variable_test.rb index 38096e41d..b81a2106b 100644 --- a/test/integration/variable_test.rb +++ b/test/integration/variable_test.rb @@ -11,6 +11,24 @@ def test_simple_variable assert_template_result('worked wonderfully', "{{test}}", { 'test' => 'worked wonderfully' }) end + def test_equality + assert_template_result('true', "{{ 5 == 5 }}") + assert_template_result('false', "{{ 5 == 3 }}") + end + + def test_comparison + assert_template_result('true', "{{ 5 > 3 }}") + assert_template_result('false', "{{ 5 < 3 }}") + end + + def test_expression_piped_into_filter + assert_template_result('TRUE', "{{ 5 == 5 | upcase }}") + end + + def test_expression_used_as_filter_argument + assert_template_result('A: TRUE', "{{ 'a: $a' | replace: '$a', 5 == 5 | upcase }}") + end + def test_variable_render_calls_to_liquid assert_template_result('foobar', '{{ foo }}', { 'foo' => ThingWithToLiquid.new }) end From 585270e007f63f20449fe5ae9e2d87d4e851e5f1 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Thu, 4 Dec 2025 14:52:41 -0500 Subject: [PATCH 24/32] Make Condition unit tests go through BinaryExpression Instead of having Condition parse the left op right, make BinaryExpression do it. Make sure all the tests pass as they used to in the process. --- lib/liquid/binary_expression.rb | 2 ++ test/unit/condition_unit_test.rb | 15 +++++++++------ 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/lib/liquid/binary_expression.rb b/lib/liquid/binary_expression.rb index 00e3873d2..8ac743f87 100644 --- a/lib/liquid/binary_expression.rb +++ b/lib/liquid/binary_expression.rb @@ -30,6 +30,8 @@ def evaluate(context) !equal_variables(left, right) when 'contains' contains(left, right) + else + raise(Liquid::ArgumentError, "Unknown operator #{operator}") end rescue ::ArgumentError => e raise Liquid::ArgumentError, e.message diff --git a/test/unit/condition_unit_test.rb b/test/unit/condition_unit_test.rb index fee8f7263..09f654a69 100644 --- a/test/unit/condition_unit_test.rb +++ b/test/unit/condition_unit_test.rb @@ -170,18 +170,18 @@ def test_parse_expression environment = Environment.build parse_context = ParseContext.new(environment: environment) parser = parse_context.new_parser('product.title') - result = Condition.parse_expression(parser) + result = parser.expression assert_instance_of(VariableLookup, result) assert_equal('product', result.name) assert_equal(['title'], result.lookups) end - def test_parse_expression_returns_method_literal_for_blank_and_empty + def test_parser_expression_returns_method_literal_for_blank_and_empty environment = Environment.build parse_context = ParseContext.new(environment: environment) parser = parse_context.new_parser('blank') - result = Condition.parse_expression(parser) + result = parser.expression assert_instance_of(MethodLiteral, result) end @@ -189,22 +189,25 @@ def test_parse_expression_returns_method_literal_for_blank_and_empty private def assert_evaluates_true(left, op, right) + expr = BinaryExpression.new(left, op, right) assert( - Condition.new(left, op, right).evaluate(@context), + Condition.new(expr).evaluate(@context), "Evaluated false: #{left.inspect} #{op} #{right.inspect}", ) end def assert_evaluates_false(left, op, right) + expr = BinaryExpression.new(left, op, right) assert( - !Condition.new(left, op, right).evaluate(@context), + !Condition.new(expr).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) + expr = BinaryExpression.new(left, op, right) + Condition.new(expr).evaluate(@context) end end end # ConditionTest From c6e297d9dca1492e411b174a0b59685f03f0659d Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Thu, 4 Dec 2025 14:53:37 -0500 Subject: [PATCH 25/32] Remove Condition.operators feature that let you define operators It was incompatible with the lexer anyway. Not used internally either. --- test/unit/condition_unit_test.rb | 9 --------- 1 file changed, 9 deletions(-) diff --git a/test/unit/condition_unit_test.rb b/test/unit/condition_unit_test.rb index 09f654a69..a706c7396 100644 --- a/test/unit/condition_unit_test.rb +++ b/test/unit/condition_unit_test.rb @@ -136,15 +136,6 @@ def test_and_condition assert_equal(false, condition.evaluate(Context.new)) end - def test_should_allow_custom_proc_operator - Condition.operators['starts_with'] = proc { |_cond, left, right| left =~ /^#{right}/ } - - assert_evaluates_true('bob', 'starts_with', 'b') - assert_evaluates_false('bob', 'starts_with', 'o') - ensure - Condition.operators.delete('starts_with') - end - def test_left_or_right_may_contain_operators @context = Liquid::Context.new @context['one'] = @context['another'] = "gnomeslab-and-or-liquid" From 1130dc7138a6b73e4802eb3721d9e2956eff6f30 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Thu, 4 Dec 2025 15:12:25 -0500 Subject: [PATCH 26/32] Remove Condition.op, Condition.right - Remove comparison expression logic --- lib/liquid/condition.rb | 89 ++------------------------------ lib/liquid/tags/case.rb | 4 +- lib/liquid/tags/if.rb | 4 -- test/unit/condition_unit_test.rb | 47 +++++++++-------- 4 files changed, 33 insertions(+), 111 deletions(-) diff --git a/lib/liquid/condition.rb b/lib/liquid/condition.rb index bc3927295..dbac9e29f 100644 --- a/lib/liquid/condition.rb +++ b/lib/liquid/condition.rb @@ -5,51 +5,15 @@ module Liquid # # Example: # - # c = Condition.new(1, '==', 1) + # c = Condition.new(expr) # c.evaluate #=> true # class Condition # :nodoc: - @@operators = { - '==' => ->(cond, left, right) { cond.send(:equal_variables, left, right) }, - '!=' => ->(cond, left, right) { !cond.send(:equal_variables, left, right) }, - '<>' => ->(cond, left, right) { !cond.send(:equal_variables, left, right) }, - '<' => :<, - '>' => :>, - '>=' => :>=, - '<=' => :<=, - 'contains' => lambda do |_cond, left, right| - if left && right && left.respond_to?(:include?) - right = right.to_s if left.is_a?(String) - left.include?(right) - else - false - end - rescue Encoding::CompatibilityError - # "✅".b.include?("✅") raises Encoding::CompatibilityError despite being materially equal - left.b.include?(right.b) - end, - } - @@method_literals = { - 'blank' => MethodLiteral.new(:blank?, '').freeze, - 'empty' => MethodLiteral.new(:empty?, '').freeze, - } - - def self.operators - @@operators - end - - def self.parse_expression(parser) - markup = parser.expression_string - @@method_literals[markup] || parser.unsafe_parse_expression(markup) - end - attr_reader :attachment, :child_condition - attr_accessor :left, :operator, :right + attr_accessor :left - def initialize(left = nil, operator = nil, right = nil) - @left = left - @operator = operator - @right = right + def initialize(left = nil) + @left = left @child_relation = nil @child_condition = nil @@ -59,7 +23,7 @@ def evaluate(context = deprecated_default_context) condition = self result = nil loop do - result = interpret_condition(condition.left, condition.right, condition.operator, context) + result = context.evaluate(condition.left) case condition.child_relation when :or @@ -102,48 +66,6 @@ def inspect private - def equal_variables(left, right) - if left.is_a?(MethodLiteral) - if right.respond_to?(left.method_name) - return right.send(left.method_name) - else - return nil - end - end - - if right.is_a?(MethodLiteral) - if left.respond_to?(right.method_name) - return left.send(right.method_name) - else - return nil - end - end - - left == right - end - - def interpret_condition(left, right, op, context) - # If the operator is empty this means that the decision statement is just - # a single variable. We can just poll this variable from the context and - # return this as the result. - return context.evaluate(left) if op.nil? - - left = Liquid::Utils.to_liquid_value(context.evaluate(left)) - right = Liquid::Utils.to_liquid_value(context.evaluate(right)) - - operation = self.class.operators[op] || raise(Liquid::ArgumentError, "Unknown operator #{op}") - - if operation.respond_to?(:call) - operation.call(self, left, right) - elsif left.respond_to?(operation) && right.respond_to?(operation) && !left.is_a?(Hash) && !right.is_a?(Hash) - begin - left.send(operation, right) - rescue ::ArgumentError => e - raise Liquid::ArgumentError, e.message - end - end - end - def deprecated_default_context warn("DEPRECATION WARNING: Condition#evaluate without a context argument is deprecated" \ " and will be removed from Liquid 6.0.0.") @@ -154,7 +76,6 @@ class ParseTreeVisitor < Liquid::ParseTreeVisitor def children [ @node.left, - @node.right, @node.child_condition, @node.attachment ].compact diff --git a/lib/liquid/tags/case.rb b/lib/liquid/tags/case.rb index 0f23ead1d..225c40922 100644 --- a/lib/liquid/tags/case.rb +++ b/lib/liquid/tags/case.rb @@ -99,8 +99,8 @@ def parse_when(markup, body) parser = @parse_context.new_parser(markup) loop do - expr = parser.expression - block = Condition.new(@left, '==', expr) + expr = BinaryExpression.new(@left, '==', parser.expression) + block = Condition.new(expr) block.attach(body) @blocks << block diff --git a/lib/liquid/tags/if.rb b/lib/liquid/tags/if.rb index b07a91c73..97d8ba61c 100644 --- a/lib/liquid/tags/if.rb +++ b/lib/liquid/tags/if.rb @@ -73,10 +73,6 @@ def push_block(tag, markup) block.attach(new_body) end - def parse_expression(parser) - Condition.parse_expression(parser) - end - def parse_markup(markup) p = @parse_context.new_parser(markup) condition = parse_binary_comparisons(p) diff --git a/test/unit/condition_unit_test.rb b/test/unit/condition_unit_test.rb index a706c7396..3416c8730 100644 --- a/test/unit/condition_unit_test.rb +++ b/test/unit/condition_unit_test.rb @@ -9,11 +9,6 @@ def setup @context = Liquid::Context.new end - def test_basic_condition - assert_equal(false, Condition.new(1, '==', 2).evaluate(Context.new)) - assert_equal(true, Condition.new(1, '==', 1).evaluate(Context.new)) - end - def test_default_operators_evalute_true assert_evaluates_true(1, '==', 1) assert_evaluates_true(1, '!=', 2) @@ -72,11 +67,11 @@ def test_comparation_of_int_and_str end def test_hash_compare_backwards_compatibility - assert_nil(Condition.new({}, '>', 2).evaluate(Context.new)) - assert_nil(Condition.new(2, '>', {}).evaluate(Context.new)) - assert_equal(false, Condition.new({}, '==', 2).evaluate(Context.new)) - assert_equal(true, Condition.new({ 'a' => 1 }, '==', 'a' => 1).evaluate(Context.new)) - assert_equal(true, Condition.new({ 'a' => 2 }, 'contains', 'a').evaluate(Context.new)) + assert_evaluates_nil({}, '>', 2) + assert_evaluates_nil(2, '>', {}) + assert_evaluates_false({}, '==', 2) + assert_evaluates_true({ 'a' => 1 }, '==', 'a' => 1) + assert_evaluates_true({ 'a' => 2 }, 'contains', 'a') end def test_contains_works_on_arrays @@ -110,29 +105,30 @@ def test_contains_with_string_left_operand_coerces_right_operand_to_string end def test_or_condition - condition = Condition.new(1, '==', 2) - assert_equal(false, condition.evaluate(Context.new)) - - condition.or(Condition.new(2, '==', 1)) + false_expr = Parser.new('1 == 2').expression + true_expr = Parser.new('1 == 1').expression + condition = Condition.new(false_expr) assert_equal(false, condition.evaluate(Context.new)) - condition.or(Condition.new(1, '==', 1)) + condition.or(Condition.new(false_expr)) + assert_equal(false, condition.evaluate(Context.new)) + condition.or(Condition.new(true_expr)) assert_equal(true, condition.evaluate(Context.new)) end def test_and_condition - condition = Condition.new(1, '==', 1) + false_expr = Parser.new('1 == 2').expression + true_expr = Parser.new('1 == 1').expression + condition = Condition.new(true_expr) assert_equal(true, condition.evaluate(Context.new)) - condition.and(Condition.new(2, '==', 2)) - + condition.and(Condition.new(true_expr)) assert_equal(true, condition.evaluate(Context.new)) - condition.and(Condition.new(2, '==', 1)) - + condition.and(Condition.new(false_expr)) assert_equal(false, condition.evaluate(Context.new)) end @@ -149,7 +145,8 @@ def test_default_context_is_deprecated end _out, err = capture_io do - assert_equal(true, Condition.new(1, '==', 1).evaluate) + expr = Parser.new('1 == 1').expression + assert_equal(true, Condition.new(expr).evaluate) end expected = "DEPRECATION WARNING: Condition#evaluate without a context argument is deprecated" \ @@ -179,6 +176,14 @@ def test_parser_expression_returns_method_literal_for_blank_and_empty private + def assert_evaluates_nil(left, op, right) + expr = BinaryExpression.new(left, op, right) + assert_nil( + Condition.new(expr).evaluate(@context), + "Evaluated not nil: #{left.inspect} #{op} #{right.inspect}", + ) + end + def assert_evaluates_true(left, op, right) expr = BinaryExpression.new(left, op, right) assert( From 7275dfe79655f84b489887e11332432f959d88d1 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Thu, 4 Dec 2025 16:26:51 -0500 Subject: [PATCH 27/32] Add support for logical expressions --- lib/liquid/binary_expression.rb | 10 ++++++- lib/liquid/parser.rb | 22 ++++++++++++-- lib/liquid/tags/case.rb | 2 +- test/unit/binary_expression_test.rb | 45 +++++++++++++++++++++++++++++ test/unit/parser_unit_test.rb | 29 +++++++++++++++++++ 5 files changed, 104 insertions(+), 4 deletions(-) diff --git a/lib/liquid/binary_expression.rb b/lib/liquid/binary_expression.rb index 8ac743f87..71d1c4064 100644 --- a/lib/liquid/binary_expression.rb +++ b/lib/liquid/binary_expression.rb @@ -13,6 +13,14 @@ def initialize(left, operator, right) def evaluate(context) left = value(left_node, context) + + # logical relation short circuiting + if operator == 'and' + return left && value(right_node, context) + elsif operator == 'or' + return left || value(right_node, context) + end + right = value(right_node, context) case operator @@ -38,7 +46,7 @@ def evaluate(context) end def to_s - "(#{left_node} #{operator} #{right_node})" + "(#{left_node.inspect} #{operator} #{right_node.inspect})" end private diff --git a/lib/liquid/parser.rb b/lib/liquid/parser.rb index 8d5151e7d..f715e6ed4 100644 --- a/lib/liquid/parser.rb +++ b/lib/liquid/parser.rb @@ -47,12 +47,30 @@ def look(type, ahead = 0) tok[0] == type end - # expression := equality + # expression := logical + # logical := equality (("and" | "or") equality)* # equality := comparison (("==" | "!=" | "<>") comparison)* # comparison := primary ((">=" | ">" | "<" | "<=" | ... ) primary)* # primary := string | number | variable_lookup | range | boolean def expression - equality + logical + end + + # Logical relations in Liquid, unlike other languages, are right-to-left + # associative. This creates a right-leaning tree and is why the method + # looks a bit more complicated + # + # `a == b and b or c` is evaluated like (a and (b or c)) + def logical + expr = equality + while (operator = id?('and') || id?('or')) + if expr.is_a?(BinaryExpression) && (expr.operator == 'and' || expr.operator == 'or') + expr.right_node = BinaryExpression.new(expr.right_node, operator, equality) + else + expr = BinaryExpression.new(expr, operator, equality) + end + end + expr end def equality diff --git a/lib/liquid/tags/case.rb b/lib/liquid/tags/case.rb index 225c40922..9288af3ad 100644 --- a/lib/liquid/tags/case.rb +++ b/lib/liquid/tags/case.rb @@ -99,7 +99,7 @@ def parse_when(markup, body) parser = @parse_context.new_parser(markup) loop do - expr = BinaryExpression.new(@left, '==', parser.expression) + expr = BinaryExpression.new(@left, '==', parser.equality) block = Condition.new(expr) block.attach(body) @blocks << block diff --git a/test/unit/binary_expression_test.rb b/test/unit/binary_expression_test.rb index 19fe33d7f..6b5bb2367 100644 --- a/test/unit/binary_expression_test.rb +++ b/test/unit/binary_expression_test.rb @@ -2,6 +2,25 @@ require 'test_helper' +class ExecutionSpy + attr_reader :called + attr_accessor :value + + def initialize(value) + @called = false + @value = value + end + + def to_liquid_value + @called = true + @value + end + + def reset + @called = false + end +end + class BinaryExpressionTest < Minitest::Test include Liquid @@ -13,6 +32,32 @@ def test_simple_comparison_evaluation assert_eval(true, BinaryExpression.new("abcd", "contains", "a")) end + def test_logical_expression_short_circuiting + spy = ExecutionSpy.new(true) + + # false or spy should try spy + assert_eval(true, BinaryExpression.new(false, 'or', spy)) + assert_equal(true, spy.called) + + spy.reset + + # true or spy should not call spy + assert_eval(true, BinaryExpression.new(true, 'or', spy)) + assert_equal(false, spy.called) + + spy.reset + + # true and spy should try spy + assert_eval(true, BinaryExpression.new(true, 'and', spy)) + assert_equal(true, spy.called) + + spy.reset + + # false and spy should not try spy + assert_eval(false, BinaryExpression.new(false, 'and', spy)) + assert_equal(false, spy.called) + end + def test_complex_evaluation # 1 > 2 == 2 > 3 assert_eval(true, BinaryExpression.new( diff --git a/test/unit/parser_unit_test.rb b/test/unit/parser_unit_test.rb index 49ef252ad..20e8692cf 100644 --- a/test/unit/parser_unit_test.rb +++ b/test/unit/parser_unit_test.rb @@ -77,6 +77,35 @@ def test_expression assert_equal((0..5), p.expression) end + def test_logical + p = new_parser("a and b") + expr = p.expression + assert(expr.is_a?(BinaryExpression)) + assert_equal('and', expr.operator) + assert_equal('a', expr.left_node.name) + assert_equal('b', expr.right_node.name) + + p = new_parser("a and b or c") + expr = p.expression + assert(expr.is_a?(BinaryExpression)) + assert_equal('and', expr.operator) + assert_equal('a', expr.left_node.name) + assert_equal('or', expr.right_node.operator) + assert_equal('b', expr.right_node.left_node.name) + assert_equal('c', expr.right_node.right_node.name) + + p = new_parser("a == b and c or d") + expr = p.expression + assert(expr.is_a?(BinaryExpression)) + assert_equal('and', expr.operator) + assert_equal('==', expr.left_node.operator) + assert_equal('a', expr.left_node.left_node.name) + assert_equal('b', expr.left_node.right_node.name) + assert_equal('or', expr.right_node.operator) + assert_equal('c', expr.right_node.left_node.name) + assert_equal('d', expr.right_node.right_node.name) + end + def test_equality p = new_parser("a == b") expr = p.expression From 0613db4f4ee016a93595c783926133d1627d0119 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Thu, 4 Dec 2025 16:33:52 -0500 Subject: [PATCH 28/32] Remove Condition#{child_relation,and,or} - Remove Condition#child_relation - Remove Condition#and - Remove Condition#or - Simplify Condition#evaluate This logic was moved to the Parser & BinaryExpression --- lib/liquid/condition.rb | 33 ++------------------------------ lib/liquid/tags/if.rb | 17 +--------------- test/unit/condition_unit_test.rb | 32 ++++++++++++++++++++----------- 3 files changed, 24 insertions(+), 58 deletions(-) diff --git a/lib/liquid/condition.rb b/lib/liquid/condition.rb index dbac9e29f..01acbff91 100644 --- a/lib/liquid/condition.rb +++ b/lib/liquid/condition.rb @@ -9,43 +9,15 @@ module Liquid # c.evaluate #=> true # class Condition # :nodoc: - attr_reader :attachment, :child_condition + attr_reader :attachment attr_accessor :left def initialize(left = nil) @left = left - - @child_relation = nil - @child_condition = nil end def evaluate(context = deprecated_default_context) - condition = self - result = nil - loop do - result = context.evaluate(condition.left) - - case condition.child_relation - when :or - break if Liquid::Utils.to_liquid_value(result) - when :and - break unless Liquid::Utils.to_liquid_value(result) - else - break - end - condition = condition.child_condition - end - result - end - - def or(condition) - @child_relation = :or - @child_condition = condition - end - - def and(condition) - @child_relation = :and - @child_condition = condition + context.evaluate(left) end def attach(attachment) @@ -76,7 +48,6 @@ class ParseTreeVisitor < Liquid::ParseTreeVisitor def children [ @node.left, - @node.child_condition, @node.attachment ].compact end diff --git a/lib/liquid/tags/if.rb b/lib/liquid/tags/if.rb index 97d8ba61c..559dce08c 100644 --- a/lib/liquid/tags/if.rb +++ b/lib/liquid/tags/if.rb @@ -75,26 +75,11 @@ def push_block(tag, markup) def parse_markup(markup) p = @parse_context.new_parser(markup) - condition = parse_binary_comparisons(p) + condition = Condition.new(p.expression) p.consume(:end_of_string) condition end - def parse_binary_comparisons(p) - condition = parse_comparison(p) - first_condition = condition - while (op = p.id?('and') || p.id?('or')) - child_condition = parse_comparison(p) - condition.send(op, child_condition) - condition = child_condition - end - first_condition - end - - def parse_comparison(p) - Condition.new(p.expression) - end - class ParseTreeVisitor < Liquid::ParseTreeVisitor def children @node.blocks diff --git a/test/unit/condition_unit_test.rb b/test/unit/condition_unit_test.rb index 3416c8730..128d01469 100644 --- a/test/unit/condition_unit_test.rb +++ b/test/unit/condition_unit_test.rb @@ -105,31 +105,37 @@ def test_contains_with_string_left_operand_coerces_right_operand_to_string end def test_or_condition - false_expr = Parser.new('1 == 2').expression - true_expr = Parser.new('1 == 1').expression + false_expr = '1 == 2' + true_expr = '1 == 1' - condition = Condition.new(false_expr) + condition = Condition.new(expression(false_expr)) assert_equal(false, condition.evaluate(Context.new)) - condition.or(Condition.new(false_expr)) + condition = Condition.new(expression("#{false_expr} or #{false_expr}")) assert_equal(false, condition.evaluate(Context.new)) - condition.or(Condition.new(true_expr)) + condition = Condition.new(expression("#{false_expr} or #{true_expr}")) + assert_equal(true, condition.evaluate(Context.new)) + + condition = Condition.new(expression("#{true_expr} or #{false_expr}")) assert_equal(true, condition.evaluate(Context.new)) end def test_and_condition - false_expr = Parser.new('1 == 2').expression - true_expr = Parser.new('1 == 1').expression + false_expr = '1 == 2' + true_expr = '1 == 1' - condition = Condition.new(true_expr) + condition = Condition.new(expression(true_expr)) assert_equal(true, condition.evaluate(Context.new)) - condition.and(Condition.new(true_expr)) - assert_equal(true, condition.evaluate(Context.new)) + condition = Condition.new(expression("#{true_expr} and #{false_expr}")) + assert_equal(false, condition.evaluate(Context.new)) - condition.and(Condition.new(false_expr)) + condition = Condition.new(expression("#{false_expr} and #{true_expr}")) assert_equal(false, condition.evaluate(Context.new)) + + condition = Condition.new(expression("#{true_expr} and #{true_expr}")) + assert_equal(true, condition.evaluate(Context.new)) end def test_left_or_right_may_contain_operators @@ -206,4 +212,8 @@ def assert_evaluates_argument_error(left, op, right) Condition.new(expr).evaluate(@context) end end + + def expression(markup) + Parser.new(markup).expression + end end # ConditionTest From 34bba2b5ff33a254cc8cd8b63d87c92a5e6f5ccf Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Thu, 4 Dec 2025 19:58:48 -0500 Subject: [PATCH 29/32] Prepare logical for grouping --- lib/liquid/lexer.rb | 22 ++++++++++++++-------- lib/liquid/parser.rb | 10 +++------- lib/liquid/tags/case.rb | 2 +- 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/lib/liquid/lexer.rb b/lib/liquid/lexer.rb index c0a6463bb..f6079f31a 100644 --- a/lib/liquid/lexer.rb +++ b/lib/liquid/lexer.rb @@ -6,22 +6,24 @@ class Lexer CLOSE_SQUARE = [:close_square, "]"].freeze COLON = [:colon, ":"].freeze COMMA = [:comma, ","].freeze - COMPARISION_NOT_EQUAL = [:equality, "!="].freeze COMPARISON_CONTAINS = [:comparison, "contains"].freeze - COMPARISON_EQUAL = [:equality, "=="].freeze COMPARISON_GREATER_THAN = [:comparison, ">"].freeze COMPARISON_GREATER_THAN_OR_EQUAL = [:comparison, ">="].freeze COMPARISON_LESS_THAN = [:comparison, "<"].freeze COMPARISON_LESS_THAN_OR_EQUAL = [:comparison, "<="].freeze - COMPARISON_NOT_EQUAL_ALT = [:equality, "<>"].freeze + EQUALITY_EQUAL_EQUAL = [:equality, "=="].freeze + EQUALITY_NOT_EQUAL = [:equality, "!="].freeze + EQUALITY_NOT_EQUAL_ALT = [:equality, "<>"].freeze DASH = [:dash, "-"].freeze DOT = [:dot, "."].freeze DOTDOT = [:dotdot, ".."].freeze DOT_ORD = ".".ord DOUBLE_STRING_LITERAL = /"[^\"]*"/ EOS = [:end_of_string].freeze - IDENTIFIER = /[a-zA-Z_][\w-]*\??/ - NUMBER_LITERAL = /-?\d+(\.\d+)?/ + IDENTIFIER = /[a-zA-Z_][\w-]*\??/ + LOGICAL_AND = [:logical, 'and'].freeze + LOGICAL_OR = [:logical, 'or'].freeze + NUMBER_LITERAL = /-?\d+(\.\d+)?/ OPEN_ROUND = [:open_round, "("].freeze OPEN_SQUARE = [:open_square, "["].freeze PIPE = [:pipe, "|"].freeze @@ -38,11 +40,11 @@ class Lexer TWO_CHARS_COMPARISON_JUMP_TABLE = [].tap do |table| table["=".ord] = [].tap do |sub_table| - sub_table["=".ord] = COMPARISON_EQUAL + sub_table["=".ord] = EQUALITY_EQUAL_EQUAL sub_table.freeze end table["!".ord] = [].tap do |sub_table| - sub_table["=".ord] = COMPARISION_NOT_EQUAL + sub_table["=".ord] = EQUALITY_NOT_EQUAL sub_table.freeze end table.freeze @@ -51,7 +53,7 @@ class Lexer COMPARISON_JUMP_TABLE = [].tap do |table| table["<".ord] = [].tap do |sub_table| sub_table["=".ord] = COMPARISON_LESS_THAN_OR_EQUAL - sub_table[">".ord] = COMPARISON_NOT_EQUAL_ALT + sub_table[">".ord] = EQUALITY_NOT_EQUAL_ALT sub_table.freeze end table[">".ord] = [].tap do |sub_table| @@ -151,6 +153,10 @@ def tokenize(ss) # Special case for "contains" output << if type == :id && t == "contains" && output.last&.first != :dot COMPARISON_CONTAINS + elsif type == :id && t == "and" && output.last&.first != :dot + LOGICAL_AND + elsif type == :id && t == "or" && output.last&.first != :dot + LOGICAL_OR else [type, t] end diff --git a/lib/liquid/parser.rb b/lib/liquid/parser.rb index f715e6ed4..15a11bc30 100644 --- a/lib/liquid/parser.rb +++ b/lib/liquid/parser.rb @@ -62,14 +62,10 @@ def expression # # `a == b and b or c` is evaluated like (a and (b or c)) def logical + operator = nil expr = equality - while (operator = id?('and') || id?('or')) - if expr.is_a?(BinaryExpression) && (expr.operator == 'and' || expr.operator == 'or') - expr.right_node = BinaryExpression.new(expr.right_node, operator, equality) - else - expr = BinaryExpression.new(expr, operator, equality) - end - end + expr = BinaryExpression.new(expr, operator, equality) if (operator = consume?(:logical)) + expr.right_node = BinaryExpression.new(expr.right_node, operator, equality) while (operator = consume?(:logical)) expr end diff --git a/lib/liquid/tags/case.rb b/lib/liquid/tags/case.rb index 9288af3ad..4f4731ccf 100644 --- a/lib/liquid/tags/case.rb +++ b/lib/liquid/tags/case.rb @@ -104,7 +104,7 @@ def parse_when(markup, body) block.attach(body) @blocks << block - break unless parser.id?('or') || parser.consume?(:comma) + break unless parser.consume?(:logical) == 'or' || parser.consume?(:comma) end parser.consume(:end_of_string) From 3895bcae0ccb63df450be86e0f5d8a3dc303bb5d Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Fri, 5 Dec 2025 09:35:08 -0500 Subject: [PATCH 30/32] Add support for parenthesized expressions --- lib/liquid/parser.rb | 15 ++++++++++- test/integration/assign_test.rb | 15 ++++++++++- test/integration/parsing_quirks_test.rb | 9 +++---- test/unit/parser_unit_test.rb | 34 ++++++++++++++++++++----- 4 files changed, 60 insertions(+), 13 deletions(-) diff --git a/lib/liquid/parser.rb b/lib/liquid/parser.rb index 15a11bc30..e01117a29 100644 --- a/lib/liquid/parser.rb +++ b/lib/liquid/parser.rb @@ -100,7 +100,7 @@ def primary when :number number when :open_round - range_lookup + grouping_or_range_lookup else raise SyntaxError, "#{token} is not a valid expression" end @@ -131,6 +131,19 @@ def unnamed_variable_lookup VariableLookup.new(name, lookups, command_flags) end + # Parenthesized expressions are recursive + def grouping_or_range_lookup + consume(:open_round) + expr = expression + if consume?(:dotdot) + RangeLookup.create(expr, expression) + else + expr + end + ensure + consume(:close_round) + end + def range_lookup consume(:open_round) first = expression diff --git a/test/integration/assign_test.rb b/test/integration/assign_test.rb index 69163ab96..b3a04252e 100644 --- a/test/integration/assign_test.rb +++ b/test/integration/assign_test.rb @@ -35,13 +35,26 @@ def test_assign_with_filter ) end + def test_assign_boolean_expression_assignment + assert_template_result( + 'it rendered', + <<~LIQUID, + {%- assign should_render = a == 0 or (b == 1 and c == 2) -%} + {%- if should_render -%} + it rendered + {%- endif -%} + LIQUID + { 'b' => 1, 'c' => 2 }, + ) + end + def test_assign_syntax_error assert_match_syntax_error(/assign/, '{% assign foo not values %}.') end def test_assign_throws_on_unsupported_syntax assert_match_syntax_error( - "Expected dotdot but found pipe", + "Expected close_round but found pipe", "{% assign foo = ('X' | downcase) %}", ) end diff --git a/test/integration/parsing_quirks_test.rb b/test/integration/parsing_quirks_test.rb index 75954f93f..7927444ef 100644 --- a/test/integration/parsing_quirks_test.rb +++ b/test/integration/parsing_quirks_test.rb @@ -35,11 +35,10 @@ def test_error_on_empty_filter assert_raises(Liquid::SyntaxError) { Template.parse("{{test |a|b|}}") } end - def test_meaningless_parens_error - assert_raises(SyntaxError) do - markup = "a == 'foo' or (b == 'bar' and c == 'baz') or false" - Template.parse("{% if #{markup} %} YES {% endif %}") - end + def test_supported_parens + markup = "a == 'foo' or (b == 'bar' and c == 'baz') or false" + out = Template.parse("{% if #{markup} %} YES {% endif %}").render({ 'b' => 'bar', 'c' => 'baz' }) + assert_equal(' YES ', out) end def test_unexpected_characters_syntax_error diff --git a/test/unit/parser_unit_test.rb b/test/unit/parser_unit_test.rb index 20e8692cf..bc2b5ea55 100644 --- a/test/unit/parser_unit_test.rb +++ b/test/unit/parser_unit_test.rb @@ -98,12 +98,12 @@ def test_logical expr = p.expression assert(expr.is_a?(BinaryExpression)) assert_equal('and', expr.operator) - assert_equal('==', expr.left_node.operator) - assert_equal('a', expr.left_node.left_node.name) - assert_equal('b', expr.left_node.right_node.name) - assert_equal('or', expr.right_node.operator) - assert_equal('c', expr.right_node.left_node.name) - assert_equal('d', expr.right_node.right_node.name) + assert_equal('==', expr.left_node.operator) + assert_equal('a', expr.left_node.left_node.name) + assert_equal('b', expr.left_node.right_node.name) + assert_equal('or', expr.right_node.operator) + assert_equal('c', expr.right_node.left_node.name) + assert_equal('d', expr.right_node.right_node.name) end def test_equality @@ -197,6 +197,28 @@ def test_ranges assert_equal('(hi[5].wat..old)', p.expression_string) end + def test_groupings_aka_parenthesized_expressions + # without the parens, this would be evaled as a and (b or c) + p = new_parser("(a and b) or c") + expr = p.expression + assert_equal('or', expr.operator) + assert_equal('and', expr.left_node.operator) + assert_equal('a', expr.left_node.left_node.name) + assert_equal('b', expr.left_node.right_node.name) + assert_equal('c', expr.right_node.name) + end + + def test_groupings_can_be_used_to_hijack_operation_priority + # without parens would be parsed as `a and (b == c)` + p = new_parser("(a and b) == c") + expr = p.expression + assert_equal('==', expr.operator) + assert_equal('and', expr.left_node.operator) + assert_equal('a', expr.left_node.left_node.name) + assert_equal('b', expr.left_node.right_node.name) + assert_equal('c', expr.right_node.name) + end + def test_argument_string p = new_parser("filter: hi.there[5], keyarg: 7") assert_equal('filter', p.consume(:id)) From fd3bd8176bd79dba744867d38518556edf695cc6 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Thu, 4 Dec 2025 15:15:50 -0500 Subject: [PATCH 31/32] Update History.md --- History.md | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/History.md b/History.md index 4337aeb98..8ebe43962 100644 --- a/History.md +++ b/History.md @@ -3,21 +3,22 @@ ## 6.0.0 ### Features -* (TODO) Add support for boolean expressions everywhere +* Add support for boolean expressions everywhere * As variable output `{{ a or b }}` * As filter argument `{{ collection | where: 'prop', a or b }}` * As tag argument `{% render 'snip', enabled: a or b %}` * As conditional tag argument `{% if cond %}` (extending previous behaviour) -* (TODO) Add support for subexpression prioritization and associativity +* Add support for subexpression prioritization and associativity * In ascending order of priority: * Logical: `and`, `or` (right to left) * Equality: `==`, `!=`, `<>` (left to right) * Comparison: `>`, `>=`, `<`, `<=`, `contains` (left to right) + * Groupings: `( expr )` - For example, this is now supported * `{{ a > b == c < d or e == f }}` which is equivalent to * `{{ ((a > b) == (c < d)) or (e == f) }}` -- (TODO) Add support for parenthesized expressions - * e.g. `(a or b) and c` +- Add support for parenthesized expressions + * e.g. `(a or b) == c` ### Architectural changes * `parse_expression` and `safe_parse_expression` have been removed from `Tag` and `ParseContext` @@ -32,10 +33,17 @@ * `:lax` and `lax_parse` is no longer supported * `:strict` and `strict_parse` is no longer supported * `strict2_parse` is renamed to `parse_markup` -* The `warnings` system has been removed. -* `Parser#expression` is renamed to `Parser#expression_string` -* `safe_parse_expression` methods are replaced by `Parser#expression` -* `parse_expression` methods are replaced by `Parser#unsafe_parse_expression` +* Expressions + * The `warnings` system has been removed. + * `Parser#expression` is renamed to `Parser#expression_string` + * `safe_parse_expression` methods are replaced by `Parser#expression` + * `parse_expression` methods are replaced by `Parser#unsafe_parse_expression` +* `Condition` + * `new(expr)` no longer accepts an `op` or `right`. Logic moved to BinaryExpression. + * `Condition#or` and `Condition#and` were replaced by `BinaryExpression`. + * `Condition#child_relation` replaced by `BinaryExpression`. + * `Condition.operations` was removed. + * `Condtion::MethodLiteral` was moved to the `Liquid` namespace ### Migrating from `^5.11.0` - In custom tags that include `ParserSwitching`, rename `strict2_parse` to `parse_markup` From a33443caaf6b864baa546392b0e49649a224e6fc Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Fri, 5 Dec 2025 09:52:47 -0500 Subject: [PATCH 32/32] Annotate Parser with grammar rules --- lib/liquid/parser.rb | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/lib/liquid/parser.rb b/lib/liquid/parser.rb index e01117a29..df4d6a277 100644 --- a/lib/liquid/parser.rb +++ b/lib/liquid/parser.rb @@ -13,6 +13,8 @@ def jump(point) @p = point end + # Consumes a token of specific type. + # Throws SyntaxError if token doesn't match type expectation. def consume(type = nil) token = @tokens[@p] if type && token[0] != type @@ -41,6 +43,7 @@ def id?(str) token[1] end + # Peeks the ahead token, returning true if matching expectation def look(type, ahead = 0) tok = @tokens[@p + ahead] return false unless tok @@ -51,7 +54,7 @@ def look(type, ahead = 0) # logical := equality (("and" | "or") equality)* # equality := comparison (("==" | "!=" | "<>") comparison)* # comparison := primary ((">=" | ">" | "<" | "<=" | ... ) primary)* - # primary := string | number | variable_lookup | range | boolean + # primary := string | number | variable_lookup | range | boolean | grouping def expression logical end @@ -60,7 +63,8 @@ def expression # associative. This creates a right-leaning tree and is why the method # looks a bit more complicated # - # `a == b and b or c` is evaluated like (a and (b or c)) + # `a and b or c` is evaluated like (a and (b or c)) + # logical := equality (("and" | "or") equality)* def logical operator = nil expr = equality @@ -69,6 +73,7 @@ def logical expr end + # equality := comparison (("==" | "!=" | "<>") comparison)* def equality expr = comparison while look(:equality) @@ -88,11 +93,12 @@ def comparison expr end + # primary := string | number | variable_lookup | range | boolean | grouping def primary token = @tokens[@p] case token[0] when :id - variable_lookup + variable_lookup_or_literal when :open_square unnamed_variable_lookup when :string @@ -115,7 +121,18 @@ def string consume(:string)[1..-2] end + # variable_lookup := id (lookup)* + # lookup := indexed_lookup | dot_lookup + # indexed_lookup := "[" expression "]" + # dot_lookup := "." id def variable_lookup + name = consume(:id) + lookups, command_flags = variable_lookups + VariableLookup.new(name, lookups, command_flags) + end + + # a variable_lookup without lookups could be a literal + def variable_lookup_or_literal name = consume(:id) lookups, command_flags = variable_lookups if Expression::LITERALS.key?(name) && lookups.empty? @@ -125,6 +142,7 @@ def variable_lookup end end + # unnamed_variable_lookup := indexed_lookup (lookup)* def unnamed_variable_lookup name = indexed_lookup lookups, command_flags = variable_lookups @@ -132,6 +150,7 @@ def unnamed_variable_lookup end # Parenthesized expressions are recursive + # grouping := "(" expression ")" def grouping_or_range_lookup consume(:open_round) expr = expression @@ -144,6 +163,7 @@ def grouping_or_range_lookup consume(:close_round) end + # range_lookup := "(" expression ".." expression ")" def range_lookup consume(:open_round) first = expression