Ecto: Dialyzer warning on Repo.transaction with Ecto.Multi

[danielberkompas]

Environment

• Elixir version (elixir -v): 1.4.0-rc.1 (16a14c1)
• Database and version (PostgreSQL 9.4, MongoDB 3.2, etc.): (PostgreSQL) 9.6.1
• Ecto version (mix deps): 2.1.1 (fa8bdb14)
• Database adapter and version (mix deps): postgrex 0.13.0 (e101ab47)
• Operating system: macOS Sierra (10.12.2)

Current behavior

This code produces a dialyzer warning:

defmodule MyApp.Test do
  alias MyApp.Repo

  def test do
    Repo.transaction(Ecto.Multi.new)
  end
end

Warning:

The call 'Elixir.MyApp.Repo':transaction('Elixir.Ecto.Multi':t()) contains an opaque term as 1st argument when a structured term of type fun(() -> any()) | #{'__struct__':='Elixir.Ecto.Multi', 'names':=_, 'operations':=_, _=>_} is expected

Expected behavior

This code should not raise a dialyzer warning.

[josevalim]

@michalmuskala we should probably not make it opaque?

[michalmuskala]

I like having it opaque since it actually is opaque for public uses - we give no guarantees about the internal structure.

I'll play a bit with it to see if we can get away with it somehow.

[fishcakez]

We can fix this by not pattern matching on %Ecto.Multi{} outside of Ecto.Multi and adding specs to __apply__ and any other public functions in Ecto.Multi that currently do not have specs. If we must pattern match on %Ecto.Multi{} outside Ecto.Multi we can add @dialyzer {:no_opaque, fun: arity} to let dialyzer we know what we are doing. However we should still add specs for all Ecto.Multi functions using the opaque term.

[michalmuskala]

@danielberkompas Please, try with current master.

[danielberkompas]

@michalmuskala I retested and it look like it's fixed!

[mrkaspa]

@michalmuskala I have the same problem with ecto 2.1.4, is there a new release for this?

[umamaistempo]

@mrkaspa the fix for this issue was commit 12f27d9ec25d035fc2f71c25052c2cae6c5eb781 changing Ecto.Multi.__apply__ spec and Ecto.Repo.Queryable.transaction function clause. This change was not shipped on Ecto 2.1.4 but is available on the master branch.

You can fix this right now by updating your mix.exs to point to an specific commit whose tree includes the aforementioned commit.

As an example, for the lastest commit on master (945e154), you'd put on your mix.exs: {:ecto, github: "elixir-ecto/ecto", ref: "945e154", override: true}.

This should fix this problem until v2.2 if passing dialyzer is necessary on your PR workflow (like it is on mine, Jenkins won't allow me to merge on dialyzer warnings :grin:).
Good luck! :)

[josevalim]

Thanks for the info @mememori. I have just backported the commit to the v2.1 branch. So if we do a new release, it will be included.

[mariusbutuc]

Thanks for backporting the commit to the v2.1 branch @josevalim!

Ecto v2.1.4 was released back in March, so this change came after. Any ETA on a v2.1.5?

[waiting-for-dev]

With ecto v2.2.1, I have to add @dialyzer {:no_opaque, fun: arity} to the function matching on the result of Repo.transaction(multi). Also, I can't properly spec functions which take or return a Multi.t (I get "no function return" and "never will be called" errors). For now, I'm using multi :: any instead.