Temporarily violating record type constraints annoys dialyzer

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

Temporarily violating record type constraints annoys dialyzer

Roger Lipscombe-2
I've got a record defined as follows (e.g., and very simplified):

-record widget {
    id :: binary(),
    name :: binary(),
    size :: integer()
}.

I parse that from (e.g.) a proplist:

parse_widget(Props) ->
    parse_widget(Props, #widget{}).

parse_widget([{name, Name} | Rest], Acc) ->
    parse_widget(Rest, Acc#widget { name = Name });
% etc.

Dialyzer isn't happy that my fields are initially set to 'undefined',
even though this only occurs during the parsing step, and isn't a big
deal.

What can I do to deal with this? Either re-structuring my code or
persuading dialyzer that it's OK would both be acceptable.
_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Temporarily violating record type constraints annoys dialyzer

Brujo Benavides-3
Hi Roger,

According to how you use your record, its spec should actually be…

-record widget {
   id :: undefined | binary(),
   name :: undefined | binary(),
   size :: undefined | integer()
}.

That will silence dialyzer or, putting it in the right perspective: That will be a spec that matches how your code actually treats that record.
And that’s because: What happens if Props doesn’t have a tuple for name?

If not having a {name, Name} tuple in Props is an invalid scenario, you should raise some sort of error.
In that case I would recommend you to keep the record definition as-is, but fill the whole record at once:

parse_widget(Props) ->
  #widget{
    id = get_value(id, Props),
    name = get_value(name, Props),
    …
  }.

get_value(Key, Props) ->
  {Key, Value} = lists:keyfind(Key, 1, Props),
  Value.

That way, get_value/2 will raise an error if a property is missing.

If not having a {name, Name} tuple in Props is a valid scenario but you still don’t want that record property to be undefined, you will need a sane default for it.
In that case, you should amend your record definition as…

-record widget {
   id = <<>> :: binary(),
   name = <<>> :: binary(),
   size = 0 :: integer()
}.

Hope this helps :)


On 12 Nov 2018, at 07:58, Roger Lipscombe <[hidden email]> wrote:

I've got a record defined as follows (e.g., and very simplified):

-record widget {
   id :: binary(),
   name :: binary(),
   size :: integer()
}.

I parse that from (e.g.) a proplist:

parse_widget(Props) ->
   parse_widget(Props, #widget{}).

parse_widget([{name, Name} | Rest], Acc) ->
   parse_widget(Rest, Acc#widget { name = Name });
% etc.

Dialyzer isn't happy that my fields are initially set to 'undefined',
even though this only occurs during the parsing step, and isn't a big
deal.

What can I do to deal with this? Either re-structuring my code or
persuading dialyzer that it's OK would both be acceptable.
_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions


_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Temporarily violating record type constraints annoys dialyzer

Roger Lipscombe-2
On 12 November 2018 at 11:11, Brujo Benavides <[hidden email]> wrote:

> Hi Roger,
>
> According to how you use your record, its spec should actually be…
>
> -record widget {
>    id :: undefined | binary(),
>    name :: undefined | binary(),
>    size :: undefined | integer()
> }.
>
> That will silence dialyzer or, putting it in the right perspective: That
> will be a spec that matches how your code actually treats that record.
> And that’s because: What happens if Props doesn’t have a tuple for name?

I figured someone would say that. The answer is that Props *always*
has a tuple for name -- it was written to a data store with that
invariant. But I can see that it's hard to tell dialyzer this.

> If not having a {name, Name} tuple in Props is an invalid scenario, you
> should raise some sort of error.
> In that case I would recommend you to keep the record definition as-is, but
> fill the whole record at once:
>
> parse_widget(Props) ->
>   #widget{
>     id = get_value(id, Props),
>     name = get_value(name, Props),
>     …
>   }.
>
> get_value(Key, Props) ->
>   {Key, Value} = lists:keyfind(Key, 1, Props),
>   Value.

I was kinda hoping to avoid that -- I've got a vague feeling that we
did this for performance reasons. Of course, those assumptions are
probably no longer valid and need re-checking.

> Hope this helps :)

