Dialyzer error when using Mint.HTTP2.get_window_size/2

elixir-mint / mint

Functional HTTP client for Elixir with support for HTTP/1 and HTTP/2 🌱

Apache License 2.0

1.36k stars 106 forks source link

Dialyzer error when using Mint.HTTP2.get_window_size/2 #380

Closed mruoss closed 1 year ago

mruoss commented 1 year ago

Not sure I'm using this correctly but if I define the following function, I make dialyzer angry:

  @spec get_window_size(Mint.HTTP.t()) :: non_neg_integer() | :error
  def get_window_size(conn) do
    case Mint.HTTP.protocol(conn) do
      :http1 -> :error
      :http2 -> Mint.HTTP2.get_window_size(conn, :connection)
    end
  end

The error returned by Dialyzer:

Function call without opaqueness type mismatch.
Call does not have expected opaque term of type Mint.HTTP2.t() in the 1st position.
Mint.HTTP2.get_window_size(_conn :: Mint.HTTP.t(), :connection)

Is there a better way of doing this?

whatyouhide commented 1 year ago

Yuck, Dialyzer :shakes_fist:

@ericmj ideas?

mruoss commented 1 year ago

Yuck, Dialyzer :shakes_fist:

Indeed. I guess #263 reported the same problem but then it was supposedly solved by Mint.HTTP.protocol/1 which is why I thought I might be using it wrong. But I can't wrap my head around it.

mruoss commented 1 year ago

I have played around with this a bit. I see two ways to make Dialyzer happy. They're not very elegant, though.

Implement two functions (one per protocol) which take a conn and return it but the typespecs "harden" its type. For HTTP2 it would look like this (with a better function name?):

# In Mint.HTTP2

@spec specify_type(Mint.HTTP.t()) :: t() # Mint.HTTP2.t()
def specify_type(conn), do: conn

The code snipped from above would then look like this:

@spec get_window_size(Mint.HTTP.t()) :: non_neg_integer() | :error
def get_window_size(conn) do
  case Mint.HTTP.protocol(conn) do
    :http1 -> :error
    :http2 -> conn |> Mint.HTTP2.specify_type() |> Mint.HTTP2.get_window_size(:connection)
  end
end

If abstracting away the protocol is the goal, Mint could provide its full API over Mint.HTTP, i.e. implement Mint.HTTP.get_window_size/2 which returns an error or something for a HTTP1 conn...

whatyouhide commented 1 year ago

I am pretty strongly against changing APIs to please Dialyzer, because here this is Dialyzer being unhappy over functional correct code. Both the solutions you proposed would work, but make the API semantically worse IMO, because they are not needed.

In this case, I'd suggest ignoring the Dialyzer warning 🤷

mruoss commented 1 year ago

Yeah I do agree. Also, I just figured out that the first approach (obviously) is also a valid workaround. I can implement that stupidity myself in my lib if I want to make Dialyzer shut up. Closing this.

mruoss commented 1 year ago

Oops, too soon. I actually can't implement it. Dialyzer no likes. So ignoring it is.

mruoss commented 1 year ago

Okay. I can. As long as the function is public. 🤷 I'm completely lost now.

whatyouhide commented 1 year ago

:shakes_fist: indeed 🙃

ericmj commented 1 year ago

I am surprised dialyzer is warning in this case since it cannot prove that the code wont succeed. Are you using the default dialyzer settings?

Today Mint.HTTP only exposes the common functionality between HTTP/1 and HTTP/2, but we could also expose the HTTP/2 functions and have them error when given a HTTP/1 connection.

mruoss commented 1 year ago

Are you using the default dialyzer settings?

Yeah, fresh project with default settings.

mruoss commented 1 year ago

I've added this code now to silence Dialyzer. Note the Mint.HTTP.t() | Mint.HTTP2.t() in the specs...

  @spec get_window_size(
          Mint.HTTP.t() | Mint.HTTP2.t(),
          :connection | {:request, Mint.Types.request_ref()}
        ) :: non_neg_integer
  defp get_window_size(conn, connection_or_request) do
    Mint.HTTP2.get_window_size(conn, connection_or_request)
  end