Move parse_expression to Parser.unsafe_parse_expression

- Add Parser#string
- Add Parser#unsafe_parse_expression
- Add private Parser#parse_expression
- Remove ParseContext.parse_expression
- Remove Tag.parse_expression
- Condition.parse_expression now takes a parser as argument

History.md

@@ -29,6 +29,8 @@
   * `strict2_parse` is renamed to `parse_markup`
 * The `warnings` system has been removed.
 * `safe_parse_expression` has been moved to `Parser.expression_node`
+* `parse_expression` methods have been moved to `Parser#unsafe_parse_expression`
+  * Use `Parser#expression_node`, `Parser#string`, etc. instead
 
 ### Migrating from `^5.11.0`
 - In custom tags that include `ParserSwitching`, rename `strict2_parse` to `parse_markup`

lib/liquid/condition.rb

@@ -48,8 +48,9 @@ module Liquid
       @@operators
     end
 
-    def self.parse_expression(parse_context, markup, safe: false)
+    def self.parse_expression(parser)
-      @@method_literals[markup] || parse_context.parse_expression(markup, safe: safe)
+      markup = parser.expression
+      @@method_literals[markup] || parser.unsafe_parse_expression(markup)
     end
 
     attr_reader :attachment, :child_condition

lib/liquid/parse_context.rb

@@ -49,17 +49,6 @@ module Liquid
       )
     end
 
-    def parse_expression(markup, safe: false)
-      # markup MUST come from a string returned by the parser
-      # (e.g., parser.expression). We're not calling the parser here to
-      # prevent redundant parser overhead. The `safe` opt-in
-      # exists to ensure it is not accidentally still called with
-      # the result of a regex.
-      raise Liquid::InternalError, "unsafe parse_expression cannot be used" unless safe
-
-      Expression.parse(markup, @string_scanner, @expression_cache)
-    end
-
     def partial=(value)
       @partial = value
       @options = value ? partial_options : @template_options

lib/liquid/parser.rb

@@ -72,9 +72,9 @@ module Liquid
       end
     end
 
+
     def expression_node
-      expr = expression
+      parse_expression(expression)
-      Expression.parse(expr, @ss, @cache)
     end
 
     def argument
@@ -104,5 +104,17 @@ module Liquid
       end
       str
     end
+
+    # Assumes safe input. For cases where you need the string.
+    # Don't use this unless you're sure about what you're doing.
+    def unsafe_parse_expression(markup)
+      parse_expression(markup)
+    end
+
+    private
+
+    def parse_expression(markup)
+      Expression.parse(markup, @ss, @cache)
+    end
   end
 end

lib/liquid/parser.rb.orig