Only in the "you're doing it wrong" sense :)
_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Temporarily violating record type constraints annoys dialyzer

zxq9-2
In reply to this post by Roger Lipscombe-2
On 2018年11月12日月曜日 10時58分36秒 JST Roger Lipscombe wrote:
> What can I do to deal with this? Either re-structuring my code or
> persuading dialyzer that it's OK would both be acceptable.

Providing a more complete typespec can work:

  -record(widget
      {id   :: undefined | binary(),
       name :: undefined | binary(),
       size :: undefined | integer()}.

Or providing typed defaults that you know are bottom types:

  -record(widget
      {id   = <<>> :: binary(),
       name = <<>> :: binary(),
       size = 0    :: integer()}.


Depending on the rest of the code, sometimes a type definition to mask
the record is an easy way to sidestep a few wierd Dialyzer corner cases:

  -type widget() :: #widget{}.

And sometimes cutting straight through with line-by-line code conceals
the problem more directly (which allows you to ignore extra args at a
negligble performance cost:

  parse_widget(Props) ->
          #widget{id   = proplists:get_value(id,   Props),
                                name = proplists:get_value(name, Props),
                                size = proplists:get_value(size, Props)}.


-Craig
_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Temporarily violating record type constraints annoys dialyzer

Brujo Benavides-3
In reply to this post by Roger Lipscombe-2
And what about adding sane defaults to the fields, then?

On 12 Nov 2018, at 08:35, Roger Lipscombe <[hidden email]> wrote:

On 12 November 2018 at 11:11, Brujo Benavides <[hidden email]> wrote:
Hi Roger,

According to how you use your record, its spec should actually be…

-record widget {
  id :: undefined | binary(),
  name :: undefined | binary(),
  size :: undefined | integer()
}.

That will silence dialyzer or, putting it in the right perspective: That
will be a spec that matches how your code actually treats that record.
And that’s because: What happens if Props doesn’t have a tuple for name?

I figured someone would say that. The answer is that Props *always*
has a tuple for name -- it was written to a data store with that
invariant. But I can see that it's hard to tell dialyzer this.

If not having a {name, Name} tuple in Props is an invalid scenario, you
should raise some sort of error.
In that case I would recommend you to keep the record definition as-is, but
fill the whole record at once:

parse_widget(Props) ->
 #widget{
   id = get_value(id, Props),
   name = get_value(name, Props),
   …
 }.

get_value(Key, Props) ->
 {Key, Value} = lists:keyfind(Key, 1, Props),
 Value.

I was kinda hoping to avoid that -- I've got a vague feeling that we
did this for performance reasons. Of course, those assumptions are
probably no longer valid and need re-checking.

Hope this helps :)

Only in the "you're doing it wrong" sense :)


_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Temporarily violating record type constraints annoys dialyzer

zxq9-2
In reply to this post by zxq9-2
On 2018年11月12日月曜日 20時35分42秒 JST [hidden email] wrote:
>   parse_widget(Props) ->
>  #widget{id  = proplists:get_value(id,  Props),
>            name = proplists:get_value(name, Props),
>            size = proplists:get_value(size, Props)}.

Holy tabs!
_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Temporarily violating record type constraints annoys dialyzer

Krzysztof Jurewicz
In reply to this post by Roger Lipscombe-2
A similar problem arises if we try to use record syntax to construct generators for property testing:

-module(foo_tests).

-include_lib("triq/include/triq.hrl").

-record(
   project,
   {name :: binary(),
    budget :: non_neg_integer(),
    successful :: boolean()}).

prop_successful_projects_are_succesful() ->
    ?FORALL(
       Project,
       #project{
          name=binary(),
          budget=non_neg_integer(),
          successful=true},
       Project#project.successful).

The tests pass, but Dialyzer complains. To silence it, we could rewrite the property as:

