Elixir: Revisit strictness of Access

[josevalim]

Today Access and Keyword expect literal keyword lists, which becomes an issue when interfacing with Erlang libraries. For example:

iex(1)> config = [http: [:inet6, port: 0]]
[http: [:inet6, {:port, 0}]]
iex(2)> put_in config[:http][:port], 13
** (FunctionClauseError) no function clause matching in Keyword.get_and_update/4

I am opening this issue so we relax those rules.

[OvermindDL1]

_/me is entirely for relaxing those constraints_

I would actually be all for having it follow Erlang's property list style where something like:

[{:a, 1}, :b, {:c, 2}]

Is inferred as this for the purpose of getting values from keys:

[{:a, 1}, {:b, true}, {:c, 2}]

EDIT: On an aside, I'd love the Elixir Syntax relaxed to allow this too (perhaps only within lists, not function heads, unless of course they are wrapped in a list):

[a: 1, :b, c: 2]

[hubertlepicki]

@OvermindDL1 so this should work like this:

iex(9)> put_in config[:http][:port], 12
[http: [:inet6, {:port, 12}]]

But should this work too?

iex(10)> put_in config[:http][:inet6], 1
[http: [{:inet6, 1}, {:port, 0}]]

[OvermindDL1]

@hubertlepicki If it followed erlang's proplists then yes it would. An plain atom in a proplist is for all intents and purposes is inferred as a 2-tuple of the first element being that atom and the second element being true.

[hubertlepicki]

@OvermindDL1 there's a possibly big problem with that in fact that the current behavior of keyword list is:

iex(2)> [{:foo,  1}, :bar][:bar]
nil

if I understand correctly, you'd return true that may blow some things up :/

[josevalim]

Our plan is not to support proplists. The proplist discussion can be moved to a mailing list discussion if preferred. The goal here is simply to ignore non-tuples in the list instead of raising.

[fishcakez]

Are we able to still use BIFs with a relaxed approach? I believe so but a few edges cases would end up being more expensive as we'd need to check tuple size. I think relaxed is better because some functions are relaxed and some are not, which could be confusing. It is less good for dialyzer or if someone every writes an elixir type system. However we could still let those warn/error but allow it in the code, which is perfectly reasonable.

