Should `Base.decodeXX` functions return more than just `:error`?

victorolinasc · September 5, 2018, 3:08pm

I was about to create an issue on the Elixir repo about the current behaviour of Base.decodeXX functions in core. I’ll first post it here and if people agree I’ll open the issue and send a PR. Just wanted to avoid polluting the issue tracker and getting some feedback beforehand.

Issue description

Base.decodeXX functions return a very unuseful :error only atom. This is fine for all the cases that you trust the encoding. Sometimes, though, we need to define some external “protocol” that will send/receive encoded binaries. In these cases, it is common to have something like:

def some_input_handling_fun(binary) do
  with {:ok, decoded} <- Base.decode64(binary),
         {:ok, result} <- do_something_with_decoded(decoded) do
      {:ok, result}
  end
end

The issue is that the return of decodeXX funs, if it fails, is simply :error which makes debugging this a bit tricky. If anything else returns :error (which it shouldn’t but you know how mankind works, right?) then we will need more context to understand what went wrong.

If this is ok, I can send a PR changing these specs and implementation here, here and here.

This is public API and so, I am not sure if this can be done as it would be backwards incompatible. I think this is a bug since many times it was mentioned that bad error “practices” in the core language should be treated as such. If this is not the case, then I’ll wrap the function locally anyway

Environment

Elixir & Erlang/OTP versions (elixir --version):

elixir --version
Erlang/OTP 21 [erts-10.0.6] [source] [64-bit] [smp:8:8] [ds:8:8:10] [async-threads:1] [hipe]
Elixir 1.7.3 (compiled with Erlang/OTP 21)

Operating system:

uname -a

Linux 4.17.19-200.fc28.x86_64 #1 SMP Fri Aug 24 15:47:41 UTC 2018 x86_64 x86_64 x86_64 GNU/Linux

Current behavior

Not following error tuples for decoding operations.

Expected behavior

Return {:error, :bad_encoding} instead of just :error

LostKobrakai · September 5, 2018, 3:29pm

To me this feels like you try to change something just because it’s inconvenient with with. That’s imho not a reason to change anything. You’ll have the same issues with not just the Base module’s functions, but with many other functions.

I feel like changing the return types of the Base module might be justified if the additional second error atom would provide more details about the error, but in this case there’s only one error case: Can’t be decoded.

idi527 · September 5, 2018, 3:56pm

You can “isolate” Base.decodeXX errors by using case instead of with:

case Base.decode64(binary) do
  {:ok, decoded} ->
    case do_something_with_decoded(decoded) do
      {:ok, result} = success -> success
      failure -> failure
    end

  :error ->
    {:error, :bad_encoding}
end

As for your suggestion, what tuples other than {:error, :bad_encoding} do you have in mind for failed base.decodeXX?

victorolinasc · September 5, 2018, 4:16pm

I understand that the function is not “wrong” in its current form. That is not my intention here. Nor is it to make it more “convenient” with with. My intention is to help our code bases from the point of view of introspection.

Even if there is currently only one possible return, consider these 2 forms:

: error
{:error, :bad_encoding}

Tell me which gives you more information about where to search for the cause of this? For another example, think of an action_fallback in Phoenix with this def:

  def call(conn, :error) do
    conn
    |> put_status(:bad_request)
    |> json(%{"error" => "Some :error"})
  end

How many “things” can fall into that clause?

Without seeing the stacktrace, you would need to “know” that decoding functions in the Base module might return just :error. The difference is that when you see {:error, :bad_encoding} you’d probably grep your source code for decoding functions. IMHO the second option is better.

Also, if we “know” before hand that the core elixir libraries “always” return tagged tuples, then it would be least surprising where to look for that.

alexcastano · September 5, 2018, 5:30pm

I think if you return a tuple is because you’re returning more information about the error. However, {:error, :bad_encoding} is not giving you more information than {:error}. If I would need this for a with, I would create a simple wrapper:

defmodule MyBase do
  def decode64(bin) do
    case Base.decode(bin) do
      :error -> {:error, :bad_encoding}
      any -> any
    end
  end
end

victorolinasc · September 5, 2018, 5:54pm

The more information, in this case, is that this is an error from a decoding function. We could go even further and provide {:error, :bad_base64_encoding} and so on.

Of course you can wrap the function and that is valid even for functions that DO return error tuples, but my point here is about how you instrospect errors. If you “know” beforehand this return is possible from decode functions, then sure, you can wrap it. But what if you don’t know this and has this log on your production system “Some error”?

So: :error is not as obvious as {:error, :bad_encoding} to my understanding at least. What I was hoping to achieve here is to provide just what you people are saying: being obvious about what happened in an error other than just saying “an error occured”.

In any case, it seems like I am alone in this quest hehehe that’s fine too. I thought this would turn out the other way around but it is not like there is a wrong or right here.

