[erlang-questions] Best practice for defining functions with edoc,erlc,eunit and the dialyzer

[Kostis Sagonas]

Joe Armstrong wrote:
> Best practice for writing documenting and testing code
> 
> I'd like to try and define "best practice" for writing documenting and
> testing Erlang code. I want to use:
> 
>     - only the tools supplied in the OTP release
> 
> So I use:
> 
>     - eunit for unit testing
>     - the dialyzer for checking my code
>     - edoc for documenting things
>     - type specifications for specifying types
> 
> These tools do not completely "play together" in a satisfactory manner,
> so I'd like to define what I thing is "best practice" and hope that by doing
> so the tools will converge.
> 
> Let's suppose I want to define the good 'ol factorial. Here's a module
> called elib1_best.erl. I've written it in such a way that it can be
> processed by erlc,eunit,edoc and the dialyzer - read the footnotes
> in brackets for an explanation.
> 
>        -module(elib1_best). %% [1]
> 
>        %% elib1_best: Best practice template for library modules [2]
>        %% Time-stamp: <2009-12-02 09:43:12 ejoearm> [3]
> 
>        %%----------------------------------------------------------------------
>        %% Copyright (c) 2009 Joe Armstrong <erlang@REDACTED> [4]
>        %% Copyright (c) 2009 Whoomph Software AB
>        %%
>        %% Permission is hereby granted, free of charge, to any person
>        %% obtaining a copy of this software and associated documentation
>        %% files (the "Software"), to deal in the Software without
>        %% restriction, including without limitation the rights to use, copy,
>        %% modify, merge, publish, distribute, sublicense, and/or sell copies
>        %% of the Software, and to permit persons to whom the Software is
>        %% furnished to do so, subject to the following conditions:
>        %%
>        %% The above copyright notice and this permission notice shall be
>        %% included in all copies or substantial portions of the Software.
>        %%
>        %% THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>        %% EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>        %% MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
>        %% NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
>        %% BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
>        %% ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
>        %% CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
>        %% SOFTWARE.
>        %%-----------------------------------------------------------------------
> 
>        -include_lib("eunit/include/eunit.hrl"). %% [5]
> 
>        -export([fac/1]).  %% [6]
> 
> 
>        %% @doc fac(N) computes factorial(N) using a fast
>        %% iterative algorithm. [7]
> 
>        -spec fac(integer()) -> integer(). [8]
> 
>        fac(N) when is_integer(N), N >= 0 -> fac1(N, 1).
> 
>        fac1(0, K) -> K;
>        fac1(N, K) -> fac1(N-1, K*N).
> 
>        fac_test() ->  %% [9]
>            6  = fac(3),
> 	       24 = fac(4).
> 
> 	       %% Notes:
> 	       %% [1] - module on line 1
> 	       %% [2] - module comment
> 	       %% [3] - Time stamp auto generated by emacs.
>                %%       Must be near start of file
> 	       %% [4] - Copyright (I always forget this, but adding a
>                %%       copyright reduces the pain later
> 	       %% [5] - Needed for eunit
> 	       %% [6] - use export and NOT compile(export_all)
> 	       %% [7] - @doc comes first
> 	       %% [8] - -spec comes immediately *before* the function
> 	       %% [9] - test cases come immediately after the function
> 
> 	       %% end of module
> ...
> 
> Now let's see what happens:
> 
>     1) Compiling
> 
>     erlc  +warn_missing_spec -o ../ebin -W elib1_best.erl
> 
>     ./elib1_best.erl:0: Warning: missing specification for function test/0
>     ./elib1_best.erl:44: Warning: missing specification for function fac_test/0
> 
> Best practice is to support type specifications for all exported
> functions.  But eunit magically adds a function test/0 and I really
> don't want to have to add manual exports and type specs for
> fac_test/0.
> 
>     [A fix is needed to erlc here, OR eunit can add type specs,
>      I think the latter is better - erlc should not need to know about eunit]

I agree with you that it's eunit's job to add these specs, but note that
 this can be done automatically in a type accurate way only if there is
some convention about user-supplied test functions.  Your fac_test/0
function currently returns 24 so the appropriate spec for it is:

	-spec fac_test() -> 24.

which is quite hard for eunit to generate.  The "best practice" way to
write test functions would be to make them return 'ok'.  I.e., write the
function as:

	fac_test() ->
	     6 = fac(3),
	    24 = fac(4),
	    ok.

>    2) Dialyzing
> 
> dialyzer --src elib1_best.erl
>   Checking whether the PLT /home/ejoearm/.dialyzer_plt is up-to-date... yes
>   Proceeding with analysis...
> Unknown functions:
>   eunit:test/1
>  done in 0m0.32s
> done (passed successfully)
> 
>     This is ok - I could add eunit to my plt if I wanted ...

Yes.  This can be done very easily with the command:

	dialyzer --add_to_plt --apps eunit

> Now for questions.
> 
>     1) Does this represent best practice? Is this the best way to
>        write code? - can anybody improve on this?

Well, yes.  One relatively minor problem is that the spec you specified
for the function, though not wrong, does not accurately represent your
intention.  You wrote:

         -spec fac(integer()) -> integer().

         fac(N) when is_integer(N), N >= 0 -> fac1(N, 1).

         fac1(0, K) -> K;
         fac1(N, K) -> fac1(N-1, K*N).

and it's pretty clear from your code that do not intend that other
modules use your fac/1 function with negative integers.  Why not
document this intention in the -spec?  In fact, if you dialyze as follows:

	dialyzer -Wunderspecs elib1_best.erl

dialyzer will tell you that:

elib1_best.erl:38: Type specification elib1_best:fac(integer()) ->
integer() is a supertype of the success typing:
elib1_best:fac(non_neg_integer()) -> pos_integer()

effectively prompting you to change the spec to:

	-spec fac(non_neg_integer()) -> pos_integer().

instead.

>     2) If I write like this can I assume that one day edoc
>        and eunit and erlc will converge so that I get correctly displayed
>        types in edoc and no warnings in erlc etc?

Yes.  The only issue is when.

Kostis