remove Expression2 strscan workaround

Shopify · Jan 10, 2025 · 97cada6 · 97cada6
1 parent 7c9b77e
commit 97cada6
Show file tree

Hide file tree

Showing 4 changed files with 10 additions and 168 deletions.
diff --git a/lib/liquid/expression.rb b/lib/liquid/expression.rb
@@ -3,52 +3,7 @@
 require "lru_redux"
 
 module Liquid
-  class Expression1
-    LITERALS = {
-      nil => nil,
-      'nil' => nil,
-      'null' => nil,
-      '' => nil,
-      'true' => true,
-      'false' => false,
-      'blank' => '',
-      'empty' => ''
-    }.freeze
-
-    INTEGERS_REGEX       = /\A(-?\d+)\z/
-    FLOATS_REGEX         = /\A(-?\d[\d\.]+)\z/
-
-    # Use an atomic group (?>...) to avoid pathological backtracing from
-    # malicious input as described in https://github.com/Shopify/liquid/issues/1357
-    RANGES_REGEX         = /\A\(\s*(?>(\S+)\s*\.\.)\s*(\S+)\s*\)\z/
-
-    def self.parse(markup, _ss = nil, _cache = nil)
-      return nil unless markup
-
-      markup = markup.strip
-      if (markup.start_with?('"') && markup.end_with?('"')) ||
-         (markup.start_with?("'") && markup.end_with?("'"))
-        return markup[1..-2]
-      end
-
-      case markup
-      when INTEGERS_REGEX
-        Regexp.last_match(1).to_i
-      when RANGES_REGEX
-        RangeLookup.parse(Regexp.last_match(1), Regexp.last_match(2), nil)
-      when FLOATS_REGEX
-        Regexp.last_match(1).to_f
-      else
-        if LITERALS.key?(markup)
-          LITERALS[markup]
-        else
-          VariableLookup.parse(markup, nil)
-        end
-      end
-    end
-  end
-
-  class Expression2
+  class Expression
     LITERALS = {
       nil => nil,
       'nil' => nil,
@@ -161,7 +116,4 @@ def parse_number(markup, ss)
       end
     end
   end
-
-  # Remove this once we can depend on strscan >= 3.1.1
-  Expression = HAS_STRING_SCANNER_SCAN_BYTE ? Expression2 : Expression1
 end
diff --git a/performance/unit/expression_benchmark.rb b/performance/unit/expression_benchmark.rb
@@ -75,109 +75,20 @@
   "range" => RANGE_MARKUPS,
 }
 
-def compare_objects(object_1, object_2)
-  if object_1.is_a?(Liquid::VariableLookup) && object_2.is_a?(Liquid::VariableLookup)
-    return false if object_1.name != object_2.name
-  elsif object_1 != object_2
-    return false
-  end
-
-  true
-end
-
-def compare_range_lookup(expression_1_result, expression_2_result)
-  return false unless expression_1_result.is_a?(Liquid::RangeLookup) && expression_2_result.is_a?(Liquid::RangeLookup)
-
-  start_obj_1 = expression_1_result.start_obj
-  start_obj_2 = expression_2_result.start_obj
-
-  return false unless compare_objects(start_obj_1, start_obj_2)
-
-  end_obj_1 = expression_1_result.end_obj
-  end_obj_2 = expression_2_result.end_obj
-
-  return false unless compare_objects(end_obj_1, end_obj_2)
-
-  true
-end
-
-MARKUPS.values.flatten.each do |markup|
-  expression_1_result = Liquid::Expression1.parse(markup)
-  expression_2_result = Liquid::Expression2.parse(markup)
-
-  next if expression_1_result == expression_2_result
-
-  if expression_1_result.is_a?(Liquid::RangeLookup) && expression_2_result.is_a?(Liquid::RangeLookup)
-    next if compare_range_lookup(expression_1_result, expression_2_result)
-  end
-
-  warn "Expression1 and Expression2 results are different for markup: #{markup}"
-  warn "expected: #{expression_1_result}"
-  warn "got: #{expression_2_result}"
-  abort
-end
-
-warmed_up = false
-
-MARKUPS.each do |type, markups|
-  Benchmark.ips do |x|
-    if warmed_up
-      x.config(time: 10, warmup: 5)
-      warmed_up = true
-    else
-      x.config(time: 10)
-    end
-
-    x.report("Liquid::Expression1#parse: #{type}") do
-      if Liquid::Expression != Liquid::Expression1
-        Liquid.send(:remove_const, :Expression)
-        Liquid.const_set(:Expression, Liquid::Expression1)
-      end
-
-      markups.each do |markup|
-        Liquid::Expression1.parse(markup)
-      end
-    end
-
-    x.report("Liquid::Expression2#parse: #{type}") do
-      if Liquid::Expression != Liquid::Expression2
-        Liquid.send(:remove_const, :Expression)
-        Liquid.const_set(:Expression, Liquid::Expression2)
-      end
+Benchmark.ips do |x|
+  x.config(time: 5, warmup: 5)
 