prop_successful_projects_are_succesful() ->
    ?FORALL(
       Project,
       ?LET(
          {Name, Budget},
          {binary(), non_neg_integer()},
          #project{
             name=Name,
             budget=Budget,
             successful=true}),
       Project#project.successful).

This is however more verbose.

We may want to put a generator into a function:

project() ->
    #project{
       name=binary(),
       budget=non_neg_integer(),
       successful=boolean()}.

The property may then be written as:

prop_successful_projects_are_succesful() ->
    ?FORALL(
       Project,
       (project())#project{successful=true},
       Project#project.successful).

But again Dialyzer complains. If we wanted to rewrite project() in a ?LET form, then we wouldn’t be able to write (project())#project{successful=true}, as ?LET results in a different data structure.

The root problem here is that by writing #project{name=binary(), budget=non_neg_integer(), successful=boolean()} we don’t really want to create a project record. Instead, we want to create a tuple with a structure resembling the project record which will then be used to create a concrete project record. Dialyzer however doesn’t know about that.

In this particular case one way to silence Dialyzer is to write the project record as:

-record(
   project,
   {name :: binary() | triq_dom:domain(binary()),
    budget :: non_neg_integer() | triq_dom:domain(non_neg_integer()),
    successful :: boolean() | triq_dom:domain(boolean())}).

This is however repetitive and also superfluous in production environment.

There is a parse transform named dynarec ( https://github.com/dieswaytoofast/dynarec ) “that automaticaly generates and exports accessors for all records declared within a module”. Theoretically it could be expanded to generate a function named from_map/2 which would look like this:

from_map(
  project,
  #{name := Name,
    budget := Budget,
    successful := Successful}) ->
    #project{
       name=Name,
       budget=Budget,
       successful=Successful}.

We could then expand the project/0 generator as:

project(FieldMap) ->
    ?LET(
       ProjectMap,
       maps:merge(
         #{name => binary(),
           budget => non_neg_integer(),
           successful => bool()},
         FieldMap),
       from_map(project, ProjectMap)).

It would allow us to leave the project record in its original form and rewrite the property as:

