Can't extract wrapped array through pattern-matching

[waiting-for-dev]

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.

[flash-gordon]

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.

[waiting-for-dev]

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? 🤔

[flash-gordon]

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.

[waiting-for-dev]

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! 🙂

[waiting-for-dev]

For context, I'd like to use it for the code at dry-rb/dry-operation#6

[flash-gordon]

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! 🙂

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?

[waiting-for-dev]

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 {}.

[flash-gordon]

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.

[tylerhunt]

Would it be possible to adjust the destructuring behavior based on how the result is created (i.e. parentheses vs. square brackets)? Something like:

Success([1, 2, 3]) => Success(array)
array # =>  [1, 2, 3]

Success([1, 2, 3]) => Success[first, *]
first # =>  1

[tylerhunt]

Rough proof-of-concept shows it's possible:

        def self.[](*value) = new(value, array: true)
        
        def initialize(value, array: !value.is_a?(::Array))
          super()
          @value = value
          @array = array
        end

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

Haven't dug into it enough to know what this might break.