Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dialyzer: false positive #8222

Open
ilya-klyuchnikov opened this issue Mar 4, 2024 · 4 comments
Open

dialyzer: false positive #8222

ilya-klyuchnikov opened this issue Mar 4, 2024 · 4 comments
Assignees
Labels
bug Issue is reported as a bug stalled waiting for input by the Erlang/OTP team team:VM Assigned to OTP team VM

Comments

@ilya-klyuchnikov
Copy link
Contributor

Describe the bug

-module(dial).

-export([test1/01]).
-record(in, {
  f1 :: binary() | undefined
}).

-spec test1(In :: #in{}) -> binary().
test1(#in{f1 = F1}) when F1 =/= undefined ->
  to_binary(F1).


-spec to_binary(string() | binary()) -> binary().
to_binary(Data) when is_binary(Data) ->
  Data;
to_binary(Data) ->
  list_to_binary(Data).
dialyzer dial.erl
dial.erl:17:18: The call erlang:list_to_binary
         (Data :: 'undefined') breaks the contract 
          (IoList) -> binary() when IoList :: iolist()

dialyzer considers Data == undefined, though it can never be undefined - since the only code path from (exported) test1/1 checks that F1 =/= undefined.

Expected behavior
No report about Data :: undefined.

Affected versions
Reproducible with OTP 26.0.2

Additional information

Changing the type of the record field from f1 :: binary() | undefined to f1 :: binary() | string() | undefined make the error gone.

@ilya-klyuchnikov ilya-klyuchnikov added the bug Issue is reported as a bug label Mar 4, 2024
@jhogberg jhogberg added not a bug Issue is determined as not a bug by OTP bug Issue is reported as a bug and removed bug Issue is reported as a bug not a bug Issue is determined as not a bug by OTP labels Mar 4, 2024
@elbrujohalcon
Copy link
Contributor

Well… the error is pretty confusing (as usual), but the second clause for to_binary/1 is actually unneeded, right?
It would be good if dialyzer can tell you that it will never be used, but it instead assumes that it will called with undefined.
So, it's not exactly a "false positive", it's an "incorrectly labeled true positive" I would say :)

@jhogberg jhogberg added the stalled waiting for input by the Erlang/OTP team label Mar 4, 2024
@jhogberg jhogberg self-assigned this Mar 4, 2024
@jhogberg
Copy link
Contributor

jhogberg commented Mar 4, 2024

This is unfortunate. dialyzer doesn't handle negation at the moment, and only handles simple cases of subtraction from unions opportunistically whenever the representation happens to allow it.

Here, we have excluded binary() in the first clause because it happened to be convenient to do so, but failed to do the same in test1/1 because it wasn't. We won't be able to fix this in the near future. :-(

@ilya-klyuchnikov
Copy link
Contributor Author

@jhogberg - is it somehow connected to records?

changing the example to not use records make the error gone:

-module(dial).

-export([test1/01]).

-spec test1(binary() | undefined) -> binary().
test1(In) when In =/= undefined ->
  to_binary(In).

-spec to_binary(string() | binary()) -> binary().
to_binary(Data) when is_binary(Data) ->
  Data;
to_binary(Data) ->
  list_to_binary(Data).

@jhogberg
Copy link
Contributor

jhogberg commented Mar 4, 2024

The error disappears as specs describe arguments upon successful return, not as they enter the function (see point 4 in the EEP draft). This means that In, and therefore Data, can be anything in those functions.

#in{f1 = F1} in the clause head constrains the input to that record type, making F1 binary() | undefined.

@IngelaAndin IngelaAndin added the team:VM Assigned to OTP team VM label Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is reported as a bug stalled waiting for input by the Erlang/OTP team team:VM Assigned to OTP team VM
Projects
None yet
Development

No branches or pull requests

4 participants