In Ruby repeated fields, each_index actually iterates over the index (#11767)

shaldengeki · copybara-github · commit f869cfa479c2 · 2023-12-26T12:14:24.000-08:00
Currently we're aliasing `each_index` to `each_with_index`, incorrectly passing both the index and the value of a repeated field to the block. What we want is to just pass the index. Luckily this is a method on Ruby arrays, so we just wrap the native Ruby array method. Fixes #7806 Closes #11767 COPYBARA_INTEGRATE_REVIEW=#11767 from shaldengeki:shaldengeki-repeated-field-each-index-returns-actual-index 874916c PiperOrigin-RevId: 593835025
diff --git a/ruby/compatibility_tests/v3.0.0/tests/repeated_field_test.rb b/ruby/compatibility_tests/v3.0.0/tests/repeated_field_test.rb
@@ -122,6 +122,20 @@ def test_each
   end
 
 
+  def test_each_index
+    m = TestMessage.new
+    5.times{|i| m.repeated_string << 'string' }
+
+    expected = 0
+    m.repeated_string.each_index do |idx|
+      assert_equal expected, idx
+      expected += 1
+      assert_equal 'string', m.repeated_string[idx]
+    end
+    assert_equal 5, expected
+  end
+
+
   def test_empty?
     m = TestMessage.new
     assert_empty m.repeated_string
diff --git a/ruby/lib/google/protobuf/repeated_field.rb b/ruby/lib/google/protobuf/repeated_field.rb
@@ -94,7 +94,6 @@ def empty?
       end
 
       # array aliases into enumerable
-      alias_method :each_index, :each_with_index
       alias_method :slice, :[]
       alias_method :values_at, :select
       alias_method :map, :collect
@@ -145,7 +144,7 @@ def define_array_wrapper_with_result_method(method_name)
       end
 
 
-      %w(collect! compact! delete_if fill flatten! insert reverse!
+      %w(collect! compact! delete_if each_index fill flatten! insert reverse!
         rotate! select! shuffle! sort! sort_by! uniq!).each do |method_name|
         define_array_wrapper_with_result_method(method_name)
       end
diff --git a/ruby/tests/repeated_field_test.rb b/ruby/tests/repeated_field_test.rb
@@ -143,6 +143,20 @@ def test_each
   end
 
 
+  def test_each_index
+    m = TestMessage.new
+    5.times{|i| m.repeated_string << 'string' }
+
+    expected = 0
+    m.repeated_string.each_index do |idx|
+      assert_equal expected, idx
+      expected += 1
+      assert_equal 'string', m.repeated_string[idx]
+    end
+    assert_equal 5, expected
+  end
+
+
   def test_empty?
     m = TestMessage.new
     assert_empty m.repeated_string
