`Monads::Do` strips visibility modifiers from methods

Support
#1

I first raised this as an issue over on Github, before I read the rules. Oops, sorry :grimacing:

In short, the way Do wraps methods replaces whatever modifier they had with whatever the default is (probably public).

As you can see from this example, I can call a previously-private method directly, without resorting to send chicanery.

1 Like
#2

I did some digging, and have been able to fix this behaviour in Do::All. In that context, we can use Module.private_instance_methods & Module.protected_instance_methods to determine what visibility each method has, and explicitly provide that visibility to the wrapped methods.

Like this: (from all.rb)

        # @api private
        module InstanceMixin
          # @api private
          def extended(object)
            super

            wrapper = ::Module.new
            object.singleton_class.prepend(wrapper)
            object.define_singleton_method(:singleton_method_added) do |method|
              super(method)

              next if method.equal?(:singleton_method_added)

              visibility = if private_method_defined?(method, true)
                             :private
                           elsif protected_method_defined?(method, true)
                             :protected
                           else
                             :public
                           end

              Do.wrap_method(wrapper, method, visibility)
            end

            object.singleton_class.public_instance_methods(false).each do |m|
              Do.wrap_method(wrapper, m, :public)
            end

            object.singleton_class.protected_instance_methods(false).each do |m|
              Do.wrap_method(wrapper, m, :protected)
            end

            object.singleton_class.private_instance_methods(false).each do |m|
              Do.wrap_method(wrapper, m, :private)
            end
          end
        end
#3

Do.for has an additional challenge: for it to work properly, you need to specify which methods to wrap ahead of time - and that means we donā€™t know what visibility they will have once theyā€™re actually provided.

The only way I can think of to resolve that dilemma is to extend the API for Do.for to accept both a list of methods (that will be treated as public, per current behaviour), and keywords for explicit visibility:

        # do.rb - L86
        def for(*public_methods, **methods_with_visibility)
          mod = ::Module.new do
            public_methods.each do |method_name|              
              Do.wrap_method(self, method_name, nil)
            end

            methods_with_visibility.each do |visibility, methods|
              methods.each do |method_name|
                Do.wrap_method(self, method_name, visibility)
              end
            end
          end

          ::Module.new do
            singleton_class.send(:define_method, :included) do |base|
              base.prepend(mod)
            end
          end
        end

Usage:

class Example
  include Dry::Monads[:result]
  include Dry::Monads::Do.for(:call, :internal_method) # [1] current behaviour
  include Dry::Monads::Do.for(:call, private: [:internal_method]) # [2] proposed API

  def call(value)
    result = yield internal_method(value)
  end

  private
  def internal_method(v)
    i = yield interstitial_result.to_monad
    Success(v + i)
  end

  def interstitial_result
    Success(100)
  end
end

e = Example.new
e.call(1) # => Success(101)

# when [1]:
e.internal_method # => Success(101)

# when [2]:
e.internal_method # => NoMethodError (private method `internal_method' called for #<Example:0xdeadbeef>
#4

Thanks for investigating this!

I think the intention behind this API was that youā€™d only decorate one public method. As you noticed, fixing it would be a challenge and I honestly donā€™t think itā€™s worth the effort. OTOH it would probably be relatively simple to rely on method_added hook and decorate methods there (which would allow you to inspect visibility and act accordingly).

#5

Iā€™ll put what Iā€™ve done up as a PR and you can decide whether itā€™s worth a merge. If we decide not to try and fix Do.for, I think itā€™s worth putting an explanation in the docs to highlight this surprising behaviour. WDYT?

1 Like
#6

All of these visibility problems should be fixed, Iā€™ll take a closer look soon. Personally, I donā€™t pay attention to visibility since most of my classes have only one ā€œpublicā€, namely call.

1 Like
#7

:+1: for fixing this with Do::All

While I usually only have one public call method, I use do notation a lot to clean up private methods too, and I donā€™t want that to make them public.

2 Likes