+  MARKUPS.each do |type, markups|
+    x.report("Liquid::Expression#parse: #{type}") do
       markups.each do |markup|
-        Liquid::Expression2.parse(markup)
+        Liquid::Expression.parse(markup)
       end
     end
-
-    x.compare!
-  end
-end
-
-Benchmark.ips do |x|
-  x.config(time: 10)
-
-  x.report("Liquid::Expression1#parse: all") do
-    if Liquid::Expression != Liquid::Expression1
-      Liquid.send(:remove_const, :Expression)
-      Liquid.const_set(:Expression, Liquid::Expression1)
-    end
-
-    MARKUPS.values.flatten.each do |markup|
-      Liquid::Expression1.parse(markup)
-    end
   end
 
-  x.report("Liquid::Expression2#parse: all") do
-    if Liquid::Expression != Liquid::Expression2
-      Liquid.send(:remove_const, :Expression)
-      Liquid.const_set(:Expression, Liquid::Expression2)
-    end
-
+  x.report("Liquid::Expression#parse: all") do
     MARKUPS.values.flatten.each do |markup|
-      Liquid::Expression2.parse(markup)
+      Liquid::Expression.parse(markup)
     end
   end
-
-  x.compare!
 end
diff --git a/performance/unit/lexer_benchmark.rb b/performance/unit/lexer_benchmark.rb
@@ -29,31 +29,12 @@
   "foo | default: -1",
 ]
 
-EXPRESSIONS.each do |expr|
-  lexer_1_result = Liquid::Lexer1.new(expr).tokenize
-  lexer_2_result = Liquid::Lexer2.new(expr).tokenize
-
-  next if lexer_1_result == lexer_2_result
-
-  warn "Lexer1 and Lexer2 results are different for expression: #{expr}"
-  warn "expected: #{lexer_1_result}"
-  warn "got: #{lexer_2_result}"
-  abort
-end
-
 Benchmark.ips do |x|
   x.config(time: 10, warmup: 5)
 
-  x.report("Liquid::Lexer1#tokenize") do
-    EXPRESSIONS.each do |expr|
-      l = Liquid::Lexer1.new(expr)
-      l.tokenize
-    end
-  end
-
-  x.report("Liquid::Lexer2#tokenize") do
+  x.report("Liquid::Lexer#tokenize") do
     EXPRESSIONS.each do |expr|
-      l = Liquid::Lexer2.new(expr)
+      l = Liquid::Lexer.new(expr)
       l.tokenize
     end
   end

diff --git a/test/integration/expression_test.rb b/test/integration/expression_test.rb
@@ -57,7 +57,6 @@ def test_quirky_negative_sign_expression_markup
 
   def test_expression_cache
     skip("Liquid-C does not support Expression caching") if defined?(Liquid::C) && Liquid::C.enabled
-    skip("Expression Caching is only available with Expression2") if Liquid::Expression != Liquid::Expression2
 
     cache = LruRedux::Cache.new(10)
     template = <<~LIQUID
@@ -79,7 +78,6 @@ def test_expression_cache
 
   def test_disable_expression_cache
     skip("Liquid-C does not support Expression caching") if defined?(Liquid::C) && Liquid::C.enabled
-    skip("Expression Caching is only available with Expression2") if Liquid::Expression != Liquid::Expression2
 
     template = <<~LIQUID
       {% assign x = 1 %}