WIP3

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`

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'

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)
+      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

@@ -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,48 +38,6 @@ def inspect
 
     private
 
-    def equal_variables(left, right)
-      if left.is_a?(MethodLiteral)
-        if right.respond_to?(left.method_name)
-          return right.send(left.method_name)
-        else
-          return nil
-        end
-      end
-
-      if right.is_a?(MethodLiteral)
-        if left.respond_to?(right.method_name)
-          return left.send(right.method_name)
-        else
-          return nil
-        end
-      end
-
-      left == right
-    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.")
@@ -164,8 +48,6 @@ class ParseTreeVisitor < Liquid::ParseTreeVisitor
       def children
         [
           @node.left,
-          @node.right,
-          @node.child_condition,
           @node.attachment
         ].compact
       end

lib/liquid/expression.rb

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

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