Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Deprecate delegation to private methods #2736

Closed
wants to merge 14 commits into from

5 participants

@dasch

This pull request accompanies #2733, and provides two deprecations related to the use of delegate:

  1. Delegating to a private method on the target object
  2. Delegating to an attribute writer method (e.g. foo=) that accepts multiple arguments

The code is a bit tricky, so you're welcome to reviewing it. However, the acceptance of this request should be predicated upon the successful inclusion of #2733.

...port/lib/active_support/core_ext/module/delegation.rb
((18 lines not shown))
+ 'Writer methods should only accept one argument. Support ' +
+ 'for multiple arguments will be removed in the future.', caller)
+ end
+ DEPRECATION
+
+ definition = "args"
+ else
+ deprecation = ""
+ definition = "*args, &block"
+ end
+
+ module_eval(<<-EOS, file, line - 2)
+ def #{prefix}#{method}(#{definition}) # def customer_name(*args, &block)
+ #{deprecation} #
+ #{to}.#{method}(#{definition}) # client.name(*args, &block)
+ rescue NoMethodError => e # rescue NoMethodError => e
@jonleighton Collaborator

=> e not needed. raise with no args will re-raise the most recent exception.

@dasch
dasch added a note

The e is currently used to maintain a neat stack trace. Otherwise it would be the __send__ call which would have caused the error which eventually propagates up.

I'll look into a better way of doing this.

@jonleighton Collaborator

Ah right, that's fair enough - leave it then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...port/lib/active_support/core_ext/module/delegation.rb
((8 lines not shown))
- if #{to}.nil? # if client.nil?
- #{on_nil} # return # depends on :allow_nil
- else # else
- raise # raise
- end # end
- end # end
+ if method.to_s =~ /[^]]=/
+ deprecation = <<-DEPRECATION
+ if args.length > 1
+ ActiveSupport::Deprecation.warn(
+ 'Writer methods should only accept one argument. Support ' +
+ 'for multiple arguments will be removed in the future.', caller)
+ end
+ DEPRECATION
+
+ definition = "args"
@jonleighton Collaborator

args.length will never be > 1 since this means the method will be defined with no splat.

@dasch
dasch added a note

I'm not sure how best to do this. This is valid syntax: def foo=(*args); p args; end but this is not: self.foo=(*args).

@jonleighton Collaborator

You're aiming for something like this:

def foo=(*args)
  if args.length > 1
    [emit deprecation warning]
    target.__send__(:foo, *args)
  else
    target.foo=args.first
  end
end
@dasch
dasch added a note

I've pushed a version with a customized code path for writer methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...port/lib/active_support/core_ext/module/delegation.rb
@@ -126,16 +126,39 @@ class Module
%(raise "#{self}##{prefix}#{method} delegated to #{to}.#{method}, but #{to} is nil: \#{self.inspect}")
end
- module_eval(<<-EOS, file, line - 1)
- def #{prefix}#{method}(*args, &block) # def customer_name(*args, &block)
- #{to}.__send__(#{method.inspect}, *args, &block) # client.__send__(:name, *args, &block)
- rescue NoMethodError # rescue NoMethodError
- if #{to}.nil? # if client.nil?
- #{on_nil} # return # depends on :allow_nil
- else # else
- raise # raise
- end # end
- end # end
+ if method.to_s =~ /[^]]=/
+ deprecation = <<-DEPRECATION
+ if args.length > 1
@jonleighton Collaborator

should also check for whether a block is given

@dasch
dasch added a note

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...port/lib/active_support/core_ext/module/delegation.rb
((29 lines not shown))
+ module_eval(<<-EOS, file, line - 2)
+ def #{prefix}#{method}(#{definition}) # def customer_name(*args, &block)
+ #{deprecation} #
+ #{to}.#{method}(#{definition}) # client.name(*args, &block)
+ rescue NoMethodError => e # rescue NoMethodError => e
+ begin # begin
+ #{to}.__send__(#{method.inspect}, #{definition}) # client.__send__(:name, *args, &block)
+ rescue NoMethodError # rescue NoMethodError
+ if #{to}.nil? # if client.nil?
+ #{on_nil} # return # depends on :allow_nil
+ else # else
+ raise(e) # raise(e)
+ end # end
+ else # else
+ ActiveSupport::Deprecation.warn( # ActiveSupport::Deprecation.warn(
+ 'Delegating to private methods is deprecated.', caller) # 'Delegating to private methods is deprecated.', caller)
@jonleighton Collaborator

