Project

General

Profile

Bug #14380

Expected transform_keys! to work just as transform_keys, but it doesn't

Added by taw (Tomasz Wegrzanowski) 30 days ago. Updated 14 days ago.

Status:
Open
Priority:
Normal
Target version:
-
ruby -v:
ruby 2.5.0p0 (2017-12-25 revision 61468) [x86_64-darwin17]
[ruby-core:85385]

Description

This seriously violates the Principle of Least Surprise to me:

{1 => :a, -1 => :b}.transform_keys{|k| -k} #=> {-1=>:a, 1=>:b}
{1 => :a, -1 => :b}.transform_keys!{|k| -k} # => {1=>:a}

# This fails:
ht=(1..10).map{|k| [k,k]}.to_h; ht.transform_keys(&:succ) # => {2=>1, 3=>2, 4=>3, 5=>4, 6=>5, 7=>6, 8=>7, 9=>8, 10=>9, 11=>10}
ht=(1..10).map{|k| [k,k]}.to_h; ht.transform_keys!(&:succ) # => {11=>1}

# This code with same issue works just because of key ordering:
ht=(1..10).map{|k| [k,k]}.to_h; ht.transform_keys(&:pred) #=> {0=>1, 1=>2, 2=>3, 3=>4, 4=>5, 5=>6, 6=>7, 7=>8, 8=>9, 9=>10}
ht=(1..10).map{|k| [k,k]}.to_h; ht.transform_keys!(&:pred) #=> {0=>1, 1=>2, 2=>3, 3=>4, 4=>5, 5=>6, 6=>7, 7=>8, 8=>9, 9=>10}

Of course in these examples it's very easy to see the problem, but in bigger programs it could be really difficult.

If the implementation instead did equivalent of:

class Hash
  def transform_values!(&block)
    replace transform_values(&block)
  end
end

it would be much less surprising.

Hash#transform_keys / Hash#transform_keys! inherently require that resulting values don't collide, but in these examples it works in surprising ways even though there's no collision between results.

Associated revisions

Revision 62042
Added by mrkn (Kenta Murata) 24 days ago

hash.c: support key swapping in Hash#transform_keys!

  • hash.c (rb_hash_transform_keys_bang): support key swapping in
    Hash#transform_keys!
    [Bug #14380]

  • test/ruby/test_hash.rb (test_transform_keys_bang):
    add assertions for this change

Revision 62044
Added by mrkn (Kenta Murata) 24 days ago

Fix rubyspec against the change in Hash#transform_keys!

[Bug #14380]

History

#1 [ruby-core:84952] Updated by shevegen (Robert A. Heiler) 30 days ago

Here is the current documentation:

http://ruby-doc.org/core-2.5.0/Hash.html#method-i-transform_keys

After looking at the example you gave, I can not say whether this is
deliberate or not - but either way, I think IF the behaviour is
retained as-is, then it should at the least be documented too.

In particular this code surprised me too:

hash = {1 => :a, -1 => :b}
pp hash.transform_keys{|k| -k} # => {-1=>:a, 1=>:b}
hash.transform_keys!{|k| -k}
pp hash # => {1=>:a}

I would have assumed that the method with the ! acts as the
method without ! but this is not the case. It may probably
not be a bug (I don't know though), but it should be documented
either way. People may look at the above code and ask "what
happened to key -1, if all keys are flipped from negative to
positive and vice versa?"

To your code example of an alternative, I am confused too.

Why do you have a method called "transform_values!" if the
values that are modified are on the keys, as exemplified
by the name "transform_keys!"?

#2 [ruby-core:84959] Updated by taw (Tomasz Wegrzanowski) 29 days ago

Oops, I meant to suggest this, accidentally said "values" instead of "keys" :

class Hash
  def transform_keys!(&block)
    replace transform_keys(&block)
  end
end

#3 [ruby-core:84960] Updated by phluid61 (Matthew Kerwin) 29 days ago

taw (Tomasz Wegrzanowski) wrote:

Oops, I meant to suggest this, accidentally said "values" instead of "keys" :

class Hash
  def transform_keys!(&block)
    replace transform_keys(&block)
  end
end

IIRC this was discussed when the feature was originally proposed. What happens to a break inside the block?

#4 [ruby-core:84962] Updated by shyouhei (Shyouhei Urabe) 28 days ago

phluid61 (Matthew Kerwin) wrote:

IIRC this was discussed when the feature was originally proposed.

Here you are. https://bugs.ruby-lang.org/issues/13583

#5 [ruby-core:85034] Updated by mrkn (Kenta Murata) 26 days ago

  • Assignee set to mrkn (Kenta Murata)
  • Status changed from Open to Assigned

#6 Updated by mrkn (Kenta Murata) 24 days ago

  • Status changed from Assigned to Closed

Applied in changeset trunk|r62042.


hash.c: support key swapping in Hash#transform_keys!

  • hash.c (rb_hash_transform_keys_bang): support key swapping in
    Hash#transform_keys!
    [Bug #14380]

  • test/ruby/test_hash.rb (test_transform_keys_bang):
    add assertions for this change

#7 [ruby-core:85218] Updated by Eregon (Benoit Daloze) 21 days ago

Will this be backported to 2.5?

#8 [ruby-core:85234] Updated by marcandre (Marc-Andre Lafortune) 21 days ago

  • Assignee changed from mrkn (Kenta Murata) to matz (Yukihiro Matsumoto)
  • Status changed from Closed to Open

I raised this issue previously https://bugs.ruby-lang.org/issues/13583#note-8

This is a spec change. Moreover it introduces incompatibilities with ActiveSupport.

Matz: final verdict please?

#9 [ruby-core:85367] Updated by taw (Tomasz Wegrzanowski) 16 days ago

marcandre (Marc-Andre Lafortune) wrote:

I raised this issue previously https://bugs.ruby-lang.org/issues/13583#note-8

This is a spec change. Moreover it introduces incompatibilities with ActiveSupport.

I'd say that ActiveSupport is testing group for possible new ruby features,
and when getting them into ruby core library, it's good time to clean up their edge cases.

Also available in: Atom PDF