Fix incorrect #bind_call monkey patch

[amomchilov]

Description

Ruby/debug monkey patches in an implementation of UnboundMethod#bind_call for compatibility with Rubies older than 2.7. Unfortunately, there is a bug in the implementation, in that it doesn't pass the block parameter correctly. Here's a demo:

$ chruby 2.6.10 && ruby ./bind_call_bug_demo.rb
Backporting UnboundMethod#bind_call in Ruby 2.6.10.

Incorrect result from buggy bind_call: nil
  Correct result from fixed bind_call: #<Proc:0x00000001100df230@./bind_call_bug_demo.rb:12>
                        Expected proc: #<Proc:0x00000001100df230@./bind_call_bug_demo.rb:12>

$ chruby 3.2.2 && ruby ./bind_call_bug_demo.rb
Ruby 3.2.2 already has UnboundMethod#bind_call natively defined.

Correct result from native bind_call: #<Proc:0x00000001055ab5f8 ./bind_call_bug_demo.rb:12>
                       Expected proc: #<Proc:0x00000001055ab5f8 ./bind_call_bug_demo.rb:12>

bind_call_bug_demo.rb

require "test/unit"

class SampleTarget < Struct.new(:id)
  def pass_block_through(&block)
    block
  end
end

target_object = SampleTarget.new

m = SampleTarget.instance_method(:pass_block_through)
proc = Proc.new { |x| x }


if UnboundMethod.method_defined?(:bind_call)
  puts "Ruby #{RUBY_VERSION} already has UnboundMethod#bind_call natively defined.\n\n"

  puts "Correct result from native bind_call: #{m.bind_call(target_object, &proc).inspect}" # => #<Proc:...>
  puts "                       Expected proc: #{proc}"  # => #<Proc:...>
else
  puts "Backporting UnboundMethod#bind_call in Ruby #{RUBY_VERSION}.\n\n"

  class UnboundMethod
    def bind_call(receiver, *args)
      bind(receiver).call(*args)
    end

    def bind_call_fixed(receiver, *args, &block)
      bind(receiver).call(*args, &block)
    end
  end

  puts "Incorrect result from buggy bind_call: #{m.bind_call(target_object, &proc).inspect}" # => nil
  puts "  Correct result from fixed bind_call: #{m.bind_call_fixed(target_object, &proc).inspect}" # => #<Proc:...>
  puts "                        Expected proc: #{proc}"  # => #<Proc:...>
end

This repo introduces a corrected patch, and with a test that confirms it:

# for Ruby 2.6 compatibility
unless UnboundMethod.method_defined?(:bind_call)
  class UnboundMethod
    def bind_call(receiver, *args, &block)
      bind(receiver).call(*args, &block)
    end
  end
end

I also introduces a new Reflection module which extracts all the bind_call-ed reflection methods, so they're more readable. It also adds tests for them.

[amomchilov]

Connecting the debugger to a Ruby version pre-2.7 will have the effect of back-porting the bind_call method, which would otherwise exist.

This is convenient for the implementation of the Ruby/debug itself, but perhaps it's not a good idea, as developers' code can call bind_call successfully, but only when they're debugging, which could be quite confusing.

Although it's more tedious, perhaps it's a better idea to conditionally define one of two implementations of these helpers, rather than polluting UnboundMethod? E.g.

Suggested change

	module_function def instance_variables_of(o)
	M_INSTANCE_VARIABLES.bind_call(o)
	end
	if UnboundMethod.method_defined?(:bind_call)
	module_function def instance_variables_of(o)
	M_INSTANCE_VARIABLES.bind_call(o)
	end
	else
	module_function def instance_variables_of(o)
	M_INSTANCE_VARIABLES.bind(o).call
	end
	end

[ko1]

I'm not sure we should support pre-2.7 though...

[amomchilov]

If we don't want to support pre-2.7, then we should remove the monkey patch entirely.

I don't personally use anything pre-3.2, but it feels too soon to drop 2.7. In any case, the current implementation of it is incorrect, and we should not leave it as-is.

[amomchilov]

Interestingly, respond_to?'s include_all argument is positional, not keyword (notice =, not : in the docs):

respond_to?(symbol, include_all=false) → true or false

The way this is called causes an implicit Hash to be passed positionally, as if it was:

if M_RESPOND_TO_P.bind_call(b.receiver, expr, { include_all: true })

