diff --git a/lib/liquid.rb b/lib/liquid.rb index d069f6ca..0ffd07a1 100644 --- a/lib/liquid.rb +++ b/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' diff --git a/lib/liquid/binary_expression.rb b/lib/liquid/binary_expression.rb index 944099f7..00e3873d 100644 --- a/lib/liquid/binary_expression.rb +++ b/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 diff --git a/lib/liquid/condition.rb b/lib/liquid/condition.rb index e0f527eb..893706d0 100644 --- a/lib/liquid/condition.rb +++ b/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, diff --git a/lib/liquid/expression.rb b/lib/liquid/expression.rb index a0112f66..8eea983e 100644 --- a/lib/liquid/expression.rb +++ b/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 diff --git a/lib/liquid/method_literal.rb b/lib/liquid/method_literal.rb new file mode 100644 index 00000000..812c7199 --- /dev/null +++ b/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 diff --git a/lib/liquid/tags/if.rb b/lib/liquid/tags/if.rb index 3471f677..b07a91c7 100644 --- a/lib/liquid/tags/if.rb +++ b/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 diff --git a/test/integration/tags/if_else_tag_test.rb b/test/integration/tags/if_else_tag_test.rb index 3f04f2b8..b37e4e41 100644 --- a/test/integration/tags/if_else_tag_test.rb +++ b/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 %})) diff --git a/test/unit/binary_expression_test.rb b/test/unit/binary_expression_test.rb new file mode 100644 index 00000000..19fe33d7 --- /dev/null +++ b/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 diff --git a/test/unit/condition_unit_test.rb b/test/unit/condition_unit_test.rb index 08cb4686..5390e201 100644 --- a/test/unit/condition_unit_test.rb +++ b/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 diff --git a/test/unit/parser_unit_test.rb b/test/unit/parser_unit_test.rb index dccee43f..1274fe92 100644 --- a/test/unit/parser_unit_test.rb +++ b/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