Skip to content

Add Hash#transform_keys! #16280

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
+49 −3
Open

Add Hash#transform_keys! #16280

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f79ec79
Add Hash#transform_keys!
andrykonchin Oct 27, 2025
d61b81d
Add a test case for key subtypes
andrykonchin Nov 3, 2025
2382ff3
Use a union type as a super type in specs
andrykonchin Nov 4, 2025
9c98056
Merge remote-tracking branch 'upstream/master' into ak/add-hash-trans…
andrykonchin Nov 5, 2025
8151b0c
Simplify Set#map! using Hash#transform_keys!
andrykonchin Nov 5, 2025
01e502d
Refactor Hash#transform_keys! and inline #transform_keys
andrykonchin Nov 5, 2025
44b8f05
Reverse inlining
andrykonchin Nov 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions spec/std/hash_spec.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -923,6 +923,29 @@ describe "Hash" do
h2.should eq({"1a" => "a", "2b" => "b", "3c" => "c"})
end

describe "transform_keys!" do
Copy link
Member

@straight-shoota straight-shoota Nov 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: It would be nice to add an example where the transformed key type is a subtype of the original type. This should already work correctly because the proc's output type is K (this forces a cast on captured blocks). But a spec would make sure it stays that way.

{"1" => "foo", 2 => "bar"}.transform_keys!(&.to_i).should eq({1 => "foo", 2 => "bar"})

Copy link
Contributor Author

@andrykonchin andrykonchin Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

@straight-shoota straight-shoota Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a particular reason for introducing explicit types for this instead of using the example I provided?
The key type of the hash is Int32 | String, but to_i returns only Int32.
Union types work just fine for demonstration, there should be no need for explicit type inheritance.
This would be a bit more concise because we don't need the type definitions.

Copy link
Contributor Author

@andrykonchin andrykonchin Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right. I haven't noticed that keys in {"1" => ..., 2 => ...} are of different types. And indeed a union type is a super type of any of its parts.

Copy link
Contributor Author

@andrykonchin andrykonchin Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

@Fryguy Fryguy Nov 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there also be a negative assertion test, where the transformed key type is not a subtype of the original type?

Copy link
Contributor Author

@andrykonchin andrykonchin Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It will lead to a compilation error. Is it convenient to have such tests?

Copy link
Contributor

@Fryguy Fryguy Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure if we do those kinds of tests to show the intention that it shouldn't work and/or that that's a known case. In this case, it "works" with transform_keys, but not with transform_keys!, so it highlights a difference between the two.

Copy link
Member

@straight-shoota straight-shoota Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah unfortunately, tests for compilation errors are quite inconvenient. Especially the runtime cost is prohibitive. They need to run an extra compiler which is multiple orders of magnitude slower than a simple positive spec.
So we usually don't do that.
It would be pretty awesome to have a more efficient way via some compiler primitive.

Copy link
Contributor

@Fryguy Fryguy Nov 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense @straight-shoota thanks. We do have a doc comment that the type of the keys can't change, so at least that's in place.

it "transforms keys in place" do
h = {1 => "a", 2 => "b", 3 => "c"}

h.transform_keys! { |x| x + 1 }.should be(h)
h.should eq({2 => "a", 3 => "b", 4 => "c"})
end

it "does nothing when empty hash" do
h = {} of Int32 => String

h.transform_keys! { |x| x + 1 }
h.should be_empty
end

it "transforms keys with values included" do
h = {"1" => "a", "2" => "b", "3" => "c"}

h.transform_keys! { |k, v| "#{k}#{v}" }
h.should eq({"1a" => "a", "2b" => "b", "3c" => "c"})
end
end

it "transforms values" do
h1 = {"a" => 1, "b" => 2, "c" => 3}

Expand Down
18 changes: 18 additions & 0 deletions src/hash.cr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1755,6 +1755,24 @@ class Hash(K, V)
end
end

# Destructively transforms all keys using a block. Same as transform_keys but modifies in place.
# The block cannot change a type of keys.
# The block yields the key and value.
#
# ```
# hash = {"a" => 1, "b" => 2, "c" => 3}
# hash.transform_keys! { |key| key.upcase }
# hash # => {"A" => 1, "B" => 2, "C" => 3}
# hash.transform_keys! { |key, value| key * value }
# hash # => {"a" => 1, "bb" => 2, "ccc" => 3}
# ```
def transform_keys!(&block : K, V -> K) : self
transformed_hash = self.transform_keys(&block)
Copy link
Contributor

@HertzDevil HertzDevil Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably the block forwarding was chosen here because the yielding form wouldn't compile due to a mismatch between K2 and K. In this case I think it is acceptable to just inline #transform_keys's body here, since captured blocks have a non-negligible runtime cost:

Suggested change
transformed_hash = self.transform_keys(&block)
transformed_hash = Hash(K, V).new(initial_capacity: entries_capacity)
each_with_object(transformed_hash) do |(key, value), memo|
memo[yield(key, value)] = value
end

And then Set#map! could also use the yielding form.

Copy link
Contributor Author

@andrykonchin andrykonchin Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you mean by "yielding form" this approach:

transformed_hash = self.transform_keys { |k, v| yield k, v }

then yes, it doesn't compile:

bin/crystal spec/std/hash_spec.cr
Using compiled compiler at .build/crystal
Showing last frame. Use --error-trace for full trace.

In src/pointer.cr:146:29

 146 | (self + offset).value = value
                               ^----
Error: type must be Hash::Entry(Int32 | String, String), not Hash::Entry(Int32, String)

Copy link
Contributor Author

@andrykonchin andrykonchin Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thank you!

Copy link
Member

@straight-shoota straight-shoota Nov 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we should be fine with an explicit cast to K. No need to duplicate the implementation.

    transformed_hash = transform_keys{ |k, v| (yield k, v).as(K) }

Sorry for the back and forth Andrii, and thanks for staying around with us 🫶

Copy link
Contributor Author

@andrykonchin andrykonchin Nov 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙂

Done.

initialize_dup_entries(transformed_hash) # we need only to copy the buffer
initialize_copy_non_entries_vars(transformed_hash)
self
end
Copy link
Contributor Author

@andrykonchin andrykonchin Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably makes sense to move the copying logic into a new method #replace, similar to Hash#replace in Ruby.


# Returns a new hash with the results of running block once for every value.
# The block can change a type of values.
# The block yields the value and key.
Expand Down