Warning for using quotes around keywords when they are not necessary, such as `"key": value`

[Licenser]

Environment

• Elixir & Erlang/OTP versions (elixir --version):

  
  Erlang/OTP 19 [erts-8.3.5.3] [source] [64-bit] [smp:8:8] [async-threads:10] [hipe] [kernel-poll:false] [dtrace]

Elixir 1.7.0-dev (e07a41d9d) (compiled with OTP 19)

### Current behavior

The difference in meaning of `"key":` and `"key" =>` in maps can lead to some quite interesting and really hard to find bugs.

The two syntaxes for writing map keys are similar enough to be easily confused and not spotted instantly. 

While the convenience of writing `key: value`  in a map is great mixing it with the syntax of complex atoms makes it icky

### Expected behavior

I would like to propose a warning for this, it is a valid syntax so while confusing it shouldn't be an error in my opinion (even so I'd personally kind of like it :P).

I'm not sure how far to take it.

The first and kind of 'safest' option in my eyes would be to warn that `"key": value` should be written as `:"key" => value` to prevent confusion and make it obvious that `key` is an atom here.

I can understand however that enforcing a convention like that might not be appreciated by everyone, so as a middle ground it might be okay to warn on this re-write of `"key": value` and `key:` are the same (the atom would not need quoting). While not catching all the cases it'd probably warn on a majority of mistaken uses of `"key": value`

[josevalim]

I am ok with moving this forward, my concern is what should we do on keyword lists. For consistency, we would want both keyword lists and maps to behave the same.

This means that %{foo: 1, "WEIRD-BAR": 2} needs to be rewritten to %{:"WEIRD-BAR" => 2, foo: 1} and [foo: 1, "WEIRD-BAR": 2] needs to be rewritten to [{:"WEIRD-BAR", 2}, foo: 1].

In any case this is at best a warning because an error would be backwards incompatible.

[Licenser]

I totally agree with not being an error. breaking existing code is never nice :).

I didn't think about keyword lists but it's right, it should be consistent for both uses of the "key": syntax.

I don't want to just go and ask "go make this warming" :P so I'd give making a PR a shot but it'd be really helpful to get a pointer where those things exist in the source code as I've never looked at it before and I suspect digging thorugh it where syntax warnings are generated would take longer then adding the warning tiself.

[josevalim]

Before implementing we should wait a bit more and collect more feedback. I understand the gains here but for everybody who writes something such as "FOO-BAR": 1 on purpose to mean an atom are now forced to write it somehow differently. I don't think those cases are very common though. Ww will see what others have to say.

[Licenser]

Oh I'm in no hurry. I just wanted to offer that I'll try making a PR. I know too well how quickly tickets become frustrating for the maintainers if they only ask for something and don't offer to contribute. I think "be the change you want to see in the world" is the saying that goes with that.

[fertapric]

Almost every Phoenix project contains an example of keyword lists in the aliases method of mix.exs (here is the template).

In Elixir projects, keyword lists using the "key": value syntax can be seen in the :preferred_cli_env option, in the docs configuration and in the mix.lock.

In those cases, I think the tuple form of the keyword list reads a bit worse :)

[Licenser]

I'm not sure how elixir handles parsing but the .lock file should not be affected by warnings does it? It's a data file, not a code file. Can someone say that confirm that?

But yes the Phoenix template would generate a warning. On the bright side

    [
      {:"ecto.setup", ["ecto.create", "ecto.migrate", "run priv/repo/seeds.exs"]},
      {:"ecto.reset", ["ecto.drop", "ecto.setup"]},
      test: ["ecto.create --quiet", "ecto.migrate", "test"]
    ]

would make it much clearer that the keys are atoms not strings :)

[josevalim]

@Licenser although I would say in the aliases case the readability will largely decrease.

[michalmuskala]

Yes, readability will decrease in places that use this syntax on purpose. Also while in maps it's fine to reorder the keys, it's not always in case of keywords forcing everything to be written using the explicit tuple syntax.

[Licenser]

idk, "test": ["ecto.create --quiet", "ecto.migrate", "test"] for example reads like the first "test" is a string same as the last test.

But I see what you all mean it is less clear but it reads better to have the : inline in the list.

I suppose the real solution would be not to escape atoms with " to prevent them for having a double meaning but adding a new syntax feels a lot more invasive.

Erlang gets around this problem by having ' for atoms and " for string(-lists) (and <<" for binaries) but since ' is already taken for string-lists in elixir. perhaps ticks (I don't know how the hell to escape them in markdown) would be nice?

    [
      `ecto.setup`:, ["ecto.create", "ecto.migrate", "run priv/repo/seeds.exs"]},
      `ecto.reset`:, ["ecto.drop", "ecto.setup"]},
      test: ["ecto.create --quiet", "ecto.migrate", "test"]
    ]

That would make it totally obvious not being a string or erlang-string. And the current syntax could be maintained as valid but causing a warning?

More invasive but perhaps worthwhile down the road?

[josevalim]

