[Ruby] Support for currently ignored Array methods in RepeatedField

[marianosimone]

What language does this apply to?
This is about protobuffer's RepeatedField compatibility with Ruby 3 Array methods.

Describe the problem you are trying to solve.
I discovered this when trying to apply Class: RuboCop::Cop::Style::ArrayIntersect to the Ruby application I'm working on. In particular, this lints against (array1 & array2).empty?, as it can be replaced by the more efficient and idiomatic !array1.intersect?(array2).

However, when array1 is a RepeatedField instead, this doesn't work, as RepeatedField does not have that method, even though it mostly behaves like an array.

Describe the solution you'd like
I'd like to see this code also delegate intersect?:

protobuf/ruby/lib/google/protobuf/repeated_field.rb

Lines 30 to 55 in a4576cb

	# methods defined in C or Java:
	# +
	# [], at
	# []=
	# concat
	# clear
	# dup, clone
	# each
	# push, <<
	# replace
	# length, size
	# ==
	# to_ary, to_a
	# also all enumerable
	#
	# NOTE: using delegators rather than method_missing to make the
	# relationship explicit instead of implicit
	def_delegators :to_ary,
	:&, :*, :-, :'<=>',
	:assoc, :bsearch, :bsearch_index, :combination, :compact, :count,
	:cycle, :dig, :drop, :drop_while, :eql?, :fetch, :find_index, :flatten,
	:include?, :index, :inspect, :join,
	:pack, :permutation, :product, :pretty_print, :pretty_print_cycle,
	:rassoc, :repeated_combination, :repeated_permutation, :reverse,
	:rindex, :rotate, :sample, :shuffle, :shelljoin,
	:to_s, :transpose, :uniq, :|

It seems like this method is explicitly being ignored in a compatibility test (alongside other interesting ones):

protobuf/ruby/compatibility_tests/v3.0.0/tests/repeated_field_test.rb

Lines 23 to 26 in a4576cb

	# ruby 2.7 methods we can ignore
	arr_methods -= [:intersection, :deconstruct, :resolve_feature_path]
	# ruby 3.1 methods we can ignore
	arr_methods -= [:intersect?]

I see that the exclusion was added in [protobuf/pr/9645] Fixed Ruby 3.1 tests by marking intersect? as unimplemented by @haberman, but I couldn't find the reasoning behind excluding them, instead of adding the functionality. I'd be happy to create a PR to get this moving, but didn't know if there's any reason to avoid it.

Describe alternatives you've considered
Ignoring the linting suggestion, and adding a comment in the code explaining why it doesn't work

[BrianHawley]

It looks like the list of forwarded methods is a fixed list that was written 9 years ago, and hasn't been updated since.

The gem now requires Ruby 2.7 or above, so it would make sense to update the list of forwarded and wrapped methods to include the ones added to Array as of at least that version. It would also make sense to use feature detection code (respond_to? checks, for instance), to delegate more methods if they exist in Array.

@marianosimone in the short term, you can patch the class to add the missing methods. You'd have to check the source to determine which methods could be delegated and which would need custom wrappers.

[marianosimone]

It looks like the list of forwarded methods is a fixed list that was written 9 years ago, and hasn't been updated since.

The gem now requires Ruby 2.7 or above, so it would make sense to update the list of forwarded and wrapped methods to include the ones added to Array as of at least that version

I created [protobuf/pr/15652] [Ruby] Delegate difference, intersection, union from RepeatedField to Array to start delegating some of the methods, as I found it confusing that some of their versions were delegated, while others weren't (e.g. | vs union)

It would also make sense to use feature detection code (respond_to? checks, for instance), to delegate more methods if they exist in Array.

I'm hesitant about this because of this comment: https://github.com/marianosimone/protobuf/blob/58b582ba310afda710585211cc75e7b140284593/ruby/lib/google/protobuf/repeated_field.rb#L45-L46

It looks like being explicit was a design decision.

It would be nice, though, to be able to have per-version delegation (e.g., if using Ruby 3, I'd like to be able to use intersect?). Is there any prior art on supporting something like this?

@marianosimone in the short term, you can patch the class to add the missing methods. You'd have to check the source to determine which methods could be delegated and which would need custom wrappers.

Yeah, this is what we are going to do internally for now, but would love to see it fixed upstream :)

[JasonLunn]

Closing as #15652 has merged. Please reopen if there is still more to do.

[marianosimone]

I don't have permissions to re-open, so I can't :)

Arguably, this is not completely solved, as there's no ongoing solution for all methods, and it just works with the ones I added to the allowlist for delegation.