prop_successful_projects_are_succesful() ->
    ?FORALL(
       Project,
       project(#{successful => true}),
       Project#project.successful).

The parse_widget/1 function from the original post could be written as:

parse_widget(Props) ->
    from_map(widget, maps:from_list(Props)).

Generation of from_map/2 is however currently not implemented in dynarec.
_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Temporarily violating record type constraints annoys dialyzer

Krzysztof Jurewicz
> Generation of from_map/2 is however currently not implemented in dynarec.

I’ve implemented generation of from_map/2 and to_map/1; see this pull request: https://github.com/dieswaytoofast/dynarec/pull/7 . It should now be possible to convert records to maps and vice versa without writing boilerplate code.
_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Temporarily violating record type constraints annoys dialyzer

Fred Hebert-2
In reply to this post by Krzysztof Jurewicz
On 11/12, Krzysztof Jurewicz wrote:
>A similar problem arises if we try to use record syntax to construct generators for property testing:
>
> ...
>
>The tests pass, but Dialyzer complains. To silence it, we could rewrite
>the property as:
>
> ...

That's interesting. I pretty much never run Dialyzer against test suites
simply because there are cases where I want my tests to trigger failures
and validate in/out of boundary conditions are being checked, sometimes
to know if the right kind of exceptions is raised (or that the right
logs are produced as side-effects)

Every time I purposefully send in data that breaks boundaries and raises
exceptions which end up part of my program's contract even if it isn't
part of its type annotations, Dialyzer is guaranteed to never pass.

One of the small unspoken "rules" or practices I have is to never run
Dialyzer on test code because it never works super well for plenty of
cases, particularly when your tests explicitly try to break things.
_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Temporarily violating record type constraints annoys dialyzer

Brujo Benavides-3
Funny… I do exactly the opposite thing.

Since many of my projects are libraries and dialyzer is mostly clueless on the correctness of exported function specs when it has no other code using them, I use the tests to let dialyzer know how the functions are meant to be used. That way it can tell me if my specs are wrong or if something I meant to do with those functions is actually not possible.

That’s why, more often than not, instead of `rebar3 dialyzer`, I do `rebar3 as test dialyzer`.

BTW, I don’t think Krzysztof was saying that he run dialyzer against tests, just that (on one hand) tests pass but (on the other hand) when he checked his code with dialyzer, it complained. Mostly because I can see how dialyzer would complain with just his code, and no tests :)

Cheers

On 13 Nov 2018, at 09:23, Fred Hebert <[hidden email]> wrote:

On 11/12, Krzysztof Jurewicz wrote:
A similar problem arises if we try to use record syntax to construct generators for property testing:

...

The tests pass, but Dialyzer complains. To silence it, we could rewrite the property as:

...

That's interesting. I pretty much never run Dialyzer against test suites simply because there are cases where I want my tests to trigger failures and validate in/out of boundary conditions are being checked, sometimes to know if the right kind of exceptions is raised (or that the right logs are produced as side-effects)

Every time I purposefully send in data that breaks boundaries and raises exceptions which end up part of my program's contract even if it isn't part of its type annotations, Dialyzer is guaranteed to never pass.

One of the small unspoken "rules" or practices I have is to never run Dialyzer on test code because it never works super well for plenty of cases, particularly when your tests explicitly try to break things.
_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions


_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Temporarily violating record type constraints annoys dialyzer

Fred Hebert-2
On 11/13, Brujo Benavides wrote:

>Funny… I do exactly the opposite thing.
>
>Since many of my projects are libraries and dialyzer is mostly clueless on the correctness of exported function specs when it has no other code using them, I use the tests to let dialyzer know how the functions are meant to be used. That way it can tell me if my specs are wrong or if something I meant to do with those functions is actually not possible.
>
>That’s why, more often than not, instead of `rebar3 dialyzer`, I do `rebar3 as test dialyzer`.
>
>BTW, I don’t think Krzysztof was saying that he run dialyzer against tests, just that (on one hand) tests pass but (on the other hand) when he checked his code with dialyzer, it complained. Mostly because I can see how dialyzer would complain with just his code, and no tests :)
>
>Cheers
>
>Brujo Benavides <http://about.me/elbrujohalcon>
>

Do you just not do negative tests or you silence each of them
individually?
_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Temporarily violating record type constraints annoys dialyzer

Attila Rajmund Nohl
Fred Hebert <[hidden email]> ezt írta (időpont: 2018. nov. 13., K, 13:44):
[...]
> Do you just not do negative tests or you silence each of them
> individually?

If the library returns {error, Reason}, not simply crashes, then
dialyzer should not complain.
_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Temporarily violating record type constraints annoys dialyzer

Brujo Benavides-3
In reply to this post by Fred Hebert-2
I generally tend to follow the rule that people (users of my libs, in particular) should use dialyzer themselves.
So, for instance, if the the spec says this function should be called with a positive_integer(), I don’t test it with 0.
If anybody tries to use the function with the wrong input… something that will trigger a warning from dialyzer… they should detect it when running dialyzer on their code.

So, no… I don’t do negative tests, in that sense.

On the other hand, if I have a function like…

my_fun(X) when X > 0 -> work_with(X);
my_fun(X) -> error({invalid_input, X}).

I don’t spec it as…
-spec my_fun(pos_integer()) -> ….

I spec it as…
-spec my_fun(pos_integer()) -> …
     ; my_fun(X) -> no_return().

(or something similar, I typed that off-the-top-of-my-head)

On 13 Nov 2018, at 09:44, Fred Hebert <[hidden email]> wrote:

On 11/13, Brujo Benavides wrote:
Funny… I do exactly the opposite thing.

Since many of my projects are libraries and dialyzer is mostly clueless on the correctness of exported function specs when it has no other code using them, I use the tests to let dialyzer know how the functions are meant to be used. That way it can tell me if my specs are wrong or if something I meant to do with those functions is actually not possible.

