Get attribute fixes

lib/twig/extension/core.rb

@@ -5,6 +5,10 @@ module Extension
     class Core < Base
       DEFAULT_TRIM_CHARS = " \t\n\r\0\x0B"
 
+      # Returned by subscript_key when nothing matched. A sentinel rather than
+      # nil, because nil is a perfectly good hash key and a perfectly good value.
+      KEY_NOT_FOUND = Object.new.freeze
+
       class << self
         include ActiveSupport::NumberHelper
       end
@@ -860,25 +864,64 @@ def self.matches(regexp, string)
         raise Error::Runtime, "Invalid regular expression passed to matches: #{e.message}"
       end
 
+      # The key or index that +attribute+ matches on a subscriptable object, or
+      # KEY_NOT_FOUND when it matches none.
+      #
+      # Returning the key rather than the value is the point: it lets the caller
+      # subscript exactly once. Fetching first and testing the result instead —
+      # as `object[attribute] || object[attribute.to_s]` did — can't tell a key
+      # holding nil or false from a key that isn't there, so { 'billable' =>
+      # false } came back as nil.
+      #
+      # @return [Object] the matching key, or KEY_NOT_FOUND
+      def self.subscript_key(object, attribute)
+        if object.is_a?(Array)
+          return KEY_NOT_FOUND unless attribute.is_a?(Integer)
+
+          # Ruby indexes from the end with a negative, so -1 is the last element
+          # while anything below -length is as absent as anything above length.
+          in_bounds = attribute >= -object.length && attribute < object.length
+
+          return in_bounds ? attribute : KEY_NOT_FOUND
+        end
+
+        return KEY_NOT_FOUND unless object.respond_to?(:key?)
+        return attribute if object.key?(attribute)
+
+        # A template writes h.foo, h['foo'] and h[:foo] for the same Ruby hash,
+        # so both forms of the name count as a match.
+        symbol = attribute.to_sym if attribute.respond_to?(:to_sym)
+        return symbol if !symbol.nil? && object.key?(symbol)
+
+        string = attribute.to_s if attribute.respond_to?(:to_s)
+        return string if !string.nil? && object.key?(string)
+
+        KEY_NOT_FOUND
+      end
+
+      # Names the keys that do exist rather than dumping the object, which for
+      # anything the size of a request's parameters buries the one absent key
+      # under everything else.
+      #
+      # @return [String]
+      def self.key_not_found_message(object, attribute)
+        keys = object.respond_to?(:keys) ? " with keys \"#{object.keys.join(', ')}\"" : ''
+
+        "Key \"#{attribute}\" for sequence/mapping#{keys} does not exist."
+      end
+
       # @param [Environment] environment
       def self.get_attribute(
         environment, source, object, attribute, type, arguments: {}, defined_test: false,
         ignore_strict_check: false, lineno: -1, &
       )
         if type == Template::ARRAY_CALL || object.respond_to?(:[])
-          if object.respond_to?(:[]) && (
-            (object.is_a?(Array) && attribute.is_a?(Integer) && attribute < object.length) ||
-            (
-              object.respond_to?(:key?) && (
-                object.key?(attribute) ||
-                (attribute.respond_to?(:to_sym) && object.key?(attribute.to_sym)) ||
-                  (attribute.respond_to?(:to_s) && object.key?(attribute.to_s))
-              )
-            )
-          )
+          key = object.respond_to?(:[]) ? subscript_key(object, attribute) : KEY_NOT_FOUND
+
+          unless key.equal?(KEY_NOT_FOUND)
             return true if defined_test
 
-            return object[attribute] || (attribute.is_a?(String) ? object[attribute.to_sym] : object[attribute.to_s])
+            return object[key]
           end
 
           if type == Template::ARRAY_CALL
