[GR-18163] Fix Enumerable#reduce to handle non-Symbol method name parameter

PullRequest: truffleruby/4296

Author: andrykonchin

CHANGELOG.md

@@ -29,6 +29,7 @@ Compatibility:
 * Support `:buffer` keyword argument to `Array#pack` (#3559, @andrykonchyn).
 * Set `RbConfig::CONFIG['host_cpu']` to `arm64` on darwin platform (#3571, @andrykonchin).
 * Fix `RegexpError` messages to match CRuby better (#3398, @andrykonchin).
+* Fix `Enumerable#reduce` to handle non-Symbol method name parameter (#2931, @andrykonchin).
 
 Performance:
 

spec/ruby/core/enumerable/shared/inject.rb

@@ -16,6 +16,23 @@
 
   it "can take two argument" do
     EnumerableSpecs::Numerous.new(1, 2, 3).send(@method, 10, :-).should == 4
+    EnumerableSpecs::Numerous.new(1, 2, 3).send(@method, 10, "-").should == 4
+
+    [1, 2, 3].send(@method, 10, :-).should == 4
+    [1, 2, 3].send(@method, 10, "-").should == 4
+  end
+
+  it "converts non-Symbol method name argument to String with #to_str if two arguments" do
+    name = Object.new
+    def name.to_str; "-"; end
+
+    EnumerableSpecs::Numerous.new(1, 2, 3).send(@method, 10, name).should == 4
+    [1, 2, 3].send(@method, 10, name).should == 4
+  end
+
+  it "raises TypeError when the second argument is not Symbol or String and it cannot be converted to String if two arguments" do
+    -> { EnumerableSpecs::Numerous.new(1, 2, 3).send(@method, 10, Object.new) }.should raise_error(TypeError, /is not a symbol nor a string/)
+    -> { [1, 2, 3].send(@method, 10, Object.new) }.should raise_error(TypeError, /is not a symbol nor a string/)
   end
 
   it "ignores the block if two arguments" do
@@ -39,6 +56,25 @@
 
   it "can take a symbol argument" do
     EnumerableSpecs::Numerous.new(10, 1, 2, 3).send(@method, :-).should == 4
+    [10, 1, 2, 3].send(@method, :-).should == 4
+  end
+
+  it "can take a String argument" do
+    EnumerableSpecs::Numerous.new(10, 1, 2, 3).send(@method, "-").should == 4
+    [10, 1, 2, 3].send(@method, "-").should == 4
+  end
+
+  it "converts non-Symbol method name argument to String with #to_str" do
+    name = Object.new
+    def name.to_str; "-"; end
+
+    EnumerableSpecs::Numerous.new(10, 1, 2, 3).send(@method, name).should == 4
+    [10, 1, 2, 3].send(@method, name).should == 4
+  end
+
+  it "raises TypeError when passed not Symbol or String method name argument and it cannot be converted to String" do
+    -> { EnumerableSpecs::Numerous.new(10, 1, 2, 3).send(@method, Object.new) }.should raise_error(TypeError, /is not a symbol nor a string/)
+    -> { [10, 1, 2, 3].send(@method, Object.new) }.should raise_error(TypeError, /is not a symbol nor a string/)
   end
 
   it "without argument takes a block with an accumulator (with first element as initial value) and the current element. Value of block becomes new accumulator" do
@@ -77,7 +113,6 @@
     EnumerableSpecs::EachDefiner.new('a','b','c').send(@method) {|result, i| i+result}.should == "cba"
     EnumerableSpecs::EachDefiner.new(3, 4, 5).send(@method) {|result, i| result*i}.should == 60
     EnumerableSpecs::EachDefiner.new([1], 2, 'a','b').send(@method){|r,i| r<<i}.should == [1, 2, 'a', 'b']
-
   end
 
   it "returns nil when fails(legacy rubycon)" do

src/main/java/org/truffleruby/core/array/ArrayNodes.java

@@ -50,6 +50,7 @@
 import org.truffleruby.core.array.library.SharedArrayStorage;
 import org.truffleruby.core.cast.BooleanCastNode;
 import org.truffleruby.core.cast.CmpIntNode;
+import org.truffleruby.core.cast.NameToJavaStringNode;
 import org.truffleruby.core.cast.ToAryNode;
 import org.truffleruby.core.cast.ToIntNode;
 import org.truffleruby.core.cast.ToLongNode;
@@ -71,7 +72,6 @@
 import org.truffleruby.core.string.RubyString;
 import org.truffleruby.core.string.StringHelperNodes;
 import org.truffleruby.core.support.TypeNodes.CheckFrozenNode;
-import org.truffleruby.core.symbol.RubySymbol;
 import org.truffleruby.extra.ffi.Pointer;
 import org.truffleruby.interop.ToJavaStringNode;
 import org.truffleruby.language.Nil;
@@ -1352,34 +1352,39 @@ public void accept(Node node, CallBlockNode yieldNode, RubyArray array, Object s
 
         // Uses Symbol and no block
 
-        @Specialization(guards = { "isEmptyArray(array)" })
-        Object injectSymbolEmptyArrayNoInitial(
-                RubyArray array, RubySymbol initialOrSymbol, NotProvided symbol, Nil block) {
+        @Specialization(guards = { "isEmptyArray(array)", "wasProvided(initialOrSymbol)" })
+        Object injectSymbolEmptyArrayNoInitial(RubyArray array, Object initialOrSymbol, NotProvided symbol, Nil block,
+                @Cached @Shared NameToJavaStringNode nameToJavaStringNode) {
+            nameToJavaStringNode.execute(this, initialOrSymbol); // ensure a method name is either a Symbol or could be converted to String
             return nil;
         }
 
         @Specialization(
                 guards = {
                         "isEmptyArray(array)",
-                        "wasProvided(initialOrSymbol)" })
-        Object injectSymbolEmptyArray(RubyArray array, Object initialOrSymbol, RubySymbol symbol, Nil block) {
+                        "wasProvided(initialOrSymbol)",
+                        "wasProvided(symbol)" })
+        Object injectSymbolEmptyArray(RubyArray array, Object initialOrSymbol, Object symbol, Nil block,
+                @Cached @Shared NameToJavaStringNode nameToJavaStringNode) {
+            nameToJavaStringNode.execute(this, symbol); // ensure a method name is either a Symbol or could be converted to String
             return initialOrSymbol;
         }
 
         @Specialization(
-                guards = { "!isEmptyArray(array)" },
+                guards = { "!isEmptyArray(array)", "wasProvided(initialOrSymbol)" },
                 limit = "storageStrategyLimit()")
         Object injectSymbolNoInitial(
-                VirtualFrame frame, RubyArray array, RubySymbol initialOrSymbol, NotProvided symbol, Nil block,
+                VirtualFrame frame, RubyArray array, Object initialOrSymbol, NotProvided symbol, Nil block,
                 @Bind("array.getStore()") Object store,
                 @CachedLibrary("store") ArrayStoreLibrary stores,
                 @Cached @Shared IntValueProfile arraySizeProfile,
                 @Cached @Exclusive LoopConditionProfile loopProfile,
-                @Cached @Shared ToJavaStringNode toJavaString) {
+                @Cached @Shared NameToJavaStringNode nameToJavaStringNode) {
+            String methodName = nameToJavaStringNode.execute(this, initialOrSymbol); // ensure a method name is either a Symbol or could be converted to String
             return injectSymbolHelper(
                     frame,
                     array,
-                    toJavaString.execute(this, initialOrSymbol),
+                    methodName,
                     stores,
                     store,
                     stores.read(store, 0),
@@ -1391,19 +1396,21 @@ Object injectSymbolNoInitial(
         @Specialization(
                 guards = {
                         "!isEmptyArray(array)",
-                        "wasProvided(initialOrSymbol)" },
+                        "wasProvided(initialOrSymbol)",
+                        "wasProvided(symbol)" },
                 limit = "storageStrategyLimit()")
         Object injectSymbolWithInitial(
-                VirtualFrame frame, RubyArray array, Object initialOrSymbol, RubySymbol symbol, Nil block,
+                VirtualFrame frame, RubyArray array, Object initialOrSymbol, Object symbol, Nil block,
                 @Bind("array.getStore()") Object store,
                 @CachedLibrary("store") ArrayStoreLibrary stores,
                 @Cached @Shared IntValueProfile arraySizeProfile,
                 @Cached @Exclusive LoopConditionProfile loopProfile,
-                @Cached @Shared ToJavaStringNode toJavaString) {
+                @Cached @Shared NameToJavaStringNode nameToJavaStringNode) {
+            String methodName = nameToJavaStringNode.execute(this, symbol); // ensure a method name is either a Symbol or could be converted to String
             return injectSymbolHelper(
                     frame,
                     array,
-                    toJavaString.execute(this, symbol),
+                    methodName,
                     stores,
                     store,
                     initialOrSymbol,

src/main/ruby/truffleruby/core/enumerable.rb

@@ -555,7 +555,7 @@ def inject(initial = undefined, sym = undefined, &block)
       #   inject(initial_operand, symbol) -> object
 
       sym, initial = initial, undefined if Primitive.undefined?(sym)
-      sym = sym.to_sym
+      sym = Truffle::Type.coerce_to_symbol(sym)
 
       each do
         o = Primitive.single_block_arg

src/main/ruby/truffleruby/core/type.rb

@@ -441,13 +441,18 @@ def self.coerce_to_path(obj, check_null = true)
       path
     end
 
-    def self.coerce_to_symbol(obj)
-      if Primitive.is_a? obj, Symbol
-        obj
-      else
-        obj = obj.to_str if obj.respond_to?(:to_str)
-        coerce_to(obj, Symbol, :to_sym)
+    # Convert an object to Symbol (e.g. a method name).
+    # Non-Symbol values are converted to String with #to_str. If it couldn't be converted - TypeError is raised.
+    # Uses the logic of rb_check_id/rb_check_string_type/rb_check_convert_type_with_id
+    def self.coerce_to_symbol(object)
+      return object if Primitive.is_a?(object, Symbol)
+
+      string = Truffle::Type.rb_check_convert_type(object, String, :to_str)
+      if Primitive.nil?(string)
+        raise TypeError, "#{object} is not a symbol nor a string"
       end
+
+      string.to_sym
     end
 
     def self.symbol_or_string_to_symbol(obj)