Fix leaking error when comparison leads to TypeError

.travis.yml

@@ -19,10 +19,6 @@ matrix:
   allow_failures:
     - rvm: jruby-head
 
-install:
-  - gem install rainbow -v 2.2.1
-  - bundle install
-
 script: "bundle exec rake"
 
 notifications:

Gemfile

@@ -3,12 +3,9 @@ source 'https://rubygems.org'
 gemspec
 gem 'stackprof', platforms: :mri_21
 
-group :benchmark, :test do
-  gem 'benchmark-ips'
-end
-
 group :test do
   gem 'spy', '0.4.1'
+  gem 'benchmark-ips'
   gem 'rubocop', '0.34.2'
 
   platform :mri do

History.md

@@ -20,13 +20,10 @@
 * Add concat filter to concatenate arrays (#429) [Diogo Beato]
 * Ruby 1.9 support dropped (#491) [Justin Li]
 * Liquid::Template.file_system's read_template_file method is no longer passed the context. (#441) [James Reid-Smith]
-* Remove `liquid_methods` (See https://github.com/Shopify/liquid/pull/568 for replacement)
+* Remove support for `liquid_methods`
 * Liquid::Template.register_filter raises when the module overrides registered public methods as private or protected (#705) [Gaurav Chande]
 
 ### Fixed
-
-* Fix variable names being detected as an operator when starting with contains (#788) [Michael Angell]
-* Fix include tag used with strict_variables (#828) [QuickPay]
 * Fix map filter when value is a Proc (#672) [Guillaume Malette]
 * Fix truncate filter when value is not a string (#672) [Guillaume Malette]
 * Fix behaviour of escape filter when input is nil (#665) [Tanel Jakobsoo]

lib/liquid/block_body.rb

@@ -93,11 +93,10 @@ module Liquid
         rescue MemoryError => e
           raise e
         rescue UndefinedVariable, UndefinedDropMethod, UndefinedFilter => e
-          context.handle_error(e, token.line_number)
+          context.handle_error(e, token.line_number, token.raw)
           output << nil
         rescue ::StandardError => e
-          line_number = token.is_a?(String) ? nil : token.line_number
-          output << context.handle_error(e, line_number)
+          output << context.handle_error(e, token.line_number, token.raw)
         end
       end
 
@@ -107,7 +106,7 @@ module Liquid
     private
 
     def render_node(node, context)
-      node_output = node.is_a?(String) ? node : node.render(context)
+      node_output = (node.respond_to?(:render) ? node.render(context) : node)
       node_output = node_output.is_a?(Array) ? node_output.join : node_output.to_s
 
       context.resource_limits.render_length += node_output.length

lib/liquid/condition.rb

@@ -7,7 +7,6 @@ module Liquid
   #   c.evaluate #=> true
   #
   class Condition #:nodoc:
-    @@depth = 0
     @@operators = {
       '=='.freeze => ->(cond, left, right) {  cond.send(:equal_variables, left, right) },
       '!='.freeze => ->(cond, left, right) { !cond.send(:equal_variables, left, right) },
@@ -48,11 +47,6 @@ module Liquid
       when :or
         result || @child_condition.evaluate(context)
       when :and
-        @@depth += 1
-        if @@depth >= 500
-          @@depth = 0
-          raise StackLevelError, "Nesting too deep".freeze
-        end
         result && @child_condition.evaluate(context)
       else
         result
@@ -116,10 +110,10 @@ module Liquid
 
       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)
+      elsif left.respond_to?(operation) && right.respond_to?(operation)
         begin
           left.send(operation, right)
-        rescue ::ArgumentError => e
+        rescue ::ArgumentError, TypeError => e
           raise Liquid::ArgumentError.new(e.message)
         end
       end

lib/liquid/context.rb

@@ -74,7 +74,7 @@ module Liquid
       @interrupts.pop
     end
 
-    def handle_error(e, line_number = nil)
+    def handle_error(e, line_number = nil, raw_token = nil)
       e = internal_error unless e.is_a?(Liquid::Error)
       e.template_name ||= template_name
       e.line_number ||= line_number
@@ -160,7 +160,7 @@ module Liquid
     end
 
     # Fetches an object starting at the local scope and then moving up the hierachy
-    def find_variable(key, raise_on_not_found: true)
+    def find_variable(key)
       # This was changed from find() to find_index() because this is a very hot
       # path and find_index() is optimized in MRI to reduce object allocation
       index = @scopes.find_index { |s| s.key?(key) }
@@ -170,7 +170,7 @@ module Liquid
 
       if scope.nil?
         @environments.each do |e|
-          variable = lookup_and_evaluate(e, key, raise_on_not_found: raise_on_not_found)
+          variable = lookup_and_evaluate(e, key)
           unless variable.nil?
             scope = e
             break
@@ -179,7 +179,7 @@ module Liquid
       end
 
       scope ||= @environments.last || @scopes.last
-      variable ||= lookup_and_evaluate(scope, key, raise_on_not_found: raise_on_not_found)
+      variable ||= lookup_and_evaluate(scope, key)
 
       variable = variable.to_liquid
       variable.context = self if variable.respond_to?(:context=)
@@ -187,8 +187,8 @@ module Liquid
       variable
     end
 
-    def lookup_and_evaluate(obj, key, raise_on_not_found: true)
-      if @strict_variables && raise_on_not_found && obj.respond_to?(:key?) && !obj.key?(key)
+    def lookup_and_evaluate(obj, key)
+      if @strict_variables && obj.respond_to?(:key?) && !obj.key?(key)
         raise Liquid::UndefinedVariable, "undefined variable #{key}"
       end
 

lib/liquid/lexer.rb

@@ -18,10 +18,10 @@ module Liquid
     DOUBLE_STRING_LITERAL = /"[^\"]*"/
     NUMBER_LITERAL = /-?\d+(\.\d+)?/
     DOTDOT = /\.\./
-    COMPARISON_OPERATOR = /==|!=|<>|<=?|>=?|contains(?=\s)/
+    COMPARISON_OPERATOR = /==|!=|<>|<=?|>=?|contains/
 
     def initialize(input)
-      @ss = StringScanner.new(input)
+      @ss = StringScanner.new(input.rstrip)
     end
 
     def tokenize
@@ -29,7 +29,6 @@ module Liquid
 
       until @ss.eos?
         @ss.skip(/\s*/)
-        break if @ss.eos?
         tok = case
         when t = @ss.scan(COMPARISON_OPERATOR) then [:comparison, t]
         when t = @ss.scan(SINGLE_STRING_LITERAL) then [:string, t]

lib/liquid/standardfilters.rb

@@ -384,7 +384,7 @@ module Liquid
       end
 
       def join(glue)
-        to_a.join(glue.to_s)
+        to_a.join(glue)
       end
 
       def concat(args)

lib/liquid/strainer.rb

@@ -27,7 +27,7 @@ module Liquid
 
     def self.add_filter(filter)
       raise ArgumentError, "Expected module but got: #{filter.class}" unless filter.is_a?(Module)
-      unless self.include?(filter)
+      unless self.class.include?(filter)
         invokable_non_public_methods = (filter.private_instance_methods + filter.protected_instance_methods).select { |m| invokable?(m) }
         if invokable_non_public_methods.any?
           raise MethodOverrideError, "Filter overrides registered public methods as non public: #{invokable_non_public_methods.join(', ')}"

lib/liquid/tags/for.rb

@@ -23,7 +23,7 @@ module Liquid
   #      {{ item.name }}
   #    {% end %}
   #
-  #  To reverse the for loop simply use {% for item in collection reversed %} (note that the flag's spelling is different to the filter `reverse`)
+  #  To reverse the for loop simply use {% for item in collection reversed %}
   #
   # == Available variables:
   #

lib/liquid/tags/include.rb

@@ -50,7 +50,7 @@ module Liquid
       variable = if @variable_name_expr
         context.evaluate(@variable_name_expr)
       else
-        context.find_variable(template_name, raise_on_not_found: false)
+        context.find_variable(template_name)
       end
 
       old_template_name = context.template_name

performance/benchmark.rb

@@ -5,7 +5,7 @@ Liquid::Template.error_mode = ARGV.first.to_sym if ARGV.first
 profiler = ThemeRunner.new
 
 Benchmark.ips do |x|
-  x.time = 10
+  x.time = 60
   x.warmup = 5
 
   puts
@@ -13,6 +13,5 @@ Benchmark.ips do |x|
   puts
 
   x.report("parse:") { profiler.compile }
-  x.report("render:") { profiler.render }
-  x.report("parse & render:") { profiler.run }
+  x.report("parse & run:") { profiler.run }
 end

performance/theme_runner.rb

@@ -21,100 +21,53 @@ class ThemeRunner
     end
   end
 
-  # Initialize a new liquid ThemeRunner instance
-  # Will load all templates into memory, do this now so that we don't profile IO.
+  # Load all templates into memory, do this now so that
+  # we don't profile IO.
   def initialize
     @tests = Dir[__dir__ + '/tests/**/*.liquid'].collect do |test|
       next if File.basename(test) == 'theme.liquid'
 
       theme_path = File.dirname(test) + '/theme.liquid'
-      {
-        liquid: File.read(test),
-        layout: (File.file?(theme_path) ? File.read(theme_path) : nil),
-        template_name: test
-      }
-    end.compact
 
-    compile_all_tests
+      [File.read(test), (File.file?(theme_path) ? File.read(theme_path) : nil), test]
+    end.compact
   end
 
-  # `compile` will test just the compilation portion of liquid without any templates
   def compile
-    @tests.each do |test_hash|
-      Liquid::Template.new.parse(test_hash[:liquid])
-      Liquid::Template.new.parse(test_hash[:layout])
+    # Dup assigns because will make some changes to them
+
+    @tests.each do |liquid, layout, template_name|
+      tmpl = Liquid::Template.new
+      tmpl.parse(liquid)
+      tmpl = Liquid::Template.new
+      tmpl.parse(layout)
     end
   end
 
-  # `run` is called to benchmark rendering and compiling at the same time
   def run
-    each_test do |liquid, layout, assigns, page_template, template_name|
+    # Dup assigns because will make some changes to them
+    assigns = Database.tables.dup
+
+    @tests.each do |liquid, layout, template_name|
+      # Compute page_tempalte outside of profiler run, uninteresting to profiler
+      page_template = File.basename(template_name, File.extname(template_name))
       compile_and_render(liquid, layout, assigns, page_template, template_name)
     end
   end
 
-  # `render` is called to benchmark just the render portion of liquid
-  def render
-    @compiled_tests.each do |test|
-      tmpl = test[:tmpl]
-      assigns = test[:assigns]
-      layout = test[:layout]
-
-      if layout
-        assigns['content_for_layout'] = tmpl.render!(assigns)
-        layout.render!(assigns)
-      else
-        tmpl.render!(assigns)
-      end
-    end
-  end
-
-  private
-
   def compile_and_render(template, layout, assigns, page_template, template_file)
-    compiled_test = compile_test(template, layout, assigns, page_template, template_file)
-    assigns['content_for_layout'] = compiled_test[:tmpl].render!(assigns)
-    compiled_test[:layout].render!(assigns) if layout
-  end
-
-  def compile_all_tests
-    @compiled_tests = []
-    each_test do |liquid, layout, assigns, page_template, template_name|
-      @compiled_tests << compile_test(liquid, layout, assigns, page_template, template_name)
-    end
-    @compiled_tests
-  end
-
-  def compile_test(template, layout, assigns, page_template, template_file)
-    tmpl = init_template(page_template, template_file)
-    parsed_template = tmpl.parse(template).dup
-
-    if layout
-      parsed_layout = tmpl.parse(layout)
-      { tmpl: parsed_template, assigns: assigns, layout: parsed_layout }
-    else
-      { tmpl: parsed_template, assigns: assigns }
-    end
-  end
-
-  # utility method with similar functionality needed in `compile_all_tests` and `run`
-  def each_test
-    # Dup assigns because will make some changes to them
-    assigns = Database.tables.dup
-
-    @tests.each do |test_hash|
-      # Compute page_template outside of profiler run, uninteresting to profiler
-      page_template = File.basename(test_hash[:template_name], File.extname(test_hash[:template_name]))
-      yield(test_hash[:liquid], test_hash[:layout], assigns, page_template, test_hash[:template_name])
-    end
-  end
-
-  # set up a new Liquid::Template object for use in `compile_and_render` and `compile_test`
-  def init_template(page_template, template_file)
     tmpl = Liquid::Template.new
     tmpl.assigns['page_title'] = 'Page title'
     tmpl.assigns['template'] = page_template
     tmpl.registers[:file_system] = ThemeRunner::FileSystem.new(File.dirname(template_file))
-    tmpl
+
+    content_for_layout = tmpl.parse(template).render!(assigns)
+
+    if layout
+      assigns['content_for_layout'] = content_for_layout
+      tmpl.parse(layout).render!(assigns)
+    else
+      content_for_layout
+    end
   end
 end

test/integration/parsing_quirks_test.rb

@@ -115,8 +115,4 @@ class ParsingQuirksTest < Minitest::Test
       assert_template_result('12345', "{% for i in (1...5) %}{{ i }}{% endfor %}")
     end
   end
-
-  def test_contains_in_id
-    assert_template_result(' YES ', '{% if containsallshipments == true %} YES {% endif %}', 'containsallshipments' => true)
-  end
 end # ParsingQuirksTest

test/integration/standard_filter_test.rb

@@ -170,7 +170,6 @@ class StandardFiltersTest < Minitest::Test
   def test_join
     assert_equal '1 2 3 4', @filters.join([1, 2, 3, 4])
     assert_equal '1 - 2 - 3 - 4', @filters.join([1, 2, 3, 4], ' - ')
-    assert_equal '1121314', @filters.join([1, 2, 3, 4], 1)
   end
 
   def test_sort

test/integration/tags/include_tag_test.rb

@@ -235,11 +235,4 @@ class IncludeTagTest < Minitest::Test
 
     assert_template_result "Product: Draft 151cm ", "{% assign page = 'product' %}{% include page for foo %}", "foo" => { 'title' => 'Draft 151cm' }
   end
-
-  def test_including_with_strict_variables
-    template = Liquid::Template.parse("{% include 'simple' %}", error_mode: :warn)
-    template.render(nil, strict_variables: true)
-
-    assert_equal [], template.errors
-  end
 end # IncludeTagTest

test/unit/condition_unit_test.rb

@@ -57,6 +57,11 @@ class ConditionUnitTest < Minitest::Test
     assert_evaluates_argument_error 1, '~~', 0
   end
 
+  def test_comparing_hash_and_integer
+    assert_evaluates_argument_error({a: 1}, '>', 1)
+    assert_evaluates_argument_error(1, '>', {a: 1})
+  end
+
   def test_comparation_of_int_and_str
     assert_evaluates_argument_error '1', '>', 0
     assert_evaluates_argument_error '1', '<', 0
@@ -64,14 +69,6 @@ class ConditionUnitTest < Minitest::Test
     assert_evaluates_argument_error '1', '<=', 0
   end
 
-  def test_hash_compare_backwards_compatibility
-    assert_equal nil, Condition.new({}, '>', 2).evaluate
-    assert_equal nil, Condition.new(2, '>', {}).evaluate
-    assert_equal false, Condition.new({}, '==', 2).evaluate
-    assert_equal true, Condition.new({ 'a' => 1 }, '==', { 'a' => 1 }).evaluate
-    assert_equal true, Condition.new({ 'a' => 2 }, 'contains', 'a').evaluate
-  end
-
   def test_contains_works_on_arrays
     @context = Liquid::Context.new
     @context['array'] = [1, 2, 3, 4, 5]
@@ -130,17 +127,6 @@ class ConditionUnitTest < Minitest::Test
     assert_equal false, condition.evaluate
   end
 
-  def test_maximum_recursion_depth
-    condition = Condition.new(1, '==', 1)
-
-    assert_raises(Liquid::StackLevelError) do
-      (1..510).each do
-        condition.evaluate
-        condition.and Condition.new(2, '==', 2)
-      end
-    end
-  end
-
   def test_should_allow_custom_proc_operator
     Condition.operators['starts_with'] = proc { |cond, left, right| left =~ %r{^#{right}} }
 

test/unit/lexer_unit_test.rb

@@ -19,7 +19,7 @@ class LexerUnitTest < Minitest::Test
   end
 
   def test_comparison
-    tokens = Lexer.new('== <> contains ').tokenize
+    tokens = Lexer.new('== <> contains').tokenize
     assert_equal [[:comparison, '=='], [:comparison, '<>'], [:comparison, 'contains'], [:end_of_string]], tokens
   end
 

test/unit/strainer_unit_test.rb

@@ -145,20 +145,4 @@ class StrainerUnitTest < Minitest::Test
     Strainer.global_filter(LateAddedFilter)
     assert_equal 'filtered', Strainer.create(nil).invoke('late_added_filter', 'input')
   end
-
-  def test_add_filter_does_not_include_already_included_module
-    mod = Module.new do
-      class << self
-        attr_accessor :include_count
-        def included(mod)
-          self.include_count += 1
-        end
-      end
-      self.include_count = 0
-    end
-    strainer = Context.new.strainer
-    strainer.class.add_filter(mod)
-    strainer.class.add_filter(mod)
-    assert_equal 1, mod.include_count
-  end
 end # StrainerTest