Project

General

Profile

Actions

Bug #14380

closed

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

Added by taw (Tomasz Wegrzanowski) about 6 years ago. Updated about 6 years ago.

Status:
Closed
Target version:
-
ruby -v:
ruby 2.5.0p0 (2017-12-25 revision 61468) [x86_64-darwin17]
[ruby-core:84951]

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.

Updated by shevegen (Robert A. Heiler) about 6 years 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!"?

Updated by taw (Tomasz Wegrzanowski) about 6 years 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

Updated by phluid61 (Matthew Kerwin) about 6 years 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?

Updated by shyouhei (Shyouhei Urabe) about 6 years 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

Updated by mrkn (Kenta Murata) about 6 years ago

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

Updated by mrkn (Kenta Murata) about 6 years 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] [ruby-core:84951]

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

Updated by Eregon (Benoit Daloze) about 6 years ago

Will this be backported to 2.5?

Updated by marcandre (Marc-Andre Lafortune) about 6 years ago

  • 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?

Updated by taw (Tomasz Wegrzanowski) about 6 years 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.

Actions #10

Updated by usa (Usaku NAKAMURA) about 6 years ago

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

Updated by naruse (Yui NARUSE) about 6 years ago

  • Status changed from Open to Closed

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

Updated by rafaelfranca (Rafael França) about 6 years ago

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.

Updated by naruse (Yui NARUSE) about 6 years ago

  • 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.

Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0Like0