From e83f7800211b7fcc3543c0297d71590072bf6317 Mon Sep 17 00:00:00 2001 From: Andrew Konchin Date: Thu, 27 Jun 2024 17:22:19 +0300 Subject: [PATCH 1/3] Fix Hash#to_h called with a block and pass key and value to the block as separate arguments --- CHANGELOG.md | 1 + spec/ruby/core/hash/to_h_spec.rb | 10 ++++++++++ src/main/ruby/truffleruby/core/hash.rb | 16 +++++++++++++++- 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 100324b302a0..05c606d3286a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,7 @@ Compatibility: * Fix `Enumerable#reduce` to handle non-Symbol method name parameter (#2931, @andrykonchin). * Fix `RangeError` message to match CRuby for `Integer#chr` called with invalid codepoint argument (#2795, @andrykonchin). * Joni has been updated from 2.1.44 to 2.2.1 (@andrykonchin). +* Fix `Hash#to_h` called with a block and pass key and value to the block as separate arguments (#3607, @andrykonchin). Performance: diff --git a/spec/ruby/core/hash/to_h_spec.rb b/spec/ruby/core/hash/to_h_spec.rb index 75ebce68b1eb..e17ca7e67112 100644 --- a/spec/ruby/core/hash/to_h_spec.rb +++ b/spec/ruby/core/hash/to_h_spec.rb @@ -37,6 +37,16 @@ { a: 1, b: 2 }.to_h { |k, v| [k.to_s, v*v]}.should == { "a" => 1, "b" => 4 } end + it "passes to a block each pair's key and value as separate arguments" do + ScratchPad.record [] + { a: 1, b: 2 }.to_h { |k, v| ScratchPad << [k, v]; [k, v] } + ScratchPad.recorded.sort.should == [[:a, 1], [:b, 2]] + + ScratchPad.record [] + { a: 1, b: 2 }.to_h { |*args| ScratchPad << args; [args[0], args[1]] } + ScratchPad.recorded.sort.should == [[:a, 1], [:b, 2]] + end + it "raises ArgumentError if block returns longer or shorter array" do -> do { a: 1, b: 2 }.to_h { |k, v| [k.to_s, v*v, 1] } diff --git a/src/main/ruby/truffleruby/core/hash.rb b/src/main/ruby/truffleruby/core/hash.rb index 010e1d330403..b9da01386904 100644 --- a/src/main/ruby/truffleruby/core/hash.rb +++ b/src/main/ruby/truffleruby/core/hash.rb @@ -355,7 +355,21 @@ def slice(*keys) def to_h if block_given? - super + h = {} + each do |k, v| + elem = yield(k, v) + unless elem.respond_to?(:to_ary) + raise TypeError, "wrong element type #{Primitive.class(elem)} (expected array)" + end + + ary = elem.to_ary + if ary.size != 2 + raise ArgumentError, "element has wrong array length (expected 2, was #{ary.size})" + end + + h[ary[0]] = ary[1] + end + h elsif instance_of? Hash self else From 97182712ec1e8c42b03f8bdab608ae8ed6646aaf Mon Sep 17 00:00:00 2001 From: Andrew Konchin Date: Thu, 27 Jun 2024 17:29:26 +0300 Subject: [PATCH 2/3] Adjust "wrong element type..." error message for Enumerable#to_h --- src/main/ruby/truffleruby/core/enumerable.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/main/ruby/truffleruby/core/enumerable.rb b/src/main/ruby/truffleruby/core/enumerable.rb index 30667a96105e..2ba17ea14ce1 100644 --- a/src/main/ruby/truffleruby/core/enumerable.rb +++ b/src/main/ruby/truffleruby/core/enumerable.rb @@ -314,12 +314,13 @@ def to_a(*args, **kwargs) end alias_method :entries, :to_a - def to_h(*arg) + def to_h(*args) h = {} - each_with_index(*arg) do |elem, i| + each(*args) do + elem = Primitive.single_block_arg elem = yield(elem) if block_given? unless elem.respond_to?(:to_ary) - raise TypeError, "wrong element type #{Primitive.class(elem)} at #{i} (expected array)" + raise TypeError, "wrong element type #{Primitive.class(elem)} (expected array)" end ary = elem.to_ary From ca0c802c3bc054ddbc5ef0cd37f2563f1e32ff7b Mon Sep 17 00:00:00 2001 From: Andrew Konchin Date: Fri, 28 Jun 2024 16:38:04 +0300 Subject: [PATCH 3/3] Refactor {Hash, Array, Enumerable, Struct , ENV}#to_h methods and extract processing of returned by a block value into a helper method --- spec/ruby/core/array/to_h_spec.rb | 6 ++++ spec/ruby/core/enumerable/to_h_spec.rb | 8 +++++ spec/ruby/core/env/to_h_spec.rb | 12 ++++++++ spec/ruby/core/struct/to_h_spec.rb | 12 ++++++++ src/main/ruby/truffleruby/core/array.rb | 13 ++------ src/main/ruby/truffleruby/core/enumerable.rb | 11 +------ src/main/ruby/truffleruby/core/env.rb | 15 ++-------- src/main/ruby/truffleruby/core/hash.rb | 13 ++------ src/main/ruby/truffleruby/core/struct.rb | 15 ++-------- .../core/truffle/hash_operations.rb | 30 +++++++++++++++++++ 10 files changed, 79 insertions(+), 56 deletions(-) diff --git a/spec/ruby/core/array/to_h_spec.rb b/spec/ruby/core/array/to_h_spec.rb index f4578211a1f1..1c814f3d012c 100644 --- a/spec/ruby/core/array/to_h_spec.rb +++ b/spec/ruby/core/array/to_h_spec.rb @@ -45,6 +45,12 @@ [:a, :b].to_h { |k| [k, k.to_s] }.should == { a: 'a', b: 'b' } end + it "passes to a block each element as a single argument" do + ScratchPad.record [] + [[:a, 1], [:b, 2]].to_h { |*args| ScratchPad << args; [args[0], args[1]] } + ScratchPad.recorded.sort.should == [[[:a, 1]], [[:b, 2]]] + end + it "raises ArgumentError if block returns longer or shorter array" do -> do [:a, :b].to_h { |k| [k, k.to_s, 1] } diff --git a/spec/ruby/core/enumerable/to_h_spec.rb b/spec/ruby/core/enumerable/to_h_spec.rb index 0489134552ef..11a4933c10c4 100644 --- a/spec/ruby/core/enumerable/to_h_spec.rb +++ b/spec/ruby/core/enumerable/to_h_spec.rb @@ -53,6 +53,14 @@ def enum.each(*args) @enum.to_h { |k| [k, k.to_s] }.should == { a: 'a', b: 'b' } end + it "passes to a block each element as a single argument" do + enum_of_arrays = EnumerableSpecs::EachDefiner.new([:a, 1], [:b, 2]) + + ScratchPad.record [] + enum_of_arrays.to_h { |*args| ScratchPad << args; [args[0], args[1]] } + ScratchPad.recorded.sort.should == [[[:a, 1]], [[:b, 2]]] + end + it "raises ArgumentError if block returns longer or shorter array" do -> do @enum.to_h { |k| [k, k.to_s, 1] } diff --git a/spec/ruby/core/env/to_h_spec.rb b/spec/ruby/core/env/to_h_spec.rb index 3c4a92aa579a..58ea2d2030e5 100644 --- a/spec/ruby/core/env/to_h_spec.rb +++ b/spec/ruby/core/env/to_h_spec.rb @@ -18,6 +18,18 @@ ENV.to_h { |k, v| [k, v.upcase] }.should == { 'a' => "B", 'c' => "D" } end + it "passes to a block each pair's key and value as separate arguments" do + ENV.replace("a" => "b", "c" => "d") + + ScratchPad.record [] + ENV.to_h { |k, v| ScratchPad << [k, v]; [k, v] } + ScratchPad.recorded.sort.should == [["a", "b"], ["c", "d"]] + + ScratchPad.record [] + ENV.to_h { |*args| ScratchPad << args; [args[0], args[1]] } + ScratchPad.recorded.sort.should == [["a", "b"], ["c", "d"]] + end + it "does not require the array elements to be strings" do ENV.replace("a" => "b", "c" => "d") ENV.to_h { |k, v| [k.to_sym, v.to_sym] }.should == { :a => :b, :c => :d } diff --git a/spec/ruby/core/struct/to_h_spec.rb b/spec/ruby/core/struct/to_h_spec.rb index bfb0af07baf8..861ce3f49df4 100644 --- a/spec/ruby/core/struct/to_h_spec.rb +++ b/spec/ruby/core/struct/to_h_spec.rb @@ -21,6 +21,18 @@ h.should == { "make" => "ford", "model" => "ranger", "year" => "" } end + it "passes to a block each pair's key and value as separate arguments" do + s = StructClasses::Ruby.new('3.2.4', 'macos') + + ScratchPad.record [] + s.to_h { |k, v| ScratchPad << [k, v]; [k, v] } + ScratchPad.recorded.sort.should == [[:platform, 'macos'], [:version, '3.2.4']] + + ScratchPad.record [] + s.to_h { |*args| ScratchPad << args; [args[0], args[1]] } + ScratchPad.recorded.sort.should == [[:platform, 'macos'], [:version, '3.2.4']] + end + it "raises ArgumentError if block returns longer or shorter array" do -> do StructClasses::Car.new.to_h { |k, v| [k.to_s, "#{v}".downcase, 1] } diff --git a/src/main/ruby/truffleruby/core/array.rb b/src/main/ruby/truffleruby/core/array.rb index 2c5d30941371..2cd95a5ddca8 100644 --- a/src/main/ruby/truffleruby/core/array.rb +++ b/src/main/ruby/truffleruby/core/array.rb @@ -1296,19 +1296,10 @@ def to_ary end def to_h - h = Hash.new + h = {} each_with_index do |elem, i| elem = yield(elem) if block_given? - unless elem.respond_to?(:to_ary) - raise TypeError, "wrong element type #{Primitive.class(elem)} at #{i} (expected array)" - end - - ary = elem.to_ary - if ary.size != 2 - raise ArgumentError, "wrong array length at #{i} (expected 2, was #{ary.size})" - end - - h[ary[0]] = ary[1] + Truffle::HashOperations.assoc_key_value_pair_with_position(h, elem, i) end h end diff --git a/src/main/ruby/truffleruby/core/enumerable.rb b/src/main/ruby/truffleruby/core/enumerable.rb index 2ba17ea14ce1..64ff9973cdbb 100644 --- a/src/main/ruby/truffleruby/core/enumerable.rb +++ b/src/main/ruby/truffleruby/core/enumerable.rb @@ -319,16 +319,7 @@ def to_h(*args) each(*args) do elem = Primitive.single_block_arg elem = yield(elem) if block_given? - unless elem.respond_to?(:to_ary) - raise TypeError, "wrong element type #{Primitive.class(elem)} (expected array)" - end - - ary = elem.to_ary - if ary.size != 2 - raise ArgumentError, "element has wrong array length (expected 2, was #{ary.size})" - end - - h[ary[0]] = ary[1] + Truffle::HashOperations.assoc_key_value_pair(h, elem) end h end diff --git a/src/main/ruby/truffleruby/core/env.rb b/src/main/ruby/truffleruby/core/env.rb index bb8abfdd3b6e..5b813a2b7e7f 100644 --- a/src/main/ruby/truffleruby/core/env.rb +++ b/src/main/ruby/truffleruby/core/env.rb @@ -286,18 +286,9 @@ def to_h return to_hash unless block_given? h = {} - each_pair do |*elem| - elem = yield(elem) - unless elem.respond_to?(:to_ary) - raise TypeError, "wrong element type #{Primitive.class(elem)} (expected array)" - end - - ary = elem.to_ary - if ary.size != 2 - raise ArgumentError, "element has wrong array length (expected 2, was #{ary.size})" - end - - h[ary[0]] = ary[1] + each_pair do |k, v| + pair = yield(k, v) + Truffle::HashOperations.assoc_key_value_pair(h, pair) end h end diff --git a/src/main/ruby/truffleruby/core/hash.rb b/src/main/ruby/truffleruby/core/hash.rb index b9da01386904..7cae15155910 100644 --- a/src/main/ruby/truffleruby/core/hash.rb +++ b/src/main/ruby/truffleruby/core/hash.rb @@ -357,17 +357,8 @@ def to_h if block_given? h = {} each do |k, v| - elem = yield(k, v) - unless elem.respond_to?(:to_ary) - raise TypeError, "wrong element type #{Primitive.class(elem)} (expected array)" - end - - ary = elem.to_ary - if ary.size != 2 - raise ArgumentError, "element has wrong array length (expected 2, was #{ary.size})" - end - - h[ary[0]] = ary[1] + pair = yield(k, v) + Truffle::HashOperations.assoc_key_value_pair(h, pair) end h elsif instance_of? Hash diff --git a/src/main/ruby/truffleruby/core/struct.rb b/src/main/ruby/truffleruby/core/struct.rb index 6a930a38071a..b976191691c8 100644 --- a/src/main/ruby/truffleruby/core/struct.rb +++ b/src/main/ruby/truffleruby/core/struct.rb @@ -109,18 +109,9 @@ def select def to_h h = {} - each_pair.each_with_index do |elem, i| - elem = yield(elem) if block_given? - unless elem.respond_to?(:to_ary) - raise TypeError, "wrong element type #{Primitive.class(elem)} at #{i} (expected array)" - end - - ary = elem.to_ary - if ary.size != 2 - raise ArgumentError, "element has wrong array length (expected 2, was #{ary.size})" - end - - h[ary[0]] = ary[1] + each_pair do |k, v| + pair = block_given? ? yield(k, v) : [k, v] + Truffle::HashOperations.assoc_key_value_pair(h, pair) end h end diff --git a/src/main/ruby/truffleruby/core/truffle/hash_operations.rb b/src/main/ruby/truffleruby/core/truffle/hash_operations.rb index fae40ff1afad..4a3aade6a679 100644 --- a/src/main/ruby/truffleruby/core/truffle/hash_operations.rb +++ b/src/main/ruby/truffleruby/core/truffle/hash_operations.rb @@ -17,5 +17,35 @@ def self.hash_merge(current, other) end new_hash end + + # MRI: rb_hash_set_pair (rb_ary_to_h also contains similar logic for Array#to_h) + def self.assoc_key_value_pair(hash, pair) + ary = Truffle::Type.rb_check_convert_type pair, Array, :to_ary + + unless ary + raise TypeError, "wrong element type #{Primitive.class(pair)} (expected array)" + end + + if ary.size != 2 + raise ArgumentError, "element has wrong array length (expected 2, was #{ary.size})" + end + + hash[ary[0]] = ary[1] + end + + # MRI: extracted from rb_ary_to_h, is similar to rb_hash_set_pair + def self.assoc_key_value_pair_with_position(hash, pair, index) + ary = Truffle::Type.rb_check_convert_type pair, Array, :to_ary + + unless ary + raise TypeError, "wrong element type #{Primitive.class(pair)} at #{index} (expected array)" + end + + if ary.size != 2 + raise ArgumentError, "wrong array length at #{index} (expected 2, was #{ary.size})" + end + + hash[ary[0]] = ary[1] + end end end