This would cause include_all's value to be literally the Hash { include_all: true }. It's truthy, so it behaves equivalent to true, so it happens to be accidentally correct. This wouldn't work though:

if M_RESPOND_TO_P.bind_call(b.receiver, expr, include_all: false)

Since the { include_all: false } Hash would still be truthy.

I made this a kwarg in the Reflection helper, so it's more clear.

[amomchilov]

This causes a fair bit of noise in the test output:

=================================================================================================================================================
Omission: This Ruby version (3.2.2) has a native #bind_call implementation, so it doesn't need the backport. [test_bind_call_backport(DEBUGGER__::ReflectionTest)]
./test/debug/reflection_test.rb:60:in `test_bind_call_backport'
=================================================================================================================================================
Finished in 0.001056 seconds.
-------------------------------------------------------------------------------------------------------------------------------------------------
10 tests, 15 assertions, 0 failures, 0 errors, 0 pendings, 1 omissions, 0 notifications

Perhaps it would be better to just wrap the def test_bind_call_backport in a big if statement?

[amomchilov]

The buggy patch would cause this to be nil, but it now works correctly.

[ko1]

The name "Reflection" is not clear for me. Reflection is important programming technique and debugger should support it especially on Ruby. However, this module doesn't represent this kind of general idea.
Could you make another word?

[amomchilov]

@ko1 I think Reflection is exactly the right term. Have a look at the Wikiepdia page:

• Discover and modify source-code constructions (such as code blocks, classes, methods, protocols, etc.) as first-class objects at runtime.
• Convert a string matching the symbolic name of a class or function into a reference to or invocation of that class or function.

[ko1]

Yes, this module uses "Reflection" feature, but this module doesn't represent the feature of "Reflection".

[amomchilov]

@ko1 Sorry, I don't think I understand.

I suppose ReflectionHelpers/ReflectionUtils would be more precise names, but *Helpers names are smelly. It reads better at the callsite, e.g. the standard library gives you Math.sin not MathHelpers.sin.

This naming also what Tapioca calls it.

If you would like an alternative name, could you suggest one? I don't have any better ideas.

[ko1]

why do you use module_function?

[amomchilov]

If the module were included, it could override kernel methods like #responds_to?, which wouldn't be desirable. So these methods should be called directly on the module.

Would you prefer I use include self instead?

[ko1]

If you would like an alternative name, could you suggest one? I don't have any better ideas.

This module provides a feature to call essential Ruby methods so

• EssentialRubyFeature
• PrimitiveRubyFeature
• OriginalRubyFeature

and so on...?

[amomchilov]

Personally, I think those are too generic.

___RubyFeature raises the question: "Which Ruby features are essential, primitive or original?"

The answer is: "the reflection-related features"

I really do think Reflection is the exact right term for this, but it's more important to me that this lands in any form. Please feel free to use whichever you find best.

[ko1]

___RubyFeature raises the question: "Which Ruby features are essential, primitive or original?"
The answer is: "the reflection-related features"

I understand the misunderstanding with us.

• You understand these methods are "reflection related features"
• I understand these methods are guaranteed to be original methods.

Original methods mean that they are guaranteed to provide original feature even if they are re-defined by other classes.

For example, if we get trouble with Array#each redefinition, I think Array.instance_method(:each) should be in this module. But maybe you don't think so.

Hmm.

[amomchilov]

Hi Koichi! You're correct, I think this is exactly the misunderstanding.

For example, if we get trouble with Array#each redefinition, I think Array.instance_method(:each) should be in this module. But maybe you don't think so.

Correct! I was thinking of it more by purpose ("reflection related features") than implementation detail ("it's an original Ruby method, ignoring redefinition"). If I wanted the original Array#each, I would put it in some new array-specific module which better represents some relevant high-level idea (e.g. “Enumeration”)

I'm okay with either I approach. Please feel free to merge whichever you prefer.

[ko1]

I also misunderstand that you introduced different naming like

    module_function def instance_variable_get_from(o, name)
      M_INSTANCE_VARIABLE_GET.bind_call(o, name)
    end

(not instance_variable_get).

[ko1]

OK, How about ObjectIntrospector?

@mametter told me that
https://stackoverflow.com/questions/25198271/what-is-the-difference-between-introspection-and-reflection#answer-41161878

here we need the ability for "Introspection"