That’s why, more often than not, instead of `rebar3 dialyzer`, I do `rebar3 as test dialyzer`.

BTW, I don’t think Krzysztof was saying that he run dialyzer against tests, just that (on one hand) tests pass but (on the other hand) when he checked his code with dialyzer, it complained. Mostly because I can see how dialyzer would complain with just his code, and no tests :)

Cheers

Brujo Benavides <http://about.me/elbrujohalcon>


Do you just not do negative tests or you silence each of them individually?


_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Temporarily violating record type constraints annoys dialyzer

Brujo Benavides-3
Forgot to say that, in the case of my_fun, I do test it with stuff like 0 or -1 or something_that_is_not_even_an_integer… But dialyzer doesn’t complain, since the spec allows it.

(Sorry for the double-emailing)

On 13 Nov 2018, at 09:49, Brujo Benavides <[hidden email]> wrote:

I generally tend to follow the rule that people (users of my libs, in particular) should use dialyzer themselves.
So, for instance, if the the spec says this function should be called with a positive_integer(), I don’t test it with 0.
If anybody tries to use the function with the wrong input… something that will trigger a warning from dialyzer… they should detect it when running dialyzer on their code.

So, no… I don’t do negative tests, in that sense.

On the other hand, if I have a function like…

my_fun(X) when X > 0 -> work_with(X);
my_fun(X) -> error({invalid_input, X}).

I don’t spec it as…
-spec my_fun(pos_integer()) -> ….

I spec it as…
-spec my_fun(pos_integer()) -> …
     ; my_fun(X) -> no_return().

(or something similar, I typed that off-the-top-of-my-head)

On 13 Nov 2018, at 09:44, Fred Hebert <[hidden email]> wrote:

On 11/13, Brujo Benavides wrote:
Funny… I do exactly the opposite thing.

Since many of my projects are libraries and dialyzer is mostly clueless on the correctness of exported function specs when it has no other code using them, I use the tests to let dialyzer know how the functions are meant to be used. That way it can tell me if my specs are wrong or if something I meant to do with those functions is actually not possible.

That’s why, more often than not, instead of `rebar3 dialyzer`, I do `rebar3 as test dialyzer`.

BTW, I don’t think Krzysztof was saying that he run dialyzer against tests, just that (on one hand) tests pass but (on the other hand) when he checked his code with dialyzer, it complained. Mostly because I can see how dialyzer would complain with just his code, and no tests :)

Cheers

Brujo Benavides <http://about.me/elbrujohalcon>


Do you just not do negative tests or you silence each of them individually?



_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Temporarily violating record type constraints annoys dialyzer

Loïc Hoguin-3
In reply to this post by Fred Hebert-2
On 11/13/18 1:44 PM, Fred Hebert wrote:

> On 11/13, Brujo Benavides wrote:
>> Funny… I do exactly the opposite thing.
>>
>> Since many of my projects are libraries and dialyzer is mostly
>> clueless on the correctness of exported function specs when it has no
>> other code using them, I use the tests to let dialyzer know how the
>> functions are meant to be used. That way it can tell me if my specs
>> are wrong or if something I meant to do with those functions is
>> actually not possible.
>>
>> That’s why, more often than not, instead of `rebar3 dialyzer`, I do
>> `rebar3 as test dialyzer`.
>>
>> BTW, I don’t think Krzysztof was saying that he run dialyzer against
>> tests, just that (on one hand) tests pass but (on the other hand) when
>> he checked his code with dialyzer, it complained. Mostly because I can
>> see how dialyzer would complain with just his code, and no tests :)
>>
>> Cheers
>>
>> Brujo Benavides <http://about.me/elbrujohalcon>
>>
>
> Do you just not do negative tests or you silence each of them individually?

I personally silence both Dialyzer warnings in expected test failures
(for example Cowboy handler crashes) and any error/crash logs that would
be produced, if any. I've just a few weeks ago enabled Dialyzer against
the Cowboy tests by default (whereas I was running it manually from time
to time before) and this helped me fix a few bad types.

