Odd match warnings when compiling

Questions / Help
,
1

I am getting odd warnings about pattern matching when compiling the following code (it is from a Phoenix controller, but as the warning doesn’t have anything to do with Phoenix, I posted this in the generic questions section):

  # Edit case for editing user's password
  def do_edit(conn, %{
        "user" =>
          %{
            "type" => "password",
            "old_password" => old_password,
            "password" => _
          } = params
      }) do
    user = AuthUtils.get_current_user(conn)
    password_changeset = User.password_changeset(user, params)

    with {:old_pass, true} <- {:old_pass, AuthUtils.check_user_password(user, old_password)},
         {:updated, %User{}} <- {:updated, AuthUtils.update_user(password_changeset)} do
      conn
      |> put_flash(:success, "Password changed.")
      |> redirect(to: Routes.preferences_path(conn, :edit))
    else
      err ->
        error_changeset =
          case err do
            {:old_pass, false} ->
              # We need to add an action to the changeset so that Phoenix will display the error,
              # otherwise it will think the changeset was not processed (as it has not been passed
              # to any Repo call) and will not show the errors
              %{password_changeset | action: :update}
              |> Ecto.Changeset.add_error(:old_password, "does not match your current password")

            {:updated, cset} ->
              cset
          end

        conn
        |> common_edit_assigns()
        |> put_flash(:error, "Error changing password.")
        |> render("preferences.html", pass_changeset: error_changeset)
    end
  end

The case statement inside the with’s else block gets the following warnings:

warning: this clause cannot match because of different types/sizes
  lib/code_stats_web/controllers/preferences_controller.ex:67

warning: this clause cannot match because of different types/sizes
  lib/code_stats_web/controllers/preferences_controller.ex:74

The lines refer to the {:old_pass, false} -> and {:updated, cset} ->. But I know that they do match and the code works. The function AuthUtils.check_user_password/2 returns a boolean, and AuthUtils.update_user/1 returns either a User struct or a changeset.

If I hover over the clauses in VSCode, it tells me that the first clause can never match {:updated, <changeset>} and the second clause can never match {:old_pass, false}, but that makes no sense to me (as the opposite clauses match those!).

Now to be clear, I’m not looking for advice how to structure the code better. I am simply wondering why the warnings are given. I cannot see the problem in the match.

2

That’s one of those with bugs, I get a LOT of those warnings, so many that I’ve ended up stripping out almost all of with from my project and going back to other libraries and patterns because of that as well as two other annoying issues I have with it. There is a still-open github issue about it at:

2 Likes
3

Is there any extra benefits to use with syntax? IMHO it’s just hard to read compared with regular syntax.

1 Like
4

In this case it is not so easy to read because I haven’t refactored it into a nicer format yet. But sometimes you need to do many things in succession and handle all of their failures. Then with helps you to avoid a pyramid of nested conditionals. I know my code is not the best example but here is one with that has more things going on:

    with {:ok, %DateTime{} = datetime} <- parse_timestamp(timestamp),
         {:ok, datetime} <- check_datetime_diff(datetime),
         {:ok, offset} <- get_offset(timestamp),
         {:ok, %Pulse{} = pulse} <- create_pulse(user, machine, datetime, offset),
         {:ok, inserted_xps} <- create_xps(pulse, xps) do
      # Broadcast XP data to possible viewers on profile page and frontpage
      ProfileChannel.send_pulse(user, %{pulse | xps: inserted_xps})

      # Coordinates are not sent for private profiles
      coords = if user.private_profile, do: nil, else: GeoIPPlug.get_coords(conn)
      FrontpageChannel.send_pulse(coords, %{pulse | xps: inserted_xps})

      conn |> put_status(201) |> json(%{ok: "Great success!"})
    else
      {:error, :not_found, reason} ->
        conn |> put_status(404) |> json(%{error: reason})

      {:error, :generic, reason} ->
        conn |> put_status(400) |> json(%{error: reason})

      {:error, :internal, reason} ->
        conn |> put_status(500) |> json(%{error: reason})
    end

Here you can see the happy path is easily readable and the error handling is after it, and there is no nesting.

2 Likes
5

Just as an example, the alternative to that with in pure elixir itself (I still prefer a few specific libraries like ok and exceptional):

    def blah(timestamp) do
      {:ok, %DateTime{} = datetime} = parse_timestamp(timestamp)
      {:ok, datetime} = check_datetime_diff(datetime)
      {:ok, offset} = get_offset(timestamp)
      {:ok, %Pulse{} = pulse} = create_pulse(user, machine, datetime, offset)
      {:ok, inserted_xps} = create_xps(pulse, xps)

      # Broadcast XP data to possible viewers on profile page and frontpage
      ProfileChannel.send_pulse(user, %{pulse | xps: inserted_xps})

      # Coordinates are not sent for private profiles
      coords = if user.private_profile, do: nil, else: GeoIPPlug.get_coords(conn)
      FrontpageChannel.send_pulse(coords, %{pulse | xps: inserted_xps})

      conn |> put_status(201) |> json(%{ok: "Great success!"})
    rescue
      m in MatchError ->
        case m.term do
          {:error, :not_found, reason} ->
            conn |> put_status(404) |> json(%{error: reason})

          {:error, :generic, reason} ->
            conn |> put_status(400) |> json(%{error: reason})

          {:error, :internal, reason} ->
            conn |> put_status(500) |> json(%{error: reason})

          _ ->
            reraise m
        end
    end

Again, the happy path is easily readable and the error handling is after it with no nesting. In addition this will cause no odd match warnings when compiling.

This is one example of why I find the current with implementation improperly designed. It should have been something like:

with do
  {:ok, %DateTime{} = datetime} = parse_timestamp(timestamp)
  {:ok, datetime} = check_datetime_diff(datetime)
  {:ok, offset} = get_offset(timestamp)
  {:ok, %Pulse{} = pulse} = create_pulse(user, machine, datetime, offset)
  {:ok, inserted_xps} = create_xps(pulse, xps)

  # Broadcast XP data to possible viewers on profile page and frontpage
  ProfileChannel.send_pulse(user, %{pulse | xps: inserted_xps})

  # Coordinates are not sent for private profiles
  coords = if user.private_profile, do: nil, else: GeoIPPlug.get_coords(conn)
  FrontpageChannel.send_pulse(coords, %{pulse | xps: inserted_xps})

  conn |> put_status(201) |> json(%{ok: "Great success!"})
else
  {:error, :not_found, reason} ->
    conn |> put_status(404) |> json(%{error: reason})

  {:error, :generic, reason} ->
    conn |> put_status(400) |> json(%{error: reason})

  {:error, :internal, reason} ->
    conn |> put_status(500) |> json(%{error: reason})
end

Which would allow you to even add in rescue/catch/after clauses as well, plus it doesn’t have those weird , droppings at the end of the expressions and it follows the rest of the Elixir language’s form (excepting the also weird for in the same way). The with syntax was put in with relatively little discussion and resolution of issues so it really stands out as an oddity… >.>