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

Fix conversion of singleton classes in Ruby #9342

Merged
merged 2 commits into from Feb 11, 2022

Conversation

antstorm
Copy link
Contributor

@antstorm antstorm commented Dec 24, 2021

I found a nasty issue when passing an object who's singleton class was accessed as a Protobuf field value. In my case I was using a string and got this cryptic message:

Invalid argument for string field 'name' (given String). (Google::Protobuf::TypeError)

After digging deeper I've discovered that Ruby has a weird way of representing a singleton class (also known as eigenclass in Ruby) — basically once it was accessed via the Object#singleton_class method the true class of an object becomes a wrapper (such as #<Class:#<String:0x00000001168372e8>> in case of a string).

This is normally an issue because all the class accessor methods in Ruby take this into account and unwrap the real class, so it's not even noticeable. However when using Ruby C API's CLASS_OF() method, it shows the wrapper class and therefore is not very usable with wrapped classes. Instead a rb_obj_class should be used, which will correctly account for wrapped classes. More details (as well as an example) can be found here — https://bugs.ruby-lang.org/issues/18428.

The error itself is also pretty confusing because it's using rb_class2name() which is wrapper aware and shows the unwrapped class (which is not the same one that was used in the conditional).

Note: I've included a test which should replicate this issue, however I wasn't able to run rake test locally. There seems to be a big circular dependency issue between ruby/lib/google/protobuf/descriptor_dsl.rb and ruby/lib/google/protobuf/descriptor_pb.rb (DSL depends on the generated proto, which itself is built using the DSL). Not sure how it was supposed to work. Hoping on the CI run to make sure everything is good.

@acozzette acozzette merged commit 2ce9604 into protocolbuffers:master Feb 11, 2022
@antstorm antstorm deleted the singleton-string branch February 14, 2022 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants