Dialyzer only taking one path in `with` statement

GoNZooo commented 6 years ago

Precheck

Do not use the issues tracker for help or support (try Elixir Forum, Stack Overflow, IRC, etc.)
For proposing a new feature, please start a discussion on the Elixir Core mailing list
For bugs, do a quick search and make sure the bug has not yet been reported
Finally, be nice and have fun!

Environment

Elixir & Erlang versions (elixir --version): Elixir 1.5.2 & 1.6.0-rc.0 & 1.7.0-dev with Erlang/OTP 20
Operating system: Linux 4.14.11-1-ARCH

Current behavior

The following function will incorrectly be assumed to always return {:error, _} by dialyzer:

@spec read(url :: String.t()) :: {:ok, [Downloader.Torrent.t()]} | {:error, String.t()}
def read(url) do
  with {:ok, %HTTPoison.Response{body: body}} <- HTTPoison.get(url),
       {:ok, %FeederEx.Feed{entries: entries}, _} <- FeederEx.parse(body) do
    {:ok, Enum.map(entries, &Downloader.Torrent.from_entry/1)}
  else
    {:error, %HTTPoison.Error{reason: reason}} ->
      {:error, reason}

    {:fatal_error, _, reason, _, _} ->
      {:error, reason}
  end
end

If one were to change the second else clause to return {:fatal_error, reason} it still will not recognize that there is one more possible return value, but will still insist that only {:error, _} can be returned.

When matching with the following pattern:

case Downloader.RSS.read(state.url) do
  {:ok, ts} ->
    ts

  {:error, reason} ->
    Logger.error("Error reading '#{state.url}': #{reason}")
    []
end

will give a dialyzer error stating: The pattern {'ok', Vts@1} can never match the type {'error',_}

Expected behavior

To have no dialyzer errors.

josevalim commented 6 years ago

Can you please provide a sample application that reproduces the error after a simple command, such as mix dialyzer? Or send a patch that adds a failing test case to our suite?

Thanks!

josevalim commented 6 years ago

Here is an issue that may be related: #6738

Btw, is there any chance that one of the two with conditions will never match, meaning it will indeed always returns {:error, _}?

GoNZooo commented 6 years ago

I will create an app to make this easily reproducable. If one of the errors wouldn't hit (according to spec it should be fine and it works in practice with malformed XML, so I know it can hit the :fatal_error one) I would still expect dialyzer to tell me {:ok, [Downloader.Torrent.t()]} is a possible return value, but it simply doesn't think so.

GoNZooo commented 6 years ago

I've made a repo for this: https://github.com/GoNZooo/with_only_one_path

There's some additional info and repro steps in the README.

josevalim commented 6 years ago

Thanks @GoNZooo. I was able to reproduce the issue although I cannot reproduce it if I replace the call to FeederEx by something else with a similar return type.

This Erlang file can also reproduce the error and there is nothing suspicious about it:

-module(sample).
-export([read/0,use_read/0]).

