Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type coercion issue when to_str raises NoMethodError #2903

Open
nirvdrum opened this issue Mar 3, 2023 · 4 comments
Open

Type coercion issue when to_str raises NoMethodError #2903

nirvdrum opened this issue Mar 3, 2023 · 4 comments
Assignees

Comments

@nirvdrum
Copy link
Collaborator

nirvdrum commented Mar 3, 2023

While working on getting the Sorbet test suite passing on TruffleRuby, @paracycle discovered an issue with some of our type coercion logic. In this case, an object is attempted to be coerced to a String via to_str. If to_str raises a NoMethodError then some of our type coercion logic assumes the type cannot be converted and raises a TypeError rather than the NoMethodError.

A brief search indicates ToStringOrSymbolNode needs to check if the method exists. Our logic in Truffle::Type.try_convert does perform the extra respond_to? check, but it looks like that didn't get carried forward to type coercion performed in Java.

class Foo
  def to_str
    raise NoMethodError, "foo"
  end
end

"bar " + Foo.new

MRI 3.1.3 (warnings removed):

)> ruby -v type_error.rb
ruby 3.1.3p185 (2022-11-24 revision 1a6b16756e) [arm64-darwin21]
type_error.rb:3:in `to_str': foo (NoMethodError)

    raise NoMethodError, "foo"
    ^^^^^
	from type_error.rb:7:in `+'
	from type_error.rb:7:in `<main>'

TruffleRuby 23.0.0-dev (a671781, warnings removed):

> jt ruby -v type_error.rb
truffleruby 23.0.0-dev-a6717812, like ruby 3.1.3, GraalVM CE JVM [aarch64-darwin]
type_error.rb:7:in `+': no implicit conversion of Foo into String (TypeError)
	from type_error.rb:7:in `<main>'
@nirvdrum nirvdrum self-assigned this Mar 3, 2023
@eregon
Copy link
Member

eregon commented Mar 3, 2023

Right, it seems this is a problem for most To*Node, they catch NoMethodError and they can't easily know if that's from that method call or some other method call.

I think unfortunately it's semantically incorrect in those cases to use a @Cached(parameters = "PRIVATE_RETURN_MISSING") DispatchNode (which behaves a bit like rb_check_funcall()), because e.g. that wouldn't call method_missing, but IIRC the semantics on such conversion is to call method_missing.

A possibility is to do it like ToRubyIntegerNode does it by calling a Ruby helper (rb_to_int_fallback).

@eregon
Copy link
Member

eregon commented Mar 3, 2023

Indeed it calls method_missing in that case on CRuby (even if respond_to? is false):

$ ruby -e 'o=Object.new; def o.method_missing(name, *); p name; "foo"; end; p "a " + o'
:to_str
"a foo"

@eregon
Copy link
Member

eregon commented Mar 3, 2023

Oh yeah it's really weird and full of corner cases:

ruby 3.1.3p185 (2022-11-24 revision 1a6b16756e) [x86_64-linux]

$ ruby -e 'o=Object.new; def o.respond_to?(m,*); p [:resp, m]; end; def o.method_missing(name, *); p name; "foo"; end; p "a " + o'
[:resp, :to_str]
:to_str
"a foo"

$ ruby -e 'o=Object.new; def o.respond_to?(m,*); p [:resp, m]; false; end; def o.method_missing(name, *); p name; "foo"; end; p "a " + o'
[:resp, :to_str]
-e:1:in `+': no implicit conversion of Object into String (TypeError)
	from -e:1:in `<main>'

$ ruby -e 'o=Object.new; def o.method_missing(name, *); p name; "foo"; end; p o.respond_to?(:to_str); p "a " + o'
false
:to_str
"a foo"

So it does call respond_to? (or at least it calls the custom one), and if a custom respond_to? returns false it believes it and doesn't call method_missing. But if it's the default respond_to? it calls method_missing anyway 😅

@eregon
Copy link
Member

eregon commented Mar 3, 2023

Right, I think this is simply the "full" semantics of rb_check_funcall in CRuby.
We have them implemented in Truffle::Type.check_funcall and so I think using Truffle::Type.rb_convert_type should implement the correct semantics here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants