Check for missing specs (all/exported_only) about elvis_core HOT 11 CLOSED

I've created an Erlang bugs - question to check if exposing warn_missing_spec and sibling warn_missing_spec_all is desirable.

from elvis_core.

I have mixed feelings about this.

I see value in the rule, but at the same time wouldn't the output be just as "complicated" as the compiler option's? Or are you just trying to bypass the warnings_as_errors bit?

If we go forward, you'd get the advantage of being able to disable on a per-module/per-function basis, which I guess would be the biggest advantage (couple with "no warnings as errors").

Let me write some notes for future discussion also:

• I'd put this rule in the elvis_style module,
• I'd name this rule missing_spec,
• I'd add option all (boolean()) to this rule: it would evaluate private functions if it was true, but default to false,
• I'd have this rule disabled by default.

from elvis_core.

Hi @paulo-ferraz-oliveira and thanks for the quick reply!

Our use case is a monolithic code base of 1.5 million lines of code where we adopt the warning_as_errors option and we therefore cannot enable the missing_spec as an option in a trivial way. The idea is to use Elvis for metrics and to avoid the introduction of new offenders compared to a "baseline". We used a similar approach with dialyzer which allowed us to fix a lot of issues (most of them in specs, some of them in code) in the code base and we plan to use the same approach with Elvis.

from elvis_core.

Sure.

I haven't looked into how the parsing of the -spec(_). directive is done yet, though. I'm unsure making sure -spec(_) exists prior to a function definition is enough, since I believe it is possible to have it in a header, but I'd have to check closer.

Would a rule like "if not immediately preceding the function definition, issue a warning" work for you? Or would it be OK (with the associated performance penalty) to check the whole file for functions, then the whole file for specs, then compare those lists? - assuming the compiler does everything else, of course.

You see...

-module(a).
-module(a).

-export([a/0, b/0]).

-spec a() -> ok.
b() ->
    ok.

a() ->
    ok.

compiles, but should probably issue an Elvis warning.

from elvis_core.

Hi. I assume Elvis can keep state while parsing the forms? Since the spec attribute contains the name of the function it refers to, it should be possible to go through the forms only once and match each function with the respective spec. At the end of the scan, raise a warning for all the functions without specs.

I can imagine how, for example in generated code, specs could be in a separate file, in which case we'd need to expand the surface to be scanned by including (transitively) all the included files. If Elvis does this already for other use cases, that would be ideal, otherwise we could start by checking for specs in the current module only.

Regarding your example, you have a point. Yes, I would expect a warning, too. So maybe just simplify the whole thing and check for specs/function in a strict sequence is the way to go.

from elvis_core.

Yeah… analyzing more than one file at a time is out of scope for elvis.
Also, analyzing more than one form at a time, even when not out of scope, it's something that elvis don't usually do.
If you check the code for elvis_style (for instance) you'll see that most rules are coded as just a simple code traversal that checks either the text of the module line by line or the AST of the module form by form.
Each rule has its own implementation and there is no single state-preserving file traversal.

from elvis_core.

@robertoaloi correct me if I'm wrong but the use case you shared seems specific to the code base and configuration you are working with. Have you considered writing your own user defined elvis rule?

from elvis_core.

Would it be possible to just parse the module, top to bottom, and make sure all functions have a previous spec declaration? I guess checking function name and arity should be enough - the rest is dialyzer's work.

from elvis_core.

(I'm tentatively assigning this to me lest I forget about it)

from elvis_core.

I'm closing this one. I don't see us implementing this in the near future, @paulo-ferraz-oliveira. Feel free to reopen it if you plan to work on it.

from elvis_core.

Hm... yeah. Since the compiler already has this as an option, somehow, I'm not sure repeating it in elvis_core wouldn't do more "damage" than not.

from elvis_core.