read() ->
    case file:read_file(<<"example.com">>) of
        {ok,_} ->
            case 'Elixir.FeederEx':parse(<<"incorrect xml here">>) of
                {ok,#{'__struct__' := 'Elixir.FeederEx.Feed'}} ->
                    ok;
                _@1 ->
                    case _@1 of
                        {fatal_error,_,_,_,_} ->
                            feeder_error;
                        _@2 ->
                            error({with_clause,_@2})
                    end
            end;
        _@1 ->
            case _@1 of
                {error, _} ->
                    httpoison_error
            end
    end.

use_read() ->
    case read() of
        ok ->
            successful_read;
        httpoison_error ->
            httpoison_error;
        feeder_error ->
            feeder_error
    end.

So it is not an Elixir bug. It may be either a dialyzer bug or a bug in the feeder dependency.

Therefore I would like to ask you to reproduce the issue without the FeederEx dependency. The feeder library defines a very poor type for its stream/2 function and it is unclear if that is affecting the results. Maybe you can try calling xmerl_sax_parser directly. If we have a minimal case, we can report it to upstream to the OTP team.

GoNZooo commented 6 years ago

Ok, so after testing a few versions of this it does seem pretty conclusively that feeder (by way of FeederEx) was causing it. Both xmerl_sax_parser and Scrape did fine.

Thanks for taking a serious look at it! If there's a conclusion I guess it's that underspecs can cause dialyzer to act pretty strangely?

josevalim commented 6 years ago

Thanks for taking a serious look at it! If there's a conclusion I guess it's that underspecs can cause dialyzer to act pretty strangely?

I am not proficient enough in Dialyzer to say if that's the conclusion or if it is a bug. Maybe @fishcakez is though. :)

In any case, closing this as it is decidedly not an Elixir bug.

fishcakez commented 6 years ago

I don't think this is related to underspecs. I looked at the repo and this issue and FeederEx.parse(body) is supposed to be returning a 2-tuple in one and a 3-tuple in another but it may not matter here.

My assumption is that dialyzer believes that FeederEx.parse/1 is not going to return {:ok, %FeederEx.Feed{}, _}, {:ok, %FeederEx.Feed{}} or {:fatal_error, _, _, _, _}. Therefore the only way this function does not raise is if HTTPoison.get/1 returns an {:error, _} tuple.

percygrunwald commented 6 years ago

I was considering opening a new issue, but I found this issue and feel like it describes my case as well.

I think the error in this case is related to the use of raise in the with/else branches.

Code

  def hello do
    with {:value1, :ok} <- {:value1, get_value()},
         {:value2, :ok} <- {:value2, get_value()} do
      :ok
    else
      {:value1, :error} ->
        raise "value1 error"

      {:value2, :error} ->
        raise "value2 error"
    end
  end

  defp get_value do
    if :rand.uniform() >= 0.5, do: :ok, else: :error
  end

Current behavior

mix dialyzer outputs the following:

➜  dialyzer_path mix dialyzer
Compiling 1 file (.ex)
Generated dialyzer_path app
Finding suitable PLTs
Checking PLT...
[:compiler, :dialyxir, :elixir, :kernel, :logger, :stdlib]
PLT is up to date!
Starting Dialyzer
[
  check_plt: false,
  init_plt: '/Users/percy/Code/dialyzer_path/_build/dev/dialyxir_erlang-20.3.4_elixir-1.7.2_deps-dev.plt',
  files_rec: ['/Users/percy/Code/dialyzer_path/_build/dev/lib/dialyzer_path/ebin'],
  warnings: [:unknown]
]
Total errors: 1, Skipped: 0
done in 0m1.21s
lib/dialyzer_path.ex:16:pattern_match
The pattern
{:value1, :error}

can never match the type
{:value2, :error}
________________________________________________________________________________
done (warnings were emitted)

Expected behavior

No errors from mix dialyzer.

Steps to reproduce

Fresh mix project
Add dialyxir as per https://github.com/jeremyjh/dialyxir/wiki/Phoenix-Dialyxir-Quickstart
Paste in code as above
mix dialyzer

Additional data

Changing the raise lines to tuples removes the errors:

  def hello do
    with {:value1, :ok} <- {:value1, get_value()},
         {:value2, :ok} <- {:value2, get_value()} do
      :ok
    else
      {:value1, :error} ->
        {:error, :value1}

      {:value2, :error} ->
        {:error, :value2}
    end
  end

  defp get_value do
    if :rand.uniform() >= 0.5, do: :ok, else: :error
  end

➜  dialyzer_path mix dialyzer
Compiling 1 file (.ex)
Finding suitable PLTs
Checking PLT...
[:compiler, :dialyxir, :elixir, :kernel, :logger, :stdlib]
PLT is up to date!
Starting Dialyzer
[
  check_plt: false,
  init_plt: '/Users/percy/Code/dialyzer_path/_build/dev/dialyxir_erlang-20.3.4_elixir-1.7.2_deps-dev.plt',
  files_rec: ['/Users/percy/Code/dialyzer_path/_build/dev/lib/dialyzer_path/ebin'],
  warnings: [:unknown]
]
Total errors: 0, Skipped: 0
done in 0m1.15s
done (passed successfully)

josevalim commented 6 years ago

There is also #6738. This is a known bug due to how we compile with and it will be fixed in future releases once we contribute some patches to erlang to perform function inlining.

andrelip commented 3 years ago

Any news on that?

@spec get_person_by_reference_id(String.t()) :: Person.t() | nil
def get_person_by_reference_id(reference_id) do
  Repo.get_by(Person, %{reference_id: reference_id})
end

with {:get_person, %Person{} = person} <-  {:get_person, get_person_by_reference_id(person_reference_id)},
  ...
else 
  {:get_person, nil} -> {:error, :not_found}
  ...
end

It gives me that error:

The pattern can never match the type.

Pattern:
{:get_person, nil}

Divij-jain commented 1 year ago

still a bug

elixir-lang / elixir