optionally remove empty nodes from AST

Shopify · Jan 7, 2025 · c680e11 · c680e11
1 parent 6e3e26b
commit c680e11
Show file tree

Hide file tree

Showing 5 changed files with 117 additions and 28 deletions.
diff --git a/lib/liquid/block_body.rb b/lib/liquid/block_body.rb
@@ -59,7 +59,7 @@ def freeze
           end
           new_tag = tag.parse(tag_name, markup, tokenizer, parse_context)
 
-          next if new_tag.is_a?(Comment)
+          next if parse_context.omit_blank_nodes && blank_node?(new_tag)
 
           @blank &&= new_tag.blank?
           @nodelist << new_tag
@@ -157,7 +157,7 @@ def self.rescue_render_node(context, output, line_number, exc, blank_tag)
           end
           new_tag = tag.parse(tag_name, markup, tokenizer, parse_context)
 
-          next if new_tag.is_a?(Comment)
+          next if parse_context.omit_blank_nodes && blank_node?(new_tag)
 
           @blank &&= new_tag.blank?
           @nodelist << new_tag
@@ -275,5 +275,18 @@ def raise_missing_tag_terminator(token, parse_context)
     def raise_missing_variable_terminator(token, parse_context)
       BlockBody.raise_missing_variable_terminator(token, parse_context)
     end
+
+    def blank_node?(node)
+      case node
+      when Comment
+        return true
+      when BlockBody
+        return true if node.nodelist.empty?
+      when Tag
+        return node.nodelist.all? { |n| blank_node?(n) }
+      end
+
+      false
+    end
   end
 end
diff --git 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, :error_mode, :environment, :omit_blank_nodes
 
     def initialize(options = Const::EMPTY_HASH)
       @environment = options.fetch(:environment, Environment.default)
@@ -12,6 +12,10 @@ def initialize(options = Const::EMPTY_HASH)
       @locale   = @template_options[:locale] ||= I18n.new
       @warnings = []
 
+      # remove blank nodes such as
+      # comment tags, empty if tags, etc from the AST
+      @omit_blank_nodes = options.fetch(:omit_blank_nodes, false)
+
       self.depth   = 0
       self.partial = false
     end

diff --git a/performance/tests/tribble/404.liquid b/performance/tests/tribble/404.liquid
@@ -1,4 +1,3 @@
-  <div id="page" class="innerpage clearfix">
 
     <div id="text-page">
       <div class="entry">

diff --git a/test/unit/block_unit_test.rb b/test/unit/block_unit_test.rb
@@ -49,12 +49,107 @@ def test_variable_many_embedded_fragments
 
   def test_with_block
     template = Liquid::Template.parse("  {% comment %} {% endcomment %} ")
-    assert_equal([String, String], block_types(template.root.nodelist))
-    assert_equal(2, template.root.nodelist.size)
+    assert_equal([String, Comment, String], block_types(template.root.nodelist))
+    assert_equal(3, template.root.nodelist.size)
+  end
+
+  def test_remove_empty_for_blocks_with_optimization_option
+    source = <<~LIQUID.chomp
+      {% for i in (1..1000000) %}
+      {% endfor %}
+    LIQUID
+
+    assert_root_nodelist_size(source, 0, omit_blank_nodes: true)
+
+    source = <<~LIQUID.chomp
+      {% for i in (1..1000000) %}
+      {% else %}
+      {% endfor %}
+    LIQUID
+
+    assert_root_nodelist_size(source, 0, omit_blank_nodes: true)
+
+    source = <<~LIQUID.chomp
+      {% for i in list %}
+        i
+      {% endfor %}
+    LIQUID
+
+    assert_root_nodelist_size(source, 1, omit_blank_nodes: true)
+
+    source = <<~LIQUID.chomp
+      {% for i in list %}
+      {% else %}
+        1
+      {% endfor %}
+    LIQUID
+
+    assert_root_nodelist_size(source, 1, omit_blank_nodes: true)
+  end
+
+  def test_remove_comment_nodes_with_optimization_option
+    source = <<~LIQUID.chomp
+      {% comment %}
+        {% if true %}
+        {% endif %}
+      {% endcomment %}
+    LIQUID
+
+    assert_root_nodelist_size(source, 0, omit_blank_nodes: true)
+
+    source = <<~LIQUID.chomp
+      {% liquid
+        comment
+          if true
+          endif
+        endcomment
+      %}
+    LIQUID
+
+    assert_root_nodelist_size(source, 0, omit_blank_nodes: true)
+  end
+
+  def test_remove_if_nodes_with_optimization_option
+    source = <<~LIQUID.chomp
+      {% if true %}
+      {% endif %}
+    LIQUID
+
+    assert_root_nodelist_size(source, 0, omit_blank_nodes: true)
+
+    source = <<~LIQUID.chomp
+      {% unless true %}
+      {% endunless %}
+    LIQUID
+
+    assert_root_nodelist_size(source, 0, omit_blank_nodes: true)
+
+    source = <<~LIQUID.chomp
+      {% if false %}
+      {% else %}
+      {% endif %}
+    LIQUID
+
+    assert_root_nodelist_size(source, 0, omit_blank_nodes: true)
+
+    source = <<~LIQUID.chomp
+      {% if false %}
+      {% else %}
+        Hello!
+      {% endif %}
+    LIQUID
+
+    assert_root_nodelist_size(source, 1, omit_blank_nodes: true)
   end
 
   private
 
+  def assert_root_nodelist_size(source, expected_size, parse_options = {})
+    template = Liquid::Template.parse(source, parse_options)
+
+    assert_equal(expected_size, template.root.nodelist.size)
+  end
+
   def block_types(nodelist)
     nodelist.collect(&:class)
   end

diff --git a/test/unit/tags/comment_tag_unit_test.rb b/test/unit/tags/comment_tag_unit_test.rb
@@ -199,26 +199,4 @@ def test_dont_override_liquid_tag_whitespace_control
       World!
     LIQUID
   end
-
-  def test_comment_tag_node_is_not_in_nodelist
-    template = Liquid::Template.parse(<<~LIQUID.chomp)
-      {% comment %}
-        {% if true %}
-        {% endif %}
-      {% endcomment %}
-    LIQUID
-
-    assert_equal(0, template.root.nodelist.size)
-
-    template = Liquid::Template.parse(<<~LIQUID.chomp)
-      {% liquid
-        comment
-          if true
-          endif
-        endcomment
-      %}
-    LIQUID
-
-    assert_equal(0, template.root.nodelist.size)
-  end
 end