diff --git a/.github/workflows/liquid.yml b/.github/workflows/liquid.yml index c018a4954..51b0bee29 100644 --- a/.github/workflows/liquid.yml +++ b/.github/workflows/liquid.yml @@ -56,11 +56,7 @@ jobs: bundler-cache: true bundler: latest - name: Run liquid-spec for all adapters - run: | - for adapter in spec/*.rb; do - echo "=== Running $adapter ===" - bundle exec liquid-spec run "$adapter" --no-max-failures - done + run: bin/liquid-spec-all-adapters memory_profile: runs-on: ubuntu-latest diff --git a/History.md b/History.md index 4337aeb98..8ebe43962 100644 --- a/History.md +++ b/History.md @@ -3,21 +3,22 @@ ## 6.0.0 ### Features -* (TODO) Add support for boolean expressions everywhere +* Add support for boolean expressions everywhere * As variable output `{{ a or b }}` * As filter argument `{{ collection | where: 'prop', a or b }}` * As tag argument `{% render 'snip', enabled: a or b %}` * As conditional tag argument `{% if cond %}` (extending previous behaviour) -* (TODO) Add support for subexpression prioritization and associativity +* Add support for subexpression prioritization and associativity * In ascending order of priority: * Logical: `and`, `or` (right to left) * Equality: `==`, `!=`, `<>` (left to right) * Comparison: `>`, `>=`, `<`, `<=`, `contains` (left to right) + * Groupings: `( expr )` - For example, this is now supported * `{{ a > b == c < d or e == f }}` which is equivalent to * `{{ ((a > b) == (c < d)) or (e == f) }}` -- (TODO) Add support for parenthesized expressions - * e.g. `(a or b) and c` +- Add support for parenthesized expressions + * e.g. `(a or b) == c` ### Architectural changes * `parse_expression` and `safe_parse_expression` have been removed from `Tag` and `ParseContext` @@ -32,10 +33,17 @@ * `:lax` and `lax_parse` is no longer supported * `:strict` and `strict_parse` is no longer supported * `strict2_parse` is renamed to `parse_markup` -* The `warnings` system has been removed. -* `Parser#expression` is renamed to `Parser#expression_string` -* `safe_parse_expression` methods are replaced by `Parser#expression` -* `parse_expression` methods are replaced by `Parser#unsafe_parse_expression` +* Expressions + * The `warnings` system has been removed. + * `Parser#expression` is renamed to `Parser#expression_string` + * `safe_parse_expression` methods are replaced by `Parser#expression` + * `parse_expression` methods are replaced by `Parser#unsafe_parse_expression` +* `Condition` + * `new(expr)` no longer accepts an `op` or `right`. Logic moved to BinaryExpression. + * `Condition#or` and `Condition#and` were replaced by `BinaryExpression`. + * `Condition#child_relation` replaced by `BinaryExpression`. + * `Condition.operations` was removed. + * `Condtion::MethodLiteral` was moved to the `Liquid` namespace ### Migrating from `^5.11.0` - In custom tags that include `ParserSwitching`, rename `strict2_parse` to `parse_markup` diff --git a/bin/liquid-spec-all-adapters b/bin/liquid-spec-all-adapters new file mode 100755 index 000000000..eb403268f --- /dev/null +++ b/bin/liquid-spec-all-adapters @@ -0,0 +1,5 @@ +#!/usr/bin/env bash +for adapter in spec/*.rb; do + echo "=== Running $adapter ===" + bundle exec liquid-spec run "$adapter" --no-max-failures +done diff --git a/lib/liquid.rb b/lib/liquid.rb index 4d0a71a64..0ffd07a1d 100644 --- a/lib/liquid.rb +++ b/lib/liquid.rb @@ -62,6 +62,8 @@ module Liquid require 'liquid/tags' require "liquid/environment" require 'liquid/lexer' +require 'liquid/method_literal' +require 'liquid/binary_expression' require 'liquid/parser' require 'liquid/i18n' require 'liquid/drop' diff --git a/lib/liquid/binary_expression.rb b/lib/liquid/binary_expression.rb new file mode 100644 index 000000000..cfd8a755f --- /dev/null +++ b/lib/liquid/binary_expression.rb @@ -0,0 +1,94 @@ +# frozen_string_literal: true + +module Liquid + class BinaryExpression + attr_reader :operator + attr_accessor :left_node, :right_node + + def initialize(left, operator, right) + @left_node = left + @operator = operator + @right_node = right + end + + def evaluate(context) + left = value(left_node, context) + + # logical relation short circuiting + if operator == 'and' + return left && value(right_node, context) + elsif operator == 'or' + return left || value(right_node, context) + end + + right = value(right_node, context) + + case operator + when '>' + left > right if can_compare?(left, right) + when '>=' + left >= right if can_compare?(left, right) + when '<' + left < right if can_compare?(left, right) + when '<=' + left <= right if can_compare?(left, right) + when '==' + equal_variables(left, right) + when '!=', '<>' + !equal_variables(left, right) + when 'contains' + contains(left, right) + else + raise(Liquid::ArgumentError, "Unknown operator #{operator}") + end + rescue ::ArgumentError => e + raise Liquid::ArgumentError, e.message + end + + def to_s + "(#{left_node.inspect} #{operator} #{right_node.inspect})" + end + + private + + 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) + node.apply(other) + 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 e0f527ebc..9cce15683 100644 --- a/lib/liquid/condition.rb +++ b/lib/liquid/condition.rb @@ -5,93 +5,19 @@ module Liquid # # Example: # - # c = Condition.new(1, '==', 1) + # c = Condition.new(expr) # c.evaluate #=> true # class Condition # :nodoc: - @@operators = { - '==' => ->(cond, left, right) { cond.send(:equal_variables, left, right) }, - '!=' => ->(cond, left, right) { !cond.send(:equal_variables, left, right) }, - '<>' => ->(cond, left, right) { !cond.send(:equal_variables, left, right) }, - '<' => :<, - '>' => :>, - '>=' => :>=, - '<=' => :<=, - 'contains' => lambda do |_cond, 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, - } + attr_reader :attachment + attr_accessor :left - 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, - } - - def self.operators - @@operators - end - - def self.parse_expression(parser) - markup = parser.expression_string - @@method_literals[markup] || parser.unsafe_parse_expression(markup) - end - - attr_reader :attachment, :child_condition - attr_accessor :left, :operator, :right - - def initialize(left = nil, operator = nil, right = nil) - @left = left - @operator = operator - @right = right - - @child_relation = nil - @child_condition = nil + def initialize(left = nil) + @left = left end def evaluate(context = deprecated_default_context) - condition = self - result = nil - loop do - result = interpret_condition(condition.left, condition.right, condition.operator, context) - - case condition.child_relation - when :or - break if Liquid::Utils.to_liquid_value(result) - when :and - break unless Liquid::Utils.to_liquid_value(result) - else - break - end - condition = condition.child_condition - end - result - end - - def or(condition) - @child_relation = :or - @child_condition = condition - end - - def and(condition) - @child_relation = :and - @child_condition = condition + context.evaluate(left) end def attach(attachment) @@ -112,91 +38,6 @@ def inspect private - def equal_variables(left, right) - if left.is_a?(MethodLiteral) - return call_method_literal(left, right) - end - - if right.is_a?(MethodLiteral) - return call_method_literal(right, left) - end - - left == right - end - - def call_method_literal(literal, value) - method_name = literal.method_name - - # If the object responds to the method (e.g., ActiveSupport is loaded), use it - if value.respond_to?(method_name) - value.send(method_name) - else - # Emulate ActiveSupport's blank?/empty? to make Liquid invariant - # to whether ActiveSupport is loaded or not - case method_name - when :blank? - liquid_blank?(value) - when :empty? - liquid_empty?(value) - else - false - end - end - end - - # Implement blank? semantics matching ActiveSupport - # blank? returns true for nil, false, empty strings, whitespace-only strings, - # empty arrays, and empty hashes - def liquid_blank?(value) - case value - when NilClass, FalseClass - true - when TrueClass, Numeric - false - when String - # Blank if empty or whitespace only (matches ActiveSupport) - value.empty? || value.match?(/\A\s*\z/) - when Array, Hash - value.empty? - else - # Fall back to empty? if available, otherwise false - value.respond_to?(:empty?) ? value.empty? : false - end - end - - # Implement empty? semantics - # Note: nil is NOT empty. empty? checks if a collection has zero elements. - def liquid_empty?(value) - case value - when String, Array, Hash - value.empty? - else - value.respond_to?(:empty?) ? value.empty? : false - end - end - - def interpret_condition(left, right, op, context) - # If the operator is empty this means that the decision statement is just - # a single variable. We can just poll this variable from the context and - # return this as the result. - return context.evaluate(left) if op.nil? - - left = Liquid::Utils.to_liquid_value(context.evaluate(left)) - right = Liquid::Utils.to_liquid_value(context.evaluate(right)) - - operation = self.class.operators[op] || raise(Liquid::ArgumentError, "Unknown operator #{op}") - - if operation.respond_to?(:call) - operation.call(self, left, right) - elsif left.respond_to?(operation) && right.respond_to?(operation) && !left.is_a?(Hash) && !right.is_a?(Hash) - begin - left.send(operation, right) - rescue ::ArgumentError => e - raise Liquid::ArgumentError, e.message - end - end - end - def deprecated_default_context warn("DEPRECATION WARNING: Condition#evaluate without a context argument is deprecated " \ "and will be removed from Liquid 6.0.0.") @@ -207,8 +48,6 @@ class ParseTreeVisitor < Liquid::ParseTreeVisitor def children [ @node.left, - @node.right, - @node.child_condition, @node.attachment ].compact end diff --git a/lib/liquid/expression.rb b/lib/liquid/expression.rb index a0112f663..4a6401fcf 100644 --- a/lib/liquid/expression.rb +++ b/lib/liquid/expression.rb @@ -9,8 +9,8 @@ class Expression '' => nil, 'true' => true, 'false' => false, - 'blank' => '', - 'empty' => '', + 'blank' => MethodLiteral::BLANK, + 'empty' => MethodLiteral::EMPTY, }.freeze DOT = ".".ord diff --git a/lib/liquid/lexer.rb b/lib/liquid/lexer.rb index f1740dbad..f6079f31a 100644 --- a/lib/liquid/lexer.rb +++ b/lib/liquid/lexer.rb @@ -6,22 +6,24 @@ class Lexer CLOSE_SQUARE = [:close_square, "]"].freeze COLON = [:colon, ":"].freeze COMMA = [:comma, ","].freeze - COMPARISION_NOT_EQUAL = [:comparison, "!="].freeze COMPARISON_CONTAINS = [:comparison, "contains"].freeze - COMPARISON_EQUAL = [:comparison, "=="].freeze COMPARISON_GREATER_THAN = [:comparison, ">"].freeze COMPARISON_GREATER_THAN_OR_EQUAL = [:comparison, ">="].freeze COMPARISON_LESS_THAN = [:comparison, "<"].freeze COMPARISON_LESS_THAN_OR_EQUAL = [:comparison, "<="].freeze - COMPARISON_NOT_EQUAL_ALT = [:comparison, "<>"].freeze + EQUALITY_EQUAL_EQUAL = [:equality, "=="].freeze + EQUALITY_NOT_EQUAL = [:equality, "!="].freeze + EQUALITY_NOT_EQUAL_ALT = [:equality, "<>"].freeze DASH = [:dash, "-"].freeze DOT = [:dot, "."].freeze DOTDOT = [:dotdot, ".."].freeze DOT_ORD = ".".ord DOUBLE_STRING_LITERAL = /"[^\"]*"/ EOS = [:end_of_string].freeze - IDENTIFIER = /[a-zA-Z_][\w-]*\??/ - NUMBER_LITERAL = /-?\d+(\.\d+)?/ + IDENTIFIER = /[a-zA-Z_][\w-]*\??/ + LOGICAL_AND = [:logical, 'and'].freeze + LOGICAL_OR = [:logical, 'or'].freeze + NUMBER_LITERAL = /-?\d+(\.\d+)?/ OPEN_ROUND = [:open_round, "("].freeze OPEN_SQUARE = [:open_square, "["].freeze PIPE = [:pipe, "|"].freeze @@ -38,11 +40,11 @@ class Lexer TWO_CHARS_COMPARISON_JUMP_TABLE = [].tap do |table| table["=".ord] = [].tap do |sub_table| - sub_table["=".ord] = COMPARISON_EQUAL + sub_table["=".ord] = EQUALITY_EQUAL_EQUAL sub_table.freeze end table["!".ord] = [].tap do |sub_table| - sub_table["=".ord] = COMPARISION_NOT_EQUAL + sub_table["=".ord] = EQUALITY_NOT_EQUAL sub_table.freeze end table.freeze @@ -51,7 +53,7 @@ class Lexer COMPARISON_JUMP_TABLE = [].tap do |table| table["<".ord] = [].tap do |sub_table| sub_table["=".ord] = COMPARISON_LESS_THAN_OR_EQUAL - sub_table[">".ord] = COMPARISON_NOT_EQUAL_ALT + sub_table[">".ord] = EQUALITY_NOT_EQUAL_ALT sub_table.freeze end table[">".ord] = [].tap do |sub_table| @@ -151,6 +153,10 @@ def tokenize(ss) # Special case for "contains" output << if type == :id && t == "contains" && output.last&.first != :dot COMPARISON_CONTAINS + elsif type == :id && t == "and" && output.last&.first != :dot + LOGICAL_AND + elsif type == :id && t == "or" && output.last&.first != :dot + LOGICAL_OR else [type, t] end diff --git a/lib/liquid/method_literal.rb b/lib/liquid/method_literal.rb new file mode 100644 index 000000000..8d700c3cd --- /dev/null +++ b/lib/liquid/method_literal.rb @@ -0,0 +1,49 @@ +# frozen_string_literal: true + +module Liquid + class MethodLiteral + attr_reader :method_name, :to_s + + def initialize(method_name, to_s, &evaluator) + @method_name = method_name + @to_s = to_s + @evaluator = evaluator + end + + def apply(value) + if value.respond_to?(@method_name) + value.send(@method_name) + elsif @evaluator + @evaluator.call(value) + end + end + + def to_liquid + to_s + end + + BLANK = MethodLiteral.new(:blank?, '') do |value| + case value + when NilClass, FalseClass + true + when TrueClass, Numeric + false + when String + value.empty? || value.match?(/\A\s*\z/) + when Array, Hash + value.empty? + else + value.respond_to?(:empty?) ? value.empty? : false + end + end.freeze + + EMPTY = MethodLiteral.new(:empty?, '') do |value| + case value + when String, Array, Hash + value.empty? + else + value.respond_to?(:empty?) ? value.empty? : nil + end + end.freeze + end +end diff --git a/lib/liquid/parser.rb b/lib/liquid/parser.rb index 7003af9be..dcdc37a37 100644 --- a/lib/liquid/parser.rb +++ b/lib/liquid/parser.rb @@ -13,6 +13,8 @@ def jump(point) @p = point end + # Consumes a token of specific type. + # Throws SyntaxError if token doesn't match type expectation. def consume(type = nil) token = @tokens[@p] if type && token[0] != type @@ -41,17 +43,62 @@ def id?(str) token[1] end + # Peeks the ahead token, returning true if matching expectation def look(type, ahead = 0) tok = @tokens[@p + ahead] return false unless tok tok[0] == type end + # expression := logical + # logical := equality (("and" | "or") equality)* + # equality := comparison (("==" | "!=" | "<>") comparison)* + # comparison := primary ((">=" | ">" | "<" | "<=" | ... ) primary)* + # primary := string | number | variable_lookup | range | boolean | grouping def expression + logical + end + + # Logical relations use right-to-left associativity. + # `a and b or c` is evaluated like (a and (b or c)) + # This enables short-circuit: if `a` is false, entire expression short-circuits. + # logical := equality (("and" | "or") logical)? + def logical + left = equality + if (operator = consume?(:logical)) + right = logical # recursive call builds proper RTL tree + BinaryExpression.new(left, operator, right) + else + left + end + end + + # equality := comparison (("==" | "!=" | "<>") comparison)* + def equality + expr = comparison + while look(:equality) + operator = consume + expr = BinaryExpression.new(expr, operator, comparison) + end + expr + end + + # comparison := primary ((">=" | ">" | "<" | "<=" | ... ) primary)* + def comparison + expr = primary + while look(:comparison) + operator = consume + expr = BinaryExpression.new(expr, operator, primary) + end + expr + end + + # primary := string | number | variable_lookup | range | boolean | grouping + def primary token = @tokens[@p] case token[0] when :id - variable_lookup + variable_lookup_or_literal when :open_square unnamed_variable_lookup when :string @@ -59,7 +106,7 @@ def expression when :number number when :open_round - range_lookup + grouping_or_range_lookup else raise SyntaxError, "#{token} is not a valid expression" end @@ -74,7 +121,18 @@ def string consume(:string)[1..-2] end + # variable_lookup := id (lookup)* + # lookup := indexed_lookup | dot_lookup + # indexed_lookup := "[" expression "]" + # dot_lookup := "." id def variable_lookup + name = consume(:id) + lookups, command_flags = variable_lookups + VariableLookup.new(name, lookups, command_flags) + end + + # a variable_lookup without lookups could be a literal + def variable_lookup_or_literal name = consume(:id) lookups, command_flags = variable_lookups if Expression::LITERALS.key?(name) && lookups.empty? @@ -84,12 +142,28 @@ def variable_lookup end end + # unnamed_variable_lookup := indexed_lookup (lookup)* def unnamed_variable_lookup name = indexed_lookup lookups, command_flags = variable_lookups VariableLookup.new(name, lookups, command_flags) end + # Parenthesized expressions are recursive + # grouping := "(" expression ")" + def grouping_or_range_lookup + consume(:open_round) + expr = expression + if consume?(:dotdot) + RangeLookup.create(expr, expression) + else + expr + end + ensure + consume(:close_round) + end + + # range_lookup := "(" expression ".." expression ")" def range_lookup consume(:open_round) first = expression diff --git a/lib/liquid/tags/case.rb b/lib/liquid/tags/case.rb index 8f58d8420..4f4731ccf 100644 --- a/lib/liquid/tags/case.rb +++ b/lib/liquid/tags/case.rb @@ -99,12 +99,12 @@ def parse_when(markup, body) parser = @parse_context.new_parser(markup) loop do - expr = Condition.parse_expression(parser) - block = Condition.new(@left, '==', expr) + expr = BinaryExpression.new(@left, '==', parser.equality) + block = Condition.new(expr) block.attach(body) @blocks << block - break unless parser.id?('or') || parser.consume?(:comma) + break unless parser.consume?(:logical) == 'or' || parser.consume?(:comma) end parser.consume(:end_of_string) diff --git a/lib/liquid/tags/if.rb b/lib/liquid/tags/if.rb index 2081c788f..559dce08c 100644 --- a/lib/liquid/tags/if.rb +++ b/lib/liquid/tags/if.rb @@ -73,38 +73,13 @@ def push_block(tag, markup) block.attach(new_body) end - def parse_expression(parser) - Condition.parse_expression(parser) - end - def parse_markup(markup) p = @parse_context.new_parser(markup) - condition = parse_binary_comparisons(p) + condition = Condition.new(p.expression) p.consume(:end_of_string) condition end - def parse_binary_comparisons(p) - condition = parse_comparison(p) - first_condition = condition - while (op = p.id?('and') || p.id?('or')) - child_condition = parse_comparison(p) - condition.send(op, child_condition) - condition = child_condition - end - first_condition - end - - def parse_comparison(p) - a = parse_expression(p) - if (op = p.consume?(:comparison)) - b = parse_expression(p) - Condition.new(a, op, b) - else - Condition.new(a) - end - end - class ParseTreeVisitor < Liquid::ParseTreeVisitor def children @node.blocks diff --git a/test/integration/assign_test.rb b/test/integration/assign_test.rb index 69163ab96..b3a04252e 100644 --- a/test/integration/assign_test.rb +++ b/test/integration/assign_test.rb @@ -35,13 +35,26 @@ def test_assign_with_filter ) end + def test_assign_boolean_expression_assignment + assert_template_result( + 'it rendered', + <<~LIQUID, + {%- assign should_render = a == 0 or (b == 1 and c == 2) -%} + {%- if should_render -%} + it rendered + {%- endif -%} + LIQUID + { 'b' => 1, 'c' => 2 }, + ) + end + def test_assign_syntax_error assert_match_syntax_error(/assign/, '{% assign foo not values %}.') end def test_assign_throws_on_unsupported_syntax assert_match_syntax_error( - "Expected dotdot but found pipe", + "Expected close_round but found pipe", "{% assign foo = ('X' | downcase) %}", ) end diff --git a/test/integration/parsing_quirks_test.rb b/test/integration/parsing_quirks_test.rb index 75954f93f..7927444ef 100644 --- a/test/integration/parsing_quirks_test.rb +++ b/test/integration/parsing_quirks_test.rb @@ -35,11 +35,10 @@ def test_error_on_empty_filter assert_raises(Liquid::SyntaxError) { Template.parse("{{test |a|b|}}") } end - def test_meaningless_parens_error - assert_raises(SyntaxError) do - markup = "a == 'foo' or (b == 'bar' and c == 'baz') or false" - Template.parse("{% if #{markup} %} YES {% endif %}") - end + def test_supported_parens + markup = "a == 'foo' or (b == 'bar' and c == 'baz') or false" + out = Template.parse("{% if #{markup} %} YES {% endif %}").render({ 'b' => 'bar', 'c' => 'baz' }) + assert_equal(' YES ', out) end def test_unexpected_characters_syntax_error diff --git a/test/integration/tags/if_else_tag_test.rb b/test/integration/tags/if_else_tag_test.rb index 3f04f2b8d..b37e4e411 100644 --- a/test/integration/tags/if_else_tag_test.rb +++ b/test/integration/tags/if_else_tag_test.rb @@ -147,28 +147,6 @@ def test_syntax_error_no_expression 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/integration/variable_test.rb b/test/integration/variable_test.rb index 38096e41d..b81a2106b 100644 --- a/test/integration/variable_test.rb +++ b/test/integration/variable_test.rb @@ -11,6 +11,24 @@ def test_simple_variable assert_template_result('worked wonderfully', "{{test}}", { 'test' => 'worked wonderfully' }) end + def test_equality + assert_template_result('true', "{{ 5 == 5 }}") + assert_template_result('false', "{{ 5 == 3 }}") + end + + def test_comparison + assert_template_result('true', "{{ 5 > 3 }}") + assert_template_result('false', "{{ 5 < 3 }}") + end + + def test_expression_piped_into_filter + assert_template_result('TRUE', "{{ 5 == 5 | upcase }}") + end + + def test_expression_used_as_filter_argument + assert_template_result('A: TRUE', "{{ 'a: $a' | replace: '$a', 5 == 5 | upcase }}") + end + def test_variable_render_calls_to_liquid assert_template_result('foobar', '{{ foo }}', { 'foo' => ThingWithToLiquid.new }) end diff --git a/test/unit/binary_expression_test.rb b/test/unit/binary_expression_test.rb new file mode 100644 index 000000000..6b5bb2367 --- /dev/null +++ b/test/unit/binary_expression_test.rb @@ -0,0 +1,146 @@ +# frozen_string_literal: true + +require 'test_helper' + +class ExecutionSpy + attr_reader :called + attr_accessor :value + + def initialize(value) + @called = false + @value = value + end + + def to_liquid_value + @called = true + @value + end + + def reset + @called = false + end +end + +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_logical_expression_short_circuiting + spy = ExecutionSpy.new(true) + + # false or spy should try spy + assert_eval(true, BinaryExpression.new(false, 'or', spy)) + assert_equal(true, spy.called) + + spy.reset + + # true or spy should not call spy + assert_eval(true, BinaryExpression.new(true, 'or', spy)) + assert_equal(false, spy.called) + + spy.reset + + # true and spy should try spy + assert_eval(true, BinaryExpression.new(true, 'and', spy)) + assert_equal(true, spy.called) + + spy.reset + + # false and spy should not try spy + assert_eval(false, BinaryExpression.new(false, 'and', spy)) + assert_equal(false, spy.called) + 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 08cb46860..501452e50 100644 --- a/test/unit/condition_unit_test.rb +++ b/test/unit/condition_unit_test.rb @@ -9,11 +9,6 @@ def setup @context = Liquid::Context.new end - def test_basic_condition - assert_equal(false, Condition.new(1, '==', 2).evaluate(Context.new)) - assert_equal(true, Condition.new(1, '==', 1).evaluate(Context.new)) - end - def test_default_operators_evalute_true assert_evaluates_true(1, '==', 1) assert_evaluates_true(1, '!=', 2) @@ -72,11 +67,11 @@ def test_comparation_of_int_and_str end def test_hash_compare_backwards_compatibility - assert_nil(Condition.new({}, '>', 2).evaluate(Context.new)) - assert_nil(Condition.new(2, '>', {}).evaluate(Context.new)) - assert_equal(false, Condition.new({}, '==', 2).evaluate(Context.new)) - assert_equal(true, Condition.new({ 'a' => 1 }, '==', 'a' => 1).evaluate(Context.new)) - assert_equal(true, Condition.new({ 'a' => 2 }, 'contains', 'a').evaluate(Context.new)) + assert_evaluates_nil({}, '>', 2) + assert_evaluates_nil(2, '>', {}) + assert_evaluates_false({}, '==', 2) + assert_evaluates_true({ 'a' => 1 }, '==', 'a' => 1) + assert_evaluates_true({ 'a' => 2 }, 'contains', 'a') end def test_contains_works_on_arrays @@ -110,39 +105,37 @@ def test_contains_with_string_left_operand_coerces_right_operand_to_string end def test_or_condition - condition = Condition.new(1, '==', 2) - assert_equal(false, condition.evaluate(Context.new)) + false_expr = '1 == 2' + true_expr = '1 == 1' - condition.or(Condition.new(2, '==', 1)) + condition = Condition.new(expression(false_expr)) + assert_equal(false, condition.evaluate(Context.new)) + condition = Condition.new(expression("#{false_expr} or #{false_expr}")) assert_equal(false, condition.evaluate(Context.new)) - condition.or(Condition.new(1, '==', 1)) + condition = Condition.new(expression("#{false_expr} or #{true_expr}")) + assert_equal(true, condition.evaluate(Context.new)) + condition = Condition.new(expression("#{true_expr} or #{false_expr}")) assert_equal(true, condition.evaluate(Context.new)) end def test_and_condition - condition = Condition.new(1, '==', 1) - - assert_equal(true, condition.evaluate(Context.new)) - - condition.and(Condition.new(2, '==', 2)) + false_expr = '1 == 2' + true_expr = '1 == 1' + condition = Condition.new(expression(true_expr)) assert_equal(true, condition.evaluate(Context.new)) - condition.and(Condition.new(2, '==', 1)) - + condition = Condition.new(expression("#{true_expr} and #{false_expr}")) assert_equal(false, condition.evaluate(Context.new)) - end - def test_should_allow_custom_proc_operator - Condition.operators['starts_with'] = proc { |_cond, left, right| left =~ /^#{right}/ } + condition = Condition.new(expression("#{false_expr} and #{true_expr}")) + assert_equal(false, condition.evaluate(Context.new)) - assert_evaluates_true('bob', 'starts_with', 'b') - assert_evaluates_false('bob', 'starts_with', 'o') - ensure - Condition.operators.delete('starts_with') + condition = Condition.new(expression("#{true_expr} and #{true_expr}")) + assert_equal(true, condition.evaluate(Context.new)) end def test_left_or_right_may_contain_operators @@ -152,38 +145,24 @@ def test_left_or_right_may_contain_operators assert_evaluates_true(VariableLookup.parse("one"), '==', VariableLookup.parse("another")) end - def test_default_context_is_deprecated - if Gem::Version.new(Liquid::VERSION) >= Gem::Version.new('6.0.0') - flunk("Condition#evaluate without a context argument is to be removed") - end - - _out, err = capture_io do - assert_equal(true, Condition.new(1, '==', 1).evaluate) - end - - expected = "DEPRECATION WARNING: Condition#evaluate without a context argument is deprecated " \ - "and will be removed from Liquid 6.0.0." - assert_includes(err.lines.map(&:strip), expected) - end - def test_parse_expression environment = Environment.build parse_context = ParseContext.new(environment: environment) parser = parse_context.new_parser('product.title') - result = Condition.parse_expression(parser) + result = parser.expression assert_instance_of(VariableLookup, result) assert_equal('product', result.name) assert_equal(['title'], result.lookups) end - def test_parse_expression_returns_method_literal_for_blank_and_empty + def test_parser_expression_returns_method_literal_for_blank_and_empty environment = Environment.build parse_context = ParseContext.new(environment: environment) parser = parse_context.new_parser('blank') - result = Condition.parse_expression(parser) + result = parser.expression - assert_instance_of(Condition::MethodLiteral, result) + assert_instance_of(MethodLiteral, result) end # Tests for blank? comparison without ActiveSupport @@ -203,7 +182,7 @@ def test_blank_with_whitespace_string # Template authors expect " " to be blank since it has no visible content. # This matches ActiveSupport's String#blank? which returns true for whitespace-only strings. @context['whitespace'] = ' ' - blank_literal = Condition.class_variable_get(:@@method_literals)['blank'] + blank_literal = Expression::LITERALS['blank'] assert_evaluates_true(VariableLookup.parse('whitespace'), '==', blank_literal) end @@ -212,7 +191,7 @@ def test_blank_with_empty_string # An empty string has no content, so it should be considered blank. # This is the most basic case of a blank string. @context['empty_string'] = '' - blank_literal = Condition.class_variable_get(:@@method_literals)['blank'] + blank_literal = Expression::LITERALS['blank'] assert_evaluates_true(VariableLookup.parse('empty_string'), '==', blank_literal) end @@ -221,7 +200,7 @@ def test_blank_with_empty_array # Empty arrays have no elements, so they are blank. # Useful for checking if a collection has items: {% if products == blank %} @context['empty_array'] = [] - blank_literal = Condition.class_variable_get(:@@method_literals)['blank'] + blank_literal = Expression::LITERALS['blank'] assert_evaluates_true(VariableLookup.parse('empty_array'), '==', blank_literal) end @@ -230,7 +209,7 @@ def test_blank_with_empty_hash # Empty hashes have no key-value pairs, so they are blank. # Useful for checking if settings/options exist: {% if settings == blank %} @context['empty_hash'] = {} - blank_literal = Condition.class_variable_get(:@@method_literals)['blank'] + blank_literal = Expression::LITERALS['blank'] assert_evaluates_true(VariableLookup.parse('empty_hash'), '==', blank_literal) end @@ -239,7 +218,7 @@ def test_blank_with_nil # nil represents "nothing" and is the canonical blank value. # Unassigned variables resolve to nil, so this enables: {% if missing_var == blank %} @context['nil_value'] = nil - blank_literal = Condition.class_variable_get(:@@method_literals)['blank'] + blank_literal = Expression::LITERALS['blank'] assert_evaluates_true(VariableLookup.parse('nil_value'), '==', blank_literal) end @@ -248,7 +227,7 @@ def test_blank_with_false # false is considered blank to match ActiveSupport semantics. # This allows {% if some_flag == blank %} to work when flag is false. @context['false_value'] = false - blank_literal = Condition.class_variable_get(:@@method_literals)['blank'] + blank_literal = Expression::LITERALS['blank'] assert_evaluates_true(VariableLookup.parse('false_value'), '==', blank_literal) end @@ -257,7 +236,7 @@ def test_not_blank_with_true # true is a definite value, not blank. # Ensures {% if flag == blank %} works correctly for boolean flags. @context['true_value'] = true - blank_literal = Condition.class_variable_get(:@@method_literals)['blank'] + blank_literal = Expression::LITERALS['blank'] assert_evaluates_false(VariableLookup.parse('true_value'), '==', blank_literal) end @@ -266,7 +245,7 @@ def test_not_blank_with_number # Numbers (including zero) are never blank - they represent actual values. # 0 is a valid quantity, not the absence of a value. @context['number'] = 42 - blank_literal = Condition.class_variable_get(:@@method_literals)['blank'] + blank_literal = Expression::LITERALS['blank'] assert_evaluates_false(VariableLookup.parse('number'), '==', blank_literal) end @@ -275,7 +254,7 @@ def test_not_blank_with_string_content # A string with actual content is not blank. # This is the expected behavior for most template string comparisons. @context['string'] = 'hello' - blank_literal = Condition.class_variable_get(:@@method_literals)['blank'] + blank_literal = Expression::LITERALS['blank'] assert_evaluates_false(VariableLookup.parse('string'), '==', blank_literal) end @@ -284,7 +263,7 @@ def test_not_blank_with_non_empty_array # An array with elements has content, so it's not blank. # Enables patterns like {% unless products == blank %}Show products{% endunless %} @context['array'] = [1, 2, 3] - blank_literal = Condition.class_variable_get(:@@method_literals)['blank'] + blank_literal = Expression::LITERALS['blank'] assert_evaluates_false(VariableLookup.parse('array'), '==', blank_literal) end @@ -293,7 +272,7 @@ def test_not_blank_with_non_empty_hash # A hash with key-value pairs has content, so it's not blank. # Useful for checking if configuration exists: {% if config != blank %} @context['hash'] = { 'a' => 1 } - blank_literal = Condition.class_variable_get(:@@method_literals)['blank'] + blank_literal = Expression::LITERALS['blank'] assert_evaluates_false(VariableLookup.parse('hash'), '==', blank_literal) end @@ -309,7 +288,7 @@ def test_empty_with_empty_string # An empty string ("") has length 0, so it's empty. # Different from blank - empty is a stricter check. @context['empty_string'] = '' - empty_literal = Condition.class_variable_get(:@@method_literals)['empty'] + empty_literal = Expression::LITERALS['empty'] assert_evaluates_true(VariableLookup.parse('empty_string'), '==', empty_literal) end @@ -319,7 +298,7 @@ def test_empty_with_whitespace_string_not_empty # This is the key difference between empty and blank: # " ".empty? => false, but " ".blank? => true @context['whitespace'] = ' ' - empty_literal = Condition.class_variable_get(:@@method_literals)['empty'] + empty_literal = Expression::LITERALS['empty'] assert_evaluates_false(VariableLookup.parse('whitespace'), '==', empty_literal) end @@ -328,7 +307,7 @@ def test_empty_with_empty_array # An array with no elements is empty. # [].empty? => true @context['empty_array'] = [] - empty_literal = Condition.class_variable_get(:@@method_literals)['empty'] + empty_literal = Expression::LITERALS['empty'] assert_evaluates_true(VariableLookup.parse('empty_array'), '==', empty_literal) end @@ -337,7 +316,7 @@ def test_empty_with_empty_hash # A hash with no key-value pairs is empty. # {}.empty? => true @context['empty_hash'] = {} - empty_literal = Condition.class_variable_get(:@@method_literals)['empty'] + empty_literal = Expression::LITERALS['empty'] assert_evaluates_true(VariableLookup.parse('empty_hash'), '==', empty_literal) end @@ -347,30 +326,45 @@ def test_nil_is_not_empty # nil is not a collection, so it cannot be empty. # This differs from blank: nil IS blank, but nil is NOT empty. @context['nil_value'] = nil - empty_literal = Condition.class_variable_get(:@@method_literals)['empty'] + empty_literal = Expression::LITERALS['empty'] assert_evaluates_false(VariableLookup.parse('nil_value'), '==', empty_literal) end private + def assert_evaluates_nil(left, op, right) + expr = BinaryExpression.new(left, op, right) + assert_nil( + Condition.new(expr).evaluate(@context), + "Evaluated not nil: #{left.inspect} #{op} #{right.inspect}", + ) + end + def assert_evaluates_true(left, op, right) + expr = BinaryExpression.new(left, op, right) assert( - Condition.new(left, op, right).evaluate(@context), + Condition.new(expr).evaluate(@context), "Evaluated false: #{left.inspect} #{op} #{right.inspect}", ) end def assert_evaluates_false(left, op, right) + expr = BinaryExpression.new(left, op, right) assert( - !Condition.new(left, op, right).evaluate(@context), + !Condition.new(expr).evaluate(@context), "Evaluated true: #{left.inspect} #{op} #{right.inspect}", ) end def assert_evaluates_argument_error(left, op, right) assert_raises(Liquid::ArgumentError) do - Condition.new(left, op, right).evaluate(@context) + expr = BinaryExpression.new(left, op, right) + Condition.new(expr).evaluate(@context) end end + + def expression(markup) + Parser.new(markup).expression + end end # ConditionTest diff --git a/test/unit/lexer_unit_test.rb b/test/unit/lexer_unit_test.rb index 73eeb7398..494769b8b 100644 --- a/test/unit/lexer_unit_test.rb +++ b/test/unit/lexer_unit_test.rb @@ -26,10 +26,17 @@ def test_float ) end + def test_equality + assert_equal( + [[:equality, '=='], [:equality, '<>'], [:equality, '!='], [:end_of_string]], + tokenize('== <> != '), + ) + end + def test_comparison assert_equal( - [[:comparison, '=='], [:comparison, '<>'], [:comparison, 'contains'], [:end_of_string]], - tokenize('== <> contains '), + [[:comparison, '>'], [:comparison, '>='], [:comparison, '<'], [:comparison, '<='], [:comparison, 'contains'], [:end_of_string]], + tokenize('> >= < <= contains'), ) end @@ -81,7 +88,7 @@ def test_fancy_identifiers def test_whitespace assert_equal( - [[:id, 'five'], [:pipe, '|'], [:comparison, '=='], [:end_of_string]], + [[:id, 'five'], [:pipe, '|'], [:equality, '=='], [:end_of_string]], tokenize("five|\n\t =="), ) end diff --git a/test/unit/parser_unit_test.rb b/test/unit/parser_unit_test.rb index cee971d11..a602c4258 100644 --- a/test/unit/parser_unit_test.rb +++ b/test/unit/parser_unit_test.rb @@ -77,6 +77,84 @@ def test_expression assert_equal(0..5, p.expression) end + def test_logical + p = new_parser("a and b") + expr = p.expression + assert(expr.is_a?(BinaryExpression)) + assert_equal('and', expr.operator) + assert_equal('a', expr.left_node.name) + assert_equal('b', expr.right_node.name) + + p = new_parser("a and b or c") + expr = p.expression + assert(expr.is_a?(BinaryExpression)) + assert_equal('and', expr.operator) + assert_equal('a', expr.left_node.name) + assert_equal('or', expr.right_node.operator) + assert_equal('b', expr.right_node.left_node.name) + assert_equal('c', expr.right_node.right_node.name) + + p = new_parser("a == b and c or d") + expr = p.expression + assert(expr.is_a?(BinaryExpression)) + assert_equal('and', expr.operator) + assert_equal('==', expr.left_node.operator) + assert_equal('a', expr.left_node.left_node.name) + assert_equal('b', expr.left_node.right_node.name) + assert_equal('or', expr.right_node.operator) + assert_equal('c', expr.right_node.left_node.name) + assert_equal('d', expr.right_node.right_node.name) + end + + def test_equality + p = new_parser("a == b") + expr = p.expression + assert(expr.is_a?(BinaryExpression)) + assert_equal('==', expr.operator) + assert_equal('a', expr.left_node.name) + assert_equal('b', expr.right_node.name) + + # BinaryExpression(==) + # 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_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 + p = new_parser("a > b") + expr = p.expression + assert(expr.is_a?(BinaryExpression)) + assert_equal('>', expr.operator) + 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_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_node.left_node) + assert_equal(5, expr.left_node.right_node) + assert_equal(4, expr.right_node) + end + def test_number p = new_parser('-1 0 1 2.0') assert_equal(-1, p.number) @@ -119,6 +197,28 @@ def test_ranges assert_equal('(hi[5].wat..old)', p.expression_string) end + def test_groupings_aka_parenthesized_expressions + # without the parens, this would be evaled as a and (b or c) + p = new_parser("(a and b) or c") + expr = p.expression + assert_equal('or', expr.operator) + assert_equal('and', expr.left_node.operator) + assert_equal('a', expr.left_node.left_node.name) + assert_equal('b', expr.left_node.right_node.name) + assert_equal('c', expr.right_node.name) + end + + def test_groupings_can_be_used_to_hijack_operation_priority + # without parens would be parsed as `a and (b == c)` + p = new_parser("(a and b) == c") + expr = p.expression + assert_equal('==', expr.operator) + assert_equal('and', expr.left_node.operator) + assert_equal('a', expr.left_node.left_node.name) + assert_equal('b', expr.left_node.right_node.name) + assert_equal('c', expr.right_node.name) + end + def test_argument_string p = new_parser("filter: hi.there[5], keyarg: 7") assert_equal('filter', p.consume(:id))