Bug #14380: Expected transform_keys! to work just as transform_keys, but it doesn't

• Status: Closed
• Assignee: matz (Yukihiro Matsumoto)
• Target version: -
• ruby -v: ruby 2.5.0p0 (2017-12-25 revision 61468) [x86_64-darwin17]
• Backport: 2.3: DONTNEED, 2.4: DONTNEED, 2.5: DONE

[taw (Tomasz Wegrzanowski)]

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.

[shevegen (Robert A. Heiler)]

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!"?

[taw (Tomasz Wegrzanowski)]

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

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

[phluid61 (Matthew Kerwin)]

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?

[shyouhei (Shyouhei Urabe)]

phluid61 (Matthew Kerwin) wrote:

IIRC this was discussed when the feature was originally proposed.

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

[mrkn (Kenta Murata)]

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

[mrkn (Kenta Murata)]

• 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] [ruby-core:84951]

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

[Eregon (Benoit Daloze)]

Will this be backported to 2.5?

[marcandre (Marc-Andre Lafortune)]

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

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?

[taw (Tomasz Wegrzanowski)]

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.

[usa (Usaku NAKAMURA)]

• Backport changed from 2.3: UNKNOWN, 2.4: UNKNOWN, 2.5: UNKNOWN to 2.3: DONTNEED, 2.4: DONTNEED, 2.5: REQUIRED

[naruse (Yui NARUSE)]

• Status changed from Open to Closed

Considered a bug and the behavior change will also be applied in ActiveSupport.

[rafaelfranca (Rafael França)]

change will also be applied in ActiveSupport

Where did you get that information?

This change silently breaks applications when upgrading to 2.6 version. Backward compatibility not only with active support but with an already released version of Ruby should be took in consideration here.

I don't think the Rails team would accept this change as a bug fix.

[naruse (Yui NARUSE)]

• Backport changed from 2.3: DONTNEED, 2.4: DONTNEED, 2.5: REQUIRED to 2.3: DONTNEED, 2.4: DONTNEED, 2.5: DONE

ruby_2_5 r62889 merged revision(s) 62042,62044.