I'm specifically talking about catch 1+a stuff, like we find here: http://erlang.org/doc/reference_manual/expressions.html#catch.

This'll generate an unused stacktrace as well as "incomprehensible" return, as explained in

For exceptions of class error, that is, run-time errors, {'EXIT',{Reason,Stack}} is returned.

For exceptions of class exit, that is, the code called exit(Term), {'EXIT',Term} is returned.

For exceptions of class throw, that is the code called throw(Term), Term is returned.

@elbrujohalcon: thoughts?

For some/most of my projects, nobody cares who authored a module.
Also generally, the author attribute usually only represents the first developer that created the module, which may as well had left the project a long long time ago.
It sometimes contains an email in atom format, sometimes in a string, sometimes it's just a name…

In general, this is a very misused attribute.

So, I'm proposing a new rule to warn against the usage of such attribute.
To be clear: it should not be part of the erl_files ruleset.

I use macroses to hide internal exception data structure.

I have .hrl file like

-define(MATCH_MY_ERROR, {my_error, _, _}).
-define(MATCH_MY_ERROR(ErrorTag), {my_error, ErrorTag, _}).

And inside my code I use constructions like

-include_lib("my_app/include/error.hrl").

try my_logick()
catch
    throw:?MATCH_MY_ERROR(bad_auth) ->
        % handle auth error
    throw:?MATCH_MY_ERROR(input_validation) ->
        % handle validation error
    throw:?MATCH_MY_ERROR ->
        % handle other application errors
end.

And elvis thinks that I use macros as function name placeholder, but that's not true.

Haven't thought through all use cases, but it's maybe something to consider.
As is no space after { and no space before }.
As is no space after [ and no space before ].
Newline doesn't count as space.

Still haven't looked into this in detail, but I get

# src/mustache.erl [FAIL]
  - macro_module_names
    - Don't use macros (like MUSTACHE_CTX on line 95) as module names.

in OTP 19, but not OTP 24.

Defining types in public header files (especially those intended for inclusion via -include_lib()) might lead to type name clashes between projects and even modules of a single big project.
Instead, types should be defined in modules which they correspond to (with -export_type()) and this way take advantage of the namespacing offered by module names.
In other words, "no type definitions in header files" rule means that we will always need to use some_mod:some_type() unless referring to a type from the same module it's defined in.

The following code:

-spec wait_for_fun(term(), non_neg_integer(), non_neg_integer())
-> {ok, any()} | ok | timeout.

Causes the linter to fail with:

# apps/els_dap/test/els_dap_test_utils.erl [FAIL]
Error: 'function_clause' while applying rule 'operator_spaces'.

Moving the -> away from the beginning of the line solves the issue.

Rule Idea

I would like to have a rule that emits a warning if anybody uses underscores in a number, like 1_000_000.

Reasoning

I'm aware that this is a radical point of view, but the goal is to be able to consistently grep the code when needed. If I want to find where a certain 123384 value is specified in the code, I don't want to have to grep for 1_?2_?3_?3_?8_?4 instead of 123384.

Options

Convenient options in this rule would be:

• Allow the user to define a value above which underscores are required, so that one can specify that numbers below 10000 don't use _, but numbers above it should always use it.
• Allow the user to define a regex (like \d{1,3}(_\d\d\d)+ or something) to validate the number format.

The warning introduced in commit 1cfbd98 gets triggered when using the git-branch subcommand to elvis, and the branch being worked on lacks changed files in some configured directories. That's not very useful, and I assume was not intended.

In my crusade of killing all duplicated code for color printing on the console I'd wanted to offer the suggestion of using http://github.com/project-fifo/cf this. It includes a io:format like syntax for terminal colors along with termcap based detection of weather colors should be used or not.

Ref: WhatsApp/erlfmt#91

• Omit () in macro names only if they represent constants

%% Bad
-define(NOT_CONSTANT, application:get_env(myapp, key)).
-define(CONSTANT(), 100).

%% Good
-define(NOT_CONSTANT(), application:get_env(myapp, key)).
-define(CONSTANT, 100).

Definition: Block Expressions.

Reasoning: Use functions… with names… don’t be shy! (by @elbrujohalcon)

