Possible bug in compiler emitted warnings about incompatibles types in guard

[lud]

Erlang/OTP 24 [erts-12.2.1] [source] [64-bit] [smp:16:16] [ds:16:16:10] [async-threads:1] [jit]

Elixir 1.13.3 (compiled with Erlang/OTP 24)

Please have a look at this demo code, notably the defguard call and how it is used:

defmodule SomeGood do
  defstruct dummy: nil
end

defmodule Demo do
  @good_modules [Somegood]

  defguard is_good(mod_or_struct)
           when (is_atom(mod_or_struct) and mod_or_struct in @good_modules) or
                  (is_map(mod_or_struct) and is_map_key(mod_or_struct, :__struct__) and
                     :erlang.map_get(:__struct__, mod_or_struct) in @good_modules)

  defguard is_bad(mod_or_struct) when not is_good(mod_or_struct)
end

defmodule Worker do
  import Demo

  def work(%contract{}) when is_good(contract) do
    :alright
  end

  def work(_) do
    :nope
  end

  def a_case(data) do
    case data do
      %contract{} when is_good(contract) -> :good_contract
      data when is_good(data) -> :good_data
      _other -> :bad_data
    end
  end
end

defmodule Dependent do
  require Demo

  def get_fun do
    fn
      %contract{} when Demo.is_bad(contract) -> :nope
      _ -> :alright
    end
  end
end

It produces the following warnings:

warning: expected Kernel.is_map_key/2 to have signature:

    :__struct__, atom() -> dynamic()

but it has signature:

    dynamic(), %{optional(dynamic()) => dynamic()} -> dynamic()

in expression:

    # lib/demo.ex:41
    is_map_key(contract, :__struct__)

Conflict found at
  lib/demo.ex:41: Dependent.get_fun/0

warning: incompatible types:

    atom() !~ map()

in expression:

    # lib/demo.ex:19
    is_map(contract)

where "contract" was given the type atom() in:

    # lib/demo.ex:19
    %contract{}

where "contract" was given the type map() in:

    # lib/demo.ex:19
    is_map(contract)

Conflict found at
  lib/demo.ex:19: Worker.work/1

warning: incompatible types:

    atom() !~ map()

in expression:

    # lib/demo.ex:29
    is_map(contract)

where "contract" was given the type atom() in:

    # lib/demo.ex:29
    %contract{}

where "contract" was given the type map() in:

    # lib/demo.ex:29
    is_map(contract)

Conflict found at
  lib/demo.ex:29: Worker.a_case/1

But to me it looks like the guards are fine.

The main guard is defined as is instead of using is_struct because I was trying to find why there was such warnings.

The warning about expected Kernel.is_map_key/2 to have signature is caused by the usage of the guard defined as defguard is_bad(mod_or_struct) when not is_good(mod_or_struct) only.

[josevalim]

The warnings are correct. Once you pattern match on the struct, then there is no chance it can be a map, so that part of the guard is failing. The big question is if we should show those warnings.

If you were writing the guard directly, then it should warn indeed, but because you encapsulated it in a defguard, perhaps you have the expectation that it should not warn as long as at least one of the "or"s succeed.

The best fix for now is to break your guard apart. You should match on an atom or a struct.

[josevalim]

I think we should at least improve the error message:

1. The banner can be clearer
2. Note the argument order is wrong
3. Shouldn't is_map_key/2 return a boolean?

Perhaps we need to go with:

warning: Kernel.is_map_key/2 was invoked with:

    atom(), :__struct__ -> dynamic()

but it has signature:

    %{optional(dynamic()) => dynamic()}, dynamic() -> boolean()

in expression:

    # lib/demo.ex:41
    is_map_key(contract, :__struct__)

[lud]

perhaps you have the expectation that it should not warn as long as at least one of the "or"s succeed.

Yes, "succeed" at the type-wise level (the guard can still fail) because the whole guard is compatible with both atoms and maps. And also because the compiler does not emit warnings if you pass something else to the simple guard is_list(my_var) for instance (but I understand now that the compiler knows it is an atom).

The big question is if we should show those warnings.

So my opinion is that we should not.

The best fix for now is to break your guard apart. You should match on an atom or a struct.

Yes, thanks, I will use the atom version only and let the parent code check for structs and use the atom guard.

(this was a reply to your first comment)

Regarding your second comment, as a user I expect the compiler to not produce the is_map_key warning at all, because the call to is_map_key is after a call to is_map, so is_map_key will never be called with an atom.