thomasbrus · September 5, 2018, 10:05pm

Just to add to this, I can see where you’re coming from and I also remember a couple of situations in which I wished some function returned something more than just :error.

But I think in this case the return value is correct since the only type of error is “can’t be decoded”. {:error, :bad_encoding} would then be needlessly specific just as Map.fetch/2 #=> {:error, :key_not_found} would also be needlessly specific.

However it seems that Elixir is not entirely consistent in this either For example Date.new/4 returns {:ok, date} or {:error, :invalid_date} (and that’s the only type of error):

github.com

elixir-lang/elixir/blob/master/lib/elixir/lib/calendar/date.ex#L219-L225


def new(year, month, day, calendar \\ Calendar.ISO) do
  if calendar.valid_date?(year, month, day) do
    {:ok, %Date{year: year, month: month, day: day, calendar: calendar}}
  else
    {:error, :invalid_date}
  end
end

Ultimately I don’t think it’s the job of the function that being called to provide a return value that still has meaning 3-4 levels deep. But rather I would say it is the responsibility of the thing that calls that function to decide whether it is okay pass along the return value (:error), or not.

So your first example I would have written like this:

def some_input_handling_fun(binary) do
  with {:ok, decoded} <- Base.decode64(binary),
       {:ok, result} <- do_something_with_decoded(decoded) do
    {:ok, result}
  else
    # Be as specific as you want here
    :error -> {:error, :bad_encoding}
    # Assuming do_something_with_decoded already returns a tagged tuple      
    {:error, reason} -> {:error, reason}
  end
end

victorolinasc · September 6, 2018, 1:11am

In your example there can be hidden things happening

If, for instance, the function do_something_with_decoded/1 also returns only :error because “there is only one error condition possible” then you might be surprised by the output. That is because it would also match on the else but you translated that to {:error, :bad_encoding} when the “only possible error” could have been something entirely different.

I know this is not broke “per se” but I still think that being obvious is better than assuming something. Being explicit here means we communicate better (and that is the reason why we’ve wrapped the error to something else on the with in the first place).

Ultimately I don’t think it’s the job of the function that being called to provide a return value that still has meaning 3-4 levels deep.

Totally agree with that! But you don’t even need nesting for this use case to happen. Think this:

def some_fun(arg) do
  with {:ok, result1} <- function1(arg),
       {:ok, result2} <- funciton2(result1) do
     IO.puts "happy path"
  else
    :error -> {:error, :which_function_returned_this?}
    _ -> raise "boom!"
  end
end

It is a subtle thing but it seems I am the only one bothered with it hehehe

Thanks for your research on the Elixir library! I am even more convinced this would be good

mgwidmann · September 6, 2018, 2:27am

I think that because we have the freedom to return whatever we are allowing that to cloud our judgement a bit. In a statically typed language there wouldn’t even be an argument about this because the same type would be returned everywhere. There can most certainly be more information returned even though the actual issue is always the same. Consider this:

case Base.decode64(text) do
  {:ok, data} -> {:ok, data}
  {:error, {:bad_encoding, byte_offset}} -> {:error, "Encountered error decoding after the #{byte_offset}th byte\n#{text}"}
end

Both what went wrong and specifics about the error are accessible in native elixir terms and we can then choose to create a more friendly or debuggable error (or even take different action depending on the error metadata if we so choose).

Supposed we had a type to represent this as part of the standard library:

defmodule Result do
  @type success :: {:ok, any()}
  @type error :: {:error, {atom(), any()}}
  @type t :: success | error
end

This is so common in Erlang and Elixir. Why is there no type for it?

Now would the question really be what should we return? In order to be consistent it would become mandatory to return a Result.t() everywhere, even where not necessary, not only for consistency sake but for backwards compatibility. Now if we ever need to add another error condition, or some metadata about what went wrong, its easy to do so.

thomasbrus · September 23, 2018, 8:17pm

@victorolinasc

Regarding your last code example, was just reading this blog post (Better Control Flow Using The “with” Macro, see last example). And I thought it was a neat trick to differentiate between the return values / errors Still a bit lengthy, I know.

def some_fun(arg) do
  with {:function1, {:ok, result1}} <- {:function1, function1(arg)},
       {:function2, {:ok, result2}} <- {:function2, function2(result1)} do
     IO.puts "happy path"
  else
    {:function1, error} -> error # error returned by function1
    {:function2, error} -> error # error returned by function2
  end
end

gon782 · September 23, 2018, 8:54pm

I use this a lot in our code base and once you get used to the verbosity of it it’s actually really good. It’s especially useful for differentiating between a bunch of boolean conditions.

victorolinasc · September 24, 2018, 11:13pm

That is a very useful pattern indeed! Thanks for remembering to post here. It is a good future reference.