Incompatible type warning

[TBK145]

Environment

• Elixir & Erlang/OTP versions (elixir --version): 1.11.2 / 23.1.1
• Operating system: Linux 5.9.3-arch1-1 If-statement in body causes warning message #1 SMP PREEMPT Sun, 01 Nov 2020 12:58:59 +0000 x86_64 unknown unknown GNU/Linux

Current behavior

We have a function for validating a certain struct, but our doctest seems to generate a warning. The warning itself is obvious, but the test is also to show it's wrong.

defmodule Foo do
  defstruct [:bar]

  @doc """
  Returns if the argument is a valid Foo struct or not.

  iex> Foo.is_valid?(%{})
  false
  iex> Foo.is_valid?(%Foo{})
  false
  iex> Foo.is_valid?(%Foo{bar: "baz"})
  true
  """
  @spec is_valid?(any()) :: boolean()
  defguard is_valid?(term)
           when is_struct(term, __MODULE__) and is_map_key(term, :bar) and not is_nil(term.bar)
end

I think that the code and tests are valid, but the doctest generates this warning:

warning: undefined field "bar" in expression:

    # (for doctest at) lib/foo.ex:7
    arg0.bar

expected one of the following fields: 

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

    # (for doctest at) lib/foo.ex:7
    {arg0} = {%{}}

Conflict found at
  (for doctest at) lib/foo.ex:7: FooTest."doctest Foo.is_valid?/1 (1)"/1

It will show the error for any type other than %Foo{}.

Expected behavior

No warnings when running the doctest.

[josevalim]

This is a common warning in guards because guards are expanded and the compiler is smart enough to "see inside". For example, we can't really test is_map(true) because the compiler will warn since true is not a map.

One possible fix for now is for you to replace %{} by Map.new() although in the future the compiler may even be smart enough to see across Map.new. :)

[TBK145]

Yeah I was suspecting something like that, but the test still looks valid to me. Is it maybe an option to not emit these warnings for (doc)tests?

[josevalim]

For now I don't want to allow to skip warnings because that's a tricky slippery slope. First we start skip warnings for tests then folks will start skip warnings in actual valid code.

The good news though is that: every time you get a warning because of your example, the user will also get the same warning, which means they will know it will fail before they even invoke it.

[michalmuskala]

I think the warnings is incorrect. The type warning should only be emitted for expressions that will fail at runtime - this is not the case, the expression discussed here will return false because of the earlier checks that the field exists before accessing it.

In other words, is_map_key(x, :foo) and x.foo should never produce a warning, regardless if x is inferred to have a :foo key or not - the expression will work correctly in both cases.

[josevalim]

Ah, yes, good point about the conditionals.

[ericmj]

To be able to fix this warning we need to implement a couple of things. is_map_key(foo, bar) needs to be smarter to add to the typing context that the map foo has the key bar in the blocks of code that execute if the function returns true (RHS of and, inside the do/else block of if, etc.).

We would also need to extend the conditional checks we do inside guards, that make sure is_integer(num) and num + 1 does not warn, to expressions. This is harder to do for expressions than guards because expressions are... well more expressive :). This will not be foolproof, but we believe that we will be able to handle the example in this issue.

Since it wont be foolproof we have the choice of accepting that we will have some false positives or do something like give all type variables from the conditional the dynamic() type inside the block the conditional affects: if is_map_key(foo, :bar) do <foo is dynamic here> end.

[josevalim]

I just had another convo with @ericmj where it seems the issue here is a bit simpler. Let's take two functions, v1 and v2:

def v1 do
  foo = %{}

  if is_map_key(foo, :bar) do
    foo.bar
  else
    :default
  end
end

def v2 do
  v2?(%{})
end

defp v2?(map) do
  if is_map_key(foo, :bar) do
    foo.bar
  else
    :default
  end
end

They are equivalent, implementation-wise, but we can arguably say v1 should warn something, because at worst it has dead code, while v2 should not warn.

The issue in this PR is that we are calling the code as in v2 but the final code looks like v1 because defguard is a macro. Therefore, I think we can address this by use the generated: true annotation.

[michalmuskala]

This still leaves "fixing" the warning in non-generated code. The code in v1 is entirely valid and will not fail at runtime, so a warning about an undefined field [1] is not helpful. A warning about dead code could be helpful, but it should probably be completely different.

[1] For the code like in the v1 example, the compiler today generates:

warning: undefined field "bar" in expression:

    # test.ex:6
    foo.bar

expected one of the following fields:

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

    # test.ex:3
    foo = %{}

Conflict found at
  test.ex:6: Test.v1/0

[josevalim]

Yes, in the future it will be a warning about dead-code because we will identify only one of the clauses match and then we won't bother to analyze what is inside that clause.

[eksperimental]

Here' s a reduced test case for a similar bug reported in Elixir Forum
https://elixirforum.com/t/incompatible-types-warning-on-complex-defguard-within-elixir-1-11-3/37885

defmodule Warning do
  defguardp is_tuple_or_nil(param) when is_nil(param) or is_tuple(param)
                   
  def hello(param) when is_atom(param) and is_tuple_or_nil(param) do
    :is_atom_or_nil
  end
end

[marcelotto]

I wanted to use the new capability from Elixir 1.11 to use the map.field syntax in guards and stumbled across the false warning from this issue similarly. Other than the title suggests, this has nothing to do with a Doctest as the warning will be produced for any use of the guard where the input term isn't a map with the field in question.

When conditionals are not taken into account, how can I ever use a map.field expression in a guard without risking getting this warning for any other input term for map than a map or struct with the given field? Am I missing something?

BTW: Shouldn't the is_struct expression be sufficient, as it ensures the presence of the field :bar already?

[mtarnovan]

I have another example that unexpectedly produces a warning:

defmodule Warning do
  defguard some_guard(x) when x == :foo or (is_tuple(x) and elem(x, 0) == :zzz)
  def bla(foo) when not some_guard(foo) do
    foo.bar
  end
end

Interestingly, if we remove the and elem(x, 0)... part the warning goes away:

defmodule NoWarning do
  defguard some_guard(x) when x == :foo or is_tuple(x)
  def bla(foo) when not some_guard(foo) do
    foo.bar
  end
end

[josevalim]

Another one from #12505:

Current behavior

Compiling this snippet raises a type warning:

defmodule Foo do
  def bar(token, number) when is_number(number) do
    case token do
      token when is_float(number) -> token
      token when is_integer(number) -> token
    end
  end
end

warning: incompatible types integer() !~ float()

warning: incompatible types:

integer() !~ float()

in expression:

# test.exs:4
is_float(number)

where "number" was given the type integer() | float() in:

# test.exs:2
is_number(number)

where "number" was given the type float() in:

# test.exs:4
is_float(number)

Conflict found at
test.exs:4: Foo.bar/1

Expected behavior

notably this code correctly doesn't warn, which is the expected behaviour for the block above as well

defmodule Foo do
  def bar(number) when is_number(number) do
    case number do
      number when is_float(number) -> :float
      number when is_integer(number) -> :integer
    end
  end
end

[novaugust]

to expand on the snippet above, i played with removing the is_integer/is_float from one or the other clause and still got the same warning, so having both isn't necessary - having either one will do it.
however! using the guard outside of a function head / case clause doesnt trigger the error, so looks like i'll be doing that to get around the warning for now

# both of these still warn
  def bar(token, number) when is_number(number) do
    case token do
      token when is_float(number) -> token
      token -> token
    end
  end

  def bar(token, number) when is_number(number) do
    case token do
      token when is_integer(number) -> token
      token -> token
    end
  end

# no warning
  def bar(token, number) when is_number(number) do
    integer? = is_integer(number)
    case token do
      token when integer? -> token
      token -> token
    end
  end

[josevalim]

Closing this, as there is a new implementation coming soon, and the old implementation has been removed.