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

[Ruby] Remove to_hash methods #6166

Merged
merged 1 commit into from Jun 10, 2019

Conversation

blowmage
Copy link
Contributor

The Ruby implementation creates the explicit coercion #to_h method and the implicit coercion #to_hash method on Message and Map. Ruby treats objects that respond to implicit coercion methods differently. I do not think Protobuf intends that Message and Map are to be treated implicitly as hashes. In fact, I think this should actually be considered a bug.

For example, here are how the explicit and implicit methods such as #to_h and #to_hash are used:

# assuming these primitive objects
ary = [1, 2, 3]
hash = { foo: :bar }
int = 42
str = "Hello world!"

# hash has an array representation it itself
hash.to_a #=> [[:foo, :bar]]
# but that doesn't mean it _is_ an array
hash.to_ary # NoMethodError (undefined method `to_ary' for {:foo=>:bar}:Hash)

# similarily, an array doesn't know how to be a hash
ary.to_h #TypeError (wrong element type Integer at 0 (expected array))
# but an array does have a string representation
ary.to_s #=> "[1, 2, 3]"
# and a hash has a string representation
hash.to_s #=> "{:foo=>:bar}"
# and an integer has a string representation
int.to_s #=> "42"
# but none of them _are_ strings
ary.to_str # NoMethodError (undefined method `to_str' for [1, 2, 3]:Array)
hash.to_str # NoMethodError (undefined method `to_str' for {:foo=>:bar}:Hash)
int.to_str # NoMethodError (undefined method `to_str' for 42:Integer)

# and a string doesn't know how to be an array, hash, or integer
str.to_a # NoMethodError (undefined method `to_a' for "Hello world!":String)
str.to_h # NoMethodError (undefined method `to_h' for "Hello world!":String)
str.to_i # NoMethodError (undefined method `to_i' for "Hello world!":String)

Similarily, Protobuf Messages and Maps are not Ruby Hashes. They can generate hash representations, but they are not themselves hashes. They should not be used in place of hashes.

What does the presence of the #to_hash method mean? It means Ruby will treat these objects as if they were hashes. Consider the following method:

def do_a_thing req_resource, opt_resource = nil, show_warnings: true
  p req_resource
  p opt_resource
  p show_warnings
end

You would expect you could pass two Protobuf objects to the method, but you cannot.

gem "googleapis-common-protos-types"
require "google/type/color_pb"

red = Google::Type::Color.new red: 1.0
blue = Google::Type::Color.new blue: 1.0

# call the method
do_a_thing red, blue # ArgumentError (unknown keywords: red, green, blue, alpha)

This is because Ruby sees that the blue object has defined #to_hash and treats it as a hash, and tries to match the optional named argument instead of the positional object.

Because of this, and because there is no documentation that states that this method is intended to be part of the public API, I think these methods can and should be removed.

@blowmage
Copy link
Contributor Author

The PR (#338) and commit (d1b52a0) that added the implicit coercion methods only mentions the explicit use-case, and this comment in particular makes me think the consequences of including the implicit methods was probably not fully understood and unintentional.

@haberman
Copy link
Member

Yes this appears to have been unintentional and not fully understood. I wasn't aware of this corner of Ruby and it's surprising to me that you can't pass two actual hashes to a function that takes optional named arguments.

I agree that it does seem better overall to not implement #to_hash. However I'm concerned about the possibility of breaking users who may possibly depend on the current behavior. Since Ruby classes are always open, could users who are broken by this add #to_hash to their classes manually? I want to avoid a scenario where users are blocked from upgrading to the new version because it breaks their code.

@blowmage
Copy link
Contributor Author

Since Ruby classes are always open, could users who are broken by this add #to_hash to their classes manually?

Yes, users can add methods to the Protobuf resource classes to restore #to_hash if they are dependent on it.

@blowmage
Copy link
Contributor Author

it's surprising to me that you can't pass two actual hashes to a function that takes optional named arguments.

Matz has stated that the way named arguments were implemented was a mistake and this will be changed in Ruby 3. I assume he means this issue of unrolling the last positional Hash object and matching the named arguments.

@dazuma
Copy link
Contributor

dazuma commented May 24, 2019

/cc @dazuma

@blowmage
Copy link
Contributor Author

Ping.

@derekargueta
Copy link

derekargueta commented Jul 23, 2020

I was surprised to come across this because the documentation still states that to_hash is implemented (https://developers.google.com/protocol-buffers/docs/reference/ruby-generated). How can we get the documentation updated? @haberman

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

Successfully merging this pull request may close these issues.

None yet

6 participants