Add support for keyword lists in Access.key/2 and Access.key!/1

[tudborg]

This adds Keyword list support to Access.key/2 and Access.key!/1.

Original discussion in #15468

I'd like to move some of. the doctest into access_test.exs instead.
Access.key was only tested in doctests, but the other accessors have dedicated describe in access_test.exs,
but this is good enough for a discussion around the feature. :)

[tudborg]

I wasn't sure how defensive to be in the returned accessor function.
Currently I'm assuming the container is a keyword list if:

• the container is a list
• the key is an atom

I could be more defensive and require the container is either [] or [{key, _} | _] when is_atom(key).
or less defensive and just assume that a list is a keyword list.

I'm currently raising an ArgumentError if the data isn't what is expected, but maybe that should be a RuntimeError instead like in Access.key! (at least those two raises should be the same type)

I think it is important that regardless of container type, the raise is the same
(i.e. not BadMapError + FunctionClauseError depending on container)

Thoughts?

[josevalim]

We are usually not that defensive when it comes to keywords. We may error but it is not a guarantee.

Note the pop/3 commit is still there. :)

Thoughts on the key changes @sabiwara?

[sabiwara]

Overall looks good, but we might need to be slightly more defensive in the following case or bugs might slip:

iex> put_in([1], [Access.key(:foo)], :bar)
[{:foo, :bar}, 1]

The raw key fails with a non-optimal error but at least it fails:

iex> put_in([1], [:foo], :bar)
** (FunctionClauseError) no function clause matching in Keyword.get_and_update/4

[tudborg]

Is something like this what you had in mind @sabiwara ?

I've added a guard in key/2 and key!/1 to catch mistakes like the one in your example.

  defguardp is_probably_keyword(list)
            when is_list(list) and (list == [] or is_atom(elem(hd(list), 0)))

It's clearer as a defguardp, but I was wondering if I should write it inline in key/2 and key!/1.
I could add a tuple_size(hd(list)) == 2 to the guard if we think that is called for.

I figure that we just want to catch most mistakes, and not do a full Keyword.keyword?/1 that traverse the entire thing.

I'm now also testing for this.
I've added key/2 and key!/1 in the same describe, since they are pretty similar, but this is a break from how the rest of the test file does it (one describe per function)

[sabiwara]

OK, seems that the reason for the put_in succeeding is that the underlying Keyword.get and Keyword.put are not raising on invalid keywords:

iex> put_in([1], [Access.key(:key)], 2)
[{:key, 2}, 1]
iex> Keyword.put([1], :key, 1)
[{:key, 1}, 1]

Ideally we'd fix Keyword.put to fail on non-kw lists since we need to loop anyway, but 1) I suppose there is a reason we haven't been doing it historically 2) hopefully the type system will make this concern obsolete anyway.

Given this context, although I kinda like the shallow check approach of is_probably_keyword (should be enough to catch most bugs since lists often have homogenous types in the first place 👍), it might actually be overkill to do it like this.

If we want to add a shallow check, maybe the best would be having it inside Keyword.put / Keyword.get / ... directly using regular pattern-matching (not necessarily in this PR then, happy to do it as a followup).
@josevalim WDYT?

[josevalim]

Correct, we don't guarantee we validate it is a full blown keyword. I am not sure I would do it later and I don't think we should do it now.

[sabiwara]

we don't guarantee we validate it is a full blown keyword. I am not sure I would do it later and I don't think we should do it now.

Full-blown yes, but WDYT of adding the shallow (not full blown) check to Keyword.put rather than in Access.key?

[josevalim]

I see. I think last time we added the checks it broke some code that should not be relying on it. Perhaps it is something for us to revisit once we have the type system, since the type system will at least help catch some of it statically?

[sabiwara]

Perhaps it is something for us to revisit once we have the type system, since the type system will at least help catch some of it statically?

Sounds good 👍

[tudborg]

I've included your suggestions and rebased everything into a single commit.

CI failed for Window Server 2022, OTP 28.1 but it seems unrelated:
https://github.com/elixir-lang/elixir/actions/runs/27507306641/job/81400861646

[josevalim]

💚 💙 💜 💛 ❤️