Use BinaryExpression instead of Condition for comparisons

lib/liquid.rb

@@ -62,6 +62,7 @@ require 'liquid/interrupts'
 require 'liquid/tags'
 require "liquid/environment"
 require 'liquid/lexer'
+require 'liquid/method_literal'
 require 'liquid/binary_expression'
 require 'liquid/parser'
 require 'liquid/i18n'

lib/liquid/binary_expression.rb

@@ -2,39 +2,41 @@
 
 module Liquid
   class BinaryExpression
-    attr_reader :left, :operator, :right
+    attr_reader :operator
+    attr_accessor :left_node, :right_node
 
     def initialize(left, operator, right)
-      @left = left
+      @left_node = left
       @operator = operator
-      @right = right
+      @right_node = right
     end
 
     def evaluate(context)
-      left_value = value(left, context)
-      right_value = value(@right, context)
+      left = value(left_node, context)
+      right = value(right_node, context)
 
       case operator
       when '>'
-        left_value > right_value
+        left > right if can_compare?(left, right)
       when '>='
-        left_value >= right_value
+        left >= right if can_compare?(left, right)
       when '<'
-        left_value < right_value
+        left < right if can_compare?(left, right)
       when '<='
-        left_value <= right_value
+        left <= right if can_compare?(left, right)
       when '=='
-        left_value == right_value
+        equal_variables(left, right)
       when '!=', '<>'
-        left_value != right_value
+        !equal_variables(left, right)
       when 'contains'
-        if left_value && right_value && left_value.respond_to?(:include?)
-          right_value = right_value.to_s if left_value.is_a?(String)
-          left_value.include?(right_value)
-        else
-          false
-        end
+        contains(left, right)
       end
+    rescue ::ArgumentError => e
+      raise Liquid::ArgumentError, e.message
+    end
+
+    def to_s
+      "(#{left_node} #{operator} #{right_node})"
     end
 
     private
@@ -42,5 +44,41 @@ module Liquid
     def value(expr, context)
       Utils.to_liquid_value(context.evaluate(expr))
     end
+
+    def can_compare?(left, right)
+      left.respond_to?(operator) && right.respond_to?(operator) && !left.is_a?(Hash) && !right.is_a?(Hash)
+    end
+
+    def contains(left, right)
+      if left && right && left.respond_to?(:include?)
+        right = right.to_s if left.is_a?(String)
+        left.include?(right)
+      else
+        false
+      end
+    rescue Encoding::CompatibilityError
+      # "✅".b.include?("✅") raises Encoding::CompatibilityError despite being materially equal
+      left.b.include?(right.b)
+    end
+
+    def apply_method_literal(node, other)
+      other.send(node.method_name) if other.respond_to?(node.method_name)
+    end
+
+    def equal_variables(left, right)
+      return apply_method_literal(left, right) if left.is_a?(MethodLiteral)
+      return apply_method_literal(right, left) if right.is_a?(MethodLiteral)
+
+      left == right
+    end
+
+    class ParseTreeVisitor < Liquid::ParseTreeVisitor
+      def children
+        [
+          @node.left_node,
+          @node.right_node,
+        ]
+      end
+    end
   end
 end

lib/liquid/condition.rb

@@ -29,16 +29,6 @@ module Liquid
         left.b.include?(right.b)
       end,
     }
-
-    class MethodLiteral
-      attr_reader :method_name, :to_s
-
-      def initialize(method_name, to_s)
-        @method_name = method_name
-        @to_s = to_s
-      end
-    end
-
     @@method_literals = {
       'blank' => MethodLiteral.new(:blank?, '').freeze,
       'empty' => MethodLiteral.new(:empty?, '').freeze,

lib/liquid/expression.rb

@@ -9,8 +9,8 @@ module Liquid
       '' => nil,
       'true' => true,
       'false' => false,
-      'blank' => '',
-      'empty' => '',
+      'blank' => MethodLiteral.new(:blank?, '').freeze,
+      'empty' => MethodLiteral.new(:empty?, '').freeze,
     }.freeze
 
     DOT = ".".ord

lib/liquid/method_literal.rb

@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+module Liquid
+  class MethodLiteral
+    attr_reader :method_name, :to_s
+
+    def initialize(method_name, to_s)
+      @method_name = method_name
+      @to_s = to_s
+    end
+
+    def to_liquid
+      to_s
+    end
+  end
+end

lib/liquid/tags/if.rb

@@ -96,13 +96,7 @@ module Liquid
     end
 
     def parse_comparison(p)
