From 372f77cbebd9c9c5319d967a1a988d7c5878b225 Mon Sep 17 00:00:00 2001 From: "Charles-P. Clermont" Date: Tue, 27 Oct 2026 09:29:25 -0400 Subject: [PATCH] Support shared expressions in for loops --- History.md | 1 + lib/liquid/parser.rb | 21 +++- lib/liquid/tags/for.rb | 36 ++++-- test/integration/tags/for_tag_test.rb | 156 ++++++++++++++++++++++++++ test/unit/parse_tree_visitor_test.rb | 7 ++ test/unit/parser_unit_test.rb | 24 ++++ 6 files changed, 235 insertions(+), 10 deletions(-) diff --git a/History.md b/History.md index addb4b100..fab3b49c5 100644 --- a/History.md +++ b/History.md @@ -8,6 +8,7 @@ * 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) + * As `for` collection, `limit` and `offset` `{% for item in fallback or items limit: (count or 2) %}` * Add support for subexpression prioritization and associativity * In ascending order of priority: * Logical: `and`, `or` (right to left) diff --git a/lib/liquid/parser.rb b/lib/liquid/parser.rb index 543903e74..f1822fdfa 100644 --- a/lib/liquid/parser.rb +++ b/lib/liquid/parser.rb @@ -2,6 +2,8 @@ module Liquid class Parser + OPERATOR_TOKEN_TYPES = [:logical, :equality, :comparison].freeze + def initialize(input, expression_cache = nil, reject_bare_brackets: false) @ss = input.is_a?(StringScanner) ? input : StringScanner.new(input) @cache = expression_cache @@ -10,6 +12,11 @@ def initialize(input, expression_cache = nil, reject_bare_brackets: false) @reject_bare_brackets = reject_bare_brackets end + # The current token position, which jump can return to. + def position + @p + end + def jump(point) @p = point end @@ -179,7 +186,7 @@ def expression_string token = @tokens[@p] case token[0] when :id - str = consume + str = consume.dup str << variable_lookups_string when :open_square raise_bare_bracket_error if @reject_bare_brackets @@ -229,6 +236,18 @@ def variable_lookups_string str end + # Markup for the tokens consumed since the start position. Tokens are + # joined like expression_string, with spaces around operators. + def markup_since(start) + @tokens[start...@p].each_with_object(+"") do |(type, text), markup| + if OPERATOR_TOKEN_TYPES.include?(type) + markup << " " << text << " " + else + markup << text + end + 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 92c4291c1..2a6f487aa 100644 --- a/lib/liquid/tags/for.rb +++ b/lib/liquid/tags/for.rb @@ -75,8 +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_string - @collection_name = p.unsafe_parse_expression(collection_name) + collection_name, @collection_name = parse_loop_expression(p) @name = "#{@variable_name}-#{collection_name}" @reversed = p.id?('reversed') @@ -159,19 +158,38 @@ def render_segment(context, output, segment) end def set_attribute(key, p) - expr = p.expression_string + markup, expression = parse_loop_expression(p) case key when 'offset' - @from = if expr == 'continue' - :continue - else - p.unsafe_parse_expression(expr) - end + @from = markup == 'continue' ? :continue : expression when 'limit' - @limit = p.unsafe_parse_expression(expr) + @limit = expression + end + end + + # Parses a collection, limit or offset and returns its markup with the + # parsed expression. Markup from the simple value and range grammar keeps + # its previous parse and loop name. Other markup uses the shared + # expression grammar. + def parse_loop_expression(p) + start = p.position + if (markup = simple_expression_string(p)) + [markup, p.unsafe_parse_expression(markup)] + else + p.jump(start) + expression = p.expression + [p.markup_since(start), expression] end end + # Returns nil when the markup needs the shared expression grammar. + def simple_expression_string(p) + markup = p.expression_string + markup unless Parser::OPERATOR_TOKEN_TYPES.any? { |type| p.look(type) } + rescue SyntaxError + nil + end + def render_else(context, output) if @else_block @else_block.render_to_output_buffer(context, output) diff --git a/test/integration/tags/for_tag_test.rb b/test/integration/tags/for_tag_test.rb index 36510eeab..4403045c9 100644 --- a/test/integration/tags/for_tag_test.rb +++ b/test/integration/tags/for_tag_test.rb @@ -546,4 +546,160 @@ def each Template.parse('{% for i in numbers %}{{ i }}{% endfor %}').render!('numbers' => range) end end + + def test_collection_accepts_grouped_and_ungrouped_logical_expressions + assigns = { 'fallback' => nil, 'items' => ['a', 'b'] } + + assert_template_result('0=a;1=b;', '{% for item in fallback or items %}{{ forloop.index0 }}={{ item }};{% endfor %}', assigns) + assert_template_result('0=a;1=b;', '{% for item in (fallback or items) %}{{ forloop.index0 }}={{ item }};{% endfor %}', assigns) + assert_template_result('ab', '{% for item in items and items %}{{ item }}{% endfor %}', assigns) + assert_template_result('empty', '{% for item in fallback and items %}{{ item }}{% else %}empty{% endfor %}', assigns) + end + + def test_collection_expression_returns_the_selected_operand + assigns = { 'first' => [1, 2], 'second' => [3, 4] } + + assert_template_result('12', '{% for item in first or second %}{{ item }}{% endfor %}', assigns) + assert_template_result('34', '{% for item in first and second %}{{ item }}{% endfor %}', assigns) + assert_template_result('12', '{% for item in first or (second and nothing) %}{{ item }}{% endfor %}', assigns) + end + + def test_collection_expression_with_ranges_and_lookups + assigns = { 'fallback' => nil, 'collections' => { 'featured' => [7, 8] }, 'key' => 'featured' } + + assert_template_result('123', '{% for item in fallback or (1..3) %}{{ item }}{% endfor %}', assigns) + assert_template_result('78', '{% for item in fallback or collections[key] %}{{ item }}{% endfor %}', assigns) + assert_template_result('78', '{% for item in collections[fallback or key] %}{{ item }}{% endfor %}', assigns) + end + + def test_collection_expression_starting_with_a_lookup + assigns = { + 'values' => ['a', 'b'], + 'key' => 0, + 'data' => { 'list' => [1, 2], 'missing' => nil, 'keys' => { 'name' => 'list' } }, + } + + assert_template_result('a', '{% for item in values[key] or values %}{{ item }}{% endfor %}', assigns) + assert_template_result('a', '{% for item in (values[key] or values) %}{{ item }}{% endfor %}', assigns) + assert_template_result('a', '{% for item in values.first or values %}{{ item }}{% endfor %}', assigns) + assert_template_result('12', '{% for item in data.list or values %}{{ item }}{% endfor %}', assigns) + assert_template_result('12', '{% for item in data.missing or data.list %}{{ item }}{% endfor %}', assigns) + assert_template_result('12', '{% for item in data[data.keys.name] or values %}{{ item }}{% endfor %}', assigns) + assert_template_result('ab', "{% for item in data[data.keys['name']] and values %}{{ item }}{% endfor %}", assigns) + assert_template_result('a', '{% for item in data.list and values[key] %}{{ item }}{% endfor %}', assigns) + end + + def test_collection_expression_starting_with_a_lookup_matches_assign + assigns = { 'values' => ['a', 'b'], 'key' => 0, 'data' => { 'missing' => nil, 'list' => [1, 2] } } + + [ + 'values[key] or values', + 'values.first or values', + '(data.missing or data.list)', + "data['missing'] or data.list", + ].each do |expression| + workaround = "{% assign selected = #{expression} %}{% for item in selected %}{{ item }},{% endfor %}" + template = "{% for item in #{expression} %}{{ item }},{% endfor %}" + + assert_equal(Template.parse(workaround).render!(assigns), Template.parse(template).render!(assigns), expression) + end + end + + def test_expression_collection_starting_with_a_lookup_keeps_its_loop_name + assigns = { 'values' => ['a', 'b'], 'key' => 0, 'data' => { 'list' => [1], 'keys' => { 'name' => 'list' } } } + + assert_template_result('item-values[key] or values', '{% for item in values[key] or values %}{{ forloop.name }}{% endfor %}', assigns) + assert_template_result( + 'item-data[data.keys.name] or values', + '{% for item in data[data.keys.name] or values %}{{ forloop.name }}{% endfor %}', + assigns, + ) + end + + def test_limit_and_offset_expressions_starting_with_a_lookup + assigns = { 'items' => [1, 2, 3, 4], 'i' => 0, 'counts' => [2, 1], 'settings' => { 'two' => 2, 'one' => 1, 'missing' => nil } } + + assert_template_result('12', '{% for item in items limit: settings.two or 5 %}{{ item }}{% endfor %}', assigns) + assert_template_result('12', '{% for item in items limit: counts[i] or 5 %}{{ item }}{% endfor %}', assigns) + assert_template_result('1', '{% for item in items limit: (counts[1] or 3) %}{{ item }}{% endfor %}', assigns) + assert_template_result('34', '{% for item in items offset: counts[i] or 0 %}{{ item }}{% endfor %}', assigns) + assert_template_result('234', '{% for item in items offset: settings.missing or counts[1] %}{{ item }}{% endfor %}', assigns) + assert_template_result( + '32', + "{% for item in items reversed limit: counts[i] or 9 offset: settings['missing'] or settings.one %}{{ item }}{% endfor %}", + assigns, + ) + end + + def test_limit_and_offset_accept_logical_expressions + assigns = { 'items' => [1, 2, 3, 4], 'candidate' => nil, 'cap' => 2, 'start' => 1 } + + assert_template_result('12', '{% for item in items limit: candidate or cap %}{{ item }}{% endfor %}', assigns) + assert_template_result('12', '{% for item in items limit: (candidate or cap) %}{{ item }}{% endfor %}', assigns) + assert_template_result('234', '{% for item in items offset: candidate or start %}{{ item }}{% endfor %}', assigns) + assert_template_result('234', '{% for item in items offset: (candidate or start) %}{{ item }}{% endfor %}', assigns) + assert_template_result('23', '{% for item in items limit: candidate or cap, offset: candidate or start %}{{ item }}{% endfor %}', assigns) + end + + def test_modifiers_delimit_expressions + assigns = { 'fallback' => nil, 'items' => [1, 2, 3, 4, 5], 'candidate' => nil, 'cap' => 2, 'skipped' => nil, 'start' => 1 } + template = '{% for item in fallback or items reversed limit: candidate or cap offset: skipped or start %}{{ item }},{% endfor %}' + workaround = '{% assign selected = fallback or items %}{% assign count = candidate or cap %}' \ + '{% assign begin = skipped or start %}' \ + '{% for item in selected reversed limit: count offset: begin %}{{ item }},{% endfor %}' + + assert_template_result('3,2,', template, assigns) + assert_template_result('3,2,', workaround, assigns) + end + + def test_expression_collection_loop_name_and_continuation + assigns = { 'fallback' => nil, 'items' => [1, 2, 3, 4] } + + assert_template_result( + 'item-fallback or items|item-(fallback or items)', + '{% for item in fallback or items limit: 1 %}{{ forloop.name }}{% endfor %}|' \ + '{% for item in (fallback or items) limit: 1 %}{{ forloop.name }}{% endfor %}', + assigns, + ) + assert_template_result( + '12|34', + '{% for item in fallback or items limit: 2 %}{{ item }}{% endfor %}|' \ + '{% for item in fallback or items offset: continue %}{{ item }}{% endfor %}', + assigns, + ) + end + + def test_simple_collections_keep_their_loop_names + assigns = { 'collections' => { 'featured' => ['a'] }, 'key' => 'featured', 'items' => ['a'] } + + assert_template_result('item-items', '{% for item in items %}{{ forloop.name }}{% endfor %}', assigns) + assert_template_result('item-(1..1)', '{% for item in ( 1 .. 1 ) %}{{ forloop.name }}{% endfor %}', assigns) + assert_template_result('item-collections[key]', '{% for item in collections[ key ] %}{{ forloop.name }}{% endfor %}', assigns) + assert_template_result('item-collections["featured"]', '{% for item in collections["featured"] %}{{ forloop.name }}{% endfor %}', assigns) + end + + def test_simple_collections_keep_their_previous_lookup_results + # These lookups render differently through the shared expression grammar. + # Templates the simple grammar accepted keep their previous output. + assigns = { 'a' => { ']' => [7, 8], 'x y' => 2 } } + + assert_template_result('empty', '{% for i in a["]"] %}{{ i }}{% else %}empty{% endfor %}', assigns) + assert_template_result('empty', '{% for i in (a["x y"]..3) %}{{ i }}{% else %}empty{% endfor %}', assigns) + end + + def test_expression_collection_keeps_resource_limits + template = Template.parse('{% for i in fallback or numbers %}{% endfor %}') + template.resource_limits.render_score_limit = 3 + + assert_raises(Liquid::MemoryError) { template.render!('numbers' => bounded_integer_range_with_tripwires) } + end + + def test_invalid_loop_expressions_remain_syntax_errors + assert_syntax_error('{% for item in fallback or %}{% endfor %}') + assert_syntax_error('{% for item in (fallback or items %}{% endfor %}') + assert_syntax_error('{% for item in items limit: candidate or %}{% endfor %}') + assert_syntax_error('{% for item in items offset: (candidate or) %}{% endfor %}') + assert_syntax_error('{% for item in fallback or items limit: 1 reversed %}{% endfor %}') + assert_match_syntax_error(/Invalid attribute in for loop/, '{% for item in fallback or items size: 1 %}{% endfor %}') + end end diff --git a/test/unit/parse_tree_visitor_test.rb b/test/unit/parse_tree_visitor_test.rb index 6d946f225..0ea16d0ce 100644 --- a/test/unit/parse_tree_visitor_test.rb +++ b/test/unit/parse_tree_visitor_test.rb @@ -166,6 +166,13 @@ def test_for_range ) end + def test_for_expressions + assert_equal( + ["candidate", "cap", "skipped", "start", "fallback", "test"], + visit(%({% for x in fallback or test limit: (candidate or cap) offset: skipped or start %}{% endfor %})), + ) + end + def test_tablerow_in assert_equal( ["test"], diff --git a/test/unit/parser_unit_test.rb b/test/unit/parser_unit_test.rb index 5c0568da8..caed3062c 100644 --- a/test/unit/parser_unit_test.rb +++ b/test/unit/parser_unit_test.rb @@ -18,6 +18,30 @@ def test_jump assert_equal('7', p.consume(:number)) end + def test_position_returns_to_a_jump_point + p = new_parser("wat: 7") + start = p.position + p.consume(:id) + p.jump(start) + assert_equal('wat', p.consume(:id)) + end + + def test_expression_string_leaves_tokens_for_a_jump_back + p = new_parser("a.b[c.d] or e") + start = p.position + assert_equal('a.b[c.d]', p.expression_string) + p.jump(start) + assert_equal(new_parser('a.b[c.d]').expression, p.expression.left_node) + assert_equal('a.b[c.d] or e', p.markup_since(start)) + end + + def test_markup_since_spaces_operators + p = new_parser("(a.b[0] or c) == 'x y' limit") + start = p.position + p.expression + assert_equal("(a.b[0] or c) == 'x y'", p.markup_since(start)) + end + def test_consume? p = new_parser("wat: 7") assert_equal('wat', p.consume?(:id))