Skip to content

rb_check_convert_type compatibility with MRI #2862

Closed
wants to merge 5 commits into from

2 participants

@YorickPeterse
Rubinius member

This PR falls in the categories "bikeshed" (in a way) and "do we really care?".
This PR changes Rbx' implementation of rb_check_convert_type (and its callers
to keep things going) so that its behaviour is the same as MRI's. Before this
commit the main difference was that MRI would raise when a coercion method
returned an unexpected type whereas Rbx would return nil instead.

A quick Github search
(https://github.com/search?q=rb_check_convert_type&ref=cmdform&type=Code)
shows that people do use the function and probably also rely on the behaviour
of raising errors.

The current changes are a bit of a mess as I wanted to get things going. In the
long term we probably should refactor things a bit but I'd like to have some
feedback first. In particular I'd like to know if it's even worth bothering,
though I'd say it would be.

YorickPeterse added some commits Dec 30, 2013
@YorickPeterse YorickPeterse MRI compatibility for rb_check_convert_type.
Due to its usage the changes are rather, well, ugly. The problem here is that a
bunch of functions use rb_check_convert_type without specifying what the
expected type should be (before this commit said type was never even used).

For now a value of `-1` as the type will ignore the extra call to `raise` if
the coercion method exists but returns an unexpected type. In the long run we
probably want to refactor this code.
9140537
@YorickPeterse YorickPeterse Specs for converting unexpected types.
These are specs for rb_check_convert_type to ensure that it raises when a
coercion method exists that returns an unexpected type.
3b3504e
@YorickPeterse
Rubinius member

To clarify, I started working on this for #2771. The requested function itself has been added but while doing so I noticed the differences between Rbx and MRI.

@YorickPeterse
Rubinius member

Extra note, the definitions of the conversion methods in MRI can be found here: http://rxr.whitequark.org/mri/source/object.c#2438

@brixen
Rubinius member
brixen commented Dec 31, 2013

Not totally following your train of thought here. Is there an issue making rb_check_convert_type and rb_convert_type compatible with MRI 2.1? Use of these functions internally isn't a blocker. If they are used in a manner that expects an incompatible semantic, we change their use or use a different method.

@brixen brixen commented on an outdated diff Dec 31, 2013
spec/ruby/optional/capi/object_spec.rb
@@ -169,6 +169,13 @@ class DescObjectTest < ObjectTest
@o.rb_check_convert_type(ao).should == []
@o.rb_check_convert_type(h).should == nil
end
+
+ it "raises when the coercion method returns a different object" do
@brixen
Rubinius member
brixen added a note Dec 31, 2013

Always include the exception name in the spec description.

it "raises a TypeError..."
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@brixen brixen commented on the diff Dec 31, 2013
vm/capi/object.cpp
@@ -118,24 +118,45 @@
return rb_funcall(env->get_handle(env->state()->globals().type.get()), rb_intern("try_convert"), 3, object_handle, rb_cString, rb_intern("to_str"));
}
- VALUE rb_check_convert_type(VALUE object_handle, int /*type*/,
+ /*
@brixen
Rubinius member
brixen added a note Dec 31, 2013

Use the following multi-line /* comment style:

/* this is a comment
 * that spans multiple lines
 */
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@brixen brixen commented on an outdated diff Dec 31, 2013
vm/capi/object.cpp
}
- return Qnil;
+ // If the method returns nil we can bail out right away.
+ if ( NIL_P(retval) ) return retval;
@brixen
Rubinius member
brixen added a note Dec 31, 2013
if(NIL_P(retval))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@brixen brixen commented on an outdated diff Dec 31, 2013
vm/capi/object.cpp
}
- return Qnil;
+ // If the method returns nil we can bail out right away.
+ if ( NIL_P(retval) ) return retval;
+
+ /*
+ When the coercion method exists but returns a different type than specified
+ in `type` MRI will raise an error. This code is mostly a copy-paste job
+ from the MRI source code.
+ */
+ if ( type != -1 && TYPE(retval) != type ) {
@brixen
Rubinius member
brixen added a note Dec 31, 2013

same

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@YorickPeterse
Rubinius member

The behaviour of these functions is different from MRI in a way that could potentially break C extensions. MRI defines two functions:

  • rb_check_convert_type
  • rb_convert_type

The first function can do 3 different things:

  1. If the type is valid, it is returned
  2. If no coercion method is found (e.g. to_ary is called on a custom object), nil is returned
  3. If the coercion method exists but returns a type different than the one specified, raise an error

The second function works similar by the looks of it. In Rbx the first function would never raise an error in case of number 3, it would simply return nil. This could potentially break C extensions that rely on the raise behaviour.

Rbs internally used the first function while setting the type to 0 as it wasn't used before this patch. Since 0 is also the #define value of T_ARRAY (0x0 to be exact) I had to change the calls to specify -1 instead. This is the part where I think that we should clean things up a bit. It doesn't make sense to call a function that is supposed to raise only to then disable the raising feature. It would be better to split those parts up into separate functions.

@brixen
Rubinius member
brixen commented Jan 1, 2014

If we have these functions but they behave differently than MRI, then I expect they already are breaking C-extensions. If there are any C-extensions that happen to be Rubinius-only and rely on this behavior, we need to fix them.

I think it makes sense to fix these. If we need to add a function to do that, it's ok, but we should do it the same way as MRI unless there is a compelling reason not to.

@YorickPeterse
Rubinius member

I'm deadlocked on this one. Due to the current Rbx usage of rb_check_convert_type and rb_convert_type I'm not really sure where to make the required changes to make them behave similar to the MRI versions (see http://rxr.whitequark.org/mri/source/object.c#2453 for more info).

The problems currently are as following:

  • A bunch of Rbx functions use rb_convert_type and expect it to not raise an error. In MRI it does raise an error, which I added to Rbx as well. This however requires that the type argument is set to a dummy value (-1) to prevent raising of errors.
  • rb_check_convert_type calls rb_convert_type and passes along the type argument, leading to the same problem as above.

To fix the above we'd need to introduce an extra function that basically does only the conversion without raising errors, then use that where needed. I'm however unsure what to name it, where to put it, etc.

@YorickPeterse YorickPeterse added the capi label Feb 7, 2014
@brixen brixen closed this in 35ae80e May 12, 2014
@YorickPeterse YorickPeterse deleted the convert-type-compat branch May 12, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.