WIP: more careful IF block merger

lib/liquid/loom.rb

@@ -2,6 +2,9 @@
 
 module Liquid
   class Loom
+    MERGABLE_IF_OPERATORS = ["==", ">", "<", "!="].freeze
+    EQUAL_OP = "==".freeze
+
     class << self
       def optimize(template)
         new(template).optimize
@@ -37,6 +40,59 @@ module Liquid
 
     private
 
+    def mergable_if_blocks?(target_if, next_if)
+      target_left = target_if.blocks.first.left
+      target_right = target_if.blocks.first.right
+      next_left = next_if.blocks.first.left
+      next_right = next_if.blocks.first.right
+
+      used_variables = Hash.new { |h, k| h[k] = 0 }
+
+      [
+        target_if.blocks.first.left,
+        target_if.blocks.first.right,
+        next_if.blocks.first.left,
+        next_if.blocks.first.right
+      ].each do |var|
+        if var.is_a?(VariableLookup)
+          used_variables[var.name] += 1
+        end
+      end
+
+      return if used_variables.keys.count > 1
+
+      most_used_variable_name = used_variables.keys[0]
+
+      # TODO: I probably can't do this
+      # It might be possible to get different result between a > b and b < a
+      # Move most commonly used variable to the left side
+      if (target_left.is_a?(VariableLookup) && target_left.name != most_used_variable_name) || (target_right.is_a?(VariableLookup) && target_right.name == most_used_variable_name)
+        target_left, target_right = target_right, target_left
+      end
+
+      if (next_left.is_a?(VariableLookup) && next_left.name != most_used_variable_name) || (next_right.is_a?(VariableLookup) && next_right.name == most_used_variable_name)
+        next_left, next_right = next_right, next_left
+      end
+
+      return false unless target_left.is_a?(VariableLookup) && next_left.is_a?(VariableLookup)
+      return false if target_left.name != next_left.name
+
+      return false if target_right.nil? || next_right.nil?
+
+
+      # we need to be conversative here and only can merge ==, >, <, and != operators
+      target_operator = target_if.blocks.first.operator
+      next_operator = next_if.blocks.first.operator
+
+      return false unless MERGABLE_IF_OPERATORS.include?(target_operator) && MERGABLE_IF_OPERATORS.include?(next_operator)
+
+      return false if target_operator == next_operator && target_right == next_right
+
+      return false if target_right.is_a?(VariableLookup) || next_right.is_a?(VariableLookup)
+
+      true
+    end
+
     def chain_if_blocks(nodelist, first_if_node, first_if_index)
       used_variables = Set.new
 
@@ -52,11 +108,7 @@ module Liquid
         break unless node.is_a?(If)
 
         # check if the variables used in the current block are used in the previous block
-        first_if_node.blocks.each do |condition|
-          if used_variables.include?(condition.left) || (condition.right && used_variables.include?(condition.right))
-            break
-          end
-        end
+        break unless mergable_if_blocks?(first_if_node, node)
 
         if_blocks << node
       end

test/unit/eager_optimize_test.rb

@@ -63,7 +63,6 @@ class EagerOptimizeTest < Minitest::Test
   def test_merge_if_blocks
     # for now, work with consecutive if blocks without any String nodes in between
     source = <<~LIQUID.gsub(/\n/, '')
-      {% assign foo = 1 %}
       {% if foo == 1 %}
         foo: {{ foo }}
       {% endif %}
@@ -75,24 +74,97 @@ class EagerOptimizeTest < Minitest::Test
       {% endif %}
     LIQUID
 
-    original_template = Liquid::Template.parse(source, eager_optimize: false)
-    template = Template.parse(source, eager_optimize: true)
+    assert_optimization([Liquid::If], source, { "foo" => nil })
+    assert_optimization([Liquid::If], source, { "foo" => 1 })
+    assert_optimization([Liquid::If], source, { "foo" => 2 })
+    assert_optimization([Liquid::If], source, { "foo" => 5 })
 
-    assert_equal(
-      [Liquid::Assign, Liquid::If],
-      template.root.nodelist.map(&:class),
-    )
+    source = <<~LIQUID.gsub(/\n/, '')
+      {% assign bar = "application" %}
+      {% if foo == 1 %}
+        foo: {{ foo }}
+      {% endif %}
+      {% if foo == 2 and bar contains "app" %}
+        foo: {{ foo }}
+      {% endif %}
+      {% if 3 == foo and bar == "application" %}
+        foo: {{ foo }}
+      {% endif %}
+    LIQUID
 
-    [nil, 1, 2, 3, 4].each do |foo|
-      assert_equal(
-        original_template.render('foo' => foo),
-        template.render('foo' => foo),
-      )
-    end
+    assert_optimization([Liquid::Assign, Liquid::If], source)
+  end
+
+  def test_does_not_merge_if_blocks
+    assert_optimization([Liquid::If, Liquid::If], <<~LIQUID.gsub(/\n/, ''))
+      {% if foo == 1 %}
+        foo: {{ foo }}
+      {% endif %}
+      {% if k == 1 %}
+        foo: {{ foo }}
+      {% endif %}
+    LIQUID
+
+    assert_optimization([Liquid::If, Liquid::If], <<~LIQUID.gsub(/\n/, ''))
+      {% if foo == 1 %}
+        foo: {{ foo }}
+      {% endif %}
+      {% if foo == 1 %}
+        foo: {{ foo }}
+      {% endif %}
+    LIQUID
+
+    assert_optimization([Liquid::If, Liquid::If], <<~LIQUID.gsub(/\n/, ''))
+      {% if foo == 1 %}
+        foo: {{ foo }}
+      {% endif %}
+      {% if a == foo %}
+        foo: {{ foo }}
+      {% endif %}
+    LIQUID
+
+    assert_optimization([Liquid::If, Liquid::If], <<~LIQUID.gsub(/\n/, ''))
+      {% if foo %}
+        foo: {{ foo }}
+      {% endif %}
+      {% if foo %}
+        foo: {{ foo }}
+      {% endif %}
+    LIQUID
+
+    assert_optimization([Liquid::If, Liquid::If], <<~LIQUID.gsub(/\n/, ''))
+      {% if foo == 1 %}
+        foo: {{ foo }}
+      {% endif %}
+      {% if foo >= 1 %}
+        foo: {{ foo }}
+      {% endif %}
+    LIQUID
+
+    assert_optimization([Liquid::If, Liquid::If], <<~LIQUID.gsub(/\n/, ''))
+      {% if foo == 1 %}
+        foo: {{ foo }}
+      {% endif %}
+      {% if 1  %}
+        foo: {{ foo }}
+      {% endif %}
+    LIQUID
   end
 
   private
 
+  def assert_optimization(expected, source, context = { "foo" => 1 })
+    template = Template.parse(source, eager_optimize: true)
+    assert_equal(expected, template.root.nodelist.map(&:class),)
+
+    baseline_template = Template.parse(source, eager_optimize: false)
+
+    assert_equal(
+      baseline_template.render(context),
+      template.render(context),
+    )
+  end
+
   def total_node_count(template)
     root = template.root
     children = root.nodelist