Sometimes Dialyzer needs to be silenced via disabling a warning on a
specific function, sometimes adding a spec with no_return() is enough.
Either way you can't really mix positive and negative tests in the same
test function.

Cheers,

--
Loïc Hoguin
https://ninenines.eu
_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Temporarily violating record type constraints annoys dialyzer

Jesper Louis Andersen-2
In reply to this post by Roger Lipscombe-2
On Mon, Nov 12, 2018 at 12:35 PM Roger Lipscombe <[hidden email]> wrote:
I was kinda hoping to avoid that -- I've got a vague feeling that we
did this for performance reasons. Of course, those assumptions are
probably no longer valid and need re-checking.


A lists:keyfind on a small list is going to be fast, especially because you are only reading and thus not putting any GC pressure in there. Your parsing variant constructs a number of intermediate tuples and I don't think the Erlang compiler currently detects it can unroll that loop, and strength reduce the tuple creation so it has higher GC pressure. Alternatively, put it into a map and then transform the map. It is at most one write more.

I'd measure this carefully.




_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Temporarily violating record type constraints annoys dialyzer

Roger Lipscombe-2
In reply to this post by Krzysztof Jurewicz
On 12 November 2018 at 22:36, Krzysztof Jurewicz
<[hidden email]> wrote:
>> Generation of from_map/2 is however currently not implemented in dynarec.
>
> I’ve implemented generation of from_map/2 and to_map/1; see this pull request: https://github.com/dieswaytoofast/dynarec/pull/7 . It should now be possible to convert records to maps and vice versa without writing boilerplate code.

That's *almost* perfect ;-)

Except that we load the code from JSON, using jiffy, which means that
the keys are binary strings. I had a quick look at doing a PR for
this, but I'm a bit sketchy when it comes to parse transforms, and I
got side tracked.
_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Temporarily violating record type constraints annoys dialyzer

Krzysztof Jurewicz
In reply to this post by Brujo Benavides-3
Brujo Benavides writes:

> BTW, I don’t think Krzysztof was saying that he run dialyzer against tests, just that (on one hand) tests pass but (on the other hand) when he checked his code with dialyzer, it complained. Mostly because I can see how dialyzer would complain with just his code, and no tests :)

Actually I do run Dialyzer against tests, not only occasionally but routinely. Test code is also code and it can benefit from being checked by Dialyzer. Here is how it looks in the Ercoin repository (ABCI server):

