WIP4

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)

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)

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)

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

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"],

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))