Use symbols for filter names now that ruby has symbol GC

lib/liquid/strainer_template.rb

@@ -24,17 +24,17 @@ module Liquid
 
         include(filter)
 
-        filter_methods.merge(filter.public_instance_methods.map(&:to_s))
+        filter.public_instance_methods.each { |name| filter_methods[name] = true }
       end
 
       def invokable?(method)
-        filter_methods.include?(method.to_s)
+        filter_methods.key?(method.to_sym)
       end
 
       private
 
       def filter_methods
-        @filter_methods ||= Set.new
+        @filter_methods ||= {}
       end
     end
 

lib/liquid/variable.rb

@@ -123,7 +123,7 @@ module Liquid
           filter_args << Expression.parse(a)
         end
       end
-      result = [filter_name, filter_args]
+      result = [filter_name.to_sym, filter_args]
       result << keyword_args if keyword_args
       result
     end

test/integration/security_test.rb

@@ -43,15 +43,22 @@ class SecurityTest < Minitest::Test
     assert_equal(expected, Template.parse(text).render!(@assigns, filters: SecurityFilter))
   end
 
-  def test_does_not_add_filters_to_symbol_table
+  def test_does_not_permanently_add_filters_to_symbol_table
     current_symbols = Symbol.all_symbols
 
-    test = %( {{ "some_string" | a_bad_filter }} )
+    # MRI imprecisely marks objects found on the C stack, which can result
+    # in uninitialized memory being marked. This can even result in the test failing
+    # deterministically for a given compilation of ruby. Using a separate thread will
+    # keep these writes of the symbol pointer on a separate stack that will be garbage
+    # collected after Thread#join.
+    Thread.new do
+      test = %( {{ "some_string" | a_bad_filter }} )
+      Template.parse(test).render!
+      nil
+    end.join
 
-    template = Template.parse(test)
+    GC.start
-    assert_equal([], (Symbol.all_symbols - current_symbols))
 
-    template.render!
     assert_equal([], (Symbol.all_symbols - current_symbols))
   end
 

test/unit/strainer_template_unit_test.rb

@@ -60,7 +60,7 @@ class StrainerTemplateUnitTest < Minitest::Test
     strainer = Context.new.strainer
     with_global_filter do
       strainer.class.add_filter(PublicMethodOverrideFilter)
-      assert(strainer.class.send(:filter_methods).include?('public_filter'))
+      assert(strainer.class.send(:filter_methods).include?(:public_filter))
     end
   end
 

test/unit/variable_unit_test.rb

@@ -13,75 +13,75 @@ class VariableUnitTest < Minitest::Test
   def test_filters
     var = create_variable('hello | textileze')
     assert_equal(VariableLookup.new('hello'), var.name)
-    assert_equal([['textileze', []]], var.filters)
+    assert_equal([[:textileze, []]], var.filters)
 
     var = create_variable('hello | textileze | paragraph')
     assert_equal(VariableLookup.new('hello'), var.name)
-    assert_equal([['textileze', []], ['paragraph', []]], var.filters)
+    assert_equal([[:textileze, []], [:paragraph, []]], var.filters)
 
     var = create_variable(%( hello | strftime: '%Y'))
     assert_equal(VariableLookup.new('hello'), var.name)
-    assert_equal([['strftime', ['%Y']]], var.filters)
+    assert_equal([[:strftime, ['%Y']]], var.filters)
 
     var = create_variable(%( 'typo' | link_to: 'Typo', true ))
     assert_equal('typo', var.name)
-    assert_equal([['link_to', ['Typo', true]]], var.filters)
+    assert_equal([[:link_to, ['Typo', true]]], var.filters)
 
     var = create_variable(%( 'typo' | link_to: 'Typo', false ))
     assert_equal('typo', var.name)
-    assert_equal([['link_to', ['Typo', false]]], var.filters)
+    assert_equal([[:link_to, ['Typo', false]]], var.filters)
 
     var = create_variable(%( 'foo' | repeat: 3 ))
     assert_equal('foo', var.name)
