Fix the exception type raised when type coercion via to_str raises a NoMethodError.

Co-authored-by: Ufuk Kayserilioglu <ufuk.kayserilioglu@shopify.com>

Author: nirvdrum

CHANGELOG.md

@@ -45,6 +45,7 @@ Bug fixes:
 * Fix processing of proc rest arguments located at the beginning if there are no actual arguments (#2921, @andrykonchin).
 * Fix `Monitor#exit` to raise `ThreadError` when monitor not owned by the current thread (#2922, @andrykonchin).
 * Fix `MatchData#[]` to support negative `length` argument (#2929, @andrykonchin).
+* Fix the exception type raised when type coercion raises a `NoMethodError` (#2903, @paracycle, @nirvdrum).
 
 Compatibility:
 

spec/ruby/core/kernel/method_spec.rb

@@ -29,7 +29,7 @@
     m.call.should == :defined
   end
 
-  it "can be called even if we only repond_to_missing? method, true" do
+  it "can be called even if we only respond_to_missing? method, true" do
     m = KernelSpecs::RespondViaMissing.new.method(:handled_privately)
     m.should be_an_instance_of(Method)
     m.call(1, 2, 3).should == "Done handled_privately([1, 2, 3])"
@@ -58,4 +58,23 @@ def method_missing(m)
     m = cls.new.method(:bar)
     m.call.should == :bar
   end
+
+  describe "converts the given name to a String using #to_str" do
+    it "calls #to_str to convert the given name to a String" do
+      name = mock("method-name")
+      name.should_receive(:to_str).and_return("hash")
+      Object.method(name).should == Object.method(:hash)
+    end
+
+    it "raises a TypeError if the given name can't be converted to a String" do
+      -> { Object.method(nil) }.should raise_error(TypeError)
+      -> { Object.method([])  }.should raise_error(TypeError)
+    end
+
+    it "raises a NoMethodError if the given argument raises a NoMethodError during type coercion to a String" do
+      name = mock("method-name")
+      name.should_receive(:to_str).and_raise(NoMethodError)
+      -> { Object.method(name) }.should raise_error(NoMethodError)
+    end
+  end
 end

spec/ruby/core/module/const_defined_spec.rb

@@ -80,10 +80,23 @@
     ConstantSpecs::ClassA.const_defined?(:CS_CONSTX).should == false
   end
 
-  it "calls #to_str to convert the given name to a String" do
-    name = mock("ClassA")
-    name.should_receive(:to_str).and_return("ClassA")
-    ConstantSpecs.const_defined?(name).should == true
+  describe "converts the given name to a String using #to_str" do
+    it "calls #to_str to convert the given name to a String" do
+      name = mock("ClassA")
+      name.should_receive(:to_str).and_return("ClassA")
+      ConstantSpecs.const_defined?(name).should == true
+    end
+
+    it "raises a TypeError if the given name can't be converted to a String" do
+      -> { ConstantSpecs.const_defined?(nil) }.should raise_error(TypeError)
+      -> { ConstantSpecs.const_defined?([])  }.should raise_error(TypeError)
+    end
+
+    it "raises a NoMethodError if the given argument raises a NoMethodError during type coercion to a String" do
+      name = mock("classA")
+      name.should_receive(:to_str).and_raise(NoMethodError)
+      -> { ConstantSpecs.const_defined?(name) }.should raise_error(NoMethodError)
+    end
   end
 
   it "special cases Object and checks it's included Modules" do

spec/ruby/core/string/append_spec.rb

@@ -5,6 +5,7 @@
 describe "String#<<" do
   it_behaves_like :string_concat, :<<
   it_behaves_like :string_concat_encoding, :<<
+  it_behaves_like :string_concat_type_coercion, :<<
 
   it "raises an ArgumentError when given the incorrect number of arguments" do
     -> { "hello".send(:<<) }.should raise_error(ArgumentError)

spec/ruby/core/string/concat_spec.rb

@@ -5,6 +5,7 @@
 describe "String#concat" do
   it_behaves_like :string_concat, :concat
   it_behaves_like :string_concat_encoding, :concat
+  it_behaves_like :string_concat_type_coercion, :concat
 
   it "takes multiple arguments" do
     str = "hello "

spec/ruby/core/string/plus_spec.rb

@@ -3,6 +3,9 @@
 require_relative 'shared/concat'
 
 describe "String#+" do
+  it_behaves_like :string_concat_encoding, :+
+  it_behaves_like :string_concat_type_coercion, :+
+
   it "returns a new string containing the given string concatenated to self" do
     ("" + "").should == ""
     ("" + "Hello").should == "Hello"
@@ -31,6 +34,4 @@
     ("hello" + StringSpecs::MyString.new("foo")).should be_an_instance_of(String)
     ("hello" + StringSpecs::MyString.new("")).should be_an_instance_of(String)
   end
-
-  it_behaves_like :string_concat_encoding, :+
 end

spec/ruby/core/string/shared/concat.rb

@@ -5,18 +5,6 @@
     str.should == "hello world"
   end
 
-  it "converts the given argument to a String using to_str" do
-    obj = mock('world!')
-    obj.should_receive(:to_str).and_return("world!")
-    a = 'hello '.send(@method, obj)
-    a.should == 'hello world!'
-  end
-
-  it "raises a TypeError if the given argument can't be converted to a String" do
-    -> { 'hello '.send(@method, [])        }.should raise_error(TypeError)
-    -> { 'hello '.send(@method, mock('x')) }.should raise_error(TypeError)
-  end
-
   it "raises a FrozenError when self is frozen" do
     a = "hello"
     a.freeze
@@ -148,3 +136,23 @@
     end
   end
 end
+
+describe :string_concat_type_coercion, shared: true do
+  it "converts the given argument to a String using to_str" do
+    obj = mock('world!')
+    obj.should_receive(:to_str).and_return("world!")
+    a = 'hello '.send(@method, obj)
+    a.should == 'hello world!'
+  end
+
+  it "raises a TypeError if the given argument can't be converted to a String" do
+    -> { 'hello '.send(@method, [])        }.should raise_error(TypeError)
+    -> { 'hello '.send(@method, mock('x')) }.should raise_error(TypeError)
+  end
+
+  it "raises a NoMethodError if the given argument raises a NoMethodError during type coercion to a String" do
+    obj = mock('world!')
+    obj.should_receive(:to_str).and_raise(NoMethodError)
+    -> { 'hello '.send(@method, obj) }.should raise_error(NoMethodError)
+  end
+end

spec/tags/core/module/const_defined_tags.txt

@@ -0,0 +1 @@
+fails:Module#const_defined? converts the given name to a String using #to_str raises a NoMethodError if the given argument raises a NoMethodError during type coercion to a String

spec/truffle/always_inlined_spec.rb

@@ -44,12 +44,6 @@
       }.should raise_error(ArgumentError) { |e| e.backtrace_locations[0].label.should == 'binding' }
     end
 
-    it "for #eval" do
-      -> {
-        eval(Object.new)
-      }.should raise_error(TypeError) { |e| e.backtrace_locations[0].label.should == 'eval' }
-    end
-
     it "for #local_variables" do
       -> {
         local_variables(:wrong)
@@ -235,6 +229,26 @@
       }
     end
 
+    it "for #eval" do
+      -> {
+        eval(Object.new)
+      }.should raise_error(TypeError) { |e|
+        e.backtrace_locations[0].label.should == 'convert_type'
+      }
+    end
+
+    it "for Module#class_eval with Object" do
+      -> {
+        Module.new.class_eval(Object.new)
+      }.should raise_error(TypeError) { |e| e.backtrace_locations[0].label.should == 'convert_type' }
+    end
+
+    it "for Module#module_eval with Object" do
+      -> {
+        Module.new.module_eval(Object.new)
+      }.should raise_error(TypeError) { |e| e.backtrace_locations[0].label.should == 'convert_type' }
+    end
+
     guard -> { RUBY_ENGINE != "ruby" } do
       it "for #respond_to?" do
         obj = Object.new

src/main/java/org/truffleruby/core/cast/ToStrNode.java

@@ -15,14 +15,11 @@
 import org.truffleruby.core.string.RubyString;
 import org.truffleruby.core.string.ImmutableRubyString;
 import org.truffleruby.language.RubyBaseNodeWithExecute;
-import org.truffleruby.language.control.RaiseException;
 import org.truffleruby.language.dispatch.DispatchNode;
 
 import com.oracle.truffle.api.dsl.Cached;
 import com.oracle.truffle.api.dsl.NodeChild;
 import com.oracle.truffle.api.dsl.Specialization;
-import com.oracle.truffle.api.profiles.BranchProfile;
-import org.truffleruby.language.library.RubyStringLibrary;
 
 @GenerateUncached
 @NodeChild(value = "childNode", type = RubyBaseNodeWithExecute.class)
@@ -53,31 +50,13 @@ protected ImmutableRubyString coerceImmutableRubyString(ImmutableRubyString stri
 
     @Specialization(guards = "isNotRubyString(object)")
     protected Object coerceObject(Object object,
-            @Cached BranchProfile errorProfile,
-            @Cached DispatchNode toStrNode,
-            @Cached RubyStringLibrary libString) {
-        final Object coerced;
-        try {
-            coerced = toStrNode.call(object, "to_str");
-        } catch (RaiseException e) {
-            errorProfile.enter();
-            if (e.getException().getLogicalClass() == coreLibrary().noMethodErrorClass) {
-                throw new RaiseException(
-                        getContext(),
-                        coreExceptions().typeErrorNoImplicitConversion(object, "String", this));
-            } else {
-                throw e;
-            }
-        }
-
-        if (libString.isRubyString(coerced)) {
-            return coerced;
-        } else {
-            errorProfile.enter();
-            throw new RaiseException(
-                    getContext(),
-                    coreExceptions().typeErrorBadCoercion(object, "String", "to_str", coerced, this));
-        }
+            @Cached DispatchNode toStrNode) {
+        return toStrNode.call(
+                coreLibrary().truffleTypeModule,
+                "rb_convert_type",
+                object,
+                coreLibrary().stringClass,
+                coreSymbols().TO_STR);
     }
 
     @Override