Port liquid-c bug compatible whitespace trimming

The parse_context no longer needs to be passed in because it is available
through through an attr_reader on the instance. However, the markup and
tokenizer weren't made available.  This refactor also makes the parameters
given to Document#unknown_tag consistent with Block#unknown_tag.

lib/liquid/block.rb

@@ -10,7 +10,7 @@ module Liquid
     end
 
     def parse(tokens)
-      @body = BlockBody.new
+      @body = new_body
       while parse_body(@body, tokens)
       end
     end
@@ -28,7 +28,12 @@ module Liquid
       @body.nodelist
     end
 
-    def unknown_tag(tag, _params, _tokens)
+    def unknown_tag(tag_name, _markup, _tokenizer)
+      Block.raise_unknown_tag(tag_name, block_name, block_delimiter, parse_context)
+    end
+
+    # @api private
+    def self.raise_unknown_tag(tag, block_name, block_delimiter, parse_context)
       if tag == 'else'
         raise SyntaxError, parse_context.locale.t("errors.syntax.unexpected_else",
           block_name: block_name)
@@ -50,8 +55,14 @@ module Liquid
       @block_delimiter ||= "end#{block_name}"
     end
 
-    protected
+    private
 
+    # @api public
+    def new_body
+      BlockBody.new
+    end
+
+    # @api public
     def parse_body(body, tokens)
       if parse_context.depth >= MAX_DEPTH
         raise StackLevelError, "Nesting too deep"

lib/liquid/block_body.rb

@@ -54,28 +54,49 @@ module Liquid
     end
 
     # @api private
-    def self.unknown_tag_in_liquid_tag(end_tag_name, end_tag_markup)
-      yield end_tag_name, end_tag_markup
-    ensure
-      Usage.increment("liquid_tag_contains_outer_tag") unless $ERROR_INFO.is_a?(SyntaxError)
+    def self.unknown_tag_in_liquid_tag(tag, parse_context)
+      Block.raise_unknown_tag(tag, 'liquid', '%}', parse_context)
     end
 
-    private def parse_liquid_tag(markup, parse_context, &block)
+    # @api private
+    def self.raise_missing_tag_terminator(token, parse_context)
+      raise SyntaxError, parse_context.locale.t("errors.syntax.tag_termination", token: token, tag_end: TagEnd.inspect)
+    end
+
+    # @api private
+    def self.raise_missing_variable_terminator(token, parse_context)
+      raise SyntaxError, parse_context.locale.t("errors.syntax.variable_termination", token: token, tag_end: VariableEnd.inspect)
+    end
+
+    # @api private
+    def self.render_node(context, output, node)
+      node.render_to_output_buffer(context, output)
+    rescue UndefinedVariable, UndefinedDropMethod, UndefinedFilter => e
+      context.handle_error(e, node.line_number)
+    rescue MemoryError
+      raise
+    rescue ::StandardError => e
+      line_number = node.is_a?(String) ? nil : node.line_number
+      output << context.handle_error(e, line_number)
+    end
+
+    private def parse_liquid_tag(markup, parse_context)
       liquid_tag_tokenizer = Tokenizer.new(markup, line_number: parse_context.line_number, for_liquid_tag: true)
-      parse_for_liquid_tag(liquid_tag_tokenizer, parse_context) do |end_tag_name, end_tag_markup|
-        next unless end_tag_name
-        self.class.unknown_tag_in_liquid_tag(end_tag_name, end_tag_markup, &block)
+      parse_for_liquid_tag(liquid_tag_tokenizer, parse_context) do |end_tag_name, _end_tag_markup|
+        if end_tag_name
+          BlockBody.unknown_tag_in_liquid_tag(end_tag_name, parse_context)
+        end
       end
     end
 
-    private def parse_for_document(tokenizer, parse_context, &block)
+    private def parse_for_document(tokenizer, parse_context)
       while (token = tokenizer.shift)
         next if token.empty?
         case
         when token.start_with?(TAGSTART)
           whitespace_handler(token, parse_context)
           unless token =~ FullToken
-            raise_missing_tag_terminator(token, parse_context)
+            BlockBody.raise_missing_tag_terminator(token, parse_context)
           end
           tag_name = Regexp.last_match(2)
           markup   = Regexp.last_match(4)
@@ -87,7 +108,7 @@ module Liquid
           end
 
           if tag_name == 'liquid'
-            parse_liquid_tag(markup, parse_context, &block)
+            parse_liquid_tag(markup, parse_context)
             next
           end
 
@@ -121,7 +142,11 @@ module Liquid
       if token[2] == WhitespaceControl
         previous_token = @nodelist.last
         if previous_token.is_a?(String)
