dry-monads

dry-monads copied to clipboard

Describe the bug

When a right-biased type is wrapping an array value, it's not possible to unwrap it via pattern-matching.

To Reproduce

require "dry/monads"
include Dry::Monads[:result]
Success(1) in Success(x) # => true
x # => 1 AS EXPECTED
Success([1]) in Success(x) # => true
x # => 1 IT SHOULD BE [1]
Success([1, 2]) in Success(x) # => true
x # => 1 IT SHOULD BE [1, 2]

Expected behavior

The wrapped value should be extracted as it is.

My environment

• Affects my production application: NO
• Ruby version: v3.2

Proposed solution

We should stop branching depending on whether the wrapped value is an array. Instead, #deconstruct should be simply:

def deconstruct
  if Unit.equal?(@value)
    EMPTY_ARRAY
  else
    [@value]
  end
end

However, the above would be a breaking change. Now we have:

Success([1]) in [1] # => true

The above would change:

Success([1]) in [1] # => false

IMO, we should nonetheless change it, as the most common use-case for results is extracting the wrapped value after some operations have been performed.

I'm happy to submit a PR fixing the issue. If we consider it as a bug fix, it should go into the next minor or patch release. However, if we consider it as a new feature breaking code, we should wait until v2.0.

It is intentional, it's also documented in the docs. I don't remember the details but I think it's related to using PM in the following scenario:

case Failure[:error, 'oops']
in Failure[:error, message]
  message # => 'oops'
end

Again, I don't remember details but I think removing the branch will force using in Failure([:error, message]) which is inconsistent with using the constructor.

Thanks for getting back, @flash-gordon. I'm failing to understand the inconsistency with the constructor, shouldn't we use Failure([:error, :message]) when using it? :thinking:

IIRC the shorthand Failure[error_code, message] was added some time before PM was a thing in ruby. Then, in order to support it in PM, I had to add that hack. The justification was using arrays of variable length is a lot less common then using arrays as tuples for encoding errors. Unfortunately, for consistency I had to make both Failure and Success work with arrays in the same manner. Clearly, it's a trade off.

I see. Do you see it as something we could change in a major release (including the constructor)? Surely not the end of the world, but it's a pity we can't blindly use pattern matching for any kind of wrapped value and need to rely on not-as-pure methods like value! :slightly_smiling_face:

For context, I'd like to use it for the code at https://github.com/dry-rb/dry-operation/pull/6

I see. Do you see it as something we could change in a major release (including the constructor)? Surely not the end of the world, but it's a pity we can't blindly use pattern matching for any kind of wrapped value and need to rely on not-as-pure methods like value! :slightly_smiling_face:

Not likely at this point. I have hundreds if not thousands of usage in my codebase alone, changing them all would be a time-consuming task. On the other hand, I paid attention to this caveat, and so far I've got only a couple of questions about this documented behavior. How did you come across this issue?

How did you come across this issue?

While working on dry-operation, we need to unwrap intermediate steps (or throw on failure). That needs to be done blindly, without knowing whether the inner type is an array or not. I was hoping to do it through pattern-matching for a couple of reasons:

• It's the canonical way to destructure composite types.
• I'm considering the idea of making it independent of the underlying data type (e.g., use it with something different from dry-monads). Implementing pattern-matching would be a lighter requirement than, for instance, implementing value! or value_or {}.

Ah, I see. I think it's ok not to use PM here, it's library code so matching cases it not as important. Moreover, calling .success? and value! is likely faster.