Also, we should support ignore as we do for other rules (let's just hope ignore is already generalized once we get here 😉).

While

try
    ...
catch
    Throw -> {error, Throw}
end

is perfectly reasonable and possible, elvis still complains that atom Throw "does not respect the format ...".

Edit:

(workaround)

try
    ...
catch
    throw:Exception -> {error, Exception}
end

Try with something like

  ci-windows:
    name: >
      Run Windows-based checks and tests over ${{matrix.otp_vsn}} and ${{matrix.os}}
    runs-on: ${{matrix.os}}
    strategy:
      matrix:
        otp_vsn: [23.2]
        os: [windows-latest]
    steps:
    - uses: actions/checkout@v2
    - uses: gleam-lang/[email protected]
      with:
        otp-version: ${{matrix.otp_vsn}}
      id: install_erlang
    - run: wget https://s3.amazonaws.com/rebar3/rebar3 -OutFile rebar3
      shell: powershell
    - run: |
        & "${{steps.install_erlang.outputs.erlpath}}\bin\escript.exe" rebar3 dialyzer
    - run: |
        & "${{steps.install_erlang.outputs.erlpath}}\bin\escript.exe" rebar3 ct
    - run: |
        & "${{steps.install_erlang.outputs.erlpath}}\bin\escript.exe" rebar3 cover
    - run: |
        & "${{steps.install_erlang.outputs.erlpath}}\bin\escript.exe" rebar3 escriptize
    - run: |
        & "_build\default\bin\elvis_core"

in ci.yml.

Bug Description

Recently I tried to run CI for my lib on Windows.

All the tests passed (surprisingly!) but Elvis (invoked through rebar3_lint) complained a lot about trailing spaces and operator spaces. Like, for almost every line of code.

So, what's happening is: git can be configured to automatically convert line endings into whatever the OS's native ones are.

And that's what it does by default on the Windows image I used, apparently.

However, elvis_core only knows how to split lines using \n (e.g. here, which leaves all of those extra \r around (and they qualify as whitespace.)

To Reproduce

Now that's tricky 🤔 I don't have Windows myself.

But it boils down to:

1. clone a rebar3 project which integrates rebar3_lint on Windows, and
2. run rebar3 lint.

Alternatively, leveraging GitHub Actions for CI'ing on Windows should be enough to confirm the problem.

Expected Behavior

I expected that any newline-affected rules (e.g. no_trailing_whitespace) would take into account Windows newline endings gracefully (\r\n rather than just \n.)

rebar3 Logs

Here it is (let me know if more details are needed.)

Additional Context

• OS: Microsoft Windows Server 2019 (10.0.17763)
• Erlang version: OTP 24.0.5
• rebar3 version: 3.16.1
• rebar3_lint version: 0.5.0 (which imports elvis_core 1.2.0)

Could we

1. load file & apply rules (then repeat with next file) per-file thus breaking the current "load all files & then apply rules"
2. make sure to not load same files twice for a same set of rules (dirs+filter can find the same files more than once)

For projects with lots of files using Elvis has proved to take quite a lot of CI time.

Using Elvis 0.2.11.

As per project-fifo/rebar3_lint#26 (comment).

It makes it much easier to adapt an existing code base with a failing test by setting the max. to the reported value (which otherwise has to be actually measured "by hand").

When analysis take too long, it's nice to have something like a waiting message, so we know the system's not stuck.

elvis analysis starting, this may take a while...

(it's basically the same message as rebar3 dialyzer already outputs, in another scope).

@elbrujohalcon, I feel this makes more sense here, but if you prefer I can transfer the issue over to `rebar3_lint and implement only at the plugin level.

• avoid and and or operators, prefer andalso and orelse, or
  in guards, where possible, combine expressions with , and ;

%% Bad
is_atom(Name) and Name =/= undefined.
is_binary(Task) or is_atom(Task).

%% Good
is_atom(Name) andalso Name =/= undefined.
is_binary(Task) orelse is_atom(Task).

%% Also good
when is_atom(Name), Name =/= undefined.
when is_binary(Task); is_atom(Task).

Ref: WhatsApp/erlfmt#91

I'm having the situation that I want to exclude some modules entirely. It are auto generated modules for a lexer and parser and enforcing syntax rules on they would be pointless.

I would like to add a ignore rule on the top level, along with dirs and filter that entirely takes modules out of the check.

If there are no objections to this I'll implement it and open a PR.

Explore some solution to explicitly mark functions as pure, .i.e. with no side effects, possibly with %% @pure comments, to have elvis check that the funcion does not access impure functions, ets tables, send or receive messages, access a database, etc.

Alternatively, have elvis warn about all impure functions, and explitly mark impure functions to suppress warnings.

Or enable both modes.

In Erlang, using spaces before and after the # symbol when dealing with maps or records, is valid. Still, it should be discouraged. I believe a rule that would prevent things like this from happening would be a nice addition:

Map = #   { this => map}.
Map     #      {}.

(don't want to get into an argument about what is an operator or not 😄)

Should we consider defaults for these?

=
+
-
*
/
%
<
=<
>
>=
==
=:=
/=
=/=
--
=>
:=
<-
<=
||
|
::
->

the Dry-ing should probably not suggest to turn a flat list/tuple creation into a more dry version. That would only be mis helpful. Perhaps there could be some added value to complexity like depth?

start_link(Server, Port, Command, From, Timeout) ->
    gen_fsm:start_link(?MODULE,
                       [Server, Port, Command, From, Timeout], []).
%% and
call(Server, Port, Command, From, Timeout) ->
    supervisor:start_child(libchunter_fsm_sup,
                           [Server, Port, Command, From, Timeout]).

trigger the DRY detector but it would make little sense to extract [Server, Port, Command, From, Timeout] into an own function, it would neither make the code more readable nor more maintainable.

Upping complexity would prevent this but then other more valid claims are perhaps ignored.

The Inaka Guidelines contain a write-function-specs rule, but the rule is not implemented as a check.

Even if an undocumented compiler option exists to warn about missing specs, this is not ideal for big, existing code bases which use it in combination with the "warning as errors" option.

It would be nice to have an Elvis check for missing specs, ideally with an option to check only exported functions / all functions.

Hi there,

I have tried to search previous issues but I cannot find this mentioned, correct me if I'm wrong.

I have been playing with an idea in my head which, before starting to work on it, I would like to discuss. Currently I'm running Elvis on a couple of large code bases and there are a bunch of modules that throw so many failures that we've had to add them to the various ignore rules since the time needed to fix them would outweigh the benefits. Occasionally during refactoring or cleanups these issues get resolved but Elvis still ignores these modules and doesn't indicate that the ignore-rule is no longer valid.

I would like to implement some form of check that can indicate whether or not ignore-rules have been resolved. Essentially even ignored-modules should be checked and if no issues are detected, some form of warning can be presented in order for the ignore-rule to be removed.

What is your stance on this feature and does it sound like something you would like to see?

Currently, our naming convention rules only allow for a single regex that identifies what is permitted/valid.
It would be nice to add an additional forbidden regex to identify things that are not valid, like function names starting with do_.

Ref: WhatsApp/erlfmt#91

I just created a new project, added no elvis.config to it, ran elvis on top of it and... got no issues.

I was assuming a default elvis.config would be applied.

Is there a preference here? Or is this a non-issue?

rebar3_lint does implement its own default, but it'll easily be outdated (or "wrong", if it's aiming at elvis' defaults - e.g. nesting_level default is now 4, not 3).

@Licenser, any thoughts on this?

The idea behind this rule comes from this email by @kvakvs.
Basically, the goal is to emit a warning if someone forgets a comma and writes something like:

[#{a => 1}
 #{b => 2}]

It will also warn on things like #{a =>1}#{b => 2}, but I think it's worth it since that expression is easily rewritable as #{a => 1, b => 2}.

As is, messages are of type Atom "<Atom>" on line <Line> does not....

I'm wondering if it would be best to make the output uniform, where possible (not all outputs have a line), like e.g.

<Line>: Atom "<Atom>" does not...

The new fixes in master deserve a new release. I hereby politely request one 😄

Edit: ah, and I also politely request it to be published to Hex.pm. Thanks.

At the moment, elvis_core depends on application:get_env which is not very usable for the consumer, especially if his/her project is not bound to a release.

The purpose of this task is to, while keeping backward compatibility, fetch further options from elvis.config + rebar.config for usability.

Note: search the code for application:.

Having:
#{key := A, x := X, y := Y, key := B} = ZZZ,
on the left of a pattern is not an error but can be very confusing. It could be a warning.

Having it on the right side of a pattern, or in a free-standing expression — is not an error but most likely is a sign of a typo, or a copy-paste error.
ZZZ = #{key => A, x => xxx, y => yyy, key => B}

Sometimes people are lazy or are very tired and write code like this:

        maps:from_list([
            {<<"A1">>, [0, 1, 2]},
            {<<"A2">>, [0, 3]}
        ]),

Elvis could detect conversion to and from maps, lists, and possibly other data types when the argument to the conversion function is a literal value. Of course it won't stop people from assigning literal values to a variable and passing that, but nobody's perfect.

Would it be possible to tag a release, here and in Hex.pm?

Many thanks.

Take an example from MongooseIM (actually a legacy leftover from ejabberd) - this inline fun is 55 lines long!
This leads to the following issues:

• the fun code has problems with indentation, since the body of the inline fun should be indented more than the outer code and using cases and ifs increases the indentation level even more,
• the inline fun is not easily traceable, because we don't know its real name (at least without referring to erlc -S) - in case of a fun this long, its logic might not be obvious, so tracing and seeing the return value might be really helpful.

An inline fun could be classified as too long/complex by counting the number of lines or indentation levels (maybe the same method as in inaka/elvis#82).

Elvis could even suggest a solution like this (possibly parameterized with the names of functions/arguments in question):

%% Before refactoring
do_something_on_a_list_of_items(ListOfItems) ->
    SomeVar1 = get_some_var1(),
    SomeVar2 = get_some_var2(),
    SomeVar3 = get_some_var3(),
    lists:map(fun (Elem) ->
                      %% very long closure using variables
                      %% SomeVar1, SomeVar2, SomeVar3 closed over
                      %% from the outer environment
              end, ListOfItems).

%% After refactoring
do_something_on_a_list_of_items(ListOfItems) ->
    SomeVar1 = get_some_var1(),
    SomeVar2 = get_some_var2(),
    SomeVar3 = get_some_var3(),
    lists:map(mk_step(SomeVar1, SomeVar2, SomeVar3), ListOfItems).

mk_step(SomeVar1, SomeVar2, SomeVar3) ->
    fun (Elem) ->
            do_something_with_one_item(SomeVar1, SomeVar2, SomeVar3, Elem)
    end.

do_something_with_one_item(SomeVar1, SomeVar2, SomeVar3, Elem) ->
    %% still a long function using variables
    %% SomeVar1, SomeVar2, SomeVar3
    %% passed in as arguments
    ...

elvis:rock/* seem to include all the printing code as well as the checking code. It could be helpful to separate this out in two step that way the output form the 1st step could be programmatically consumed while the output from step 2 could printed to the console.

https://erlang.org/doc/man/zlib.html

The functions here are called inflateInit, inflateEnd, et.c. which angers Elvis.

Any thoughts on this? I see this as the current workaround

decompress_data(Data) ->
    Bin = base64:decode(Data),
    Z = zlib:open(),
-   ok = zlib:inflateInit(Z),
+   ok = zlib:'inflateInit'(Z),
    Deflated = zlib:inflate(Z, Bin),
-   ok = zlib:inflateEnd(Z),
+   ok = zlib:'inflateEnd'(Z),
    ok = zlib:close(Z),
    jsone:decode(iolist_to_binary(Deflated)).

But I don't like working around linters.

Should atoms from the standard library be exempt?

This is an outdated rule.
The concept is not bad (When implementing an OTP behavior, define some type to hold its state), but the actual implementation of the rule requires the type to be called state/0 and that's too strict. For instance, we have implementations of gen_statem where it makes much more sense to call that type data/0 or state_data/0.

We should either allow the record name to be matched against a more flexible regular expression or just remove the rule from erl_files altogether.

    catch
        {this} = This -> This

reports Atom "{" on line ... does not respect the format defined by the regular expression

This one's potentially linked to #170, but even if not could be tackled at the same time, I guess.

When executing analysis on a given folder reference (origin: elvis.config > (inexisting)dirs => ["text/examples"] instead of (existing)dirs => ["test/examples"]), if no elements are found in a given examined path, a warning should be output, so we know we're dealing with an exception. (it also helps keep our elvis.config as trimmed as possible).

A rule to complain the use of any non-built-in macro, allowing for exceptions via a whitelist.

e.g.

{elvis_style, no_macros, ["EXCEPTION_1", "EXCEPTION_2"]}

When possible, add code suggestions to the warnings.
As an example,
Instead of…

• macro_names
  • Invalid macro name the_macro on line 11. Use UPPER_CASE.

…say…

• macro_names
  • Invalid macro name the_macro on line 11. Use UPPER_CASE.
  • QuickFix: replace it by THE_MACRO

This requires the dependencies to be availalbe via hex too
Regarding this I already opened:

• ErlyORM/aleppo#16
• inaka/zipper#22
• lambdaclass/erlang-katana#29

Once they are closed we should be able to use them in deps and use rebar3 hex upload to generate a elvis_core hex package :D

It'd be great if Elvis could be configured to ignore files matching entries in a .gitignore file. This would be useful to ignore auto-generated files without having to duplicate the content of .gitignore in elvis.config.

Tentative configuration example:

[{elvis,
  [{config,
    [#{dirs => ["src", "test"],
       filter => "*.erl",
       honor_gitignore => true,
       ruleset => erl_files
      }
    ]
   }]
 }] 

Recently, @robertoaloi reported an issue where he was getting a function_clause:

# apps/els_dap/test/els_dap_general_provider_SUITE.erl [FAIL]
Error: 'function_clause' while applying rule 'operator_spaces'.

This exception is being eaten up internally. We should figure out how to either throw it or use rebar3's logging facilities (if any) to ease this process.

They work only without defines. If I have this in the code:

-define(DBG(Fmt, Args), io:format(Fmt, Args)).
-define(CALL(M, F, A), M:F(A)).

f() ->
   ?DBG("oyh~s", ["veyh"]),
   ?CALL(el, iyahu, []).

it won't be able to find the no_call and invalid_dynamic_call violations.