Centralizing to_liquid

lib/liquid/context.rb

@@ -187,16 +187,11 @@ def find_variable(key, raise_on_not_found: true)
       # path and find_index() is optimized in MRI to reduce object allocation
       index = @scopes.find_index { |s| s.key?(key) }
 
-      variable = if index
+      if index
         lookup_and_evaluate(@scopes[index], key, raise_on_not_found: raise_on_not_found)
       else
         try_variable_find_in_environments(key, raise_on_not_found: raise_on_not_found)
       end
-
-      variable         = variable.to_liquid
-      variable.context = self if variable.respond_to?(:context=)
-
-      variable
     end
 
     def lookup_and_evaluate(obj, key, raise_on_not_found: true)
@@ -206,11 +201,17 @@ def lookup_and_evaluate(obj, key, raise_on_not_found: true)
 
       value = obj[key]
 
-      if value.is_a?(Proc) && obj.respond_to?(:[]=)
-        obj[key] = value.arity == 0 ? value.call : value.call(self)
-      else
-        value
+      # Skip contextualization and memoization when not found
+      return if value.nil?
+
+      value = contextualize(value)
+
+      # Memoization layer: Proc resolution and other to_liquid computations are persisted
+      if obj.respond_to?(:[]=) && !obj.frozen?
+        obj[key] = value
       end
+
+      value
     end
 
     def with_disabled_tags(tag_names)
@@ -228,6 +229,58 @@ def tag_disabled?(tag_name)
       @disabled_tags.fetch(tag_name, 0) > 0
     end
 
+    # Convert input objects into liquid aware representations
+    # Also assigns the context (self) through context=
+    def contextualize(object)
+      if object.is_a?(Proc)
+        object = object.arity == 0 ? object.call : object.call(self)
+      end
+
+      # TODO: This is a block of code that needs some extra polish
+      # My goal is to centralize as much as possible the contextualization/sanitization of objects being exchanged
+      #   between the inputs given by the caller and templates being executed.
+      # I desire for Filters to not have to worry about object conversion (eg.: StandardFilters#each)
+      #   Filters implemented outside Shopify/Liquid shouldn't have to worry about this layer of internals
+      #   Using a filter shouldn't create a worry for data leak and missing context
+      # Running `ruby -I test` with the following implementation is successful
+      # Running `rake test` which will also run tests with the liquid-c gem will lead to errors
+      #   This is due to liquid-c optimizing some code paths
+      #   Eg.: https://github.com/Shopify/liquid-c/blame/master/ext/liquid_c/context.h#L44-L45
+      #   We would need to also add the following code in liquid-c
+      # If we were to only consider Array and Hash as special use cases, this might be worth the effort
+      #   Alternatively I think considering Array and Hash as the only two cases of nested objects is not quite right
+      #   It might be better for extension.rb to be responsible for this. Array#to_liquid to return self is somewhat be wrong
+      #   One does not prevent the other, need to make sure the generic implementation works and we can optimize over it
+      # Note: Moving this logic to the different patches in extensions.rb
+      #   Some changes in liquid-c is most likely required
+      #   Liquid-c has early return optimization I have yet to track all code paths it relates to
+      #   if (klass == rb_cString || klass == rb_cArray || klass == rb_cHash)
+      #         return value;
+      if object.is_a?(Array)
+        object = object.map do |obj|
+          contextualize(obj)
+        end
+      elsif object.is_a?(Hash)
+        new_obj = {}
+        object.map do |k, obj|
+          new_obj[k] = contextualize(obj)
+        end
+        object = new_obj
+      else
+        object = object.to_liquid
+
+        # TODO: Ideally all contextualized object would define "context=" even if they perform a noop
+        # We want to ensure non-liquid objects aren't leaked in the context visible to the templates
+        # Having a standard interface is a direction we can take.
+        #   Alternatively, we could impose only a pre-determine array of available type is allowed
+        #   Eg.: String, Integer, Symbol, Liquid::Drop not SomeCustomModelFromTheHostApplication
+        # For now this might not be as pressing issue to deal with
+        object.context = self if object.respond_to?(:context=)
+      end
+
+      object
+    end
+
     protected
 
     attr_writer :base_scope_depth, :warnings, :errors, :strainer, :filters, :disabled_tags