I am unsure about the example and other cases though because to be able to handle these options one would need to that something is a proplist or special case (like gen_tcp and ssl). Therefore I am unsure how useful it is in practice and it will still be confusing. It feels like a no-win either way :(. Can you expand on a case where this has hurt @josevalim? Something like Keyword.merge?

Note that atom proplist style would require removable of BIFs so hit performance. Please consider if sending a proposal.

[hubertlepicki]

So I toyed with this a bit last night and support for the example @josevalim gave seems pretty easy:
https://github.com/hubertlepicki/elixir/commit/6a3ca126d8bda7b7fc1b075756c667dc63dc01e7

The result is:

iex(1)> config = [http: [:inet6, port: 0]]
[http: [:inet6, {:port, 0}]]
iex(2)> put_in config[:http][:port], 13
[http: [:inet6, {:port, 13}]]

but it does look like other operations, like mentioned by @fishcakez Keyword.merge need to be updated as well to support this - if we need to.

I can send apull request with this thing if you guys consider it any good, adding some tests etc. Then could try tackling other functions if we want to.

[fishcakez]

@hubertlepicki I think you got the wrong end of the stick. This issue is not about implementing proplist support, see https://github.com/elixir-lang/elixir/issues/6515#issuecomment-335838001. Also note that :gen_tcp and :ssl are not proplists, they are special cases of list options that are unique to their own modules. I was asking for an example where there was awkwardness because I assumed it would not be required to lookup values in these options list. I was guessing that Keyword.merge might be this case.

[ericmj]

I believe it would be better to leave Keyword strict. We keep using Keyword when interfacing with Elixir code and it will help us find bugs in our code because of its strict checks. When interfacing with Erlang code use a Proplist module, this can be in core or as a separate library.

It's very easy to understand how Keyword works today because it only accepts 2-tuples. If we remove strict checks code such as Keyword.merge([inet6: true, port: 0], [:inet6, port: 10]) becomes unclear for me.

[josevalim]

@hubertlepicki I think you got the wrong end of the stick. This issue is not about implementing proplist support, see #6515 (comment).

Which is exactly why we constantly ask for folks to not derail discussions about existing features with alternative proposals because it causes precisely this kind of confusion. /cc @OvermindDL1

Those discussions are welcome but they should happen in the mailing list, not in the issues tracker.

It's very easy to understand how Keyword works today because it only accepts 2-tuples. If we remove strict checks code such as Keyword.merge([inet6: true, port: 0], [:inet6, port: 10]) becomes unclear for me.

It is not supposed to be unclear at all, anything that is not a 2 element tuple starting with an atom is not touched and carried to the output. Forget about proplists, they should not have been made part of this discussion at all.

So I toyed with this a bit last night and support for the example @josevalim gave seems pretty easy: hubertlepicki/elixir@6a3ca12

Thanks @hubertlepicki, that's the idea except that, as @fishcakez said, we are not checking for atoms, we are just letting everything else be. In any case, we already have someone working on this feature for now, so please hold back on further patches. I will let you know if this changes. :heart:

[ericmj]

It is not supposed to be unclear at all, anything that is not a 2 element tuple starting with an atom is not touched and carried to the output. Forget about proplists, they should not have been made part of this discussion at all.

Anything that is not strict becomes less clear because you are introducing special cases. Keyword.merge is an operation where "carried to the output" is harder to understand. Do we simply copy all non 2-tuple elements to the returned list?

We are also breaking assumptions such as:

Keyword.take(keyword, Keyword.keys(keyword)) == keyword

I think this is a great example of where if it is easy to write properties for your API you have a good API.

Does this also mean that all lists are now Keyword lists? We have this is an example in the doctests:

iex> Keyword.keyword?([:key])
false

[josevalim]

@ericmj can we at least lift the restriction for Access then?

[hubertlepicki]

Which is exactly why we constantly ask for folks to not derail discussions about existing features with alternative proposals

OK so I actually originally wrote my opinion but then deleted it before linking to the commit. I think special cases are bad idea and quite confusing, I just submitted the code as illustrative solution. But I actually stand with @ericmj. It seems like a lot of work with little profit and more code to maintain.

Having said that, I sometimes have a need to get the data from nested structures, update it etc. and it would be super nice if such improper keyword lists were supported, but - I think this can be done in a stand alone library, implementing a query syntax similar to JsonPath: http://goessner.net/articles/JsonPath/

[josevalim]

OK so I actually originally wrote my opinion but then deleted it before linking to the commit. I think special cases are bad idea and quite confusing, I just submitted the code as illustrative solution. But I actually stand with @ericmj. It seems like a lot of work with little profit and more code to maintain.

Great! This is exactly the kind of feedback we expect.

I think this can be done in a stand alone library, implementing a query syntax similar to JsonPath: http://goessner.net/articles/JsonPath/

It sounds like an overkill given we already have a programmable Access module for that (you can define your own lookups using functions).

[ericmj]

can we at least lift the restriction for Access then?

I think so if we document that Access works for a superset of Keyword. This would be backwards compatible because it would be like adding a new dict type to Access.

We should probably also keep the Keyword.get_and_update/3 implementation unchanged and introduce the new functionality as a private function on Access?

[josevalim]

@ericmj agreed.

[OvermindDL1]

Which is exactly why we constantly ask for folks to not derail discussions about existing features with alternative proposals because it causes precisely this kind of confusion. /cc @OvermindDL1

Sorry... There was a discussion tag at the time and I was hoping that it could be relaxed far enough to include that support...

But overall, if access will call out to Keyword, I'd say do not support improper lists, it does break that contract that has been established. Perhaps a new function in Access would work so instead of something like put_in([b: :c], [:b], :d) perhaps something like put_in([:z | [b: :c]], [Access.prop(:b)], :d? That returns current semantics and allows a way to access improper keyword lists.

[ericmj]

@OvermindDL1 This is not about accessing atoms, it's about ignoring them. Like we have said many times, this issue is not about adding support for proplists. Please start the discussion on the mailing list or the forum instead.

There is nothing in the contract of Access that it will fail types that are not Keyword or Map. If there is any such documentation please point us to it. Supporting new/more types does not break backwards compatibility for Access.

[OvermindDL1]

There is nothing in the contract of Access that it will fail types that are not Keyword or Map. If there is any such documentation please point us to it. Supporting new/more types does not break backwards compatibility for Access.

Not in Access no, I said only if it delegates to the Keyword module, if it does not then no issue. ^.^

[josevalim]

Based on earlier discussions, it was shown we don't want to change the guarantees in keywords. After reviewing the Access pull request, we have also realized that departing Access from keyword is also not desired. For those reasons, we won't be changing this. Thanks everyone for the discussions!