Given the examples above, it is clear that deprecating the current syntax is not the way to go. Once you know that "ecto.create": is an atom, having to rewrite it to use tuples is going to drastically impact readability. At the same time, I understand that the behaviour of "docs.align": being an atom can be confusing at first.

While we could discuss a new syntax, it is out of the scope of this issue and should probably be done on the mailing list with a more thorough discussion of the pros and cons.

Thanks @Licenser!

[wojtekmach]

just some anecdotal evidence: since keyword lists can only have atom keys, ["foo": 1] is unambiguous in the sense it can only be an atom key. It made it "click" for me. This doesn't contest the point you made that one may not spot the difference instantly but I thought I'll mention it anyway.

[Licenser]

I am sorry but I disagree here. While it we probably can argue for days if readability includes things being obvious or not looking nice lets put that aside.

As stated when opening the issue completely making "...": a warning to prevent it is only one of the options. The proposed alternative of making "test": a warning (as the quotes are not needed) but "ecto.setup" not (as the quotes are required) would not force force re-factoring lists or maps it just would warn on an an ambiguous part of syntax: using quoted atoms where they are not required

[josevalim]

@Licenser Oh, that's another option indeed that I had not considered. I will reopen this and edit the title. Your proposal has almost no drawbacks and the formatter even already rewrites it today.

So to clarify, we are proposing to warn when quotes around keyword identifiers are not required. Then I have the following follow up question: should we also warn on :"EXIT" since they are semantically the same construct?

[Licenser]

On that I personally feel yes but it's more a feeling then anything else. My thought would be, while syntactically :"EXIT" is correct it adds additional cognitive work to recognizing that it's an atom. The same way that newer elixir wants parens around functions even so they are not syntactically needed in some places.

[michalmuskala]

Yeah, I think a warning for places where it's not needed is a good compromise. Additionally, the formatter already rewrites those to omit the superfluous quotes - it's another argument this shouldn't be a big issue for most codebases.

[OvermindDL1]

Before implementing we should wait a bit more and collect more feedback. I understand the gains here but for everybody who writes something such as "FOO-BAR": 1 on purpose to mean an atom are now forced to write it somehow differently. I don't think those cases are very common though. Ww will see what others have to say.

I am one of those that do this a lot because of interactions with erlang libraries (especially ASN1 stuff), and yet I am entirely for this change as it will help to reduce ambiguity.

In those cases, I think the tuple form of the keyword list reads a bit worse :)

Perhaps keyword lists need to support the key => value syntax as well? You could just redefine the => operator in kernel to just be a 2tuple constructor macro and remove some of this syntax special casing too (and fix map/list to accept macro calls that can expand to useful AST).

perhaps ticks (I don't know how the hell to escape them in markdown) would be nice?

Via a \, like: ` :-)

Given the examples above, it is clear that deprecating the current syntax is not the way to go. Once you know that "ecto.create": is an atom, having to rewrite it to use tuples is going to drastically impact readability. At the same time, I understand that the behaviour of "docs.align": being an atom can be confusing at first.

Which is why I think keyword lists should accept => too (or just make it a macro operator in kernel that makes a 2-tuple). This would also fix part of the keyword list discrepency with erlang's proplists that doesn't allow them to accept non-atom key's.

[josevalim]

We are not adding new syntax or new AST nodes. I don’t think we should support => exactly because the point of keyword lists is to handle only atoms.

The current proposal is to work only when the quotes are not necessary and make it clear in the process that we have only atoms. --

José Valimwww.plataformatec.com.br http://www.plataformatec.com.br/Founder and Director of R&D

[whatyouhide]

I somehow agree that "foo": ... can be confusing, but I don't see the point in something as strong as a warning for when you use this syntax. We can't warn on "ecto.setup": ... as others pointed out, and that feels equally confusing to me. As @josevalim mentioned, does this mean that we should warn on :"foo" too then? And as @michalmuskala pointed out, the formatter will already rewrite "foo": ... to foo: ... so I see this as a very small issue. We have many times said that warnings should be added carefully as too many warnings means folks will not listen to them.

[Licenser]

Accidentally putting "foo": instead of "foo" => into a map is a very annoying problem, I might be specially passionate about that since I've personally wasted half a day debugging failing tests until I spotted by chance that : was an atom, not a string.

I agree that the real problem is not using quotes for atoms that are not needed but that it's bad to have a double meaning for " depending if a : is glued to it or not. But I think the consensus here has been that it's out of scope.

If adding a warning can save someone else hours of debugging it is worth it as a intermediate solution. You said yourself the former will rewrite it so it should not be a big problem

[josevalim]

@whatyouhide the main goal of the warning is not to say that "foo": is the same as foo: but rather to say that "foo": is most likely not doing what you expect.

That said, I agree on your points though and we can skip the warnings for :"foo" because there is no chance of confusion in such cases.

[michalmuskala]

I think it's actually fine to issue the warning for both :"foo" and "foo": ... when the quotes are not needed. We shouldn't warn for things where they are needed.

[josevalim]

Closing in favor of #7838.

[sheharyarn]

Is there a way to keep the quotes and suppress the warning?