-      a = parse_expression(p)
-      if (op = p.consume?(:comparison) || p.consume?(:equality))
-        b = parse_expression(p)
-        Condition.new(a, op, b)
-      else
-        Condition.new(a)
-      end
+      Condition.new(p.expression)
     end
 
     class ParseTreeVisitor < Liquid::ParseTreeVisitor

test/integration/tags/if_else_tag_test.rb

@@ -147,28 +147,6 @@ class IfElseTagTest < Minitest::Test
     assert_raises(SyntaxError) { assert_template_result('', '{% if %}') }
   end
 
-  def test_if_with_custom_condition
-    original_op = Condition.operators['contains']
-    Condition.operators['contains'] = :[]
-
-    assert_template_result('yes', %({% if 'bob' contains 'o' %}yes{% endif %}))
-    assert_template_result('no', %({% if 'bob' contains 'f' %}yes{% else %}no{% endif %}))
-  ensure
-    Condition.operators['contains'] = original_op
-  end
-
-  def test_operators_are_ignored_unless_isolated
-    original_op = Condition.operators['contains']
-    Condition.operators['contains'] = :[]
-
-    assert_template_result(
-      'yes',
-      %({% if 'gnomeslab-and-or-liquid' contains 'gnomeslab-and-or-liquid' %}yes{% endif %}),
-    )
-  ensure
-    Condition.operators['contains'] = original_op
-  end
-
   def test_operators_are_whitelisted
     assert_raises(SyntaxError) do
       assert_template_result('', %({% if 1 or throw or or 1 %}yes{% endif %}))

test/unit/binary_expression_test.rb

@@ -0,0 +1,101 @@
+# frozen_string_literal: true
+
+require 'test_helper'
+
+class BinaryExpressionTest < Minitest::Test
+  include Liquid
+
+  def test_simple_comparison_evaluation
+    assert_eval(false, BinaryExpression.new(5, ">", 5))
+    assert_eval(true, BinaryExpression.new(5, ">=", 5))
+    assert_eval(false, BinaryExpression.new(5, "<", 5))
+    assert_eval(true, BinaryExpression.new(5, "<=", 5))
+    assert_eval(true, BinaryExpression.new("abcd", "contains", "a"))
+  end
+
+  def test_complex_evaluation
+    # 1 > 2 == 2 > 3
+    assert_eval(true, BinaryExpression.new(
+      BinaryExpression.new(1, '>', 2),
+      '==',
+      BinaryExpression.new(2, '>', 3),
+    ))
+
+    # 1 > 2 != 2 > 3
+    assert_eval(false, BinaryExpression.new(
+      BinaryExpression.new(1, '>', 2),
+      '!=',
+      BinaryExpression.new(2, '>', 3),
+    ))
+
+    # a > 0 == b.prop > 0
+    assert_eval(
+      true,
+      BinaryExpression.new(
+        BinaryExpression.new(var('a'), '>', 0),
+        '==',
+        BinaryExpression.new(var('b.prop'), '>', 0),
+      ),
+      { 'a' => 1, 'b' => { 'prop' => 2 } },
+    )
+  end
+
+  def test_method_literal_equality
+    empty = MethodLiteral.new(:empty?, '')
+
+    # a == empty, empty == a
+    assert_eval(false, BinaryExpression.new("123", "==", empty))
+    assert_eval(true, BinaryExpression.new("", "==", empty))
+    assert_eval(false, BinaryExpression.new(empty, "==", "123"))
+    assert_eval(true, BinaryExpression.new(empty, "==", ""))
+
+    # a does not have .empty?
+    assert_eval(nil, BinaryExpression.new(1, "==", empty))
+    assert_eval(nil, BinaryExpression.new(true, "==", empty))
+    assert_eval(nil, BinaryExpression.new(false, "==", empty))
+    assert_eval(nil, BinaryExpression.new(nil, "==", empty))
+
+    # a != empty
+    assert_eval(true, BinaryExpression.new("123", "!=", empty))
+    assert_eval(false, BinaryExpression.new("", "!=", empty))
+    assert_eval(true, BinaryExpression.new(empty, "!=", "123"))
+    assert_eval(false, BinaryExpression.new(empty, "!=", ""))
+
+    # a does not have .empty?
+    assert_eval(true, BinaryExpression.new(1, "!=", empty))
+    assert_eval(true, BinaryExpression.new(true, "!=", empty))
+    assert_eval(true, BinaryExpression.new(false, "!=", empty))
+    assert_eval(true, BinaryExpression.new(nil, "!=", empty))
+  end
+
+  def test_method_literal_comparison
+    empty = MethodLiteral.new(:empty?, '')
+
+    ['>', '>='].each do |op|
+      assert_eval(nil, BinaryExpression.new("123", op, empty))
+      assert_eval(nil, BinaryExpression.new("", op, empty))
+      assert_eval(nil, BinaryExpression.new(empty, op, "123"))
+      assert_eval(nil, BinaryExpression.new(empty, op, ""))
+    end
+
+    # Interesting case, contains on strings does include?(right.to_s)
+    assert_eval(true, BinaryExpression.new("123", "contains", empty))
+    assert_eval(true, BinaryExpression.new("", "contains", empty))
+  end
+
+  def assert_eval(expected, expr, assigns = {})
+    actual = expr.evaluate(context(assigns))
+    message = "Expected '#{expr}' to evaluate to '#{expected}'"
+    return assert_nil(actual, message) if expected.nil?
+
+    assert_equal(expected, actual, message)
+  end
+
+  def var(markup)
+    Parser.new(markup).variable_lookup
+  end
+
+  def context(assigns = {})
+    Context.build(outer_scope: assigns)
+  end
+end

