Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hash as last argument leads to unnecessary object creation #151

Open
sfdcdoug opened this issue Sep 30, 2024 · 0 comments
Open

Hash as last argument leads to unnecessary object creation #151

sfdcdoug opened this issue Sep 30, 2024 · 0 comments

Comments

@sfdcdoug
Copy link

sfdcdoug commented Sep 30, 2024

Describe the bug

Dry::Transaction::Callable seems overly aggressive in its attempt to address a shift in the ruby language around hashes passed to methods. Within Callable is the following documentation on ruby_27_last_arg_hash?:

Ruby 2.7 gives a deprecation warning about passing a hash of parameters as the last argument to a method. Ruby 3.0 outright disallows it.

This isn't 100% true. The ruby documentation around converting arrays into arguments states (emphasis mine):

If the method accepts keyword arguments [...] Note that this behavior is currently deprecated and will emit a warning. This behavior will be removed in Ruby 3.0.

As far as I can tell, the emphasized portion is not accounted for in Dry::Transaction::Callable. This may not seem like a big deal, but from a behavior standpoint it turns calls into call-by-value instead of call-by-reference, which can lead to issues when side-effects are expected.

To Reproduce

require "dry-transaction"

class Txn
  include Dry::Transaction

  step :do_something

  def do_something(input)
    input[:side_effect] *= 2
    Success()
  end
end

input = {side_effect: 1}
Txn.new.call(input)
puts "This should be 2 => #{input[:side_effect]}"

Expected behavior

In my example input should be passed without generating a copy, allowing any modifications to the hash to be seen outside of the transaction. Locally, I have put this patch in place to address the issue:

class Dry::Transaction::Callable
  alias_method :original_ruby_27_last_arg_hash?, :ruby_27_last_arg_hash?
  def ruby_27_last_arg_hash?(args)
    parameters = operation.instance_of?(Method) ? operation.parameters : operation.method(:call).parameters
    operation_uses_kwargs = parameters.any? { |(type, _)| %i[key keyrest].include?(type) }
    operation_uses_kwargs && original_ruby_27_last_arg_hash?(args)
  end
end

I won't argue that this is the right way to do this, but it seems to work for my situation.

My environment

  • Affects my production application: No. A workaround (listed above) is available.
  • Ruby version: 3.3.4
  • OS: macOS 13.6.9 (22G830)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant