-
Notifications
You must be signed in to change notification settings - Fork 127
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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) | ||||||||||||||||||||||||||
M_INSTANCE_VARIABLES.bind_call(o) | ||||||||||||||||||||||||||
end | ||||||||||||||||||||||||||
Comment on lines
+5
to
+7
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 This is convenient for the implementation of the Although it's more tedious, perhaps it's a better idea to conditionally define one of two implementations of these helpers, rather than polluting
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure we should support pre-2.7 though... There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interestingly,
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 if M_RESPOND_TO_P.bind_call(b.receiver, expr, include_all: false) Since the 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 | ||
|
@@ -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 | ||
|
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This causes a fair bit of noise in the test output:
Perhaps it would be better to just wrap the |
||
|
||
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The buggy patch would cause this to be |
||
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 |
There was a problem hiding this comment.
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
?There was a problem hiding this comment.
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?