RFC: Strict hash keys

Community
1

Hey friends, some of you may or may not realize that hashes don’t currently have type checks on keys. You can call {"a" => true}[123]? and it will compile.

This caused a silent bug in one of my services — it stopped working entirely and only my monitoring caught it. So I’ve put up a PR for restricting hash keys to the types specified in the hash that I’d love to get community feedback on. There have already been some good comments from the core team, but because this is such a fundamental change that will impact a lot of us, I’d like to get some more eyes on it.

That build uncovered 2 bugs in one of my services (one was in a shard I’m using) and I think it’d be awesome to see other people apply it to their apps. I’d really like to see Lucky and Amber apps run against that build. I don’t use those frameworks right now (my Crystal usage is mainly for small JSON and gRPC services) but I think it’d be interesting to see what other bugs can be uncovered with this idea. I’ll probably try them out this weekend.

If you run your apps against the build, it’d be great if you could share how much effort it took to get up and running with it:

  • everything worked fine
  • it uncovered bugs and they were easy to fix
  • it uncovered bugs and they were hard to fix
  • it uncovered bugs in shards
  • it didn’t uncover any bugs but made my work harder
  • etc

Real code is a very effective tool in discussing things like this, so

SHOW ME WHAT YOU GOT

4 Likes
2

+1 from me, it feels like a wart from java (?) that you can say "a".equals(3) (or in ruby "a" == 3) and has always bugged me…

3

+1 from me, it feels like a wart from java (?) that you can say "a".equals(3) (or in ruby "a" == 3 ) and has always bugged me

Just note that this is not what this change is about.

3 Likes
4

Oh OK. I was thinking of java’s Map#containsKey(Object anythingEvenIfNotPossible) which has also confused me in the past, so I assumed it was the same issue. Anyway, steps in the right direction. :)

5

Compiled 2 apps I need to be working, and to my surprise both compiled correctly and seem to work.
I was expecting trouble because both use yaml and json and one of them does un-elegant things with Hashes.
But as far as quick tests go, both seem to work.

Just to be sure though, could you post a small code snippet that should work with the current Crystal and should not work with the proposed changes?
Just to rule out that I’m compiling the wrong branch or using the wrong compiler binary. (pretty sure I did, but checking never hurts)

edit: Did check, and used the correct modified compiler. -> seems to work for me.

6

Yes, that’s what’s being discussed here: accessing a hash value by key. But your example was comparing int vs string which is not being discussed here. Just to clarify the confusion :slight_smile:

7

Example is in the second sentence of the OP: {"a" => true}[123]?

8

This seems like a really good change that will help immensely with debugging.

9

I’m 100% in favor of this. Can’t really be done post 1.0 without potential breakage and will definitely find bugs

1 Like
10

However if this isn’t merged into core I would definitely create/use a shard that adds more strict access with hash keys. Then you could use a linter like Ameba to yell at you when you don’t use them. my_hash[some_key]! basically adding ! to methods like [] and friends.

I plan to do this for Array and Hash with a safe_accessor library that basically overrides []!, first!, etc. that do exactly what [] and first do, but are clearer that this is a scary method and will raise on nil. I see tons of issues starting Crystal and getting confused that [] doesn’t work like Ruby and blows up on nil. So pair safe_acessor with an ameba rule to yell if you use [] and it’ll probably catch some bugs, or at least make it clear to readers that this is a “scary” method like not_nil! so you need to be sure it won’t be nil at runtime

Anyway, just a thought in case this isn’t merged in

What I’d really love is [], first, etc. to return T | Nil and have []! be the way to ensure not nil. That way you make it clear its scary (like not_nil!). Also the ? is odd because that typically signifies a Bool method but in Crystal it can be a Bool or signify it can return nil. I’d love if it ? was removed from everything. It’d suck to update shards, but I’d happily do it!

2 Likes
11

There’s something to that idea. But it’s a different topic. Do you care to open a separate RFC for this?

1 Like
12

Just checked that Swift performs that typecheck. If you try to compile

let d = [0: "foo"]
if let v = d[""] {
    print(v)
}

you get

foo.swift:2:15: error: cannot subscript a value of type '[Int : String]' with an argument of type 'String'
if let foo = d[""] {
             ~^~~~
2 Likes
13

want strict type check, use struct instead of hash

–from elixir

14

just strict key is not enough, value type can be any!

15

Can you expand on why hash is not a good fit for this?

Maybe I’m misunderstanding, but using the value a given hash key maps to as an input to another method will invoke that method’s type checking, whether explicit or inferred.