Permalink
Browse files

Deprecate the use of non-public methods by Module#delegate

  • Loading branch information...
1 parent 0e19c7c commit aa1d1e4962ba218f34defd0e7f0b665c795eb12b @jonleighton jonleighton committed Jul 18, 2011
View
@@ -1,5 +1,7 @@
*Rails 3.1.0 (unreleased)*
+* Using Module#delegate to delegate to non-public methods is deprecated [Jon Leighton]
+
* ActiveSupport::Dependencies now raises NameError if it finds an existing constant in load_missing_constant. This better reflects the nature of the error which is usually caused by calling constantize on a nested constant. [Andrew White]
* Deprecated ActiveSupport::SecureRandom in favour of SecureRandom from the standard library [Jon Leighton]
@@ -1,4 +1,6 @@
require "active_support/core_ext/module/remove_method"
+require 'active_support/core_ext/kernel/singleton_class'
+require 'active_support/deprecation'
class Module
# Provides a delegate class method to easily expose contained objects' methods
@@ -127,15 +129,30 @@ def delegate(*methods)
end
module_eval(<<-EOS, file, line - 5)
- 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)
+ to = #{to} # to = client
+ #
+ begin # begin
+ result = to.__send__(#{method.inspect}, *args, &block) # result = to.__send__(:name, *args, &block)
+ rescue NoMethodError # rescue NoMethodError
+ if to.nil? # if to.nil?
+ #{on_nil} # return # depends on :allow_nil
+ else # else
+ raise # raise
+ end # end
+ end # end
+ #
+ klass = to.singleton_methods.any? ? to.singleton_class : to.class # klass = to.singleton_methods.any? ? to.singleton_class : to.class
@leobessa

leobessa Aug 19, 2011

@jonleighton
I tried to update my application today from v3.1.0.rc5 to v3.1.0.rc6 and my unit tests got way slower. (from about 10 seconds to ~ 60 seconds)

So I used git bisect and I found out this was the "first bad" commit.

I tried to fix this issue, and I found that the line 145 from delegation.rb is was making my app slower. The call to to.singleton_methods.any? seems to be the cause.

Is there a nice way to cache that klass variable?

Is there anything else I can do to help to solve this issue?

@josevalim

josevalim Aug 19, 2011

Contributor

We certainly shouldn't be doing this check at runtime. If we are going to check this kind of stuff, it should be done at "compile" time (when the class is loaded). If it cannot be done at compile time, I simply suggest to not do it. Otherwise delegate becomes useless.

@jonleighton

jonleighton Aug 19, 2011

Member

Aw shit. I assumed that singleton_methods.any? would not be slow in the common case where there is no singleton class. But I am an idiot for not actually testing that hypothesis.

@josevalim I am unsure we can do the check at 'compile' time, because we don't know what the delegate object 'is'. How do you see that working? OTOH, if we have no warning then I am worried that people are going to moan when they upgrade.

One option is that we could revert this deprecation in 3.1, and instead in master we can use public_send, rescue NoMethodError, and use __send__ plus show a deprecation. This will be slow, but only for the deprecated usage. (Of course it means that we have to put off removing the feature until another release.) What do you think?

cc @tenderlove

@josevalim

josevalim Aug 19, 2011

Contributor

@jonleighton the point is not checking to give a warning. If in Rails 3.2 we are going to remove the warning, but still keep the check, it will be useless. delegate is supposed to be as fast as possible and checking for singleton_methods on all invocations is suboptimal.

I like the public_send approach BUT the rescue implementation needs to be carefully handled to avoid rescuing NoMethodError that happen internally. Another downside is that public_send is 1.9 only. The compile time check approach I mentioned would only work when delegating to a class (which is the only thing we would know upfront).

But in general, I have doubts about this commit. It is not part of Ruby philosophy to impose limitations according to "best practices". If someone is using delegate with private methods, that's is their business and responsibility. I know I certainly wouldn't.

@jonleighton

jonleighton Aug 19, 2011

Member

@josevalim

If you write a long-form delegation, you do something like this:

def bar
  foo.bar
end

If foo does not publicly respond to bar, then ruby throws a NoMethodError, so it's obvious that you're trying to call a private method.

In 3.0, using Module#delegate, the method will be more like this:

def bar
  foo.__send__(:bar)
end

It is quite possible, with this implementation, to accidentally set up delegation to private or protected methods and not realise it. For that reason I stand by the intention of this deprecation, if not the implementation. (It is also faster to avoid __send__, particularly on 1.8, I benchmarked this.)

I believe the reason that __send__ was used was to avoid a syntax error when a method ending in = is called with more than on argument. (e.g. foo.bar=(1,2) is a syntax error.)

The current implementation in master simply does foo.bar in most cases, and does foo.public_send(:bar=) if the method name ends in =. I backported an implementation of public_send for 1.8.

However, what I am now thinking for master is to remove the public_send backport and simply not support the very, very unlikely case of calling = methods with multiple arguments via Module#delegate.

Anyway, for 3.1 I think I should probably just remove this deprecation and deal with it in master instead.

@tenderlove

tenderlove Aug 19, 2011

Owner

I'm pretty sure I've seen the exact situation @jonleighton is describing (accidentally exposing private methods) in Rails source itself.

FWIW, Forwardable from stdlib has the same defect:

irb(main):001:0> class A; def foo; end; private :foo; end
=> A
irb(main):002:0> class B
irb(main):003:1>   require 'forwardable'
irb(main):004:1>   extend Forwardable
irb(main):005:1>   def initialize; @a = A.new; end
irb(main):006:1>   def_delegator :@a, :foo
irb(main):007:1> end
=> nil
irb(main):008:0> B.new.foo
=> nil
irb(main):009:0>
@josevalim

josevalim via email Aug 19, 2011

Contributor
@tenderlove

tenderlove Aug 19, 2011

Owner

Agree. We should just revert and keep the behavior the same as Forwardable. Typically when I see many methods being delegated, it feels like a code smell anyway. I'd rather just leave the functionality as is and discourage it's use through rails source.

@jonleighton

jonleighton Aug 19, 2011

Member

Ok.

  • We're all agreed we should revert for 3.1. I will do that.
  • I don't understand the argument that caring about method visibility in delegate is not "the ruby way". What's the point of having method visibility if it is not used? Nothing stops a user who really really wants to delegate to a non-public method from defining their own method that uses send.
  • I agree we shouldn't be checking anything on runtime, that's perfectly possible while observing method visibility. (For example "foo.bar" only works if bar is public but doesn't require any 'checking'... see the current implementation from master that I linked above, which doesn't do any 'checking'.)
  • I don't think the implementation of Forwardable should affect our decision
  • I'm rapidly starting to not give a shit though.
@spastorino

spastorino Aug 19, 2011

Owner

Bro I just wouldn't use delegate :)

@josevalim

josevalim via email Aug 19, 2011

Contributor
+ unless klass.public_method_defined?(#{method.inspect}) # unless klass.public_method_defined?(:name)
+ ActiveSupport::Deprecation.warn( # ActiveSupport::Deprecation.warn(
+ "Using Module#delegate to delegate to non-public methods is " \ # "..." \
+ "deprecated. Please declare your methods as public if they " \ # "..." \
+ "are going to accessed from other classes." # "..."
+ ) # )
+ end # end
+ #
+ result # result
+ end # end
EOS
end
end
@@ -1,5 +1,6 @@
require 'abstract_unit'
require 'active_support/core_ext/module'
+require 'active_support/testing/deprecation'
module One
Constant1 = "Hello World"
@@ -26,10 +27,22 @@ class Cd
end
end
-Somewhere = Struct.new(:street, :city)
+Somewhere = Struct.new(:street, :city) do
+ protected
+
+ def protected_method
+ "protected"
+ end
+
+ private
+
+ def private_method
+ "private"
+ end
+end
Someone = Struct.new(:name, :place) do
- delegate :street, :city, :to_f, :to => :place
+ delegate :street, :city, :to_f, :protected_method, :private_method, :to => :place
delegate :upcase, :to => "place.city"
end
@@ -60,6 +73,8 @@ def initialize(first, last)
end
class ModuleTest < Test::Unit::TestCase
+ include ActiveSupport::Testing::Deprecation
+
def setup
@david = Someone.new("David", Somewhere.new("Paulina", "Chicago"))
end
@@ -69,6 +84,18 @@ def test_delegation_to_methods
assert_equal "Chicago", @david.city
end
+ def test_delegation_to_protected_method
+ assert_deprecated do
+ assert_equal "protected", @david.protected_method
+ end
+ end
+
+ def test_delegation_to_private_method
+ assert_deprecated do
+ assert_equal "private", @david.private_method
+ end
+ end
+
def test_delegation_down_hierarchy
assert_equal "CHICAGO", @david.upcase
end

11 comments on commit aa1d1e4

Contributor

spohlenz replied Aug 17, 2011

Just curious, what was the rationale for this design change?

Contributor

exviva replied Aug 17, 2011

@spohlenz since delegate :foo, :to => :bar is just syntax sugar for:

def foo
  bar.foo
end

both versions should behave identically (i.e. allow calling only public methods on the target).

Contributor

spohlenz replied Aug 17, 2011

@exviva thanks, that definitely makes sense.

It looks as though false deprecation warnings are being produced when delegating a method handled by method_missing on the target class (klass.public_method_defined? isn't the right test), which led to my confusion.

Member

jonleighton replied Aug 17, 2011

Yes, not dealing with method_missing is an oversight. I've created a bug to track this: #2566

Owner

fxn replied Aug 19, 2011

I agree with José's remark about Ruby philosophy. From that point of view I'd prefer delegate not to care about visibility.

Contributor

dcrec1 replied Aug 19, 2011

+1 about not caring

Owner

spastorino replied Aug 19, 2011

This commit is the culprit of this #2561 and probably this #2592

Owner

tenderlove replied Aug 19, 2011

Can someone please revert this on 3-1-stable?

Owner

fxn replied Aug 19, 2011

Jon, regarding "What's the point of having method visibility if it is not used?" normally in Ruby you leave that to the interpreter. As a Ruby programmer I do not expect such a macro to enforce any visibility, it is my business if I want to forward to a private method.

Contributor

alexeymuranov replied Sep 2, 2011

In my opinion, it is also important that the user/programmer could guess the meaning of (macro) methods correctly without looking into API. Before reading this discussion, i assumed that delegate :foo, :to => :bar was identical to

def foo
  bar.foo
end

(see exviva). I vote for them to behave identically.

Please sign in to comment.