"non-public" would be more accurate, since protected methods are also deprecated

@dasch
dasch added a note

Fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@jonleighton
Collaborator

Please write tests. You can use assert_deprecated to check for deprecations.

...port/lib/active_support/core_ext/module/delegation.rb
((21 lines not shown))
+ DEPRECATION
+
+ definition = "args"
+ else
+ deprecation = ""
+ definition = "*args, &block"
+ end
+
+ module_eval(<<-EOS, file, line - 2)
+ def #{prefix}#{method}(#{definition}) # def customer_name(*args, &block)
+ #{deprecation} #
+ #{to}.#{method}(#{definition}) # client.name(*args, &block)
+ rescue NoMethodError => e # rescue NoMethodError => e
+ begin # begin
+ #{to}.__send__(#{method.inspect}, #{definition}) # client.__send__(:name, *args, &block)
+ rescue NoMethodError # rescue NoMethodError
@josevalim Owner

We need to check if the NoMethodError exception originates from the method call above, otherwise we can swallow other NoMethodError exceptions by mistake. Here and in the rescue above.

@dasch
dasch added a note

I'm not sure how best to do that - can you give me an example?

@dasch
dasch added a note

I've added two tests of the error handling behavior and added conditional checks based on NoMethodError#name. Is that enough?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...port/lib/active_support/core_ext/module/delegation.rb
@@ -126,16 +126,39 @@ class Module
%(raise "#{self}##{prefix}#{method} delegated to #{to}.#{method}, but #{to} is nil: \#{self.inspect}")
end
- module_eval(<<-EOS, file, line - 1)
- def #{prefix}#{method}(*args, &block) # def customer_name(*args, &block)
- #{to}.__send__(#{method.inspect}, *args, &block) # client.__send__(:name, *args, &block)
- rescue NoMethodError # rescue NoMethodError
- if #{to}.nil? # if client.nil?
- #{on_nil} # return # depends on :allow_nil
- else # else
- raise # raise
- end # end
- end # end
+ if method.to_s =~ /[^]]=/
@josevalim Owner

Shouldn't the behavior be the same also if it is the method []= ?

@dasch
dasch added a note

[]= always takes 2 arguments.

@jonleighton Collaborator

Not always:

ruby-1.9.2-p290 :004 >   def []=(one, two, three)
ruby-1.9.2-p290 :005?>   p [one,two,three]
ruby-1.9.2-p290 :006?>   end
 => nil 
ruby-1.9.2-p290 :007 > self.send(:[]=, 1, 2, 3)
[1, 2, 3]
 => [1, 2, 3] 

But self.[]=(1, 2, 3) is syntactically valid, so it's ok.

@dasch
dasch added a note

That's true. But as you say, there's no problem in doing foo.[]=(1, 2, 3).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
activesupport/test/core_ext/module_test.rb
@@ -90,6 +101,18 @@ class ModuleTest < Test::Unit::TestCase
end
end
+ def test_deprecates_delegation_to_private_methods
+ assert_deprecated do
+ Tester.new(@david).something_private
@jonleighton Collaborator

Please add assertion that the return type is what you expect it to be also.

@dasch
dasch added a note

Will do.

@dasch
dasch added a note

Caught a bug there :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dasch

Supporting send(:something=, 1, 2) is a real hassle.

@dasch

Okay, I think I've reached the limit of Ruby here: implementing something= so that it keeps the old semantics is impossible without using __send__.

This will never be valid Ruby: foo.bar=(*args, &block), and doing foo.bar=(args) breaks the semantics.

I may need to special case when it's a writer method.

@jonleighton
Collaborator

Yes, you will have to use __send__. That's why we're deprecating writers with multiple args.

@dasch

I think I've looked at my monitor for too long! :-) of course we need to use __send__.

I've fixed the code.

@dasch

I've now rewritten most of the code per your comments. Will you have another look and tell me if there's more that needs fixin'?

When it's ready I can squash the commits.

