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 warning for overlapping domains #70

Closed
paulo-ferraz-oliveira opened this issue Jan 29, 2019 · 7 comments
Closed

Dialyzer warning for overlapping domains #70

paulo-ferraz-oliveira opened this issue Jan 29, 2019 · 7 comments

Comments

@paulo-ferraz-oliveira
Copy link
Contributor

dialyzer'ing master with OTP 18+ I get:

src/recon.erl
 200: Overloaded contract for recon:info/2 has overlapping domains; such contracts are currently unsupported and are simply ignored
 610: Overloaded contract for recon:port_info/2 has overlapping domains; such contracts are currently unsupported and are simply ignored

@ferd, would you be interested in a fix for this? Or do you consider it OK to leave as-is?

Note: I found no other dialyzer -related issues for OTP 18.3 through 21.2

@ferd
Copy link
Owner

ferd commented Jan 29, 2019

There is, strictly speaking, no good fix for this. The two type specs in question are:

-spec info(pid_term(), info_type()) -> {info_type(), [{info_key(), term()}]}
    ;     (pid_term(), [atom()]) -> [{atom(), term()}]
    ;     (pid_term(), atom()) -> {atom(), term()}.

-spec port_info(port_term(), port_info_type()) -> {port_info_type(),
                                                   [{port_info_key(), _}]}
    ;          (port_term(), [atom()]) -> [{atom(), term()}]
    ;          (port_term(), atom()) -> {atom(), term()}.

The problem, as far as I recall, is that info_type() and port_info_type() are both unions of multiple atoms, and there is therefore a kind of overlap between the first and last clauses (containing just atom()), which confuses Dialyzer.

The way to "fix" this would be to change the typespecs to be like:

-spec info(pid_term(), info_type() | atom()) -> {info_type(), [{info_key(), term()}]} | {atom(), term()}.
    ;     (pid_term(), [atom()]) -> [{atom(), term()}].

But then I would lose the relationship between the two return types and the input causing them. I preferred to have type signatures that are accurately describing what goes on in there (for whoever happens to read the source or generate docs for it -- which might also be broken) rather than obscure the flow.

If you can manage to keep the multiple clauses working without detaching them from their return value nor without breaking the interface, I'd welcome a patch, though.

@paulo-ferraz-oliveira
Copy link
Contributor Author

I do understand the overlap, but doesn't the input depend on what the lib. actually wants to filter in/out? If we're accepting atom() as the 2nd arg. of info (e.g.) doesn't that mean that all atoms are OK when used in :info, for example? Will results always be present in any of those cases? Or is enumerating them not possible?

@ferd
Copy link
Owner

ferd commented Jan 30, 2019

I'm not sure what you are asking for here?

@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented Jan 30, 2019

I tried to have a type process_info_item() much like the one defined in erlang:process_info/2... a bunch of atoms, and used it for :info/2's -spec(). The thing is that Dialyzer starts to complain once I declare 13 atoms (as if a | b | c | d | e | f | g | h | i | j | k | l | m | n became atom() internally) about the initial error (the overlapping domain). Do you know if there's such a possibility?

@paulo-ferraz-oliveira
Copy link
Contributor Author

paulo-ferraz-oliveira commented Jan 30, 2019

Here's an example of what I'm talking about. Of course, importing process_info_item() directly from the Erlang doc.s (instead of exposing it in its source) isn't the smartest idea, but it's so I'm clear in regards to my initial and further message.

@ferd
Copy link
Owner

ferd commented Jan 30, 2019

Dialyzer merges down unions of atoms to atom() at 13 yeah. I mean at this point we have the choice between losing precision in the type spec (and readability) to fix Dialyzer errors, or keep it as is where it is more complete as a code descriptor, but Dialyzer can't deal with it and apparently EDoc chokes on it as well: http://ferd.github.io/recon/recon.html#info-2

Whatever we do there's no good solution here, Dialyzer/EDoc are not powerful enough to encode what we're looking to do. I don't think that there is a good fix. Like maybe the best option is to open up bugs with Erlang and see if it gets resolved. The other option is to water down and complexify the type signature so it passes but loses meaning.

Is this blocking you? Are the dialyzer warnings causing issues in one of your projects?

@paulo-ferraz-oliveira
Copy link
Contributor Author

I agree it's better to have a proper -spec() that conveys intent than to "respect a tool" and lose meaning in the process. It's not a blocking issue; it's just one of "those" itches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants