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.

Sign up for GitHub

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

Improve variable type name request #957

Merged
merged 3 commits into from
May 4, 2023
Merged

Improve variable type name request #957

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e3c5863
Add test for #790
adisonlampert Mar 29, 2023
670f96a
Improve approach to get type name
adisonlampert Mar 29, 2023
b83389a
Add test for anonymous class instance
adisonlampert Apr 5, 2023
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
17 changes: 5 additions & 12 deletions lib/debug/server_dap.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1010,16 +1010,6 @@ def evaluate_result r
variable nil, r
end

def type_name obj
klass = M_CLASS.bind_call(obj)

begin
klass.name || klass.to_s
rescue Exception => e
Copy link
Collaborator

@ko1 ko1 Apr 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

why does it drop klass.to_s?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe type_name is already a String so I don't think to_s is necessary.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No. klass.name can return nil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh, I see. Thanks for the catch!
I updated the code to use to_s and added a test for anonymous class instances to ensure the type is "".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you really need nil for the name?
Previous one is klass.name || klass.to_s but your code uses klass.name.to_s.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The previous code means that if klass.name is given, use it, otherwise use klass.to_s.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the code so that the type is set to klass.to_s if there is no type name and I updated the related test (test_anonymous_class_instance).

Copy link
Collaborator

Choose a reason for hiding this comment

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

In this case, the error because of to_s is ignored.
I'll fix it after merging.

"<Error: #{e.message} (#{e.backtrace.first}>"
end
end

def variable_ name, obj, indexedVariables: 0, namedVariables: 0
if indexedVariables > 0 || namedVariables > 0
vid = @var_map.size + 1
Expand All @@ -1037,17 +1027,20 @@ def variable_ name, obj, indexedVariables: 0, namedVariables: 0
str = value_inspect(obj)
end

klass = M_CLASS.bind_call(obj)
type_name = M_NAME.bind_call(klass)

if name
{ name: name,
value: str,
type: type_name(obj),
type: type_name || klass.to_s,
variablesReference: vid,
indexedVariables: indexedVariables,
namedVariables: namedVariables,
}
else
{ result: str,
type: type_name(obj),
type: type_name || klass.to_s,
variablesReference: vid,
indexedVariables: indexedVariables,
namedVariables: namedVariables,
Expand Down
1 change: 1 addition & 0 deletions lib/debug/thread_client.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ module DEBUGGER__
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)
Expand Down
73 changes: 73 additions & 0 deletions test/protocol/variables_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,4 +82,77 @@ def test_ordering_instance_variables
end
end
end

class DAPOverwrittenNameMethod < ProtocolTestCase
PROGRAM = <<~RUBY
1| class Foo
2| def self.name(value) end
3| end
4| f = Foo.new
5| __LINE__
RUBY

def test_overwritten_name_method
run_protocol_scenario PROGRAM, cdp: false do
req_add_breakpoint 5
req_continue

locals = gather_variables

variable_info = locals.find { |local| local[:name] == "f" }

assert_match /#<Foo:.*>/, variable_info[:value]
assert_equal "Foo", variable_info[:type]

req_terminate_debuggee
end
end
end

class DAPOverwrittenClassMethod < ProtocolTestCase
PROGRAM = <<~RUBY
1| class Foo
2| def self.class(value) end
3| end
4| f = Foo.new
5| __LINE__
RUBY

def test_overwritten_class_method
run_protocol_scenario PROGRAM, cdp: false do
req_add_breakpoint 5
req_continue

locals = gather_variables

variable_info = locals.find { |local| local[:name] == "f" }
assert_match /#<Foo:.*>/, variable_info[:value]
assert_equal "Foo", variable_info[:type]

req_terminate_debuggee
end
end
end

class DAPAnonymousClassInstance < ProtocolTestCase
PROGRAM = <<~RUBY
1| f = Class.new.new
2| __LINE__
RUBY

def test_anonymous_class_instance
run_protocol_scenario PROGRAM, cdp: false do
req_add_breakpoint 2
req_continue

locals = gather_variables

variable_info = locals.find { |local| local[:name] == "f" }
assert_match /#<Class:.*>/, variable_info[:value]
assert_match /#<Class:.*>/, variable_info[:type]

req_terminate_debuggee
end
end
end
end