+          first_byte = previous_token.getbyte(0)
           previous_token.rstrip!
+          if previous_token.empty? && parse_context[:bug_compatible_whitespace_trimming] && first_byte
+            previous_token << first_byte
+          end
         end
       end
       parse_context.trim_whitespace = (token[-3] == WhitespaceControl)
@@ -131,38 +156,47 @@ module Liquid
       @blank
     end
 
+    # Remove blank strings in the block body for a control flow tag (e.g. `if`, `for`, `case`, `unless`)
+    # with a blank body.
+    #
+    # For example, in a conditional assignment like the following
+    #
+    # ```
+    # {% if size > max_size %}
+    #   {% assign size = max_size %}
+    # {% endif %}
+    # ```
+    #
+    # we assume the intention wasn't to output the blank spaces in the `if` tag's block body, so this method
+    # will remove them to reduce the render output size.
+    #
+    # Note that it is now preferred to use the `liquid` tag for this use case.
+    def remove_blank_strings
+      raise "remove_blank_strings only support being called on a blank block body" unless @blank
+      @nodelist.reject! { |node| node.instance_of?(String) }
+    end
+
     def render(context)
       render_to_output_buffer(context, +'')
     end
 
     def render_to_output_buffer(context, output)
-      context.resource_limits.render_score += @nodelist.length
+      context.resource_limits.increment_render_score(@nodelist.length)
 
       idx = 0
       while (node = @nodelist[idx])
-        previous_output_size = output.bytesize
-
-        case node
-        when String
+        if node.instance_of?(String)
           output << node
-        when Variable
+        else
           render_node(context, output, node)
-        when Block
-          render_node(context, node.blank? ? +'' : output, node)
-          break if context.interrupt? # might have happened in a for-block
-        when Continue, Break
           # If we get an Interrupt that means the block must stop processing. An
           # Interrupt is any command that stops block execution such as {% break %}
-          # or {% continue %}
-          context.push_interrupt(node.interrupt)
-          break
-        else # Other non-Block tags
-          render_node(context, output, node)
-          break if context.interrupt? # might have happened through an include
+          # or {% continue %}. These tags may also occur through Block or Include tags.
+          break if context.interrupt? # might have happened in a for-block
         end
         idx += 1
 
-        raise_if_resource_limits_reached(context, output.bytesize - previous_output_size)
+        context.resource_limits.increment_write_score(output)
       end
 
       output
@@ -171,18 +205,7 @@ module Liquid
     private
 
     def render_node(context, output, node)
-      node.render_to_output_buffer(context, output)
-    rescue UndefinedVariable, UndefinedDropMethod, UndefinedFilter => e
-      context.handle_error(e, node.line_number)
-    rescue ::StandardError => e
-      line_number = node.is_a?(String) ? nil : node.line_number
-      output << context.handle_error(e, line_number)
-    end
-
-    def raise_if_resource_limits_reached(context, length)
-      context.resource_limits.render_length += length
-      return unless context.resource_limits.reached?
-      raise MemoryError, "Memory limits exceeded"
+      BlockBody.render_node(context, output, node)
     end
 
     def create_variable(token, parse_context)
@@ -190,15 +213,17 @@ module Liquid
         markup = content.first
         return Variable.new(markup, parse_context)
       end
-      raise_missing_variable_terminator(token, parse_context)
+      BlockBody.raise_missing_variable_terminator(token, parse_context)
     end
 
+    # @deprecated Use {.raise_missing_tag_terminator} instead
     def raise_missing_tag_terminator(token, parse_context)
-      raise SyntaxError, parse_context.locale.t("errors.syntax.tag_termination", token: token, tag_end: TagEnd.inspect)
+      BlockBody.raise_missing_tag_terminator(token, parse_context)
     end
 
+    # @deprecated Use {.raise_missing_variable_terminator} instead
     def raise_missing_variable_terminator(token, parse_context)
-      raise SyntaxError, parse_context.locale.t("errors.syntax.variable_termination", token: token, tag_end: VariableEnd.inspect)
+      BlockBody.raise_missing_variable_terminator(token, parse_context)
     end
 
     def registered_tags

lib/liquid/document.rb

@@ -1,23 +1,33 @@
 # frozen_string_literal: true
 
 module Liquid
-  class Document < BlockBody
+  class Document
     def self.parse(tokens, parse_context)
-      doc = new
+      doc = new(parse_context)
       doc.parse(tokens, parse_context)
       doc
     end
 