...port/lib/active_support/core_ext/module/delegation.rb
@@ -126,16 +128,48 @@ class Module
%(raise "#{self}##{prefix}#{method} delegated to #{to}.#{method}, but #{to} is nil: \#{self.inspect}")
end
+ rescue_clause = <<-EOS
+ rescue NoMethodError => e # rescue NoMethodError => e
+ raise unless e.name == #{method_name} # raise unless e.name == :name
@jonleighton Collaborator

could you do e.name.to_sym here and below just to be sure it's a symbol? (in case somebody is manually raising NoMethodError and using a string)

@dasch
dasch added a note

I'm not sure I get it -- the whole point of those lines are to make sure we only intercept the NoMethodErrors we cause ourselves. So we don't really care about people manually raising exceptions, as these will simply be re-raised. Or am I misunderstanding something?

@josevalim Owner

I guess this case is not important. If someone is doing NoMethodError manually, it will not be the error we want to handle anyway.

@jonleighton Collaborator

actually, I am worried about this test in case e.name is the same but the exception isn't raise here. have you looked into inspecting the first item of the backtrace to see where the exception originates?

@dasch
dasch added a note

I'm not sure what the safest way of doing this is. Checking a string would incur a pretty big overhead, but perhaps we can ignore that in this case. I'll try looking into it. If you have any good ideas on how to do this, I'd be glad to know.

@jonleighton Collaborator

You're right about the e.name.to_sym comment - ignore that.

One idea is that maybe you can check the backtrace only when e.name == #{method_name}, which would deal with the performance hit to a reasonable extent I think.

@dasch
dasch added a note

This is proving to be a bit difficult - NilClass implements a method_missing which interferes, making the first backtrace line:

activesupport/lib/active_support/whiny_nil.rb:48:in `method_missing'
@jonleighton Collaborator

I think you want something like this (I haven't tested, but hopefully it gives an idea):

def name(*args, &block)
  client.name(*args, &block)
rescue NoMethodError => e
  if e.name == :name &&
     e.message =~ /(private|protected) method/ &&
     e.backtrace.first =~ /active_support\/core_ext\/module\/delegation/

    # If we get here, we *know* that the code is relying on a non-public method in
    # some way, so we always want to show a deprecation warning, hence it is in
    # the ensure block.

    begin
      client.__send__(:name, *args, &block)
    rescue NoMethodError
      if client.nil?
        # on nil
      else
        raise
      end
    ensure
      # show deprecation warning
    end
  else
    if client.nil?
      # on nil
    else
      raise
    end
  end
end
@dasch
dasch added a note

@jonleighton thanks, I'll work from that code.

@jonleighton Collaborator

Cool. You might as well put the deprecation before the __send__ call actually - there's no real need for it to be in an ensure.

@dasch
dasch added a note

I'm not sure about e.backtrace.first =~ /active_support\/core_ext\/module\/delegation/ - won't the backtrace point to the file from which delegate is called, since the code is evaluated there?

@jonleighton Collaborator

Unsure, PDI :)

@dasch
dasch added a note

The backtrace only contains the file from which delegate is called. I'm using __FILE__ instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
...port/lib/active_support/core_ext/module/delegation.rb
((14 lines not shown))
+ raise(e) # raise(e)
+ end # end
+ else # else
+ ActiveSupport::Deprecation.warn( # ActiveSupport::Deprecation.warn(
+ 'Delegating to non-public methods is deprecated.', caller) # 'Delegating to non-public methods is deprecated.', caller)
+ result # result
+ end # end
+ EOS
+
+ method_body =
+ if method.to_s =~ /[^\]]=/
+ <<-EOS
+ if args.length > 1 || block_given? # if args.length > 1 || block_given?
+ ActiveSupport::Deprecation.warn( # ActiveSupport::Deprecation.warn(
+ 'Writer methods should only accept one argument. Support ' + # 'Writer methods should only accept one argument. Support ' +
+ 'for multiple arguments will be removed in the future.', caller) # 'for multiple arguments will be removed in the future.', caller)
@jonleighton Collaborator

Please could you rephrase this:

"Support for using Module#delegate with writer methods and multiple arguments, or block arguments, is going to be removed. If you need this functionality, please define your own delegation manually."

@dasch
dasch added a note

Will do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dasch

@jonleighton: I've changed the code per your comments and it seems to run fine now. What do you think?

@dasch

Also, rebased against 3-1-stable.

@jonleighton
Collaborator

Hi @dasch. Thanks for working on this. I have talked to @tenderlove and we think it is too late to deprecate this in 3-1-stable. So would you be up for porting this deprecation to master? Then we can make the actual change in 3.2 + 1. Thanks.

@dasch
@bogdan

I don't feel this patch move the right way.
Pass more code to eval is wrong and will never be fast.

#2275 was a try to make delegate move the right direction.
If this patch will be accepted - it will be more hard fix the direction back again.
Benchmarks is in the discussion thread.

@dasch

@bogdan: we're trying to deprecate some stuff here, this is not what's going to be in future versions of Rails.

@rafaelfranca

What is the status of this pull request?

@dasch

I think this deprecation code is way too complicated. I'd much prefer to simply change the delegation code in the 4.0 branch such that it doesn't use send and be sure to document it. It's a major version change, so I don't feel that a deprecation period is absolutely needed.

@josevalim
Owner
@dasch

@josevalim the PR for actually changing delegate is #2733.

@jonleighton
Collaborator

I agree with @josevalim. Let's just change it. Closing this so we can move discussion to #2733.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 11, 2011
  1. @dasch

    Deprecate delegation to private methods

    dasch authored
    Also, deprecate the delegation to writer methods that accept multiple
    arguments.
  2. @dasch
  3. @dasch

    Change "private" to "non-public"

    dasch authored
  4. @dasch
  5. @dasch
  6. @dasch
  7. @dasch
  8. @dasch
  9. @dasch
  10. @dasch

    Escape ] in regex

    dasch authored
  11. @dasch

    Refactor the delegate method

    dasch authored
    Remove redundancies.
  12. @dasch

    Don't use method.inspect, that'll surely break!

    dasch authored
    Construct the symbol name properly.
  13. @dasch

    Change the deprecation message

    dasch authored
  14. @dasch

    Tidy up the delegate method

    dasch authored
    Make sure we test the stack trace, too.
This page is out of date. Refresh to see the latest.
View
60 activesupport/lib/active_support/core_ext/module/delegation.rb
@@ -119,6 +119,8 @@ def delegate(*methods)
line = line.to_i
methods.each do |method|
+ method_name = ":#{method}"
+
on_nil =
if options[:allow_nil]
'return'
@@ -126,16 +128,56 @@ def delegate(*methods)
%(raise "#{self}##{prefix}#{method} delegated to #{to}.#{method}, but #{to} is nil: \#{self.inspect}")
end
+ nil_clause = <<-EOS
+ if #{to}.nil? # if client.nil?
+ #{on_nil} # return # depends on :allow_nil
+ else # else
+ raise(e) # raise(e)
+ end # end
+ EOS
+
+ rescue_clause = <<-EOS
+ rescue NoMethodError => e # rescue NoMethodError => e
+ if e.name == #{method_name} && # if e.name == :name &&
+ e.message =~ /(private|protected) method/ && # e.message =~ /(private|protected) method/ &&
+ e.backtrace.first.include?(__FILE__) # e.backtrace.first.include?(__FILE__)
+ begin # begin
+ ActiveSupport::Deprecation.warn( # ActiveSupport::Deprecation.warn(
+ 'Delegating to non-public methods is deprecated.', caller) # 'Delegating to non-public methods is deprecated.', caller)
+ #{to}.__send__(#{method_name}, *args, &block) # client.__send__(:name, *args, &block)
+ rescue NoMethodError # rescue NoMethodError
+ #{nil_clause}
+ end # end
+ else # else
+ #{nil_clause}
+ end # end
+ EOS
+
+ method_body =
+ if method.to_s =~ /[^\]]=/
+ <<-EOS
+ if args.length > 1 || block_given? # if args.length > 1 || block_given?
+ ActiveSupport::Deprecation.warn( # ActiveSupport::Deprecation.warn(
+ "Support for using Module#delegate with writer methods and " + # "Support for using Module#delegate with writer methods and " +
+ "multiple arguments, or block arguments, is going to be " + # "multiple arguments, or block arguments, is going to be " +
+ "removed. If you need this functionality, please define your " + # "removed. If you need this functionality, please define your " +
+ "own delegation manually.", caller) # "own delegation manually.", caller)
+ #{to}.__send__(#{method_name}, *args, &block) # client.__send__(:invoices=, *args, &block)
+ else # else
+ #{to}.#{method}(args.first) # client.invoices=(args.first)
+ end # end
+ EOS
+ else
+ <<-EOS
+ #{to}.#{method}(*args, &block) # client.name(*args, &block)
+ EOS
+ end
+
module_eval(<<-EOS, file, line - 1)
- def #{prefix}#{method}(*args, &block) # def customer_name(*args, &block)
- #{to}.__send__(#{method.inspect}, *args, &block) # client.__send__(:name, *args, &block)
- rescue NoMethodError # rescue NoMethodError
- if #{to}.nil? # if client.nil?
- #{on_nil} # return # depends on :allow_nil
- else # else
- raise # raise
- end # end
- end # end
+ def #{prefix}#{method}(*args, &block) # def customer_name(*args, &block)
+ #{method_body}
+ #{rescue_clause}
+ end # end
EOS
end
end
View
79 activesupport/test/core_ext/module_test.rb
@@ -34,6 +34,39 @@ class Someone < Struct.new(:name, :place)
FAILED_DELEGATE_LINE = __LINE__ + 1
delegate :foo, :to => :place
+
+ def raise_calls
+ @raise_calls
+ end
+
+ def initialize(*args)
+ @raise_calls = 0
+ super
+ end
+
+ def raises_no_method_error
+ @raise_calls += 1
+ raise NoMethodError.new("no method!", :fizzle)
+ end
+
+ def occupation=(a, b)
+ "WRITER"
+ end
+
+ private
+ def something_private
+ "PRIVATE"
+ end
+
+ def raises_no_method_error_in_private
+ @raise_calls += 1
+ raise NoMethodError.new("no method!")
+ end
+
+ protected
+ def something_protected
+ "PROTECTED"
+ end
end
Invoice = Struct.new(:client) do
@@ -52,6 +85,8 @@ class Someone < Struct.new(:name, :place)
Tester = Struct.new(:client) do
delegate :name, :to => :client, :prefix => false
+ delegate :something_private, :something_protected, :occupation=, :to => :client
+ delegate :raises_no_method_error, :raises_no_method_error_in_private, :to => :client
end
class Name
@@ -62,7 +97,7 @@ def initialize(first, last)
end
end
-class ModuleTest < Test::Unit::TestCase
+class ModuleTest < ActiveSupport::TestCase
def setup
@david = Someone.new("David", Somewhere.new("Paulina", "Chicago"))
end
@@ -90,6 +125,48 @@ def test_missing_delegation_target
end
end
+ def test_deprecates_delegation_to_private_methods
+ assert_deprecated do
+ assert_equal "PRIVATE", Tester.new(@david).something_private
+ end
+ end
+
+ def test_deprecates_delegation_to_protected_methods
+ assert_deprecated do
+ assert_equal "PROTECTED", Tester.new(@david).something_protected
+ end
+ end
+
+ def test_deprecates_delegation_to_attr_writer_with_multiple_args
+ assert_deprecated do
+ assert_equal "WRITER", Tester.new(@david).send(:occupation=, 1, 2)
+ end
+ end
+
+ def test_does_not_swallow_no_method_errors
+ @tester = Tester.new(@david)
+
+ error = assert_raise(NoMethodError) do
+ @tester.raises_no_method_error
+ end
+
+ assert_equal :fizzle, error.name
+ assert_equal 1, @david.raise_calls
+ end
+
+ def test_does_not_swallow_no_method_errors_from_private_methods
+ @tester = Tester.new(@david)
+
+ error = assert_raise(NoMethodError) do
+ assert_deprecated do
+ @tester.raises_no_method_error_in_private
+ end
+ end
+
+ assert_equal :raises_no_method_error_in_private, error.name
+ assert_equal 1, @david.raise_calls
+ end
+
def test_delegation_prefix
invoice = Invoice.new(@david)
assert_equal invoice.client_name, "David"
Something went wrong with that request. Please try again.