Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect #bind_call monkey patch #1002

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 72 additions & 0 deletions lib/debug/reflection.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
# frozen_string_literal: true

module DEBUGGER__
module Reflection
module_function def instance_variables_of(o)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you use module_function?

Copy link
Author

@amomchilov amomchilov Oct 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

M_INSTANCE_VARIABLES.bind_call(o)
end
Comment on lines +5 to +7
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

module_function def class_of(o)
M_CLASS.bind_call(o)
end

module_function def singleton_class_of(o)
M_SINGLETON_CLASS.bind_call(o)
end

module_function def is_kind_of?(object, type)
M_KIND_OF_P.bind_call(object, type)
end

module_function def responds_to?(object, message, include_all: false)
M_RESPOND_TO_P.bind_call(object, message, include_all)
end

module_function def method_of(type, method_name)
M_METHOD.bind_call(type, method_name)
end

module_function def object_id_of(o)
M_OBJECT_ID.bind_call(o)
end

module_function def name_of(type)
M_NAME.bind_call(type)
end

M_INSTANCE_VARIABLES = Kernel.instance_method(:instance_variables)
M_INSTANCE_VARIABLE_GET = Kernel.instance_method(:instance_variable_get)
M_CLASS = Kernel.instance_method(:class)
M_SINGLETON_CLASS = Kernel.instance_method(:singleton_class)
M_KIND_OF_P = Kernel.instance_method(:kind_of?)
M_RESPOND_TO_P = Kernel.instance_method(:respond_to?)
M_METHOD = Kernel.instance_method(:method)
M_OBJECT_ID = Kernel.instance_method(:object_id)
M_NAME = Module.instance_method(:name)

private_constant(
:M_INSTANCE_VARIABLES,
:M_INSTANCE_VARIABLE_GET,
:M_CLASS,
:M_SINGLETON_CLASS,
:M_KIND_OF_P,
:M_RESPOND_TO_P,
:M_METHOD,
:M_OBJECT_ID,
:M_NAME,
)
end
end

# 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
6 changes: 3 additions & 3 deletions lib/debug/server_cdp.rb
Original file line number Diff line number Diff line change
Expand Up @@ -1059,8 +1059,8 @@ def process_cdp args
result = b.local_variable_get(expr)
rescue NameError
# try to check method
if M_RESPOND_TO_P.bind_call(b.receiver, expr, include_all: true)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

result = M_METHOD.bind_call(b.receiver, expr)
if Reflection.responds_to?(b.receiver, expr, include_all: true)
result = Reflection.method_of(b.receiver, expr)
else
message = "Error: Can not evaluate: #{expr.inspect}"
end
Expand Down Expand Up @@ -1246,7 +1246,7 @@ def propertyDescriptor_ name, obj, type, description: nil, subtype: nil
v = prop[:value]
v.delete :value
v[:subtype] = subtype if subtype
v[:className] = (klass = M_CLASS.bind_call(obj)).name || klass.to_s
v[:className] = (klass = Reflection.class_of(obj)).name || klass.to_s
end
prop
end
Expand Down
12 changes: 6 additions & 6 deletions lib/debug/server_dap.rb
Original file line number Diff line number Diff line change
Expand Up @@ -937,7 +937,7 @@ def process_dap args
case expr
when /\A\@\S/
begin
result = M_INSTANCE_VARIABLE_GET.bind_call(b.receiver, expr)
result = Reflection.instance_variable_get_from(b.receiver, expr)
rescue NameError
message = "Error: Not defined instance variable: #{expr.inspect}"
end
Expand All @@ -959,8 +959,8 @@ def process_dap args
result = b.local_variable_get(expr)
rescue NameError
# try to check method
if M_RESPOND_TO_P.bind_call(b.receiver, expr, include_all: true)
result = M_METHOD.bind_call(b.receiver, expr)
if Reflection.responds_to?(b.receiver, expr, include_all: true)
result = Reflection.method_of(b.receiver, expr)
else
message = "Error: Can not evaluate: #{expr.inspect}"
end
Expand Down Expand Up @@ -1040,10 +1040,10 @@ def evaluate_result r
end

def type_name obj
klass = M_CLASS.bind_call(obj)
klass = Reflection.class_of(obj)

begin
M_NAME.bind_call(klass) || klass.to_s
Reflection.name_of(klass) || klass.to_s
rescue Exception => e
"<Error: #{e.message} (#{e.backtrace.first}>"
end
Expand All @@ -1057,7 +1057,7 @@ def variable_ name, obj, indexedVariables: 0, namedVariables: 0
vid = 0
end

namedVariables += M_INSTANCE_VARIABLES.bind_call(obj).size
namedVariables += Reflection.instance_variables_of(obj).size

if NaiveString === obj
str = obj.str.dump
Expand Down
13 changes: 2 additions & 11 deletions lib/debug/session.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2340,11 +2340,11 @@ def self.safe_inspect obj, max_length: SHORT_INSPECT_LENGTH, short: false
obj.inspect
end
rescue NoMethodError => e
klass, oid = M_CLASS.bind_call(obj), M_OBJECT_ID.bind_call(obj)
klass, oid = Reflection.class_of(obj), Reflection.object_id_of(obj)
if obj == (r = e.receiver)
"<\##{klass.name}#{oid} does not have \#inspect>"
else
rklass, roid = M_CLASS.bind_call(r), M_OBJECT_ID.bind_call(r)
rklass, roid = Reflection.class_of(r), Reflection.object_id_of(r)
"<\##{klass.name}:#{roid} contains <\##{rklass}:#{roid} and it does not have #inspect>"
end
rescue Exception => e
Expand Down Expand Up @@ -2623,12 +2623,3 @@ class Binding
alias break debugger
alias b debugger
end

# for Ruby 2.6 compatibility
unless method(:p).unbind.respond_to? :bind_call
class UnboundMethod
def bind_call(obj, *args)
self.bind(obj).call(*args)
end
end
end
30 changes: 10 additions & 20 deletions lib/debug/thread_client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,6 @@
require_relative 'color'

module DEBUGGER__
M_INSTANCE_VARIABLES = method(:instance_variables).unbind
M_INSTANCE_VARIABLE_GET = method(:instance_variable_get).unbind
M_CLASS = method(:class).unbind
M_SINGLETON_CLASS = method(:singleton_class).unbind
M_KIND_OF_P = method(:kind_of?).unbind
M_RESPOND_TO_P = method(:respond_to?).unbind
M_METHOD = method(:method).unbind
M_OBJECT_ID = method(:object_id).unbind
M_NAME = method(:name).unbind

module SkipPathHelper
def skip_path?(path)
!path ||
Expand Down Expand Up @@ -590,8 +580,8 @@ def show_ivars pat, expr = nil
end

