New rule: Prefer Unquoted Atoms

[paulo-ferraz-oliveira]

Name

prefer_unquoted_atoms

Brief Description

Prefer unquoted atoms unless quotes are necessary.

Reasoning

This one is not a technical concern, but a readability concern. The code will behave identical in both ways.

Refactoring Proposal

elvis shall warn that your atoms are quoted when they don't have to be.

Note: if you're using a formatter this could/should already be taken into account by it.

Origin

Inspired by Credo's https://hexdocs.pm/credo/Credo.Check.Readability.PreferUnquotedAtoms.html.

[paulo-ferraz-oliveira]

One thing I remembered is that language constructs need to be quoted as atoms. e.g. 'maybe' (quoted) is not the same as maybe (unquoted). That'll have to be taken into account when implementing.

[elbrujohalcon]

Maybe other formatters would leave atoms as-they-found-them, but at least rebar3_format (with the default formatter) will apply this concept appropriately (i.e., only atoms that need to be quoted will be quoted).

[elbrujohalcon]

Not saying "no" to the rule, just that it may be low priority since other tools cover this aspect.

[bormilan]

I had a long session discovering how to implement this, and I found a problem. I made a test file that contains quoted atoms that are not required to be quoted. I dug deep into the code of elvis and ktn_code, and I found out that quotes disappear if they are not needed.

Example file:

And this is the output of the file read by the function: elvis_file:src

What do I miss here?

Edit: I found out that it reads the .beam of the file that's the problem if I'm right.

[paulo-ferraz-oliveira]

If you're analysing from BEAM this might not apply; if you're analysing from source it's still interesting.

[bormilan]

I spent much time exploring how these work in the repo, but I'm still confused. I see that we have several rules that require .erl files not .beam for example macro naming conventions. But I also see that in the style_SUITE.erl, we only have a group called beam_files which contains only test that uses .beam files. I'm not sure why other test cases exist but have never been used. Or do I miss something important? I wanted to see how other rules read .erl files, bc this issue only can be solved in that way, but I'm struggling to find one.

Edit: I started working with the elvis_file:src function, and maybe elvis_utils:split_all_lines or something like that, to wrap the code and find the atoms. Things like elvis_file:parse_tree not working here (I'm not really sure, but I guess the bc of the structure).

Edit2: nevermind, I found out that ktn_code:parse_treeworks well, so I don't need to make things by hand.

[bormilan]

I think I made good progress today, it's not perfect but works. Could you please provide me with some suggestions?
main...bormilan:elvis_core:feature/prefer-unquoted-atoms

[paulo-ferraz-oliveira]

A while back, rules only supported analysis on .erl files. Along came #138 and changed that. Around that time we introduced " Works on .beam file? " in the documentation, which should exist for all rules. I can't remember if there're rules that are .beam alone, but it's possible; on the other hand, most rules (if not all) should work on .erl files (and not necessarily compiled).

I'm not sure why other test cases exist but have never been used.

all/0 states this:

-spec all() -> [atom()].
all() ->
    Exports = ?MODULE:module_info(exports),
    [F || {F, _} <- Exports, not lists:member(F, ?EXCLUDED_FUNS)] ++ [{group, beam_files}].

and it outputs 120+ ., which should correspond to the actual number of test cases. If only the group-identified tests ran we'd have only around 30+ tests; there's some 🪄 in the

%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
%% Common test
%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%

section of the identified file.

I think I made good progress today, it's not perfect but works. Could you please provide me with some suggestions?
main...bormilan:elvis_core:feature/prefer-unquoted-atoms

You should open a pull request (e.g. as a draft) so we can discuss the implementation details there.

Edit: I briefly looked at the implementation; it seems to go in the right direction. A pull request will allow us to make inline comments that'll help you better.

[bormilan]

This can be closed, isn't it?

[elbrujohalcon]

I… think so. @paulo-ferraz-oliveira ?

[paulo-ferraz-oliveira]

Depends... if we want to do nothing, assuming this is a concern for other tools, sure 😄.

If, at the same time, the tools we use to parse the code for analysis already remove quotes when not required then we'd have to change those, and that's not something we wanna do 👍.

So, yeah, I'll close it. If it becomes a hassle, in the future, we have history already.