-    assert_equal([['repeat', [3]]], var.filters)
+    assert_equal([[:repeat, [3]]], var.filters)
 
     var = create_variable(%( 'foo' | repeat: 3, 3 ))
     assert_equal('foo', var.name)
-    assert_equal([['repeat', [3, 3]]], var.filters)
+    assert_equal([[:repeat, [3, 3]]], var.filters)
 
     var = create_variable(%( 'foo' | repeat: 3, 3, 3 ))
     assert_equal('foo', var.name)
-    assert_equal([['repeat', [3, 3, 3]]], var.filters)
+    assert_equal([[:repeat, [3, 3, 3]]], var.filters)
 
     var = create_variable(%( hello | strftime: '%Y, okay?'))
     assert_equal(VariableLookup.new('hello'), var.name)
-    assert_equal([['strftime', ['%Y, okay?']]], var.filters)
+    assert_equal([[:strftime, ['%Y, okay?']]], var.filters)
 
     var = create_variable(%( hello | things: "%Y, okay?", 'the other one'))
     assert_equal(VariableLookup.new('hello'), var.name)
-    assert_equal([['things', ['%Y, okay?', 'the other one']]], var.filters)
+    assert_equal([[:things, ['%Y, okay?', 'the other one']]], var.filters)
   end
 
   def test_filter_with_date_parameter
     var = create_variable(%( '2006-06-06' | date: "%m/%d/%Y"))
     assert_equal('2006-06-06', var.name)
-    assert_equal([['date', ['%m/%d/%Y']]], var.filters)
+    assert_equal([[:date, ['%m/%d/%Y']]], var.filters)
   end
 
   def test_filters_without_whitespace
     var = create_variable('hello | textileze | paragraph')
     assert_equal(VariableLookup.new('hello'), var.name)
-    assert_equal([['textileze', []], ['paragraph', []]], var.filters)
+    assert_equal([[:textileze, []], [:paragraph, []]], var.filters)
 
     var = create_variable('hello|textileze|paragraph')
     assert_equal(VariableLookup.new('hello'), var.name)
-    assert_equal([['textileze', []], ['paragraph', []]], var.filters)
+    assert_equal([[:textileze, []], [:paragraph, []]], var.filters)
 
     var = create_variable("hello|replace:'foo','bar'|textileze")
     assert_equal(VariableLookup.new('hello'), var.name)
-    assert_equal([['replace', ['foo', 'bar']], ['textileze', []]], var.filters)
+    assert_equal([[:replace, ['foo', 'bar']], [:textileze, []]], var.filters)
   end
 
   def test_symbol
     var = create_variable("http://disney.com/logo.gif | image: 'med' ", error_mode: :lax)
     assert_equal(VariableLookup.new('http://disney.com/logo.gif'), var.name)
-    assert_equal([['image', ['med']]], var.filters)
+    assert_equal([[:image, ['med']]], var.filters)
   end
 
   def test_string_to_filter
     var = create_variable("'http://disney.com/logo.gif' | image: 'med' ")
     assert_equal('http://disney.com/logo.gif', var.name)
-    assert_equal([['image', ['med']]], var.filters)
+    assert_equal([[:image, ['med']]], var.filters)
   end
 
   def test_string_single_quoted
@@ -128,13 +128,13 @@ class VariableUnitTest < Minitest::Test
   def test_filter_with_keyword_arguments
     var = create_variable(%( hello | things: greeting: "world", farewell: 'goodbye'))
     assert_equal(VariableLookup.new('hello'), var.name)
-    assert_equal([['things', [], { 'greeting' => 'world', 'farewell' => 'goodbye' }]], var.filters)
+    assert_equal([[:things, [], { 'greeting' => 'world', 'farewell' => 'goodbye' }]], var.filters)
   end
 
   def test_lax_filter_argument_parsing
     var = create_variable(%( number_of_comments | pluralize: 'comment': 'comments' ), error_mode: :lax)
     assert_equal(VariableLookup.new('number_of_comments'), var.name)
-    assert_equal([['pluralize', ['comment', 'comments']]], var.filters)
+    assert_equal([[:pluralize, ['comment', 'comments']]], var.filters)
   end
 
   def test_strict_filter_argument_parsing