aaronrenner / phx_gen_auth

An authentication system generator for Phoenix 1.5 applications.
772 stars 55 forks source link

dialyzer show some warnings in user_settings_controller #121

Closed alaadahmed closed 3 years ago

alaadahmed commented 3 years ago

I think it is trivial, but since upgrading to erlang/otp 24.0, I see this warning from Dialyzer on the user_settings_controller.ex the warning is: The call _.'email'/() requires that _@3 is of type atom(), not {map(), map()} this is inside the update function

def update(conn, %{"action" => "update_email"} = params) do
    %{"current_password" => password, "user" => user_params} = params
    user = conn.assigns.current_user

    case Accounts.apply_user_email(user, password, user_params) do
      {:ok, applied_user} ->
        Accounts.deliver_update_email_instructions(
          applied_user,
          user.email, #<<<<<<<<<<<<<<<<<<<<<<< HERE 
          &Routes.user_settings_url(conn, :confirm_email, &1)
        )

        conn
        |> put_flash(
          :info,
          "A link to confirm your email change has been sent to the new address."
        )
        |> redirect(to: Routes.user_settings_path(conn, :edit))

      {:error, changeset} ->
        render(conn, "edit.html", email_changeset: changeset)
    end
  end
zoedsoupe commented 3 years ago

I'm also having this warning

elixir: v.1.12.1 erlang/OTP: 24.0.0

Baradoy commented 3 years ago

I was seeing this with:

elixir 1.12.1-otp-24
erlang 24.0.2

I fixed it by changing user = conn.assigns.current_user -> %User{} = user = conn.assigns.current_user. I do not understand why that worked though.

aj-foster commented 3 years ago

Not 100% sure on any of this, but here's a theory:

Prior to the line with the warning (user.email), the user value passes through Accounts.apply_user_email/3 where it gets passed to Ecto.Changeset.cast/4. The type specification on cast/4 has the first argument as:

Ecto.Schema.t() | t() | {data(), types()},

which, if you expand the types, is:

schema() :: %{
  optional(atom()) => any(),
  __struct__: atom(),
  __meta__: Ecto.Schema.Metadata.t()
}
| embedded_schema() :: %{optional(atom()) => any(), __struct__: atom()}
| t() :: Ecto.Changeset.t(Ecto.Schema.t()) :: %Ecto.Changeset{..}
| {data() :: map(), types() :: map()}

In short, Dialyzer should see a success typing on user of either {map(), map()} (like we're seeing in the error) or a struct with no specifications on keys. We can reduce this circumstance into a quick test case:

defmodule Test do
  def one(user) do
    two(user)
    user.email
  end

  def two({_b, _c}), do: nil
  def two(%{}), do: nil
end

This gives a similar error:

The call _.'email'/() requires that _@1 is of type atom(), not {_, _}.

This tells me that Dialyzer is unwilling to make the intuitive leap that user might be a map(). Even if we go so far as to specify that the argument to function two above might be a map with a required key :email, it does not respond as we might hope.

Presumably, since map() is not part of the success typing of the Accounts.apply_user_email/3 call, Dialyzer doesn't consider map access as a potential for what might be happening with user.email. The error message reflects its assumption that the . is function application, and user should be an atom (module name).

Adding the %User{} = ... or indeed even %{} = ... forces Dialyzer back to the correct path. An equivalent method would be to specify user in the call to Accounts.apply_user_email/3:

@spec apply_user_email(User.t(), String.t(), map) :: {:ok, User.t()} | {:error, Ecto.Changeset.t(User.t())}

Presuming you've defined a type User.t() in your schema module.

v0idpwn commented 3 years ago

I had very similar issues and they got solved updating to 1.12.2/OTP 24.0.5. Not sure about the details, I didn't try a lot

aaronrenner commented 3 years ago

@alaadahmed Would you mind opening this issue under the phoenix repo? Now that phx.gen.auth has been released with phoenix 1.6, this repo is no longer maintained and is being archived.