• TEST_DIR is added to DIALYZER_DIRS: https://gitlab.com/Ercoin/ercoin/blob/0c6cedda6bdae870a70655524e008a80d1c1f170/Makefile#L46 . Therefore every invocation of make check and make dialyze (and also CI) covers test code.
• If ABCI client (Tendermint) sends malformed data, then it’s a bug on their side, so we can avoid blame if we crash.
• If end user sends invalid data (via ABCI client), then it is supported and we return a relevant error code. It is also tested; for example, there is a lengthy generator for creating invalid transaction binaries: https://gitlab.com/Ercoin/ercoin/blob/0c6cedda6bdae870a70655524e008a80d1c1f170/test/ercoin_tx_gen.erl#L266 .
• No throw expressions are used. If we parse data from a non-trustworthy data source and there is a possibility of early exit, nested case expressions or monads may be employed (though the latter not without issues, see https://github.com/fogfish/datum/issues/54 ).
_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Temporarily violating record type constraints annoys dialyzer

Krzysztof Jurewicz
In reply to this post by Roger Lipscombe-2
Roger Lipscombe writes:
>> I’ve implemented generation of from_map/2 and to_map/1; see this pull request: https://github.com/dieswaytoofast/dynarec/pull/7 . It should now be possible to convert records to maps and vice versa without writing boilerplate code.
>
> That's *almost* perfect ;-)
>
> Except that we load the code from JSON, using jiffy, which means that
> the keys are binary strings. I had a quick look at doing a PR for
> this, but I'm a bit sketchy when it comes to parse transforms, and I
> got side tracked.

A workaround (though likely not most efficient) may be to write the following function:

record_from_binary_proplist(RecordName, Props) ->
    BinaryFields = [list_to_binary(atom_to_list(Field)) || Field <- fields(RecordName)],
    from_map(
      RecordName,
      maps:from_list(
        [{list_to_atom(binary_to_list(Key)), Value}
         || {Key, Value} <- Props, lists:member(Key, BinaryFields)])).

Alternatively:

record_from_binary_proplist(RecordName, Props) ->
    FieldsByBinary = maps:from_list([{list_to_binary(atom_to_list(Field)), Field} || Field <- fields(RecordName)]),
    from_map(
      RecordName,
      lists:foldl(
        fun ({Key, Value}, MapAcc) ->
                case maps:find(Key, FieldsByBinary) of
                    {ok, KeyAtom} ->
                        maps:put(KeyAtom, Value, MapAcc);
                    error ->
                        MapAcc
                end
        end,
        #{},
        Props)).
_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
Reply | Threaded
Open this post in threaded view
|

Re: Temporarily violating record type constraints annoys dialyzer

Dániel Szoboszlay
In reply to this post by Roger Lipscombe-2
Hi,

I'm a bit late to this party, but I have a suggestion I haven't seen coming up in the thread. So maybe it could be still useful for you or someone else:

Consider moving type specs out from your record definition completely! Use a custom type instead:

-record(widget, {id, name, size}).
-type widget() :: #widget{id :: binary(), name :: binary(), size :: binary()}.

This way you can explicitly tell in the function specs whether you're dealing with a properly typed widget() or not. You may even define a different type for the parser, and have Dialyzer check against that:

-type partially_parsed_widget() :: #widget{id :: binary()|undefined, name :: binary()|undefined, size :: binary()|undefined}.
-spec parse_widget(proplists:proplist()) -> widget().
-spec parse_widget(proplists:proplist(), partially_parsed_widget()) -> widget().

There are lots of cases when you want to use a record's structure with different type constraints than the "usual" use of that record. Generating property based tests and negative tests were already mentioned. But using records in match specifications is also very common. And in case of some simple records you may also use tuples that are not meant to be a record, but look like one (consider someone defining -record(error, {err_no :: integer(), msg :: string()}). for example).

An additional benefit of separating record definitions from type specs is that Dialyzer would actually generate much more useful error messages on type violations. With types in records you may get something like this:

foo.erl:10: Function test/0 has no local return
foo.erl:11: Record construction #foo{bar::0} violates the declared type of field bar::pos_integer()

The problem is that the "has no local return" error is poisonous: it will propagate to all other functions calling foo:test/0, and from there to their callers etc. You'll get a ton of error messages and it will be very hard to figure out the root cause.

On the other hand when using a separate foo() type the error message would become:

foo.erl:7: Invalid type specification for function foo:test/0. The success typing is () -> #foo{bar::0}

This error will not propagate and will be easy to debug.

Just my two cents. Cheers,

Daniel

On Mon, 12 Nov 2018 at 11:58 Roger Lipscombe <[hidden email]> wrote:
I've got a record defined as follows (e.g., and very simplified):

-record widget {
    id :: binary(),
    name :: binary(),
    size :: integer()
}.

I parse that from (e.g.) a proplist:

parse_widget(Props) ->
    parse_widget(Props, #widget{}).

parse_widget([{name, Name} | Rest], Acc) ->
    parse_widget(Rest, Acc#widget { name = Name });
% etc.

Dialyzer isn't happy that my fields are initially set to 'undefined',
even though this only occurs during the parsing step, and isn't a big
deal.

What can I do to deal with this? Either re-structuring my code or
persuading dialyzer that it's OK would both be acceptable.
_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions

_______________________________________________
erlang-questions mailing list
[hidden email]
http://erlang.org/mailman/listinfo/erlang-questions
12