OTP-28.0-rc.1: invalid opacity warning with dialyzer

[RoadRunnr]

Describe the bug

This test module:

-module(t2a).

-compile([export_all, nowarn_export_all]).

test(L) when is_map(L) ->
    maps:map(fun check/2, L).

check(atom, V) when is_atom(V) -> V;
check(list, V) when is_list(V) -> V;
check(map, V) ->
    case is_map(V) of
        true ->
            lists:foreach(fun(_) -> ok end, maps:to_list(V)),
            V;
        _ ->
            erlang:error(badarg, [set, V])
    end;
check(K, V) ->
    erlang:error(badarg, [K, V]).

Result in dialyzer warning that are IMHO invalid:

t2a.erl:8:1: Body yields the type 
          atom() which violates the opacity of the other clauses.
t2a.erl:9:1: Body yields the type 
          maybe_improper_list() which violates the opacity of the other clauses.
t2a.erl:10:1: Body yields the type 
          map() | maps:iterator(_, _) which violates the opacity of the other clauses.
 done in 0m0.22s
done (warnings were emitted)

To Reproduce

• save to t2a.erl
• compile with debug and run dialyzer on it: erlc +debug_info t2a.erl; dialyzer t2a.beam

Expected behavior
No warning should be produced by dialyzer

Affected versions
OTP-28.0-rc1

[jhogberg]

Thanks for reporting this, the warning is valid but rather counter-intuitive. The problem is that the spec for maps:to_list/1 mixes opaques with non-opaques by restricting its argument to map() | maps:iterator(_, _), which is an opacity violation in and of itself.

While the maps module is able to tell the difference between these types, its callers are not, and when the arguments are used later on they are (rightly) considered opaque unions.

Since there are a quite a bunch of questionable specs of this sort in OTP (owing to the previous implementation being too lax in this area), we're probably going to disable this new warning by default to reduce noise.

[RoadRunnr]

Sorry, I don't quite get it. The use of maps:to_list/1 comes after a is_map/1 check, so the unions should already have collapsed to map() only.
And strange enough, when the case is changed to use an guard instead, like this:

check(map, V) ->
    case V of
        _ when is_map(V) ->
            lists:foreach(fun(_) -> ok end, maps:to_list(V)),
            V;
        _ ->
            erlang:error(badarg, [set, V])
    end;

or this

check(map, V) when is_map(V) ->
    lists:foreach(fun(_) -> ok end, maps:to_list(V)),
    V;

then dialyzer stops complaining.

It seems a bit as if dialyzer treads the is_map() in the case differently.

[RoadRunnr]

BTW, the opacity also creates problems with sets. This also results on opacity warnings and there is no way to change that to a guard:

check(set, V) ->
    case sets:is_set(V) of
        true ->
            lists:foreach(fun(_) -> ok end, sets:to_list(V)),
            V;
        _ ->
            erlang:error(badarg, [set, V])
    end;

[lucioleKi]

I'll go into Dialyzer's internals to explain this. Don't take it as documentation:
Your observation is right. When the code contains a case like this, it'll be analyzed by dialyzer_dataflow:handle_case/3, which calls a helper function opaque_clauses/3 to report opacity violations. The comment in opaque_clauses/3 summarizes its logic:

Do any of the clauses return opaques? If yes, do all clauses return opaques from the same module? If not, emit a warning that the clauses mix opaques and non-opaques.

In contrast, if you rewrite the code to use guards, it'll be analyzed by dialyzer_dataflow:handle_case/3, which calls a helper function handle_opaque_guard_warnings/5. That helper function checks 1 guard at a time. As long as a guard doesn't violate opacity itself, the function won't complain.

So yes, using is_map() in guard is treated differently. IIRC, we designed it this way because function clauses separated by guards are separated at function level, while results coming out of case clauses can be easily joined together right after the case. In our opinion, it's easier for a programmer to do things with an opaque union if that's the result of case clauses, rather than the result of different function clauses with guards.

Ultimately, what is and what is not an opacity violation depends on the line we draw. In some cases, it's obvious that the programmer should be able to test whether something is a map or a set. In other cases, Dialyzer has to say "no I don't think you'd be able to tell an atom and an opaque apart". We're still trying to achieve that balance.

[jhogberg]

dialyzer doesn't follow case SOME_GUARD of true -> ... as well as it follows clause guards, be they function clauses or case clauses. So yes, it's correct to say that dialyzer treats case is_map(X) of true -> ...; false -> ... end differently.

As for sets and the like, yes, that follows the logic I wrote above. By sets:to_list/1 succeeding, you have now (by the spec) proven that its argument is an opaque type sets:set(_). If you then return that value (from a case or function), it may have its opacity violated by the other clauses, because you are now claiming to be able to tell the values apart later on.