-    def parse(tokens, parse_context)
-      super do |end_tag_name, _end_tag_params|
-        unknown_tag(end_tag_name, parse_context) if end_tag_name
+    attr_reader :parse_context, :body
+
+    def initialize(parse_context)
+      @parse_context = parse_context
+      @body = new_body
+    end
+
+    def nodelist
+      @body.nodelist
+    end
+
+    def parse(tokenizer, parse_context)
+      while parse_body(tokenizer)
       end
     rescue SyntaxError => e
       e.line_number ||= parse_context.line_number
       raise
     end
 
-    def unknown_tag(tag, parse_context)
+    def unknown_tag(tag, _markup, _tokenizer)
       case tag
       when 'else', 'end'
         raise SyntaxError, parse_context.locale.t("errors.syntax.unexpected_outer_tag", tag: tag)
@@ -25,5 +35,30 @@ module Liquid
         raise SyntaxError, parse_context.locale.t("errors.syntax.unknown_tag", tag: tag)
       end
     end
+
+    def render_to_output_buffer(context, output)
+      @body.render_to_output_buffer(context, output)
+    end
+
+    def render(context)
+      @body.render(context)
+    end
+
+    private
+
+    def new_body
+      Liquid::BlockBody.new
+    end
+
+    def parse_body(tokenizer)
+      @body.parse(tokenizer, parse_context) do |unknown_tag_name, unknown_tag_markup|
+        if unknown_tag_name
+          unknown_tag(unknown_tag_name, unknown_tag_markup, tokenizer)
+          true
+        else
+          false
+        end
+      end
+    end
   end
 end

lib/liquid/resource_limits.rb

@@ -2,8 +2,8 @@
 
 module Liquid
   class ResourceLimits
-    attr_accessor :render_length, :render_score, :assign_score,
-      :render_length_limit, :render_score_limit, :assign_score_limit
+    attr_accessor :render_length_limit, :render_score_limit, :assign_score_limit
+    attr_reader :render_score, :assign_score
 
     def initialize(limits)
       @render_length_limit = limits[:render_length_limit]
@@ -12,14 +12,51 @@ module Liquid
       reset
     end
 
+    def increment_render_score(amount)
+      @render_score += amount
+      raise_limits_reached if @render_score_limit && @render_score > @render_score_limit
+    end
+
+    def increment_assign_score(amount)
+      @assign_score += amount
+      raise_limits_reached if @assign_score_limit && @assign_score > @assign_score_limit
+    end
+
+    # update either render_length or assign_score based on whether or not the writes are captured
+    def increment_write_score(output)
+      if (last_captured = @last_capture_length)
+        captured = output.bytesize
+        increment = captured - last_captured
+        @last_capture_length = captured
+        increment_assign_score(increment)
+      elsif @render_length_limit && output.bytesize > @render_length_limit
+        raise_limits_reached
+      end
+    end
+
+    def raise_limits_reached
+      @reached_limit = true
+      raise MemoryError, "Memory limits exceeded"
+    end
+
     def reached?
-      (@render_length_limit && @render_length > @render_length_limit) ||
-        (@render_score_limit && @render_score > @render_score_limit) ||
-        (@assign_score_limit && @assign_score > @assign_score_limit)
+      @reached_limit
     end
 
     def reset
-      @render_length = @render_score = @assign_score = 0
+      @reached_limit = false
+      @last_capture_length = nil
+      @render_score = @assign_score = 0
+    end
+
+    def with_capture
+      old_capture_length = @last_capture_length
+      begin
+        @last_capture_length = 0
+        yield
+      ensure
+        @last_capture_length = old_capture_length
+      end
     end
   end
 end

lib/liquid/tags/assign.rb

@@ -27,7 +27,7 @@ module Liquid
     def render_to_output_buffer(context, output)
       val = @from.render(context)
       context.scopes.last[@to] = val
-      context.resource_limits.assign_score += assign_score_of(val)
+      context.resource_limits.increment_assign_score(assign_score_of(val))
       output
     end
 

lib/liquid/tags/break.rb

@@ -11,8 +11,11 @@ module Liquid
   #    {% endfor %}
   #
   class Break < Tag
-    def interrupt
-      BreakInterrupt.new
+    INTERRUPT = BreakInterrupt.new.freeze
+
+    def render_to_output_buffer(context, output)
+      context.push_interrupt(INTERRUPT)
+      output
     end
   end
 

lib/liquid/tags/capture.rb

@@ -25,10 +25,10 @@ module Liquid
     end
 
     def render_to_output_buffer(context, output)
-      previous_output_size = output.bytesize
-      super
-      context.scopes.last[@to] = output
-      context.resource_limits.assign_score += (output.bytesize - previous_output_size)
+      context.resource_limits.with_capture do
+        capture_output = render(context)
+        context.scopes.last[@to] = capture_output
+      end
       output
     end
 

lib/liquid/tags/case.rb

@@ -19,8 +19,11 @@ module Liquid
     end
 
     def parse(tokens)
-      body = BlockBody.new
+      body = new_body
       body = @blocks.last.attachment while parse_body(body, tokens)
+      if blank?
+        @blocks.each { |condition| condition.attachment.remove_blank_strings }
+      end
     end
 
     def nodelist
@@ -56,7 +59,7 @@ module Liquid
     private
 
     def record_when_condition(markup)
-      body = BlockBody.new
+      body = new_body
 
       while markup
         unless markup =~ WhenSyntax
@@ -77,7 +80,7 @@ module Liquid
       end
 
       block = ElseCondition.new
-      block.attach(BlockBody.new)
+      block.attach(new_body)
       @blocks << block
     end
 

lib/liquid/tags/continue.rb

@@ -11,8 +11,11 @@ module Liquid
   #    {% endfor %}
   #
   class Continue < Tag
-    def interrupt
-      ContinueInterrupt.new
+    INTERRUPT = ContinueInterrupt.new.freeze
+
+    def render_to_output_buffer(context, output)
+      context.push_interrupt(INTERRUPT)
+      output
     end
   end
 

lib/liquid/tags/for.rb

@@ -54,13 +54,18 @@ module Liquid
       super
       @from = @limit = nil
       parse_with_selected_parser(markup)
-      @for_block = BlockBody.new
+      @for_block = new_body
       @else_block = nil
     end
 
     def parse(tokens)
-      return unless parse_body(@for_block, tokens)
-      parse_body(@else_block, tokens)
+      if parse_body(@for_block, tokens)
+        parse_body(@else_block, tokens)
+      end
+      if blank?
+        @for_block.remove_blank_strings
+        @else_block&.remove_blank_strings
+      end
     end
 
     def nodelist
@@ -69,7 +74,7 @@ module Liquid
 
     def unknown_tag(tag, markup, tokens)
       return super unless tag == 'else'
-      @else_block = BlockBody.new
+      @else_block = new_body
     end
 
     def render_to_output_buffer(context, output)

lib/liquid/tags/if.rb

@@ -31,6 +31,9 @@ module Liquid
     def parse(tokens)
       while parse_body(@blocks.last.attachment, tokens)
       end
+      if blank?
+        @blocks.each { |condition| condition.attachment.remove_blank_strings }
+      end
     end
 
     def unknown_tag(tag, markup, tokens)
@@ -61,7 +64,7 @@ module Liquid
       end
 
       @blocks.push(block)
-      block.attach(BlockBody.new)
+      block.attach(new_body)
     end
 
     def lax_parse(markup)

test/integration/tags/liquid_tag_test.rb

@@ -82,15 +82,13 @@ class LiquidTagTest < Minitest::Test
   end
 
   def test_nested_liquid_tag
-    assert_usage_increment("liquid_tag_contains_outer_tag", times: 0) do
-      assert_template_result('good', <<~LIQUID)
-        {%- if true %}
-          {%- liquid
-            echo "good"
-          %}
-        {%- endif -%}
-      LIQUID
-    end
+    assert_template_result('good', <<~LIQUID)
+      {%- if true %}
+        {%- liquid
+          echo "good"
+        %}
+      {%- endif -%}
+    LIQUID
   end
 
   def test_cannot_open_blocks_living_past_a_liquid_tag
@@ -102,14 +100,12 @@ class LiquidTagTest < Minitest::Test
     LIQUID
   end
 
-  def test_quirk_can_close_blocks_created_before_a_liquid_tag
-    assert_usage_increment("liquid_tag_contains_outer_tag") do
-      assert_template_result("42", <<~LIQUID)
-        {%- if true -%}
-        42
-        {%- liquid endif -%}
-      LIQUID
-    end
+  def test_cannot_close_blocks_created_before_a_liquid_tag
+    assert_match_syntax_error("syntax error (line 3): 'endif' is not a valid delimiter for liquid tags. use %}", <<~LIQUID)
+      {%- if true -%}
+      42
+      {%- liquid endif -%}
+    LIQUID
   end
 
   def test_liquid_tag_in_raw

test/integration/template_test.rb

@@ -111,13 +111,12 @@ class TemplateTest < Minitest::Test
 
   def test_resource_limits_render_length
     t = Template.parse("0123456789")
