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

Share variable inspection logic between CDP and DAP #1001

Open
wants to merge 7 commits 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
52 changes: 52 additions & 0 deletions lib/debug/limited_pp.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
# frozen_string_literal: true

require "pp"

module DEBUGGER__
class LimitedPP
SHORT_INSPECT_LENGTH = 40

def self.pp(obj, max = 80)
out = self.new(max)
catch out do
::PP.singleline_pp(obj, out)
end
out.buf
end

attr_reader :buf

def initialize max
@max = max
@cnt = 0
@buf = String.new
end

def <<(other)
@buf << other

if @buf.size >= @max
@buf = @buf[0..@max] + '...'
throw self
end
end

def self.safe_inspect obj, max_length: SHORT_INSPECT_LENGTH, short: false
if short
LimitedPP.pp(obj, max_length)
else
obj.inspect
end
rescue NoMethodError => e
klass, oid = M_CLASS.bind_call(obj), M_OBJECT_ID.bind_call(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)
"<\##{klass.name}:#{roid} contains <\##{rklass}:#{roid} and it does not have #inspect>"
end
rescue Exception => e
"<#inspect raises #{e.inspect}>"
end
end
end
58 changes: 22 additions & 36 deletions lib/debug/server_cdp.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
require 'tmpdir'
require 'tempfile'
require 'timeout'
require_relative 'variable_inspector'

module DEBUGGER__
module UI_CDP
Expand Down Expand Up @@ -1112,46 +1113,31 @@ def process_cdp args
event! :protocol_result, :scope, req, vars
when :properties
oid = args.shift
result = []
prop = []

if obj = @obj_map[oid]
case obj
when Array
result = obj.map.with_index{|o, i|
variable i.to_s, o
}
when Hash
result = obj.map{|k, v|
variable(k, v)
}
when Struct
result = obj.members.map{|m|
variable(m, obj[m])
}
when String
prop = [
internalProperty('#length', obj.length),
internalProperty('#encoding', obj.encoding)
]
when Class, Module
result = obj.instance_variables.map{|iv|
variable(iv, obj.instance_variable_get(iv))
}
prop = [internalProperty('%ancestors', obj.ancestors[1..])]
when Range
prop = [
internalProperty('#begin', obj.begin),
internalProperty('#end', obj.end),
]
if @obj_map.key?(oid)
Copy link
Member

Choose a reason for hiding this comment

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

Good catch. I missed the edge case for falsy value.

obj = @obj_map[oid]

members = if Array === obj
VariableInspector.new.indexed_members_of(obj, start: 0, count: obj.size)
else
VariableInspector.new.named_members_of(obj)
end

result += M_INSTANCE_VARIABLES.bind_call(obj).map{|iv|
variable(iv, M_INSTANCE_VARIABLE_GET.bind_call(obj, iv))
}
prop += [internalProperty('#class', M_CLASS.bind_call(obj))]
Copy link
Author

Choose a reason for hiding this comment

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

For Chrome, #class is appended as the last prop, whereas for VSCode it was unshifted to the top.

result = members.filter_map do |member|
next if member.internal?
variable(member.name, member.value)
end

internal_properties = members.filter_map do |member|
next unless member.internal?
internalProperty(member.name, member.value)
end
else
result = []
internal_properties = []
end
event! :protocol_result, :properties, req, result: result, internalProperties: prop

event! :protocol_result, :properties, req, result: result, internalProperties: internal_properties
when :exception
oid = args.shift
exc = nil
Expand Down
137 changes: 49 additions & 88 deletions lib/debug/server_dap.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
require 'irb/completion'
require 'tmpdir'
require 'fileutils'
require_relative 'variable_inspector'

module DEBUGGER__
module UI_DAP
Expand Down Expand Up @@ -765,18 +766,11 @@ def register_vars vars, tid
end
end

class NaiveString
attr_reader :str
def initialize str
@str = str
end
end

class ThreadClient
MAX_LENGTH = 180

def value_inspect obj, short: true
# TODO: max length should be configuarable?
# TODO: max length should be configurable?
str = DEBUGGER__.safe_inspect obj, short: short, max_length: MAX_LENGTH

if str.encoding == Encoding::UTF_8
Expand Down Expand Up @@ -855,70 +849,40 @@ def process_dap args
presentationHint: 'locals',
# variablesReference: N, # filled by SESSION
namedVariables: lnum,
indexedVariables: 0,
expensive: false,
}, {
name: 'Global variables',
presentationHint: 'globals',
variablesReference: 1, # GLOBAL
namedVariables: safe_global_variables.size,
indexedVariables: 0,
expensive: false,
}]
when :scope
fid = args.shift
frame = get_frame(fid)
vars = collect_locals(frame).map do |var, val|
variable(var, val)
render_variable Variable.new(name: var, value: val)
end

event! :protocol_result, :scope, req, variables: vars, tid: self.id
when :variable
vid = args.shift
obj = @var_map[vid]
if obj
Comment on lines -878 to -879
Copy link
Author

Choose a reason for hiding this comment

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

This had a small bug, where true would have its #class expanded, but not false or nil:

Screenshot 2023-07-28 at 8 28 00 AM

