Gracefully handle bad user input

[tsloughter]

I feel like I hit this the last time I tried Gradualizer but can't remember if I brought it up anywhere, sorry if I have already brought this up :)

In OpenTelemetry we try to handle bad user input by not crashing. So we end up with code like this https://github.com/open-telemetry/opentelemetry-erlang/blob/main/apps/opentelemetry_api/src/opentelemetry.erl#L386

-type status()             :: #status{}.
-type status_code()        :: ?OTEL_STATUS_UNSET | ?OTEL_STATUS_OK | ?OTEL_STATUS_ERROR.

-spec status(Code) -> status() | undefined when
      Code :: status_code().
status(Code) ->
    status(Code, <<>>).

-spec status(Code, Message) -> status() | undefined when
      Code :: status_code(),
      Message :: unicode:unicode_binary().
status(?OTEL_STATUS_ERROR, Message) when is_binary(Message) ->
    #status{code=?OTEL_STATUS_ERROR, message=Message};
status(?OTEL_STATUS_OK, _Message) ->
    #status{code=?OTEL_STATUS_OK};
status(?OTEL_STATUS_UNSET, _Message) ->
    #status{code=?OTEL_STATUS_UNSET};
status(_, _) ->
    undefined.

Gradualizer, rightly, tells me that it is not possible to return undefined because the first argument is always one of those 3 status codes.

opentelemetry.beam: The record on line 395 at column 5 is expected to have type status() | undefined but it has type #status{}

status(error, Message) ->
    #status{code = error, message = Message};
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

In the few minutes of usage Gradualizer has shown to be very useful in finding mistakes dialyzer can't, so I'd really like to consistently be able to use it going forward, but there is too much noise caused by how we need to ignore bad input.

Unless it is already possible and I simply missed it, does it sound feasible to ignore individual functions, isntead of individual modules, like we do with dialyzer in rebar3:

-dialyzer({no_underspecs, encode_msg/2}).
-spec encode_msg('$msg'(), '$msg_name'()) -> binary().
encode_msg(Msg, MsgName) when is_atom(MsgName) -> encode_msg(Msg, MsgName, []).

I might be able to find time to add that to Gradualizer's rebar3 plugin if so.

[josefs]

What version of Gradualizer are you running? The error that you get looks like a bug. Gradualizer doesn't recognize that status() and #status{} is the same type.
Is it feasible for you to try with the latest revision from github? I believe @erszcz has fixed the bug you're running into.

[erszcz]

Gradualizer should be able to tell that #status{} is actually one of the status() | undefined union members and it usually does it properly. This is a bug related to record handling, possibly a regression due to #452.

Currently, there's no explicit "ignore this function" mechanism, but maybe we need one. A half-measure is to not provide a spec or provide one with any(), the dynamic type, for the args and/or return value - it doesn't guarantee silencing a warning, but might help in some cases.

[tsloughter]

I didn't want to add any() or remove the spec because this is a public module so it is important documentation for users.

But! Your points about it just being confused about status() and #status{} resolved the issue, I just changed it to:

-spec status(Code, Message) -> #status{} | undefined when
      Code :: status_code(),
      Message :: unicode:unicode_binary().

This is fine for now.

Thanks!

[erszcz]

That's awesome!

[erszcz]

@tsloughter it seems PRs don't get automatically linked to discussions they mention, so check out #469.

[tsloughter]

Thanks! Seems to have worked.

[zuiderkwast]

Gradualizer, rightly, tells me that it is not possible to return undefined because the first argument is always one of those 3 status codes.

I understood this differently. I thought you got an error saying that the last clause status(_, _) -> undefined cannot be reached. When the status() vs #status{} issue has been resolved, perhaps you'll run into this next?

This is what we call exhaustiveness checking. We don't have an option to disable exhaustiveness checking but we might consider it. Your use case, code that needs to handle "bad user input", i.e. the caller passing arguments that don't follow the spec, is interesting. You know that the last clause can't be reached if the caller calls it according to the spec, but it can be reached if the caller code contains a type error, but the caller code is not within the same project and might not be type checked. I think this is an interesting discussion.

[tsloughter]

might not be type checked

Yup, exactly.

And good point about exhaustive checking, but I don't get the error saying undefined can't be reached. Only have 1 error left in that module and its about a different function.