Crystal: Rename 'Hash#key?' to make clear it's not an alias to `Hash#has_key?`

[marksiemers]

Original title: 'Hash#key?' returning nil when 'Hash#has_key?' is returning true

Crystal 0.23.1

https://play.crystal-lang.org/#/r/31cd

valve = :web

pipeline = { valve => "value" }

pp pipeline.key? valve
if pipeline.key? valve
  puts "Truthy"
else
  puts "Falsey"
end

pp pipeline[valve]

if pipeline[valve]
  puts "Truthy"
else
  puts "Falsey"
end

pp pipeline.has_key? valve

if pipeline.has_key? valve
  puts "Truthy"
else
  puts "Falsey"
end

Produces:

pipeline.key?(valve) # => nil
Falsey
pipeline[valve] # => "value"
Truthy
pipeline.has_key?(valve) # => true
Truthy

I have not tested against master yet - haven't been able to build crystal locally.

[RX14]

This is intended, key? searches for a key given a value. :web isn't a value, so it has no key. See here.

[asterite]

However in Ruby it returns a boolean. I don't know why we ended up with a different behavior, it should be an alias of has_key?. And for that reason we should remove one of those two methods.

[lbguilherme]

Perhaps rename key? to key_for_value to make it not so ambiguous.

[asterite]

Ooh... Now I see the difference. Maybe key_for_vaule is a good name, or we could also leave it like that for now. It's a difference with Ruby but it doesn't matter

[marksiemers]

FWIW, I like the method signature key_for(value)

[RX14]

I'd personally be for renaming this. Should we make an issue?

[jhass]

Why not recycle this one.

[drosehn]

Maybe key_matching(value) (but key_for(value) is also fine with me)

[paulcsmith]

I think key_for_value is the most clear. Either way it should almost for sure not end with ? as that usually signifies that the return type would be a Boolean.

[Sija]

@paulcsmith would be like that in pure Ruby, yet in Crystal ? signifies nullability, see Indexable#[]? for instance.

[paulcsmith]

Oh yeah you’re right. As long as there is a non ? that makes sense. I was confused by the title that used the key? form. Also that ruby had a similar method that returns a Boolean (as was mentioned previously)

[rdp]

I'll admit this has confused me before :\

[ghost]

I am also a bit confused, even more so after the discussion here ;) but I think the most important part is to document the behaviour in crystal either way.

On a side note, one obvious advantage for a similar behavior between crystal and ruby, when referring to ruby's behaviour as a "default", is that it lowers the entry barrier for ruby people going/using (into) crystal if a lot of their code or the code behaviour would work as-is with crystal (excluding obvious differences such as the type system). I also have to admit that "key_for_value" does not really tell me much at all as a name; however had, I also agree that key? versus has_key? may cause confusion. I like .has_key? the most even though it is longer than .key?, simply because I can instantly, from the name alone, infer what it does.

[marksiemers]

@Sija and @paulcsmith - The question mark can mean both things (returns bool or nullability):

These ? methods return true or false and never nil:
Hash#empty?
Hash#has_key?

This method, the ? indicates nullability:
Hash#[]?

This is a bit like ruby's ! methods - it often means destructive, but not always.

To put it generally, I think the question mark methods mean you can (and probably should) use them in a conditional statement.

Since they can be used both ways, I think that causes the confusion for Hash#key? - since it could be either bool or nullable, and ruby chose bool, a lot of people coming from that language will guess wrong.

Since the method will need to be nullable, should it end in a question mark?
Do any of these sound good:
key_for?(value)
key_for_value?(value)
key_with?(value)
key_with_value?(value)
key_matching?(value)
key_matching_value?(value)

[Sija]

👍 for key_for?(value)

[bcardiff]

I think the idiom should go with:

def key_with_value(value) : K # raise if no match
def key_with_value?(value) : K | Nil

or key_for/key_for?

Another possible name is reverse_fetch or inverse_fetch and support the same interface as fetch:

def reverse_fetch(value) : K # raise
def reverse_fetch(value, default : D) : K | D # eager default
def reverse_fetch(value, &block : -> D) : K | D # lazy default

We are not discussion what will happen with multiplicity though: {a: 1, b: 1}.key_for(1) # => 💥 ?

---

key? was added two years ago, but I never used it and I would expect to be the same has_key? (or not exist at all).

[Sija]

key_with_value sounds to me like it should return a Tuple.

[marksiemers]

@bcardiff - Regarding multiplicity, the existing key? method does this:

Returns the first key with the given value, else nil.

I couldn't find if crystal preserves order for Hash (insertion order I assume), if that is the case, then I think that functionality is fine.

I agree with @Sija key_with_value sounds like a Tuple

Given that, I like key_for / key_for? as replacements for key?

The reverse fetch functionality is also nice, I haven't run into a case of needing it, but that interface looks helpful and intuitive.

Regarding key? - removing it altogether may be safer, as if it continues to compile, developers that have it in their code may not catch it when upgrading. If the name is kept and aliased to has_key? make some really loud compiler warnings.

[drosehn]

We are not discussing what will happen with multiplicity though: {a: 1, b: 1}.key_for(1) # => 💥 ?

I certainly write a lot of hashes were the same value appears for multiple keys, and sometimes I do want to find all keys which match a given value. In those cases I loop through all keys to see if it matches the value I care about. So maybe:

def keys_matching(value) : Array(K) # raise if no match
def keys_matching?(value) : Array(K) | Nil

Or maybe just have one, and have it return an empty array if there are no keys which match the given value.

I think an advantage of this idea is that it looks good next to the method:

#keys
     - Returns a new Array with all the keys.
#keys_matching(value)
     - Returns a new Array with all the keys which
       have a value that matches the given value.

[drosehn]

Regarding key? - removing it altogether may be safer

I agree with this. In fact, this is the part which I feel is the most important. I don't feel as strongly about exactly what method-name should provide similar functionality, but I think this method-name is a little confusing.

I agree with @Sija key_with_value sounds like a Tuple

I also agree with this.

[bcardiff]

keys_matching sounds to me like the keys that match with the argument.

Let's go for:

def key_for(value) : K # returns some key with the value, raise if none
def key_for?(value) : K | Nil # returns some key with the value, nil if none

And maybe:

def all_keys_for(value, &block : K -> ) # yields all the keys for the value
def key_for(value, default : D) : K | D # eager default
def key_for(value, &block : -> D) : K | D # lazy default

I prefer all_keys_for instead of just keys_for because the distance with the key_for(value, &block : -> D) was to short.

But since keys are usually light values we can leave out and use key_for?(v) || default

def key_for(value, default : D) : K | D # eager default
def key_for(value, &block : -> D) : K | D # lazy default

Even though I prefer all_keys_for instead of just keys_for .

[marksiemers]

@bcardiff

def all_keys_for(value, &block : K -> ) # yields all the keys for the value

Does this return an Iterator, an Array, or something else? Would it require a block or would this be possible:

 { "one" => 1, "two" => 2, "uno" => 1 }.all_keys_for(1).each{|key| puts key.to_s }

[drosehn]

Method names based on key_for and all_keys_for seem reasonable to me, although I think Brian's description is missing a few words that he meant to include, and I'm not sure that I understand all of it.

[bcardiff]

Sorry @drosehn 😅 .

I meant we can just go for:

def key_for(value) : K # returns some key with the value, raise if none
def key_for?(value) : K | Nil # returns some key with the value, nil if none
def all_keys_for(value, &block : K -> ) # yields all the keys for the value

And leave out the eager/lazy default variants.

@marksiemers, I was thinking of yielding to a block. But we can go with an iterator since we can use Hash#each and apply a filter and a map to it.

def all_keys_for(value) : Iterator(K)

Both are fine for me, but I would include only one.

[RX14]

Typically, iterator returning methods begin with each_. Array-returning operations are named identically to their yielding variants and are overloaded just on accepting a block. all_keys_for(value, &block) : Nil and all_keys_for(value) : Array(K), and each_key_for(value) : Iterator(K) would be the standard naming, as far as I can see.

[marksiemers]

I can't comment on my PR it seems, but here it is: https://github.com/crystal-lang/crystal/pull/5444

