[ruby-core:83573] [Ruby trunk Feature#14056] Add Module#deprecate_public for deprecation of public methods

Issue #14056 has been updated by jeremyevans0 (Jeremy Evans).


marcandre (Marc-Andre Lafortune) wrote:
> Just curious about the use case: since you have the control over private methods, why not simply do something like
> 
> 
> ~~~ ruby
>  class MyClass
>     def meth
>       warn "meth is deprecated, please call instead some_other_meth"
>       _meth 
>     end
>     
>     private
>     def _meth
>       1
>     end
>  end
> ~~~


In libraries where the expected use case has the user subclassing a class provided by the library or including a module provided by the library, and then overriding methods defined in the class/module and calling super to get the default behavior, "control" over methods is something that does not necessary fall on simple public/private bounds.  Think of such methods as extension points.  There may be extension points where the method should be called publicly, and extension points where the method should not be called publicly.  A similar issue occurs in plugin systems, where users provide plugins that override/augment the default behavior of the library.

For example, one thing I would consider using this for is before_* and after_* hook methods in Sequel.  These methods have always been public, but they should really be private as they shouldn't be called outside of an internal method (save).  It is expected that users would override them in their own model classes and plugins.  Renaming in this way would cause a ton of churn.

> Internally, you use `_meth` until the next major release. The, you're free to delete `meth` and rename usage of `_meth` to `meth` in your code.

This can require much more work if the methods are used extensively internally in a private manner.  Additionally, in the example given above, it's unreasonable to ask all users of your library to rename all methods operating as extension points.
 
> Also, FWIW I think of two ways to achieve your feature without modifying the VM.
> 
> One way would be to check `caller_locations.first.path`. Another way is by using `method_missing` and `respond_to_missing?`.

I don't think checking caller will work, since it can't tell you if the call was made private or public.  We'll use `caller.first` in this example, as `caller_locations.first.path` only tells you the file, and tells you nothing about whether the call was public or private: 

~~~ruby
def b; p caller.first end
def a; self.b; b; send(:b); public_send(:b); end
~~~

result:

~~~
"(irb):6:in `a'"
"(irb):6:in `a'"
"(irb):6:in `a'"
"(irb):6:in `public_send'"
~~~

So you can tell the difference between public_send and the others, but not between `self.b`, `b`, and `send(:b)`. send is optimized in this case I believe, which is why it doesn't have it's own entry in `caller`.

It is true you can use `method_missing` for this:

~~~ruby
class A
  def method_missing(meth, *a, &block)
    if meth == :b
      warn("b called publicly, not privately")
      b(*a, &block)
    else
      super
    end
  end
  def a; self.b; b; send(:b); public_send(:b); end
  private
  def b; p :b end
end
A.new.a
~~~

That seems like a reasonable solution, especially since you can implement `Module#deprecate_public` using it.

> A third way would be using the gem `binding_of_caller` and check who the caller is, but that's not portable though.
> 
> Nevertheless, I'm not opposed to the feature, I'm just not convinced of it's necessity. I would much prefer having something like `binding_of_caller` become part of the language.

I'm not sure if `binding_of_caller` can tell the difference between `foo.send(:b)` (private allowed) and `foo.b` (private not allowed) calls.

Thanks for taking the time to review and respond.  With your recommendation to use `method_missing`, I agree there is not a significant need for VM changes as this functionality can be accomplished in pure ruby assuming you know the details in how method lookup, visibility, and `method_missing` work together.

----------------------------------------
Feature #14056: Add Module#deprecate_public for deprecation of public methods
https://bugs.ruby-lang.org/issues/14056#change-67600

* Author: jeremyevans0 (Jeremy Evans)
* Status: Open
* Priority: Normal
* Assignee: matz (Yukihiro Matsumoto)
* Target version: 
----------------------------------------
The attached patch allows you to make a method private, but have
calling it publicly (either directly or via public_send) emit a
warning instead of raising a NoMethodError.

This is mostly useful in library development, where you want to
continue using a method internally, and you want to disallow external
use, but you want existing external users who are using the method to
receive a warning during a deprecation period instead of breaking their
code immediately.

I believe this feature is not possible without modifying the VM
(as the patch does).  You can emit a deprecation message inside the
method, but I don't believe you can make it conditional on whether the
method was called publicly or privately, as that information isn't exposed.

Use is similar to public/protected/private when called with arguments:

~~~ruby
  class MyClass
    def meth
      1
    end
    deprecate_public :meth
  end

  MyClass.new.meth
  # warning: calling private method meth on #<MyClass:0x00001c896dfb1a90> \
  #   via deprecated public interface
  # => 1
~~~

Module#deprecate_public makes the method private, but sets a flag on
the method entry that it is deprecated, and if calling the method would
usually raise a NoMethodError due to visibility, instead a warning is
printed and then method call works as if it were declared public.

There are some issues with this implementation of deprecate_public:

1) It doesn't handle scope visibility, so the following
   does not work like public/protected/private:

~~~ruby
  class MyClass
    deprecate_public

    def meth
      1
    end
  end
~~~

   Currently, this deprecate_public call would do nothing
   as no arguments were given.  It's probably possible to
   handle scope visibility as well, but it would require
   additional internal changes.

2) It is rather inefficient, as it first exports the method
   in the module as public and then private, before setting
   the deprecation flag.  However, this method is not likely
   to be a bottleneck in any reasonable code.  It was done this
   way to reuse the most existing code and still ensure that
   methods will be setup in the class itself and that method
   caches will be cleared appropriately.

3) When public_send is used, this does not print the receiver
   of the public_send method, instead it prints the module
   that used the deprecate_public call.  I'm not sure whether
   the calling information is available from rb_method_call_status,
   or how to access it if it is available.

4) This doesn't currently handle protected methods, but I
   think changing it to do so isn't too difficult.

5) The method name isn't great, hopefully someone can think of
   a better one that isn't much longer.

---Files--------------------------------
0001-Add-Module-deprecate_public-for-deprecation-of-publi.patch (11.1 KB)


-- 
https://bugs.ruby-lang.org/

Unsubscribe: <mailto:[e-mail]?subject=unsubscribe>
<http://lists.ruby-lang.org/cgi-bin/mailman/options/ruby-core>