-    t.resource_limits.render_length_limit = 5
+    t.resource_limits.render_length_limit = 9
     assert_equal("Liquid error: Memory limits exceeded", t.render)
     assert(t.resource_limits.reached?)
 
     t.resource_limits.render_length_limit = 10
     assert_equal("0123456789", t.render!)
-    refute_nil(t.resource_limits.render_length)
   end
 
   def test_resource_limits_render_score
@@ -176,50 +175,46 @@ class TemplateTest < Minitest::Test
   end
 
   def test_resource_limits_hash_in_template_gets_updated_even_if_no_limits_are_set
-    t = Template.parse("{% for a in (1..100) %} {% assign foo = 1 %} {% endfor %}")
+    t = Template.parse("{% for a in (1..100) %}x{% assign foo = 1 %} {% endfor %}")
     t.render!
     assert(t.resource_limits.assign_score > 0)
     assert(t.resource_limits.render_score > 0)
-    assert(t.resource_limits.render_length > 0)
   end
 
   def test_render_length_persists_between_blocks
     t = Template.parse("{% if true %}aaaa{% endif %}")
-    t.resource_limits.render_length_limit = 7
+    t.resource_limits.render_length_limit = 3
     assert_equal("Liquid error: Memory limits exceeded", t.render)
-    t.resource_limits.render_length_limit = 8
+    t.resource_limits.render_length_limit = 4
     assert_equal("aaaa", t.render)
 
     t = Template.parse("{% if true %}aaaa{% endif %}{% if true %}bbb{% endif %}")
-    t.resource_limits.render_length_limit = 13
+    t.resource_limits.render_length_limit = 6
     assert_equal("Liquid error: Memory limits exceeded", t.render)
-    t.resource_limits.render_length_limit = 14
+    t.resource_limits.render_length_limit = 7
     assert_equal("aaaabbb", t.render)
 
     t = Template.parse("{% if true %}a{% endif %}{% if true %}b{% endif %}{% if true %}a{% endif %}{% if true %}b{% endif %}{% if true %}a{% endif %}{% if true %}b{% endif %}")
     t.resource_limits.render_length_limit = 5
     assert_equal("Liquid error: Memory limits exceeded", t.render)
-    t.resource_limits.render_length_limit = 11
-    assert_equal("Liquid error: Memory limits exceeded", t.render)
-    t.resource_limits.render_length_limit = 12
+    t.resource_limits.render_length_limit = 6
     assert_equal("ababab", t.render)
   end
 
   def test_render_length_uses_number_of_bytes_not_characters
     t = Template.parse("{% if true %}すごい{% endif %}")
-    t.resource_limits.render_length_limit = 10
+    t.resource_limits.render_length_limit = 8
     assert_equal("Liquid error: Memory limits exceeded", t.render)
-    t.resource_limits.render_length_limit = 18
+    t.resource_limits.render_length_limit = 9
     assert_equal("すごい", t.render)
   end
 
   def test_default_resource_limits_unaffected_by_render_with_context
     context = Context.new
-    t = Template.parse("{% for a in (1..100) %} {% assign foo = 1 %} {% endfor %}")
+    t = Template.parse("{% for a in (1..100) %}x{% assign foo = 1 %} {% endfor %}")
     t.render!(context)
     assert(context.resource_limits.assign_score > 0)
     assert(context.resource_limits.render_score > 0)
-    assert(context.resource_limits.render_length > 0)
   end
 
   def test_can_use_drop_as_context

test/integration/trim_mode_test.rb

@@ -528,4 +528,21 @@ class TrimModeTest < Minitest::Test
     END_EXPECTED
     assert_template_result(expected, text)
   end
+
+  def test_bug_compatible_pre_trim
+    template = Liquid::Template.parse("\n {%- raw %}{% endraw %}", bug_compatible_whitespace_trimming: true)
+    assert_equal("\n", template.render)
+
+    template = Liquid::Template.parse("\n {%- if true %}{% endif %}", bug_compatible_whitespace_trimming: true)
+    assert_equal("\n", template.render)
+
+    template = Liquid::Template.parse("{{ 'B' }} \n{%- if true %}C{% endif %}", bug_compatible_whitespace_trimming: true)
+    assert_equal("B C", template.render)
+
+    template = Liquid::Template.parse("B\n {%- raw %}{% endraw %}", bug_compatible_whitespace_trimming: true)
+    assert_equal("B", template.render)
+
+    template = Liquid::Template.parse("B\n {%- if true %}{% endif %}", bug_compatible_whitespace_trimming: true)
+    assert_equal("B", template.render)
+  end
 end # TrimModeTest