@@ -0,0 +1,165 @@
+# frozen_string_literal: true
+
+module Liquid
+  class Parser
+    def initialize(input, expression_cache = nil)
+      @ss = input.is_a?(StringScanner) ? input : StringScanner.new(input)
+      @cache = expression_cache
+      @tokens = Lexer.tokenize(@ss)
+      @p      = 0 # pointer to current location
+    end
+
+    def jump(point)
+      @p = point
+    end
+
+    def consume(type = nil)
+      token = @tokens[@p]
+      if type && token[0] != type
+        raise SyntaxError, "Expected #{type} but found #{@tokens[@p].first}"
+      end
+      @p += 1
+      token[1]
+    end
+
+    # Only consumes the token if it matches the type
+    # Returns the token's contents if it was consumed
+    # or false otherwise.
+    def consume?(type)
+      token = @tokens[@p]
+      return false unless token && token[0] == type
+      @p += 1
+      token[1]
+    end
+
+    # Like consume? Except for an :id token of a certain name
+    def id?(str)
+      token = @tokens[@p]
+      return false unless token && token[0] == :id
+      return false unless token[1] == str
+      @p += 1
+      token[1]
+    end
+
+    def look(type, ahead = 0)
+      tok = @tokens[@p + ahead]
+      return false unless tok
+      tok[0] == type
+    end
+
+    def expression
+      token = @tokens[@p]
+      case token[0]
+      when :id
+        str = consume
+        str << variable_lookups
+      when :open_square
+        str = consume.dup
+        str << expression
+        str << consume(:close_square)
+        str << variable_lookups
+      when :string, :number
+        consume
+      when :open_round
+        consume
+        first = expression
+        consume(:dotdot)
+        last = expression
+        consume(:close_round)
+        "(#{first}..#{last})"
+      else
+        raise SyntaxError, "#{token} is not a valid expression"
+      end
+    end
+
+    def string
+      parse_expression(consume(:string))
+    end
+
+    def expression_node
+      parse_expression(expression)
+    end
+
+<<<<<<< HEAD
+    # Assumes safe input. For cases where you need the string.
+    # Don't use this unless you're sure about what you're doing.
+    def unsafe_parse_expression(markup)
+      parse_expression(markup)
+    end
+
+    def argument
+=======
+    def string
+      consume(:string)[1..-2]
+    end
+
+    def argument_string
+>>>>>>> 68476f39 (Move unsafe_parse_expression to the end)
+      str = +""
+      # might be a keyword argument (identifier: expression)
+      if look(:id) && look(:colon, 1)
+        str << consume << consume << ' '
+      end
+
+      str << expression
+      str
+    end
+
+    def variable_lookups
+      str = +""
+      loop do
+        if look(:open_square)
+          str << consume
+          str << expression
+          str << consume(:close_square)
+        elsif look(:dot)
+          str << consume
+          str << consume(:id)
+        else
+          break
+        end
+      end
+      str
+    end
+
+<<<<<<< HEAD
+=======
+    def variable_lookup
+      name = consume(:id)
+      lookups, command_flags = variable_lookups
+      if Expression::LITERALS.key?(name) && lookups.empty?
+        Expression::LITERALS[name]
+      else
+        VariableLookup.new(name, lookups, command_flags)
+      end
+    end
+
+    def unnamed_variable_lookup
+      name = indexed_lookup
+      lookups, command_flags = variable_lookups
+      VariableLookup.new(name, lookups, command_flags)
+    end
+
+    def range_lookup
+      consume(:open_round)
+      first = expression
+      consume(:dotdot)
+      last = expression
+      consume(:close_round)
+      RangeLookup.create(first, last)
+    end
+
+    # Assumes safe input. For cases where you need the string.
+    # Don't use this unless you're sure about what you're doing.
+    def unsafe_parse_expression(markup)
+      parse_expression(markup)
+    end
+
+>>>>>>> 68476f39 (Move unsafe_parse_expression to the end)
+    private
+
+    def parse_expression(markup)
+      Expression.parse(markup, @ss, @cache)
+    end
+  end
+end

lib/liquid/tag.rb

@@ -65,11 +65,5 @@ module Liquid
     def blank?
       false
     end
-
-    private
-
-    def parse_expression(markup, safe: false)
-      parse_context.parse_expression(markup, safe: safe)
-    end
   end
 end

lib/liquid/tags/for.rb

@@ -76,7 +76,7 @@ module Liquid
       raise SyntaxError, options[:locale].t("errors.syntax.for_invalid_in") unless p.id?('in')
 
       collection_name  = p.expression
-      @collection_name = parse_expression(collection_name, safe: true)
+      @collection_name = p.unsafe_parse_expression(collection_name)
 
       @name     = "#{@variable_name}-#{collection_name}"
       @reversed = p.id?('reversed')
@@ -87,7 +87,7 @@ module Liquid
           raise SyntaxError, options[:locale].t("errors.syntax.for_invalid_attribute")
         end
         p.consume(:colon)
-        set_attribute(attribute, p.expression, safe: true)
+        set_attribute(attribute, p)
       end
       p.consume(:end_of_string)
     end
@@ -157,16 +157,17 @@ module Liquid
       output
     end
 
-    def set_attribute(key, expr, safe: false)
+    def set_attribute(key, p)
+      expr = p.expression
       case key
       when 'offset'
         @from = if expr == 'continue'
           :continue
         else
-          parse_expression(expr, safe: safe)
+          p.unsafe_parse_expression(expr)
         end
       when 'limit'
-        @limit = parse_expression(expr, safe: safe)
+        @limit = p.unsafe_parse_expression(expr)
       end
     end
 

lib/liquid/tags/if.rb

@@ -73,8 +73,8 @@ module Liquid
       block.attach(new_body)
     end
 
-    def parse_expression(markup, safe: false)
+    def parse_expression(parser)
-      Condition.parse_expression(parse_context, markup, safe: safe)
+      Condition.parse_expression(parser)
     end
 
     def parse_markup(markup)
@@ -96,9 +96,9 @@ module Liquid
     end
 
     def parse_comparison(p)