[josevalim]

And also because the compiler does not emit warnings if you pass something else to the simple guard is_list(my_var) for instance

The compiler definitely warns:

iex(8)> defmodule Foo do
...(8)> def foo(%contract{}) when is_list(contract) do
...(8)> contract
...(8)> end
...(8)> end
warning: incompatible types:

    atom() !~ [dynamic()]

in expression:

    # iex:9
    is_list(contract)

where "contract" was given the type atom() in:

    # iex:9
    %contract{}

where "contract" was given the type [dynamic()] in:

    # iex:9
    is_list(contract)

Conflict found at
  iex:9: Foo.foo/1

Regarding your second comment, as a user I expect the compiler to not produce the is_map_key warning at all,

Good call. The not may be throwing the checker on a hoop here. @ericmj, perhaps we should avoid checking inside not for now?

[lud]

The compiler definitely warns

Yep ! I guess it is because contract is not just a variable for the compiler, it "knows" that it is an atom. I learnt that when you wrote "Once you pattern match on the struct" :)

[josevalim]

/cc @michallepicki in case you want to have some fun with this one. :)

[michallepicki]

Looks like type guards can cause warnings in presence of additional type information, like from argument pattern matches:

defmodule Mod do
  def function(:atom = arg) when is_atom(arg) or is_integer(arg) do
    arg
  end

  def function(1 = arg) when is_atom(arg) or is_integer(arg) do
    arg
  end
end

Results in (two warnings)

$ bin/elixir afile.ex
warning: incompatible types:

    integer() !~ atom()

in expression:

    # afile.ex:6
    is_atom(arg)

where "arg" was given the type integer() in:

    # afile.ex:6
    1 = arg

where "arg" was given the type atom() in:

    # afile.ex:6
    is_atom(arg)

Conflict found at
  afile.ex:6: Mod.function/1

warning: incompatible types:

    :atom !~ integer()

in expression:

    # afile.ex:2
    is_integer(arg)

where "arg" was given the type :atom in:

    # afile.ex:2
    :atom = arg

where "arg" was given the type integer() in:

    # afile.ex:2
    is_integer(arg)

Conflict found at
  afile.ex:2: Mod.function/1

But this is consistent with the current overly type-constraining nature of those guards in the checker. This module warns about incompatible types (I changed or to and), not that the first function clause will never match:

defmodule Mod do
  def function(arg) when is_atom(arg) and is_integer(arg) do
    arg
  end

  def function(arg) do
    arg
  end
end

Results in

$ bin/elixir afile.ex
warning: incompatible types:

    atom() !~ integer()

in expression:

    # afile.ex:2
    is_integer(arg)

where "arg" was given the type atom() in:

    # afile.ex:2
    is_atom(arg)

where "arg" was given the type integer() in:

    # afile.ex:2
    is_integer(arg)

Conflict found at
  afile.ex:2: Mod.function/1

I see some code in pattern.ex that is about handling :erlang.andalso/2 and :erlang.orelse/2 in guards, and some mentions in comments about types. I don't know what are examples where this code changes anything but it doesn't seem to help in this case, it also warns about incompatible types here:

defmodule Mod do
  def function(arg) when :erlang.andalso(is_atom(arg), is_integer(arg)) do
    arg
  end

  def function(arg) do
    arg
  end
end

Results in

warning: incompatible types:

    atom() !~ integer()

in expression:

    # afile.ex:2
    is_integer(arg)

where "arg" was given the type atom() in:

    # afile.ex:2
    is_atom(arg)

where "arg" was given the type integer() in:

    # afile.ex:2
    is_integer(arg)

Conflict found at
  afile.ex:2: Mod.function/1

I don't know too much about this typechecker (and only a little about typecheckers in general) but it seems like flow typing (occurrence typing) would help, type guards should only be constraining in some contexts, when considering a "branch" in which the type guard evaluates to true (or false, if this allows to continue evaluation). If a type guard can never evaluate to true/false (depending on what is required to continue), this guard is redundant and can be warned about. And after considering pattern matching and guards for a branch, if any of argument types is an empty type, this branch can never match (is dead code) and a warning can be emitted about that.

[josevalim]

Correct! All of the warnings are correct, the question is mostly how we can improve the warning in this case. Emitting those as "condition can never be reached" would be a way to improve it, but a bit unreachable for now. I think improving the current warning, argument order etc, may suffice as improvement (and perhaps stop cascading the warnings somehow?)