Notice that false and nil still have disclosure triangles. This is because of another bug, in variable_ (lines 1001-1002), which assumed that all objects would have at least 1 named member (#class), which wasn't the case here.

The new check fixes it:

Screenshot 2023-07-28 at 8 27 21 AM

Copy link
Author

@amomchilov amomchilov Jan 4, 2024

Choose a reason for hiding this comment

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

... this change allocates one more Member than before:

#<Member name="#class" value=NilClass internal>

Which bumps some of the variablesReference numbers in test/protocol/step_back_raw_dap_test.rb by 1.

See 63517f5

case req.dig('arguments', 'filter')
when 'indexed'
start = req.dig('arguments', 'start') || 0
count = req.dig('arguments', 'count') || obj.size
vars = (start ... (start + count)).map{|i|
variable(i.to_s, obj[i])
}
else
vars = []

case obj
when Hash
vars = obj.map{|k, v|
variable(value_inspect(k), v,)
}
when Struct
vars = obj.members.map{|m|
variable(m, obj[m])
}
when String
vars = [
variable('#length', obj.length),
variable('#encoding', obj.encoding),
]
printed_str = value_inspect(obj)
vars << variable('#dump', NaiveString.new(obj)) if printed_str.end_with?('...')
when Class, Module
vars << variable('%ancestors', obj.ancestors[1..])
when Range
vars = [
variable('#begin', obj.begin),
variable('#end', obj.end),
]
end
if @var_map.key?(vid)
obj = @var_map[vid]

unless NaiveString === obj
vars += M_INSTANCE_VARIABLES.bind_call(obj).sort.map{|iv|
variable(iv, M_INSTANCE_VARIABLE_GET.bind_call(obj, iv))
}
vars.unshift variable('#class', M_CLASS.bind_call(obj))
end
members = case req.dig('arguments', 'filter')
when 'indexed'
VariableInspector.new.indexed_members_of(
obj,
start: req.dig('arguments', 'start') || 0,
count: req.dig('arguments', 'count') || obj.size,
)
else
VariableInspector.new.named_members_of(obj)
end

vars = members.map { |member| render_variable member }
end
event! :protocol_result, :variable, req, variables: (vars || []), tid: self.id

Expand Down Expand Up @@ -975,7 +939,13 @@ def process_dap args
result = 'Error: Can not evaluate on this frame'
end

event! :protocol_result, :evaluate, req, message: message, tid: self.id, **evaluate_result(result)
result_variable = Variable.new(name: nil, value: result)

event! :protocol_result, :evaluate, req,
message: message,
tid: self.id,
result: result_variable.inspect_value,
**render_variable(result_variable)
Comment on lines +947 to +948
Copy link
Author

Choose a reason for hiding this comment

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

This result: value used to come from the variable_ method when name was nil. That was pretty tricky, so I moved it here


when :completions
fid, text = args
Expand Down Expand Up @@ -1051,58 +1021,49 @@ def type_name obj
end
end

def variable_ name, obj, indexedVariables: 0, namedVariables: 0
if indexedVariables > 0 || namedVariables > 0
vid = @var_map.size + 1
@var_map[vid] = obj
# Renders the given Member into a DAP Variable
# https://microsoft.github.io/debug-adapter-protocol/specification#variable
def render_variable member
indexedVariables, namedVariables = if Array === member.value
[member.value.size, 0]
else
vid = 0
[0, VariableInspector.new.named_members_of(member.value).count]
end

namedVariables += M_INSTANCE_VARIABLES.bind_call(obj).size

if NaiveString === obj
str = obj.str.dump
vid = indexedVariables = namedVariables = 0
# > If `variablesReference` is > 0, the variable is structured and its children
# > can be retrieved by passing `variablesReference` to the `variables` request
# > as long as execution remains suspended.
if indexedVariables > 0 || namedVariables > 0
# This object has children that we might need to query, so we need to remember it by its vid
vid = @var_map.size + 1
@var_map[vid] = member.value
else
str = value_inspect(obj)
# This object has no children, so we don't need to remember it in the `@var_map`
vid = 0
end

if name
Copy link
Author

Choose a reason for hiding this comment

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

This if name check is really checking if this is going to be used in a VariablesResponse (with a name:). If name is nil, then this was called from #evaluate_result for a EvaluateResponse (with a result:).

I moved this logic to the evaluate code path instead.

{ name: name,
value: str,
type: type_name(obj),
variable = if member.name
# These two hashes are repeated so the "name" can come always come first, when available,
# which improves the readability of protocol responses.
{
name: member.name,
value: member.inspect_value,
type: member.value_type_name,
variablesReference: vid,
indexedVariables: indexedVariables,
namedVariables: namedVariables,
}
else
{ result: str,
type: type_name(obj),
{
value: member.inspect_value,
type: member.value_type_name,
variablesReference: vid,
indexedVariables: indexedVariables,
namedVariables: namedVariables,
}
end
end

def variable name, obj
case obj
when Array
variable_ name, obj, indexedVariables: obj.size
when Hash
variable_ name, obj, namedVariables: obj.size
when String
variable_ name, obj, namedVariables: 3 # #length, #encoding, #to_str
Comment on lines -1095 to -1096
Copy link
Author

Choose a reason for hiding this comment

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

This separation between the code that answers "what are the members?" and "how many members are there?" made it quite difficult to make changes to the presentation objects correctly.

Now both have a single source of truth, in VariableInspector's #indexed_members_of and #named_members_of.

when Struct
variable_ name, obj, namedVariables: obj.size
when Class, Module
variable_ name, obj, namedVariables: 1 # %ancestors (#ancestors without self)
when Range
variable_ name, obj, namedVariables: 2 # #begin, #end
else
variable_ name, obj, namedVariables: 1 # #class
end
variable
end
end
end
Loading
Loading