search

ruby / forwardable

Provides delegation of specified methods to a designated object
BSD 2-Clause "Simplified" License
41 stars 17 forks source link

def_*_delegator from v1.3.0 does not return name of delegator method in Ruby 2.6 and older #10

Closed ashmaroli closed 4 years ago

ashmaroli commented 4 years ago

In v1.2.0, def_delegator :@obj, :data, :fallback_data would return :fallback_data but v1.3.0 just returns nil

The cause for this is the last expression mod.send(:ruby2_keywords, ali) if RUBY_VERSION >= '2.7' in the following definition: https://github.com/ruby/forwardable/blob/e56f0f83c67f0b3226d1aacc357dca9ef18e3ad4/lib/forwardable.rb#L182-L189

The last expression is evaluated only if RUBY_VERSION >= '2.7'. The preferred solution would be to stash the result of the previous expression and return that stashed value for Ruby 2.6 and older:

 def def_instance_delegator(accessor, method, ali = method)
   gen = Forwardable._delegator_method(self, accessor, method, ali)

   # If it's not a class or module, it's an instance
   mod = Module === self ? self : singleton_class

   # stash in a lvar
   result = mod.module_eval(&gen)
   RUBY_VERSION >= '2.7' ? mod.send(:ruby2_keywords, ali) : result
 end

Based on the diff of #5, this issue would affect :single_delegator as well.

/cc @jeremyevans @hsbt Requesting a patch to be shipped at the earliest. Thank you.

jeremyevans commented 4 years ago

The return value of def_instance_delegator and def_single_delegator is not specified in the documentation or tested by any specs. Therefore, any code that depends on the return value of the methods is relying on undefined behavior.

That being said, I am not opposed to putting out a 1.3.1 that restores the previous behavior and adds documentation and specs for it. I'll submit a pull request later today, and hopefully @hsbt can handle the release.

In the future, please use a more accurate subject. 1.3.0 isn't broken, it works for almost all cases. That the undefined behavior in 1.3.0 differs from previous undefined behavior, and it caused a problem in a widely-used application is unfortunate, but no reason for hyperbole.

ashmaroli commented 4 years ago

@jeremyevans Thank you for responding. I apologize for the hyperbolic title of this ticket. I couldn't come up with an apt one at the time. I've now renamed it by taking a cue from your patch's commit message.

I was not aware that the usage I was familiar with is an undocumented behavior that shouldn't be considered as public API. Moving forward, what would be the best way to conform to this module's Public API yet maintain the downstream's current behavior?