And here is my comment:
This is somewhat a WIP. I'm having XCode problems since upgrading to High Sierra so I haven't been able to run the spec locally yet.

There was also discussion of providing the following methods:
Hash#all_keys_for(value, &block) : Nil
Hash#all_keys_for(value) : Array(K)
Hash#each_key_for(value) : Iterator(K)

I can work on those, but I think they make sense as a separate PR.

[RX14]

@marksiemers there's no reason why you shouldn't be able to comment on your PR. It's probably at github's end.

[marksiemers]

I've been working through this, and I think just having each_key_for will be a cleaner solution (and dropping all_keys_for):

Hash#each_key_for(value) : Iterator(K)
Hash#each_key_for(value, &block) : Nil

The second will yield to the block, and the first, you can call to_a to get an Array(K)

One less method and consistently singular with key_for

@RX14 @bcardiff @drosehn - WDYT?

[drosehn]

Somehow I missed that you had explicitly asked for my opinion on this. This seems fine to me.

[drosehn]

I did a bit more checking to see how we got where we are.

Ruby has methods: Hash#key(value), Hash#key?(value), and Hash#has_key?(value)
In ruby, Hash#key?(value) is explicitly defined as a synonym for Hash#has_key?(value)
In ruby, Hash#key(value) returns the key of the first hash entry whose value is value, or it returns nil if there is no such entry.

In crystal, Hash#key(value) returns the key of the first hash entry whose value is value, or it raises an error if there is no such entry (e.g: "Missing hash key for value: 272"). So the method has a return-type of String.

In crystal, Hash#key?(value) returns the key of the first hash entry whose value is value, or it returns nil if there is no such entry.. So this method has a return-type of (String | Nil).

My own thinking is:
1) crystal's current Hash#key?(value) is a confusing name for the method (for multiple reasons!), even though I can see how we got to that name.
2) IMO both ruby and crystal's Hash#key(value) seems pretty pointless to me. I'm pretty sure that every time I have wanted to find keys based on some value, I wanted to find all such keys. Even when I'm expecting there is only one key which will point to the given value, I'll check all the keys just to be sure that's really true.

IMO, crystal should drop both Hash#key(value) and Hash#key?(value). It'd be nice to have the Hash#each_key_for(value) method as implemented in #5450 , but I do not feel as strongly about that.

[asterite]

IMO both ruby and crystal's Hash#key(value) seems pretty pointless to me. I'm pretty sure that every time I have wanted to find keys based on some value, I wanted to find all such keys

I'll have to disagree. Moth of the time a hash is a bijection. For example:

{'a' => 97, 'b' => 98, ...}
{'a' => "あ", 'i' => "う", ...}
{"Argentina" => "Buenos Aires", ...}

And here we map enums to their values in LLVM, again a bijection.

For the case where you want to do a reverse lookup, the method is super useful, if you are willing to accept the small performance penalty.

So I wouldn't drop key... I'd rename it to key_for, though.

[drosehn]

I'm happy with any solution, just as long as Hash#key?(value) disappears! 😀

[marksiemers]

The original reason for this ticket (change Hash#key? to Hash#key_for?) has been merged: https://github.com/crystal-lang/crystal/pull/5444

Another secondary method was brought up (either Hash#all_keys_for or Hash#each_key_for) - that PR is unresolved: https://github.com/crystal-lang/crystal/pull/5450

As of right now, @asterite doesn't want each_key_for as part of the standard, @RX14 does.

I think this issue should remain open until a final decision is made about each_key_for.
How can we get a final decision on that? @asterite @RX14 @bcardiff @drosehn

[bcardiff]

Given how nice and easy is to use blocks for the each_key_for when needed I'm fine with leaving this out of the stdlib.

If the implementation would allow some performance benefit, that would be a different story, but that will lead to another data structure.

If the user needs to reverse lookup a lot a hash, using inverse first, or keeping a bidirectional hash might be more reasonable.

So, I vote for closing this issue and #5450.

All the discussion served again to explain idioms, pros and cons of containers in crystal. That's always good IMO so there is more change to all be on the same page regarding api designs.

[bew]

Close then?

[bcardiff]

We can always revisit later this discussion. Closing.