@@ -890,7 +933,11 @@ def self.get_attribute(
               return
             end
 
-            raise Error::Runtime, "Can't find key #{attribute} in #{object.inspect}."
+            raise Error::Runtime.new(
+              key_not_found_message(object, attribute),
+              lineno,
+              source
+            )
           end
         end
 
@@ -925,19 +972,22 @@ def self.get_attribute(
 
           kwargs = kwargs.transform_keys(&:to_sym)
 
+          # public_send, not send: respond_to? above already excludes private and
+          # protected methods, but an object with a loose respond_to_missing?
+          # can report true for one. Templates only reach the public interface.
           if positional.length.positive? && kwargs.empty?
-            object.send(attribute, *positional, &)
+            object.public_send(attribute, *positional, &)
           elsif positional.empty? && kwargs.length.positive?
-            object.send(attribute, **kwargs, &)
+            object.public_send(attribute, **kwargs, &)
           elsif positional.length.positive? && kwargs.length.positive?
-            object.send(attribute, *positional, **kwargs, &)
+            object.public_send(attribute, *positional, **kwargs, &)
           else
-            case object
-            when Hash, Array
-              object[attribute]
-            else
-              object.send(attribute, &)
-            end
+            # Reached only when the object responds to the attribute and the
+            # key/index lookup above did not match, so subscripting a Hash or
+            # Array here could only ever miss — it raised TypeError on arrays
+            # ({{ list.any? }}) and returned nil on hashes, silently rendering
+            # nothing. Collections dispatch methods like any other object.
+            object.public_send(attribute, &)
           end
         # Constant could be nil but we should return if we find it
         elsif (constant = get_constant(object, attribute.to_s)) && constant[0] == :found
@@ -952,8 +1002,7 @@ def self.get_attribute(
           message = if object.nil?
                       "Impossible to access an attribute (\"#{attribute}\") on a null variable."
                     elsif object.respond_to?(:[]) && !object.is_a?(String)
-                      keys = object.respond_to?(:keys) ? "with keys \"#{object.keys.join(', ')}\"" : ''
-                      "Key \"#{attribute}\" for sequence/mapping #{keys} does not exist."
+                      key_not_found_message(object, attribute)
                     else
                       "Impossible to access an attribute (\"#{attribute}\") on a #{object.class} " \
                         "variable (\"#{object}\")."

lib/twig/version.rb

@@ -0,0 +1,5 @@
+# frozen_string_literal: true
+
+module Twig
+  VERSION = '0.0.9'
+end

spec/lib/twig/extension/core_spec.rb

@@ -482,6 +482,10 @@ def each
   describe '#get_attribute' do
     let(:loader) { Twig::Loader::Hash.new({}) }
     let(:environment) { Twig::Environment.new(loader) }
+    let(:strict) { Twig::Environment.new(loader, { strict_variables: true }) }
+    # A missing key raises with the source attached, so the double has to answer
+    # what Error::Base asks of it.
+    let(:source) { instance_double(Twig::Source, path: nil, name: nil) }
 
     it 'does not use bracket access if doing a method call' do
       klass = Class.new do
@@ -518,5 +522,145 @@ def foo
         )
       ).to eq(3.14)
     end
+
+    def attribute_of(object, attribute, env = environment)
+      described_class.get_attribute(env, source, object, attribute, Twig::Template::METHOD_CALL)
+    end
+
+    def subscript_of(object, attribute, env = environment)
+      described_class.get_attribute(env, source, object, attribute, Twig::Template::ARRAY_CALL)
+    end
+
+    context 'when the key is missing' do
+      it 'names the keys that exist rather than dumping the object' do
+        expect { subscript_of({ 'a' => 'x', 'b' => 'y' }, 'c', strict) }.
+          to raise_error(Twig::Error::Runtime, %r{Key "c" for sequence/mapping with keys "a, b" does not exist})
+      end
+
+      it 'reports the same way for bracket and dot access' do
+        object = { 'a' => 'x' }
+        bracket = begin
+          subscript_of(object, 'c', strict)
+        rescue Twig::Error::Runtime => e
+          e.message
+        end
+        dot = begin
+          attribute_of(object, 'c', strict)
+        rescue Twig::Error::Runtime => e
+          e.message
+        end
+
+        expect(bracket).to eq(dot)
+      end
+
+      it 'does not double the space when the object has no keys' do
+        expect { subscript_of(%w[a b], 5, strict) }.
+          to raise_error(Twig::Error::Runtime, %r{sequence/mapping does not exist})
+      end
+    end
+
+    context 'with a key holding a falsy value' do
+      it 'returns false for a string key reached by a string name' do
+        expect(subscript_of({ 'flag' => false }, 'flag')).to be(false)
+        expect(attribute_of({ 'flag' => false }, 'flag')).to be(false)
+      end
+
+      it 'returns false for a symbol key reached by a symbol name' do
+        expect(subscript_of({ flag: false }, :flag)).to be(false)
+      end
+
+      it 'returns false across the string/symbol divide' do
+        expect(subscript_of({ flag: false }, 'flag')).to be(false)
+        expect(subscript_of({ 'flag' => false }, :flag)).to be(false)
+      end
+
+      it 'returns nil for a key that genuinely holds nil' do
+        expect(subscript_of({ 'flag' => nil }, 'flag')).to be_nil
+      end
+
+      it 'prefers the key that matches over the other form of the name' do
+        # Both keys exist; the one the template asked for wins, even though it
+        # holds nil and the other holds something truthy.
+        expect(subscript_of({ 'a' => nil, a: 1 }, 'a')).to be_nil
+        expect(subscript_of({ 'a' => 1, a: nil }, :a)).to be_nil
+      end
+
+      it 'reports a falsy value as defined' do
+        expect(
+          described_class.get_attribute(
+            environment,
+            instance_double(Twig::Source),
+            { 'flag' => false },
+            'flag',
+            Twig::Template::ARRAY_CALL,
+            defined_test: true
+          )
+        ).to be(true)
+      end
+    end
+
+    context 'with an array index' do
+      it 'reads from the end with a negative index' do
+        expect(subscript_of(%w[a b c], -1)).to eq('c')
+        expect(subscript_of(%w[a b c], -3)).to eq('a')
+      end
+
+      it 'treats an out-of-range negative index as missing, not as a TypeError' do
+        expect { subscript_of(%w[a b c], -99, strict) }.
+          to raise_error(Twig::Error::Runtime, /-99/)
+      end
+
+      it 'treats an out-of-range positive index as missing' do
+        expect { subscript_of(%w[a b c], 99, strict) }.
+          to raise_error(Twig::Error::Runtime, /99/)
+      end
+
+      it 'returns nil for an out-of-range index when not strict' do
+        expect(subscript_of(%w[a b c], -99)).to be_nil
+        expect(subscript_of(%w[a b c], 99)).to be_nil
+      end
+    end
+
+    context 'with a collection' do
+      it 'calls methods on an array rather than subscripting with the name' do
+        expect(attribute_of(%w[a b], :any?)).to be(true)
+        expect(attribute_of(%w[a b], :size)).to eq(2)
+        expect(attribute_of([], :any?)).to be(false)
+      end
+
+      it 'calls methods on a hash rather than returning nil' do
+        expect(attribute_of({ 'a' => 1 }, :any?)).to be(true)
+        expect(attribute_of({}, :any?)).to be(false)
+      end
+
+      it 'still prefers a real key over a method of the same name' do
+        expect(attribute_of({ 'size' => 'from key' }, 'size')).to eq('from key')
+        expect(attribute_of({ 'first' => 'Ada' }, 'first')).to eq('Ada')
+      end
+
+      it 'still prefers a real index over a method name' do
+        expect(attribute_of(%w[a b], 0)).to eq('a')
+      end
+    end
+
+    context 'with a method the object only claims to respond to' do
+      let(:liar) do
+        Class.new do
+          def respond_to_missing?(name, include_all = false)
+            name.to_sym == :hidden || super
+          end
+
+          private
+
+          def hidden
+            'private'
+          end
+        end.new
+      end
+
+      it 'does not reach private methods' do
+        expect { attribute_of(liar, :hidden) }.to raise_error(NoMethodError)
+      end
+    end
   end
 end

twig-ruby.gemspec

@@ -1,8 +1,13 @@
 # frozen_string_literal: true
 
+# require_relative rather than requiring the gem: this needs the version and
+# nothing else, and evaluating the whole library to read one constant would pull
+# ActiveSupport in at packaging time.
+require_relative 'lib/twig/version'
+
 Gem::Specification.new do |s|
   s.name        = 'twig_ruby'
-  s.version     = '0.0.8'
+  s.version     = Twig::VERSION
   s.summary     = 'Twig Templating for Ruby'
   s.description = ''
   s.authors     = ['Craig Blanchette', 'Fabian Potencier']