Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dialyzer can't detect tuple when middleware Tuples is used #109

Closed
ahmadferdous opened this issue Sep 20, 2017 · 7 comments
Closed

Dialyzer can't detect tuple when middleware Tuples is used #109

ahmadferdous opened this issue Sep 20, 2017 · 7 comments

Comments

@ahmadferdous
Copy link

Hi,

I have a module that uses Tesla and its Tuples middleware.

  use Tesla
  plug Tesla.Middleware.Tuples

In that module, Tesla functions are used in the following way:

    login_url
    |> post(%{username: username, password: password, format: "json"})
    |> extract_body

Spec of extract_body expects a tuple with the first element :ok or :error. However, when I run mix dialyzer, it gives the following error:

The pattern {'ok', _response@1} can never match the type 
#{
    '__client__':=fun(), 
    '__module__':=atom(), 
    '__struct__':='Elixir.Tesla.Env', 
    'body':=_, 
    'headers':=#{binary()=>binary()}, 
    'method':='delete' | 'get' | 'head' | 'options' | 'patch' | 'post' | 'put' | 'trace', 
    'opts':=[any()], 
    'query':=[{_,_}], 
    'status':=integer(), 
    'url':=binary()
}

In order for dialyzer to detect that tuples will be returned, do I need to do something additional in the spec or code? What am I missing here?

@teamon
Copy link
Member

teamon commented Sep 20, 2017

The return type spec for get/post/... is Tesla.Env.t. When using Tuples middleware this is changed to {:ok, Tesla.Env.t} | {:error, any} but this is not reflected in the typespec, hence the dialyzer error.

I can think of some way to check for Tuples middleware during compilation and adjust the generated typespecs but that would require figuring out a way to make this a general feature (i.e. one could make a custom middleware with custom return values that would need to reflected in the generated typespec).

@amatalai
Copy link
Collaborator

amatalai commented Sep 20, 2017

I have workaround : use Tesla, docs: false

I had no knowledge of dialyzer when I wrote :only/:except/:docs options... docs: false unintentionally disables typespecs as well, so you can write your own inside your module.

@ahmadferdous
Copy link
Author

ahmadferdous commented Sep 20, 2017

use Tesla, docs: false worked like a charm! Thank you!

I would like to keep this issue open for a few days, hoping to get more feedback on this. @teamon , is it okay?

@amatalai
Copy link
Collaborator

amatalai commented Sep 20, 2017

Definitely, it should stay open. I should have opened this myself ~ two weeks ago 😞

@teamon
Copy link
Member

teamon commented Sep 24, 2017

Alright, it can be done.

Example usage

defmodule Sandbox do
  defmodule Client do
    use Tesla
  end

  defmodule TuplesClient do
    use Tesla

    plug Tesla.Middleware.Tuples
  end

  def main do
    case Client.get("/") do
      %{status: _status} -> :ok
    end
  end

  def main_tuple do
    case TuplesClient.get("/") do
      {:ok, _env} -> :ok
      {:error, _reason} -> :error
    end
  end
end

on curent master

⌘ ~/code/tesla/sandbox λ mix dialyzer
...
lib/sandbox.ex:18: Function main_tuple/0 has no local return
lib/sandbox.ex:20: The pattern {'ok', _env@1} can never match the type #{'__client__':=fun(), '__module__':=atom(), '__struct__':='Elixir.Tesla.Env', 'body':=_, 'headers':=#{binary()=>binary()}, 'method':='delete' | 'get' | 'head' | 'options' | 'patch' | 'post' | 'put' | 'trace', 'opts':=[any()], 'query':=[{atom() | binary(),binary() | [{atom() | binary(),binary() | [{_,_}]}]}], 'status':=integer(), 'url':=binary()}
lib/sandbox.ex:21: The pattern {'error', _reason@1} can never match the type #{'__client__':=fun(), '__module__':=atom(), '__struct__':='Elixir.Tesla.Env', 'body':=_, 'headers':=#{binary()=>binary()}, 'method':='delete' | 'get' | 'head' | 'options' | 'patch' | 'post' | 'put' | 'trace', 'opts':=[any()], 'query':=[{atom() | binary(),binary() | [{atom() | binary(),binary() | [{_,_}]}]}], 'status':=integer(), 'url':=binary()}
done (warnings were emitted)

on branch dependant-types

⌘ ~/code/tesla/sandbox λ mix dialyzer
...
done (passed successfully)

Required change to middleware

https://github.com/teamon/tesla/blob/8e8fd81e212530bb7cb73fc851faac9b54bc1621/lib/tesla/middleware/tuples.ex#L30-L34

Explanation

I must admit this is quite a hack 💃
First, I've tried with @type return :: ... in Tesla.Middleware.Tuples but I can't seem to find an easy way to get to that information after the middleware module is compiled. AFAIK, dialyzer does some magic reading from beam files to support referencing types from other modules.
Then, I've tried with macros, but I've stumbled upon unexplainable errors from elixir compiler while compiling Tesla.Builder module.
Fortunately, since quote code is just data structure if can be used not only with macros, but also in regular functions - hence the def return_type with quote inside in Tesla.Middleware.Tuples.
From this point the rest is fairly straightforward. First, I've replaced :: Tesla.Env.t with type alias :: return_type in Tesla.Builder. Then, in __before_compile__ I check the last middleware in stack for return_type/0 function. Note that I must've used Code.ensure_loaded/1 to make function_exported?/3 works correctly. Then, if there is such function I simply call it, get the quoted type spec and inject it as @type return_type :: [here]. In case there is no middleware, or the last one does not export custom return type there is a fallback to Tesla.Env.t

Here is the code for __before_compile:

https://github.com/teamon/tesla/blob/8e8fd81e212530bb7cb73fc851faac9b54bc1621/lib/tesla.ex#L345-L370

And here is the full diff of changes:

8e8fd81

So... what do you think? :)

@teamon
Copy link
Member

teamon commented Oct 7, 2017

@ahmadferdous have you got a change to try this out?

@teamon
Copy link
Member

teamon commented Nov 28, 2017

@ahmadferdous I'm going to close this one for now. I think the additional complexity is bigger than gains in this case. Feel free to reopen/comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants