Surprising return from Result.map({:ok, _}, nil)

[tshakah]

I know this library is mostly a mashup of two different functional concepts - maybe values and tagged errors.

Saying that, it looks like there isn't a way of handling "I know what the value is, and it is nil" - I was quite surprised that {:ok, 1} |> Result.map(nil) returns :none.

I can see that's even in the docs, but I think it's a flaw in the design, and even contradictory. From what I can see, no other calls to Result.map change the outer tuple.

[tshakah]

A full example of the issue I have is here:

message_map_r = {:ok, %{"Timestamp" => "2022-01-06T14:46:56.387201Z", "Value" => nil}

value_r =
  message_map_r
  |> Result.map(&Map.get(&1, "Value"))
  |> Result.default_error("Value is missing from map.")

The above code returns {:error, "Value is missing from map."}, even though Value isn't missing, it's set to nil (which is an appropriate value for this context)

(I appreciate that the Map.get would return nil if Value was missing, and so the code needs work, but that's a separate issue, although it demonstrates the same confusion 😛)

[giddie]

It could be that the intention isn't communicated clearly enough in the documentation, but the general idea is that you're either working with nillable values, or you're working with tagged Results. If you enter a Result pipeline, then your intention is to avoid / short-circuit nils, until you exit the pipeline and extract your final result.

To me, it seems that your example behaves safely - it detects a nil that was not explicitly handled, and the pipeline falls through to a default value. I think it's actually a decent example of how using a Result pipeline can help mitigate missed nil checks, which are such a huge source of bugs.

If your intention is to provide different behaviour depending on whether Value is missing from the map or its value is nil, you could do something like:

{:ok, %{"Timestamp" => "2022-01-06T14:46:56.387201Z", "Value" => nil}}
|> Result.then(&Map.fetch(&1, "Value"))
|> Result.error_map("Value is missing from map.")
|> Result.none_then(nil)
# => {:ok, nil}

As a result, the desire to return nil is explicit in the code. For such a simple pipeline, this is kind of overkill, but I'm assuming this is part of something bigger.

[tshakah]

Actually, the Result.then(&Map.fetch(&1, "Value")) alone does exactly what I want - it returns {:ok, nil}. The Result.none_then(nil) won't be called in your example.

[tshakah]

I think it's actually a decent example of how using a Result pipeline can help mitigate missed nil checks, which are such a huge source of bugs.

I can see where you're coming from, but my example is also an example of where tagged tuples shine - nil alone might mean "the value was missing or is nil" (a source of bugs), but in a traditional functional context you could have {:ok, nil} for "I have the value of nil" and :none for "I don't know what the value is"

Result.map silently converts the former to the latter, which I would say is unexpected but defined behaviour 😛 . I'll re-read the docs and see if they need to be more explicit

[giddie]

nil alone might mean "the value was missing or is nil" (a source of bugs), but in a traditional functional context you could have {:ok, nil} for "I have the value of nil" and :none for "I don't know what the value is"

For me, the problem is that I don't understand the meaning of "I have the value of nil". Nil as a concept means "no value". It's not an error, but it's not a value either. It seems to me that you're interested in two maybe-values here - the presence of the key, and the value of the key, where a missing key is an error. Essentially, the map itself is of type Map<Maybe<T>>, so when getting a value from the map, we have Ok<Maybe<T>>. We could model it more explicitly in Elixir like this, for instance:

{:error, "The key is missing."}
{:ok, :none}
{:ok, {:some, 123}}

This is certainly closer to what Rust does. The traditional approach in Elixir, of course, is to use value | nil instead of {:some, value} | :none to represent the maybe part, but this has all the usual pitfalls of nils. The first thing you'll do is unwrap the ok tuple, which is the maybe-value representing the presence of the key. What you're left with is the maybe-value for the key's value, which you're suggesting should be value | nil. That nil will be passed as a regular value into some function that probably doesn't have a nil-check. The "default" (easy) behaviour is to assume the value is not nil. The result is that when a nil appears, it's usually been passed through several functions before triggering a rather confusing message such as protocol Enumerable not implemented for nil of type Atom.

The advantage of forcing a pattern match (e.g. {:some, 123}) is that you're reminded to handle the other condition (:none). And if you don't, at least your code will raise a pattern match error at the exact point you failed to handle the missing value.

The only real difference with OkThen is that we collapse this:

{:ok, :none} | {:ok, {:some, any()} | {:error, any()}

Into this, mostly for convenience (and also because we can't rely on strict types to help enforce the nested pattern):

:none | {:ok, any()} | {:error, any()}

[giddie]

Actually, the Result.then(&Map.fetch(&1, "Value")) alone does exactly what I want - it returns {:ok, nil}. The Result.none_then(nil) won't be called in your example.

Well spotted :) That actually makes me a bit uncomfortable. I'd be tempted to add a |> Result.map(&(&1)) to convert that if the pipeline isn't trivial. (It would work, but it's not clear it's for dealing with nils.)

I wonder if Result.Private.normalize_result_input/1 should convert {atom(), nil} to :none. That would certainly help catch stuff like this. It would be a breaking change, though. The change would mean:

{:ok, nil}
|> Result.normalize()
# => :none

{:ok, nil}
|> Result.none_then("hello")
# => "hello"

Which is a bit safer, but could also cause confusion:

{:ok, nil}
|> Result.then("hello")
# => {:ok, nil)