lib/liquid/standardfilters.rb

@@ -582,8 +582,7 @@ def empty?
 
       def each
         @input.each do |e|
-          e.context = @context if e.respond_to?(:context=)
-          yield(e.respond_to?(:to_liquid) ? e.to_liquid : e)
+          yield(e)
         end
       end
     end

lib/liquid/variable_lookup.rb

@@ -49,15 +49,17 @@ def evaluate(context)
             ((object.respond_to?(:key?) && object.key?(key)) ||
              (object.respond_to?(:fetch) && key.is_a?(Integer)))
 
-          # if its a proc we will replace the entry with the proc
-          res    = context.lookup_and_evaluate(object, key)
-          object = res.to_liquid
+          object = context.lookup_and_evaluate(object, key)
 
           # Some special cases. If the part wasn't in square brackets and
           # no key with the same name was found we interpret following calls
           # as commands and call them on the current object
         elsif @command_flags & (1 << i) != 0 && object.respond_to?(key)
-          object = object.send(key).to_liquid
+
+          # TODO: These do not go through lookup_and_evaluate.
+          # Let's see if we can move the conversion back to Context
+          object = object.send(key)
+          object = context.contextualize(object)
 
           # No key was present with the desired value and it wasn't one of the directly supported
           # keywords either. The only thing we got left is to return nil or
@@ -66,9 +68,6 @@ def evaluate(context)
           return nil unless context.strict_variables
           raise Liquid::UndefinedVariable, "undefined variable #{key}"
         end
-
-        # If we are dealing with a drop here we have to
-        object.context = context if object.respond_to?(:context=)
       end
 
       object

test/integration/drop_test.rb

@@ -149,6 +149,8 @@ def test_context_drop
     assert_equal(' carrot ', output)
   end
 
+  # This test succeed in the ruby implementation, but not in liquid-c
+  # See Context#contextualize
   def test_context_drop_array_with_map
     output = Liquid::Template.parse(' {{ contexts | map: "bar" }} ').render!('contexts' => [ContextDrop.new, ContextDrop.new], 'bar' => "carrot")
     assert_equal(' carrotcarrot ', output)

test/integration/standard_filter_test.rb

@@ -417,6 +417,8 @@ def test_map_doesnt_call_arbitrary_stuff
     assert_template_result("", '{{ "foo" | map: "inspect" }}')
   end
 
+  # This test succeed in the ruby implementation, but not in liquid-c
+  # See Context#contextualize
   def test_map_calls_to_liquid
     t = TestThing.new
     assert_template_result("woot: 1", '{{ foo | map: "whatever" }}', "foo" => [t])
@@ -433,6 +435,8 @@ def test_legacy_map_on_hashes_with_dynamic_key
     assert_template_result("42", template, "thing" => hash)
   end
 
+  # This test succeed in the ruby implementation, but not in liquid-c
+  # See Context#contextualize
   def test_sort_calls_to_liquid
     t = TestThing.new
     Liquid::Template.parse('{{ foo | sort: "whatever" }}').render("foo" => [t])
@@ -861,7 +865,7 @@ def test_all_filters_never_raise_non_liquid_exception
       { foo: "bar" },
       [{ "foo" => "bar" }, { "foo" => 123 }, { "foo" => nil }, { "foo" => true }, { "foo" => ["foo", "bar"] }],
       { 1 => "bar" },
-      ["foo", 123, nil, true, false, Drop, ["foo"], { foo: "bar" }],
+      ["foo", 123, nil, true, false, test_drop, test_enum, ["foo"], { foo: "bar" }],
     ]
     StandardFilters.public_instance_methods(false).each do |method|
       arg_count = @filters.method(method).arity