-      a = parse_expression(p.expression, safe: true)
+      a = parse_expression(p)
       if (op = p.consume?(:comparison))
-        b = parse_expression(p.expression, safe: true)
+        b = parse_expression(p)
         Condition.new(a, op, b)
       else
         Condition.new(a)

lib/liquid/tags/render.rb

@@ -87,7 +87,7 @@ module Liquid
     def parse_markup(markup)
       p = @parse_context.new_parser(markup)
 
-      @template_name_expr = parse_expression(template_name(p), safe: true)
+      @template_name_expr = template_name(p)
       with_or_for         = p.id?("for") || p.id?("with")
       @variable_name_expr = p.expression_node if with_or_for
       @alias_name         = p.consume(:id) if p.id?("as")
@@ -107,7 +107,7 @@ module Liquid
     end
 
     def template_name(p)
-      p.consume(:string)
+      p.string
     end
 
     class ParseTreeVisitor < Liquid::ParseTreeVisitor

test/unit/condition_unit_test.rb

@@ -166,25 +166,24 @@ class ConditionUnitTest < Minitest::Test
     assert_includes(err.lines.map(&:strip), expected)
   end
 
-  def test_parse_expression_with_safe_true
+  def test_parse_expression
     environment = Environment.build
     parse_context = ParseContext.new(environment: environment)
-    result = Condition.parse_expression(parse_context, 'product.title', safe: true)
+    parser = parse_context.new_parser('product.title')
+    result = Condition.parse_expression(parser)
 
     assert_instance_of(VariableLookup, result)
     assert_equal('product', result.name)
     assert_equal(['title'], result.lookups)
   end
 
-  def test_parse_expression_raises_internal_error_if_not_safe
+  def test_parse_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)
 
-    error = assert_raises(Liquid::InternalError) do
+    assert_instance_of(Condition::MethodLiteral, result)
-      Condition.parse_expression(parse_context, 'product.title')
-    end
-
-    assert_match(/unsafe parse_expression cannot be used/, error.message)
   end
 
   # Tests for blank? comparison without ActiveSupport

test/unit/parse_context_unit_test.rb

@@ -5,7 +5,7 @@ require 'test_helper'
 class ParseContextUnitTest < Minitest::Test
   include Liquid
 
-  def test_safe_parse_expression_with_variable_lookup
+  def test_parser_expression_node_with_variable_lookup
     parser = parse_context.new_parser('product.title')
     result = parser.expression_node
 
@@ -14,7 +14,7 @@ class ParseContextUnitTest < Minitest::Test
     assert_equal(['title'], result.lookups)
   end
 
-  def test_safe_parse_expression_raises_syntax_error_for_invalid_expression
+  def test_parser_expression_node_raises_syntax_error_for_invalid_expression
     parser = parse_context.new_parser('')
 
     error = assert_raises(Liquid::SyntaxError) do
@@ -25,35 +25,14 @@ class ParseContextUnitTest < Minitest::Test
   end
 
   def test_parse_expression_with_variable_lookup
-    error = assert_raises(Liquid::InternalError) do
+    result = parse_context.new_parser('product.title').expression_node
-      parse_context.parse_expression('product.title')
-    end
-
-    assert_match(/unsafe parse_expression cannot be used/, error.message)
-  end
-
-  def test_parse_expression_with_safe_true
-    result = parse_context.parse_expression('product.title', safe: true)
 
     assert_instance_of(VariableLookup, result)
     assert_equal('product', result.name)
     assert_equal(['title'], result.lookups)
   end
 
-  def test_parse_expression_with_empty_string
+  def test_parser_expression_node_advances_parser_pointer
-    error = assert_raises(Liquid::InternalError) do
-      parse_context.parse_expression('')
-    end
-
-    assert_match(/unsafe parse_expression cannot be used/, error.message)
-  end
-
-  def test_parse_expression_with_empty_string_and_safe_true
-    result = parse_context.parse_expression('', safe: true)
-    assert_nil(result)
-  end
-
-  def test_safe_parse_expression_advances_parser_pointer
     parser = parse_context.new_parser('foo, bar')
 
     # parser.expression_node consumes "foo"
@@ -71,11 +50,6 @@ class ParseContextUnitTest < Minitest::Test
     parser.consume(:end_of_string)
   end
 
-  def test_parse_expression_with_whitespace
-    result = parse_context.parse_expression('   ', safe: true)
-    assert_nil(result)
-  end
-
   private
 
   def parse_context