gen_statem:start_monitor dialyzer warning

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

gen_statem:start_monitor dialyzer warning

Matt Kowlaczyk

Having a hard time understanding why the following sample code generates a dialyzer warning,

-module(foo).

-behaviour(gen_statem).

-export([start_monitor/0,callback_mode/0,init/1,start/3]).

-spec start_monitor() -> {ok, {pid(), reference()}}.
start_monitor() ->
    gen_statem:start_monitor(?MODULE, [], []).

callback_mode() ->
    state_functions.

init(_Args) ->
    {ok, start, []}.

start(_EventType, _EventContent, Data) ->
    {keep_state, Data}.

The warning is,

foo.erl:7: Invalid type specification for function foo:start_monitor/0. The success typing is
          () -> 'ignore' | {'error', _}

What's puzzling me is that this doesn't happen when calling gen_statem:start or gen_statem:start_link which are also annotated with the 'ignore' | {'error', _} clauses.  Also, when the spec statement of foo:start_monitor/0 is changed to, -spec start_monitor() -> ignore dialyzer passes successfully.

Thanks!

-- 
Matt Kowalczyk

publickey - matt@kowal.ch - 2ce7beba.asc (4K) Download Attachment
signature.asc (858 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: gen_statem:start_monitor dialyzer warning

Kostis Sagonas-2
On 1/24/21 2:14 PM, Matt Kowalczyk wrote:
> Having a hard time understanding why the following sample code generates
> a dialyzer warning,

Either because somebody added wrong specs to 'gen_statem' or forgot to
update the specs of 'gen'.


===== gen_statem has these types and specs =====

-type start_mon_ret() ::
         {'ok', {pid(),reference()}}
       | 'ignore'
       | {'error', term()}.

-spec start_monitor(
        Module :: module(), Args :: term(), Opts :: [start_opt()]) ->
                   start_mon_ret().
start_monitor(Module, Args, Opts) ->
     gen:start(?MODULE, monitor, Module, Args, Opts).


===== gen:start/5's return type is: ======

-type start_ret()  :: {'ok', pid()} | 'ignore' | {'error', term()}.


Their intersection is: 'ignore' | {'error', term()}.


Dialyzer is never wrong (C), but wrong specs can cause confusion.

Kostis
Reply | Threaded
Open this post in threaded view
|

Re: gen_statem:start_monitor dialyzer warning

Raimo Niskanen-11
On Sun, Jan 24, 2021 at 02:31:54PM +0100, Kostis Sagonas wrote:
> On 1/24/21 2:14 PM, Matt Kowalczyk wrote:
> > Having a hard time understanding why the following sample code generates
> > a dialyzer warning,
>
> Either because somebody added wrong specs to 'gen_statem' or forgot to
> update the specs of 'gen'.

There is room for improvement here.

We have a few Issues reported to bugs.erlang.org regarding type exports
from gen_server and gen_statem, and internal discussions have not completed
on how much types that would be useful to export...

Here gen does not export the right type for gen:start(_, monitor, ...).
I am not sure that we should specify types in gen; it is an internal module,
so maybe it would be better to rely on Dialyzer figuring them out instead...

Also gen_statem exports start_ret() but not start_mon_ret().  That is not
consistent.

Cheers
/ Raimo


>
>
> ===== gen_statem has these types and specs =====
>
> -type start_mon_ret() ::
>          {'ok', {pid(),reference()}}
>        | 'ignore'
>        | {'error', term()}.
>
> -spec start_monitor(
> Module :: module(), Args :: term(), Opts :: [start_opt()]) ->
>   start_mon_ret().
> start_monitor(Module, Args, Opts) ->
>      gen:start(?MODULE, monitor, Module, Args, Opts).
>
>
> ===== gen:start/5's return type is: ======
>
> -type start_ret()  :: {'ok', pid()} | 'ignore' | {'error', term()}.
>
>
> Their intersection is: 'ignore' | {'error', term()}.
>
>
> Dialyzer is never wrong (C), but wrong specs can cause confusion.
>
> Kostis

--

/ Raimo Niskanen, Erlang/OTP, Ericsson AB
Reply | Threaded
Open this post in threaded view
|

Re: gen_statem:start_monitor dialyzer warning

Nalin Ranjan
Hi Raimo,

 A little help if you could please point me to some starter.

The type-spec of net:getnameinfo can be made better as currently the type of SockAddr = term() is provided, which can be made even more explicit. An example is in the same file in the type declaration of ifaddrs(), the field addr := socket:sockaddr()

I wanted to change it and raise a pull request, but not sure where can I. Can you please help me with some pointers. Rest I can follow.

Thanks and Regards
Nalin Ranjan

On Mon, Jan 25, 2021 at 5:22 PM Raimo Niskanen <[hidden email]> wrote:
On Sun, Jan 24, 2021 at 02:31:54PM +0100, Kostis Sagonas wrote:
> On 1/24/21 2:14 PM, Matt Kowalczyk wrote:
> > Having a hard time understanding why the following sample code generates
> > a dialyzer warning,
>
> Either because somebody added wrong specs to 'gen_statem' or forgot to
> update the specs of 'gen'.

There is room for improvement here.

We have a few Issues reported to bugs.erlang.org regarding type exports
from gen_server and gen_statem, and internal discussions have not completed
on how much types that would be useful to export...

Here gen does not export the right type for gen:start(_, monitor, ...).
I am not sure that we should specify types in gen; it is an internal module,
so maybe it would be better to rely on Dialyzer figuring them out instead...

Also gen_statem exports start_ret() but not start_mon_ret().  That is not
consistent.

Cheers
/ Raimo


>
>
> ===== gen_statem has these types and specs =====
>
> -type start_mon_ret() ::
>          {'ok', {pid(),reference()}}
>        | 'ignore'
>        | {'error', term()}.
>
> -spec start_monitor(
>       Module :: module(), Args :: term(), Opts :: [start_opt()]) ->
>                  start_mon_ret().
> start_monitor(Module, Args, Opts) ->
>      gen:start(?MODULE, monitor, Module, Args, Opts).
>
>
> ===== gen:start/5's return type is: ======
>
> -type start_ret()  :: {'ok', pid()} | 'ignore' | {'error', term()}.
>
>
> Their intersection is: 'ignore' | {'error', term()}.
>
>
> Dialyzer is never wrong (C), but wrong specs can cause confusion.
>
> Kostis

--

/ Raimo Niskanen, Erlang/OTP, Ericsson AB
Reply | Threaded
Open this post in threaded view
|

Re: gen_statem:start_monitor dialyzer warning

Nalin Ranjan
I am guessing that changing here should suffice. If you think otherwise please let me know. Or is it like that because of "ifdef(USE_ESOCK)"?

Thanks and Regards
Nalin Ranjan

On Mon, Jan 25, 2021 at 11:14 PM Nalin Ranjan <[hidden email]> wrote:
Hi Raimo,

 A little help if you could please point me to some starter.

The type-spec of net:getnameinfo can be made better as currently the type of SockAddr = term() is provided, which can be made even more explicit. An example is in the same file in the type declaration of ifaddrs(), the field addr := socket:sockaddr()

I wanted to change it and raise a pull request, but not sure where can I. Can you please help me with some pointers. Rest I can follow.

Thanks and Regards
Nalin Ranjan

On Mon, Jan 25, 2021 at 5:22 PM Raimo Niskanen <[hidden email]> wrote:
On Sun, Jan 24, 2021 at 02:31:54PM +0100, Kostis Sagonas wrote:
> On 1/24/21 2:14 PM, Matt Kowalczyk wrote:
> > Having a hard time understanding why the following sample code generates
> > a dialyzer warning,
>
> Either because somebody added wrong specs to 'gen_statem' or forgot to
> update the specs of 'gen'.

There is room for improvement here.

We have a few Issues reported to bugs.erlang.org regarding type exports
from gen_server and gen_statem, and internal discussions have not completed
on how much types that would be useful to export...

Here gen does not export the right type for gen:start(_, monitor, ...).
I am not sure that we should specify types in gen; it is an internal module,
so maybe it would be better to rely on Dialyzer figuring them out instead...

Also gen_statem exports start_ret() but not start_mon_ret().  That is not
consistent.

Cheers
/ Raimo


>
>
> ===== gen_statem has these types and specs =====
>
> -type start_mon_ret() ::
>          {'ok', {pid(),reference()}}
>        | 'ignore'
>        | {'error', term()}.
>
> -spec start_monitor(
>       Module :: module(), Args :: term(), Opts :: [start_opt()]) ->
>                  start_mon_ret().
> start_monitor(Module, Args, Opts) ->
>      gen:start(?MODULE, monitor, Module, Args, Opts).
>
>
> ===== gen:start/5's return type is: ======
>
> -type start_ret()  :: {'ok', pid()} | 'ignore' | {'error', term()}.
>
>
> Their intersection is: 'ignore' | {'error', term()}.
>
>
> Dialyzer is never wrong (C), but wrong specs can cause confusion.
>
> Kostis

--

/ Raimo Niskanen, Erlang/OTP, Ericsson AB
Reply | Threaded
Open this post in threaded view
|

Re: gen_statem:start_monitor dialyzer warning

Kostis Sagonas-2
In reply to this post by Raimo Niskanen-11
On 1/25/21 12:52 PM, Raimo Niskanen wrote:
> There is room for improvement here.

Yep, definitely.  Let me throw my 2 :-

> Here gen does not export the right type for gen:start(_, monitor, ...).
> I am not sure that we should specify types in gen; it is an internal module,

For a long time now, my view has been that for OTP folks, `internal
modules' are similar to what `unicorns' are for my little daughter.
They are imaginary creatures which are "goosey" and nice.

But in reality, they do not really exist.  Moreover, they might look
innocent and nice, but they have a horn that they can put to use if
strangers manage to put their hands on them.


Seriously now, in an open source code base, you cannot expect that you
would have a key module with a name ('gen') which is the second most
general name that I can think of (question for the reader: guess what is
the most `general` one?) and that this module will "just pollute the
name space" and will not be looked at or abused by any of the thousands
of developers who use Erlang/OTP.


My view is that you should *not* have such modules in a system!

If they are really intended to be `internal', their functions should
only appear *inside* the modules that they support.  If they are used by
multiple modules and you are worried about code duplication, simply have
them as include files.

That's my view/advice at least.

Kostis
Reply | Threaded
Open this post in threaded view
|

Re: gen_statem:start_monitor dialyzer warning

Nalin Ranjan


On Tue, Jan 26, 2021, 2:17 AM Kostis Sagonas <[hidden email]> wrote:
On 1/25/21 12:52 PM, Raimo Niskanen wrote:
> There is room for improvement here.

Yep, definitely.  Let me throw my 2 :-

> Here gen does not export the right type for gen:start(_, monitor, ...).
> I am not sure that we should specify types in gen; it is an internal module,

For a long time now, my view has been that for OTP folks, `internal
modules' are similar to what `unicorns' are for my little daughter.
They are imaginary creatures which are "goosey" and nice.

But in reality, they do not really exist.  Moreover, they might look
innocent and nice, but they have a horn that they can put to use if
strangers manage to put their hands on them.


Seriously now, in an open source code base, you cannot expect that you
would have a key module with a name ('gen') which is the second most
general name that I can think of (question for the reader: guess what is
the most `general` one?

I would like to enjoy this one. So is it in a classificational sense or statistical sense that I have to guess the 'generality'? :-D 

) and that this module will "just pollute the
name space" and will not be looked at or abused by any of the thousands
of developers who use Erlang/OTP.


My view is that you should *not* have such modules in a system!

If they are really intended to be `internal', their functions should
only appear *inside* the modules that they support.  If they are used by
multiple modules and you are worried about code duplication, simply have
them as include files.

That's my view/advice at least.

Kostis
नमस्ते।
नलिन रंजन
Reply | Threaded
Open this post in threaded view
|

Re: gen_statem:start_monitor dialyzer warning

Raimo Niskanen-11
In reply to this post by Kostis Sagonas-2
On Mon, Jan 25, 2021 at 09:46:34PM +0100, Kostis Sagonas wrote:

> On 1/25/21 12:52 PM, Raimo Niskanen wrote:
> > There is room for improvement here.
>
> Yep, definitely.  Let me throw my 2 :-
>
> > Here gen does not export the right type for gen:start(_, monitor, ...).
> > I am not sure that we should specify types in gen; it is an internal module,
>
> For a long time now, my view has been that for OTP folks, `internal
> modules' are similar to what `unicorns' are for my little daughter.
> They are imaginary creatures which are "goosey" and nice.
>
> But in reality, they do not really exist.  Moreover, they might look
> innocent and nice, but they have a horn that they can put to use if
> strangers manage to put their hands on them.
>
>
> Seriously now, in an open source code base, you cannot expect that you
> would have a key module with a name ('gen') which is the second most
> general name that I can think of (question for the reader: guess what is
> the most `general` one?) and that this module will "just pollute the
> name space" and will not be looked at or abused by any of the thousands
> of developers who use Erlang/OTP.
>
>
> My view is that you should *not* have such modules in a system!
>
> If they are really intended to be `internal', their functions should
> only appear *inside* the modules that they support.  If they are used by
> multiple modules and you are worried about code duplication, simply have
> them as include files.
>
> That's my view/advice at least.
>
> Kostis

Given that 'gen' is an undocumented helper module in stdlib that is about
15..20 years older than Dialyzer, I was wondering about if, as long as it
still is undocumented, would it be better to not have type specifications
in it, possibly?

We are still struggling with best practices for Dialyzer in an old large
code base...

Best regards
--

/ Raimo Niskanen, Erlang/OTP, Ericsson AB
Reply | Threaded
Open this post in threaded view
|

Re: gen_statem:start_monitor dialyzer warning

Loïc Hoguin-3
I don't know about your specific question but...

On 26/01/2021 10:37, Raimo Niskanen wrote:
> We are still struggling with best practices for Dialyzer in an old large
> code base...

You have to run Dialyzer against tests. This is the best way to catch
the errors like the one in gen_statem:start_monitor because you need to
have calls to these functions for Dialyzer to do its work, and tests
tend to cover the entire public interface.

This does mean that some tests need to silence some Dialyzer warnings
when you do things wrong on purpose, though.

Cheers,

--
Loïc Hoguin
https://ninenines.eu