if _self
M_INSTANCE_VARIABLES.bind_call(_self).sort.each{|iv|
value = M_INSTANCE_VARIABLE_GET.bind_call(_self, iv)
Reflection.instance_variables_of(_self).sort.each{|iv|
value = Reflection.instance_variable_get_from(_self, iv)
puts_variable_info iv, value, pat
}
end
Expand All @@ -617,7 +607,7 @@ def get_consts expr = nil, only_self: false, &block
rescue Exception => e
# ignore
else
if M_KIND_OF_P.bind_call(_self, Module)
if Reflection.is_kind_of(_self, Module)
iter_consts _self, &block
return
else
Expand All @@ -626,10 +616,10 @@ def get_consts expr = nil, only_self: false, &block
end
elsif _self = current_frame&.self
cs = {}
if M_KIND_OF_P.bind_call(_self, Module)
if Reflection.is_kind_of(_self, Module)
cs[_self] = :self
else
_self = M_CLASS.bind_call(_self)
_self = Reflection.class_of(_self)
cs[_self] = :self unless only_self
end

Expand Down Expand Up @@ -769,20 +759,20 @@ def show_outline expr

locals = current_frame&.local_variables

klass = M_CLASS.bind_call(obj)
klass = Reflection.class_of(obj)
klass = obj if Class == klass || Module == klass

o.dump("constants", obj.constants) if M_RESPOND_TO_P.bind_call(obj, :constants)
o.dump("constants", obj.constants) if Reflection.responds_to?(obj, :constants)
outline_method(o, klass, obj)
o.dump("instance variables", M_INSTANCE_VARIABLES.bind_call(obj))
o.dump("instance variables", Reflection.instance_variables_of(obj))
o.dump("class variables", klass.class_variables)
o.dump("locals", locals.keys) if locals
end
end

def outline_method(o, klass, obj)
begin
singleton_class = M_SINGLETON_CLASS.bind_call(obj)
singleton_class = Reflection.singleton_class_of(obj)
rescue TypeError
singleton_class = nil
end
Expand Down Expand Up @@ -1192,7 +1182,7 @@ def wait_next_action_
obj_inspect = truncate(obj_inspect, width: width)
end

event! :result, :trace_pass, M_OBJECT_ID.bind_call(obj), obj_inspect, opt
event! :result, :trace_pass, Reflection.object_id_of(obj), obj_inspect, opt
rescue => e
puts e.message
event! :result, nil
Expand Down
2 changes: 1 addition & 1 deletion lib/debug/tracer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ def setup
@tracer = TracePoint.new(:a_call){|tp|
next if skip?(tp)

if M_OBJECT_ID.bind_call(tp.self) == @obj_id
if Reflection.object_id_of(tp.self) == @obj_id
klass = tp.defined_class
method = tp.method_id
method_info =
Expand Down
117 changes: 117 additions & 0 deletions test/debug/reflection_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
# frozen_string_literal: true

require 'test/unit'
require_relative '../../lib/debug/reflection'

module DEBUGGER__
class ReflectionTest < Test::Unit::TestCase
def setup
@sample_object = SampleClass.new(1, 2)
end

def test_instance_variables_of
assert_equal [:@a, :@b], Reflection.instance_variables_of(@sample_object)
end

def test_instance_variables_get
assert_equal 1, Reflection.instance_variable_get_from(@sample_object, :@a)
assert_equal 2, Reflection.instance_variable_get_from(@sample_object, :@b)
end

def test_class_of
assert_same SampleClass, Reflection.class_of(@sample_object)
end

def test_singleton_class_of
expected = class << SampleClass
self
end

assert_same expected, Reflection.singleton_class_of(SampleClass)
end

def test_is_kind_of?()
assert_true Reflection.is_kind_of?(@sample_object, SampleClass)
assert_false Reflection.is_kind_of?(@sample_object, Object)
end

def test_responds_to?
assert_true Reflection.responds_to?(@sample_object, :a)
assert_false Reflection.responds_to?(@sample_object, :doesnt_exist)

assert_false Reflection.responds_to?(@sample_object, :sample_private_method)
assert_false Reflection.responds_to?(@sample_object, :sample_private_method, include_all: false)
assert_true Reflection.responds_to?(@sample_object, :sample_private_method, include_all: true)
end

def test_method_of
assert_equal 1, Reflection.method_of(@sample_object, :a).call
end

def test_object_id_of
assert_equal @sample_object.__id__, Reflection.object_id_of(@sample_object)
end

def test_name_of
assert_equal "DEBUGGER__::ReflectionTest::SampleClass", Reflection.name_of(SampleClass)
end

def test_bind_call_backport
omit_if(
UnboundMethod.instance_method(:bind_call).source_location.nil?,
"This Ruby version (#{RUBY_VERSION}) has a native #bind_call implementation, so it doesn't need the backport.",
)
Comment on lines +60 to +63
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


puts caller_locations
original_object = SampleTarget.new("original")
new_target = SampleTarget.new("new")

m = original_object.method(:sample_method).unbind

rest_args = ["a1", "a2"]
kwargs = { k1: 1, k2: 2 }
proc = Proc.new { |x| x }

result = m.bind_call(new_target, "parg1", "parg2", *rest_args, **kwargs, &proc)

assert_equal "new", result.fetch(:self).id
assert_equal "parg1", result.fetch(:parg1)
assert_equal "parg2", result.fetch(:parg2)
assert_equal rest_args, result.fetch(:rest_args)
assert_equal kwargs, result.fetch(:kwargs)
assert_same proc, result.fetch(:block)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end

private

# A class for testing reflection, which doesn't implement all the usual reflection methods being tested.
class SampleClass < BasicObject
attr_reader :a, :b

def initialize(a, b)
@a = a
@b = b
end

private

def sample_private_method; end

class << self
undef_method :method
end
end

class SampleTarget
attr_reader :id

def initialize(id)
@id = id
end

def sample_method(parg1, parg2, *rest_args, **kwargs, &block)
{ self: self, parg1: parg1, parg2: parg2, rest_args: rest_args, kwargs: kwargs, block: block }
end
end
end
end