test/unit/condition_unit_test.rb

@@ -183,7 +183,7 @@ class ConditionUnitTest < Minitest::Test
     parser = parse_context.new_parser('blank')
     result = Condition.parse_expression(parser)
 
-    assert_instance_of(Condition::MethodLiteral, result)
+    assert_instance_of(MethodLiteral, result)
   end
 
   # Tests for blank? comparison without ActiveSupport

test/unit/parser_unit_test.rb

@@ -82,24 +82,24 @@ class ParserUnitTest < Minitest::Test
     expr = p.expression
     assert(expr.is_a?(BinaryExpression))
     assert_equal('==', expr.operator)
-    assert_equal('a', expr.left.name)
-    assert_equal('b', expr.right.name)
+    assert_equal('a', expr.left_node.name)
+    assert_equal('b', expr.right_node.name)
 
     # BinaryExpression(==)
-    #   left: BinaryExpression(<)
-    #     left: 0
-    #     right: 5
-    #   right: BinaryExpression(>)
-    #     left: 6
-    #     right: 1
+    #   left_node: BinaryExpression(<)
+    #     left_node: 0
+    #     right_node: 5
+    #   right_node: BinaryExpression(>)
+    #     left_node: 6
+    #     right_node: 1
     p = new_parser("0 < 5 == 6 > 1")
     expr = p.expression
     assert(expr.is_a?(BinaryExpression))
     assert_equal('==', expr.operator)
-    assert_equal(0, expr.left.left)
-    assert_equal(5, expr.left.right)
-    assert_equal(6, expr.right.left)
-    assert_equal(1, expr.right.right)
+    assert_equal(0, expr.left_node.left_node)
+    assert_equal(5, expr.left_node.right_node)
+    assert_equal(6, expr.right_node.left_node)
+    assert_equal(1, expr.right_node.right_node)
   end
 
   def test_comparison
@@ -107,23 +107,23 @@ class ParserUnitTest < Minitest::Test
     expr = p.expression
     assert(expr.is_a?(BinaryExpression))
     assert_equal('>', expr.operator)
-    assert(expr.left.is_a?(VariableLookup))
-    assert_equal('a', expr.left.name)
-    assert(expr.right.is_a?(VariableLookup))
-    assert_equal('b', expr.right.name)
+    assert(expr.left_node.is_a?(VariableLookup))
+    assert_equal('a', expr.left_node.name)
+    assert(expr.right_node.is_a?(VariableLookup))
+    assert_equal('b', expr.right_node.name)
 
     # BinaryExpression(>=)
-    #   left: BinaryExpression(>)
-    #     left: 10
-    #     right: 5
-    #   right: 4
+    #   left_node: BinaryExpression(>)
+    #     left_node: 10
+    #     right_node: 5
+    #   right_node: 4
     p = new_parser("10 > 5 >= 4")
     expr = p.expression
     assert(expr.is_a?(BinaryExpression))
     assert_equal('>=', expr.operator)
-    assert_equal(10, expr.left.left)
-    assert_equal(5, expr.left.right)
-    assert_equal(4, expr.right)
+    assert_equal(10, expr.left_node.left_node)
+    assert_equal(5, expr.left_node.right_node)
+    assert_equal(4, expr.right_node)
   end
 
   def test_number