rule idea: elvis should work on the real code as well

[tothlac]

Some rules like variable_naming_convention, line_length, module_naming_convention should use the actual source of the code, but for calls like no_call, no_debug_call, no_common_caveats_call it is not enough to check the source. With a simple parse transform the generated abstract code can easily call anything we don't want to.

Because of this the abstract code should be decompiled into source, it should be passed to modules implementing elvis checks together with the original data generated directly from source, so the abstract code can be validated using ktn_code. The above mentioned checks should operate only on the decompiled code.

[elbrujohalcon]

Not a bad idea.
I can see myself caring about line length in the code I read (i.e. before any parse transforms) and not in the actual code that is executed (i.e. after the parse transforms). Same goes for variable, function and module naming rules.
On the other hand, no_call, no_debug_call, etc… Yeah, it makes more sense to check the code after parse transforms are applied. But in my mind, if you already took the time to write a parse_transform that adds debug calls… why would you want Elvis to tell you that you can't use it?
Besides, if you want to workaround those no_call rules you don't need a parse transform. A simple erlang:apply(your, function, [With, Your, Params]) is enough.

All in all, I would not be against implementing your idea, but I would not make it a priority and I would definitely make it optional.

[tothlac]

Of course if someone intentionally wants to trick some security/code_smell checks it is always doable, does not matter how strong the checker is.

I actually have a similar plugin working only on the abstract code, but it does completely different checks. Simple no_call checks and maybe some other validation would be good in elvis running on the abstract code, so then elvis would be the right tool for implementing both code_smell checks and function call validation as well.

[tothlac]

In the meantime I've started implementing it.
parse_tree is created in elvis_file:parse_tree/2.
Only change here is to create the abstract parse tree which is generated from the source which was decompiled from the beam.

    AbstractParseTree = get_abstract_parse_tree(Path, Config),
    parse_tree(Config, File#{parse_tree => ParseTree,
                             abstract_parse_tree => AbstractParseTree});

So if there is an src/mymod.erl, there should be a mymod.beam somewhere. If we are using rebar3 it should be in _build/${REBAR_PROFILE}/lib/myapp/ebin/myapp.beam . Only problem is what if rebar3 compile has not been compiled, or if we are not using rebar3.

Maybe the path to the beam should come from the configuration.

 {elvis, [{config, [
    #{dirs => [ "src" ],
      filter => "*.erl",
      beam_path = "_build/default/lib/myapp/ebin",
      ruleset => erl_files,
      rules => [
                {elvis_style, line_length, #{limit => 84}}
               ]
     }]}.

Using this config elvis should be able to find out where the beam files are stored.

What should happen if the beam is not there because compile has not been called?
Maybe the abstract _parse_tree should be simply undefined, and rules needing abstract_parse_tree should simply fail. This way the user must call rebar3 compile before calling elvis...

[tothlac]

As I see the no_call and no_debug_call checks also don't work with a simple define. If I use

-define(DBG(Fmt, Args), io:format(Fmt, Args)).

myfunc() ->
    ?DBG("", []).

elvis also won't be able to find that io:format was called.

Would it be ok to have the above mentioned beam_paths option in rebar.config like this:

[{elvis, [
      {analyze_abstract_code, true},
      {beam_paths, "_build/default/lib/*/ebin"},
      {config, [ ....]}
 ]}.

If the analyze_abstract_code option is set to true in elvis_file:parse_tree/2 would expect that beam files are stored in the folder specified in the beam_paths option, and if the beams are not there it should simply throw a no_beams_found exception?

[elbrujohalcon]

To be honest, I'm still not convinced about requiring compilation before running the linter (even as an option). I think that…

1. No finding function calls if they're written in macros is a bug of no_call and no_debug_call.
2. If you are going to add particular rules to run on beam files, I would just add them as another group:

[{elvis, [
  {config, [
    #{dirs => ["src"],
      filter => "*.erl",
      ruleset => erl_files
     },
    #{dirs => ["_build/default/lib/*/ebin"],
      filter => "*.beam",
      ruleset => beam_files
     }
  ]}
]}].

[tothlac]

I've already implemented it differently, but your idea is very good, so I will change the implementation.

For the other question: we use elvis by calling rebar3 lint, so lint plugin should enforce compilation of the app only if the rule you mentioned is in elvis.config:

    #{dirs => ["_build/default/lib/*/ebin"],
      filter => "*.beam",
      ruleset => beam_files
     }

If it is enforced by rebar3_lint plugin and elvis is always called from the plugin, the beam files will be always stored in the given folder by the time elvis is called.

[elbrujohalcon]

I see… that sounds very reasonable.

[elbrujohalcon]

Also, do you mind opening bug reports for no_call and no_debug_call regarding them ignoring macros?

[tothlac]

Using this implementation the following rules should be moved from the erl_files section to beam_files:

  {elvis_style, invalid_dynamic_call, #{ignore => [elvis]}}
  {elvis_style, no_debug_call, #{ignore => [elvis, elvis_utils]}}

I can open a ticket for the no_call and no_debug_call, but if they will run only on the abstract code, there is no need to fix them. I've already checked it with my implementation, and it is able to find them even if define is used. Should I create the ticket?

[elbrujohalcon]

Yeah, that's the thing: I believe we can still fix them to work correctly on source files.
We can have new rules (i.e. the ones that you describe) but I don't think we should get rid of the existing ones if we can fix them.

[tothlac]

Ok. I've seen when I was debugging the no_call rule, that the parse_tree actually contained the information ?DBG is macro, and it is actually io:format/2 so the code be modified to fix the issue.

I will create the ticket.

[tothlac]

https://github.com/inaka/elvis/issues/501 created

[tothlac]

I've created a PR: inaka/elvis_core#108. Now the mentioned rules will use abstract code only if they are called from the beam_files section.

Current config I use:

        #{dirs => ["_build/default/lib/myapp/ebin"],
          filter => "*.beam",
          ruleset => beam_files,
          rules => [
                     {elvis_style, no_debug_call},
                     {elvis_style, invalid_dynamic_call}
                   ]
        }

I had to modify the specs at some places which is mainly because of the declaration of elvis_config:config/0 type:

-type config() :: [map()].

rules are written like this in elvis_style.erl:

-spec no_debug_call(elvis_config:config(),
                    elvis_file:file(),
                    no_debug_call_config()) ->
    [elvis_result:item()].
no_debug_call(Config, Target, RuleConfig) ->

I think the first parameter is a map here which contains the config, not a list containing maps as it is indicated by the type declaration.

Do you have any ideas about possible testing?

[elbrujohalcon]

You can test this project by updating deps (or using _checkouts) here and running the tests.