Ruby repeated uint64 values error out with "wrong argument type Hash"

[stanhu]

What version of protobuf and what language are you using?
Version: 3.18.0.rc.1, tried master too
Language: Ruby

What operating system (Linux, Windows, ...) and version?

What runtime / compiler are you using (e.g., python version or gcc version)

Ruby 2.7.4

What did you do?
Steps to reproduce the behavior:

1. Checkout https://github.com/pganalyze/pg_query
2. Bump google-protobuf version to 3.18.0.rc.1.
3. Run bundle exec rake.

We see:

An error occurred while loading ./spec/lib/truncate_spec.rb.
Failure/Error: repeated :notnulls, :uint64, 11, json_name: "notnulls"

TypeError:
  wrong argument type Hash (expected String)
# ./lib/pg_query/pg_query_pb.rb:295:in `repeated'
# ./lib/pg_query/pg_query_pb.rb:295:in `block (3 levels) in <top (required)>'
# ./lib/pg_query/pg_query_pb.rb:284:in `instance_eval'
# ./lib/pg_query/pg_query_pb.rb:284:in `add_message'
# ./lib/pg_query/pg_query_pb.rb:284:in `block (2 levels) in <top (required)>'
# ./lib/pg_query/pg_query_pb.rb:7:in `instance_eval'
# ./lib/pg_query/pg_query_pb.rb:7:in `add_file'
# ./lib/pg_query/pg_query_pb.rb:7:in `block in <top (required)>'
# ./lib/pg_query/pg_query_pb.rb:6:in `instance_eval'
# ./lib/pg_query/pg_query_pb.rb:6:in `build'
# ./lib/pg_query/pg_query_pb.rb:6:in `<top (required)>'
# ./lib/pg_query.rb:4:in `require'
# ./lib/pg_query.rb:4:in `<top (required)>'
# ./spec/spec_helper.rb:1:in `require'
# ./spec/spec_helper.rb:1:in `<top (required)>'
# ./spec/lib/truncate_spec.rb:1:in `require'
# ./spec/lib/truncate_spec.rb:1:in `<top (required)>'
No examples found.

The notnulls definition is here (https://github.com/pganalyze/libpg_query/blob/802caf25b78ddeb8ade13cd4804d772b6d986eb3/protobuf/pg_query.proto#L307-L322):

message TableFunc
{
  repeated Node ns_uris = 1 [json_name="ns_uris"];
  repeated Node ns_names = 2 [json_name="ns_names"];
  Node docexpr = 3 [json_name="docexpr"];
  Node rowexpr = 4 [json_name="rowexpr"];
  repeated Node colnames = 5 [json_name="colnames"];
  repeated Node coltypes = 6 [json_name="coltypes"];
  repeated Node coltypmods = 7 [json_name="coltypmods"];
  repeated Node colcollations = 8 [json_name="colcollations"];
  repeated Node colexprs = 9 [json_name="colexprs"];
  repeated Node coldefexprs = 10 [json_name="coldefexprs"];
  repeated uint64 notnulls = 11 [json_name="notnulls"];
  int32 ordinalitycol = 12 [json_name="ordinalitycol"];
  int32 location = 13 [json_name="location"];
}

What did you expect to see

No errors.

What did you see instead?

  wrong argument type Hash (expected String)

The Ruby-compiled .proto yields:

      optional :ordinalitycol, :int32, 12, json_name: "ordinalitycol"

@haberman It seems options is no longer a parameter:

protobuf/ruby/lib/google/protobuf/descriptor_dsl.rb

Lines 304 to 306 in 66d7491

	def repeated(name, type, number, type_class = nil)
	internal_add_field(:LABEL_REPEATED, name, type, number, type_class, nil)
	end

[stanhu]

Thanks for the quick fix! It looks like #8936 fixes this.

[JoshCheek]

Problem

Hi. it's unclear to me whether this is fixed. I assume the pg_query code used to work, and then stopped working when updating to this version. So presumably the last 2 arguments could be missing, or either one or the other of them could be passed. Thus, passing options without type_class will cause the options to be set into the type_class arg.

Example

We can see that the pg_query code is failing, but if I insert an extra nil argument before the options hash, then they line up correctly and work:

# versions and such
🐠  ruby -v
ruby 3.0.1p64 (2021-04-05 revision 0fb782ee38) [arm64-darwin20]

🐠  uname -a
Darwin Joshs-MacBook-Air.local 20.3.0 Darwin Kernel Version 20.3.0: Thu Jan 21 00:06:51 PST 2021; root:xnu-7195.81.3~1/RELEASE_ARM64_T8101 arm64

🐠  gem list | ag 'pg_query|protobuf'
google-protobuf (4.0.0.rc.2, 3.18.1 universal-darwin)
pg_query (2.1.1)

# I can't load pg_query, b/c the options (a hash) gets passed as the `type_class` (expects a string)
🐠  ruby -r pg_query -e 0
/Users/josh/.gem/ruby/3.0.1/gems/pg_query-2.1.1/lib/pg_query/pg_query_pb.rb:295:in `repeated': wrong argument type Hash (expected String) (TypeError)
	from /Users/josh/.gem/ruby/3.0.1/gems/pg_query-2.1.1/lib/pg_query/pg_query_pb.rb:295:in `block (3 levels) in <top (required)>'
	from /Users/josh/.gem/ruby/3.0.1/gems/pg_query-2.1.1/lib/pg_query/pg_query_pb.rb:284:in `instance_eval'
	from /Users/josh/.gem/ruby/3.0.1/gems/pg_query-2.1.1/lib/pg_query/pg_query_pb.rb:284:in `add_message'
	from /Users/josh/.gem/ruby/3.0.1/gems/pg_query-2.1.1/lib/pg_query/pg_query_pb.rb:284:in `block (2 levels) in <top (required)>'
	from /Users/josh/.gem/ruby/3.0.1/gems/pg_query-2.1.1/lib/pg_query/pg_query_pb.rb:7:in `instance_eval'
	from /Users/josh/.gem/ruby/3.0.1/gems/pg_query-2.1.1/lib/pg_query/pg_query_pb.rb:7:in `add_file'
	from /Users/josh/.gem/ruby/3.0.1/gems/pg_query-2.1.1/lib/pg_query/pg_query_pb.rb:7:in `block in <top (required)>'
	from /Users/josh/.gem/ruby/3.0.1/gems/pg_query-2.1.1/lib/pg_query/pg_query_pb.rb:6:in `instance_eval'
	from /Users/josh/.gem/ruby/3.0.1/gems/pg_query-2.1.1/lib/pg_query/pg_query_pb.rb:6:in `build'
	from /Users/josh/.gem/ruby/3.0.1/gems/pg_query-2.1.1/lib/pg_query/pg_query_pb.rb:6:in `<top (required)>'
	from <internal:/Users/josh/.rubies/ruby-3.0.1/lib/ruby/3.0.0/rubygems/core_ext/kernel_require.rb>:96:in `require'
	from <internal:/Users/josh/.rubies/ruby-3.0.1/lib/ruby/3.0.0/rubygems/core_ext/kernel_require.rb>:96:in `require'
	from /Users/josh/.gem/ruby/3.0.1/gems/pg_query-2.1.1/lib/pg_query.rb:4:in `<top (required)>'
	from <internal:/Users/josh/.rubies/ruby-3.0.1/lib/ruby/3.0.0/rubygems/core_ext/kernel_require.rb>:160:in `require'
	from <internal:/Users/josh/.rubies/ruby-3.0.1/lib/ruby/3.0.0/rubygems/core_ext/kernel_require.rb>:160:in `rescue in require'
	from <internal:/Users/josh/.rubies/ruby-3.0.1/lib/ruby/3.0.0/rubygems/core_ext/kernel_require.rb>:149:in `require'
<internal:/Users/josh/.rubies/ruby-3.0.1/lib/ruby/3.0.0/rubygems/core_ext/kernel_require.rb>:85:in `require': cannot load such file -- pg_query (LoadError)
	from <internal:/Users/josh/.rubies/ruby-3.0.1/lib/ruby/3.0.0/rubygems/core_ext/kernel_require.rb>:85:in `require'


# Here are the invalid method invocations
🍣  cat /Users/josh/.gem/ruby/3.0.1/gems/pg_query-2.1.1/lib/pg_query/pg_query_pb.rb | ag repeated | ag uint
      repeated :notnulls, :uint64, 11, json_name: "notnulls"
      repeated :selected_cols, :uint64, 32, json_name: "selectedCols"
      repeated :inserted_cols, :uint64, 33, json_name: "insertedCols"
      repeated :updated_cols, :uint64, 34, json_name: "updatedCols"
      repeated :extra_updated_cols, :uint64, 35, json_name: "extraUpdatedCols"
      repeated :funcparams, :uint64, 7, json_name: "funcparams"


# We can insert an ordinal `nil` before the options hash
🐠  ruby -i -ape 'gsub /(?=json_name)/, "nil, " if $F[0] == "repeated" && $F[2] =~ /uint64/' \
       /Users/josh/.gem/ruby/3.0.1/gems/pg_query-2.1.1/lib/pg_query/pg_query_pb.rb


# So now the invocations look like this
🐠  cat /Users/josh/.gem/ruby/3.0.1/gems/pg_query-2.1.1/lib/pg_query/pg_query_pb.rb | ag repeated | ag uint
      repeated :notnulls, :uint64, 11, nil, json_name: "notnulls"
      repeated :selected_cols, :uint64, 32, nil, json_name: "selectedCols"
      repeated :inserted_cols, :uint64, 33, nil, json_name: "insertedCols"
      repeated :updated_cols, :uint64, 34, nil, json_name: "updatedCols"
      repeated :extra_updated_cols, :uint64, 35, nil, json_name: "extraUpdatedCols"
      repeated :funcparams, :uint64, 7, nil, json_name: "funcparams"


# And now I can successfully load `pg_query`
🐠  ruby -r pg_query -e 0

Potential solution:

This will probably work, assuming that this lib doesn't need to work with really old Rubies (🤔 also, if the options can be non-symbols, then you might need to do some testing, I think that in the early incarnations of keyword args, the ** wouldn't collect non-symbolic keys)

def repeated(name, type, number, type_class = nil, **options)
  internal_add_field(:LABEL_REPEATED, name, type, number, type_class, options)
end

[JoshCheek]

🤔 that said, I have no idea how this lib gets built. Those invocations don't seem to directly call this code, I assume they call some compiled C code based on this code. That piece of wiring is completely mystifying to me, so my evidence comes from changing the arguments rather than the parameters, and thus may not be valid.

[haberman]

Those invocations don't seem to directly call this code, I assume they call some compiled C code based on this code.

Prior to #8850, the DSL was implemented in C. If you are seeing the DSL call straight into C, rather than descriptor_dsl.rb, then I suspect that you are loading a version of the C extension prior to 3.18.

[JoshCheek]

It's loading version 4.0.0.rc.2:

ruby -r pg_query -e 'puts $LOADED_FEATURES.grep(/protobuf/)'
/Users/josh/.gem/ruby/3.0.1/gems/google-protobuf-4.0.0.rc.2/lib/google/protobuf/message_exts.rb
/Users/josh/.gem/ruby/3.0.1/gems/google-protobuf-4.0.0.rc.2/lib/google/protobuf_c.bundle
/Users/josh/.gem/ruby/3.0.1/gems/google-protobuf-4.0.0.rc.2/lib/google/protobuf/repeated_field.rb
/Users/josh/.gem/ruby/3.0.1/gems/google-protobuf-4.0.0.rc.2/lib/google/protobuf.rb

[haberman]

Ah, that would explain it. 4.0.0-rc2 was from around the time of 3.13. We thought we were going to increase the major version number, but ended up not doing it, and 4.0.0 was never released.

That version should be yanked from Ruby gems if it's not already.

[JoshCheek]

Hmm. I unfortunately can't check to see what happens in version 3 (it tries to dlopen protobuf_c.bundle, which wasn't built for the Apple M1), but we can compare the args to the params without needing to load up the libraries. And when we call a fn with the new signature, using the invocation from pg_query, we can see the options are set into the wrong variable.

RUBY_VERSION # => "3.0.2"

# https://github.com/protocolbuffers/protobuf/pull/8936/files#diff-9c33a618a9c7713c83d727ffa7fb2072a2afd1a8a29a41efd13dffa195216007R304
def repeated(name, type, number, type_class = nil, options=nil)
  name       # => :notnulls
  type       # => :uint64
  number     # => 11
  type_class # => {:json_name=>"notnulls"}
  options    # => nil
end

# https://github.com/pganalyze/pg_query/blob/e66b01cc85120dba3edc461af6e8883b326b292a/lib/pg_query/pg_query_pb.rb#L295
repeated :notnulls, :uint64, 11, json_name: "notnulls"

🤔 if it's not failing for anyone else, then perhaps the underlying internal_add_field rearranges its arguments?

Ahh, yeah, that's what happens, lol: descriptor_dsl.rb#L364-L367.

Disregard me, it is probably working on version 3.18 🤷