From fb05fe31087667adbed83304d804e1198312b278 Mon Sep 17 00:00:00 2001 From: Alexander Momchilov Date: Wed, 26 Jul 2023 18:31:56 -0400 Subject: [PATCH] Share variable inspection logic between CDP and DAP --- lib/debug/limited_pp.rb | 52 +++++++ lib/debug/server_cdp.rb | 55 +++---- lib/debug/server_dap.rb | 153 ++++++------------ lib/debug/session.rb | 50 +----- lib/debug/variable.rb | 56 +++++++ lib/debug/variable_inspector.rb | 88 +++++++++++ test/debug/variable_inspector_test.rb | 215 ++++++++++++++++++++++++++ 7 files changed, 485 insertions(+), 184 deletions(-) create mode 100644 lib/debug/limited_pp.rb create mode 100644 lib/debug/variable.rb create mode 100644 lib/debug/variable_inspector.rb create mode 100644 test/debug/variable_inspector_test.rb diff --git a/lib/debug/limited_pp.rb b/lib/debug/limited_pp.rb new file mode 100644 index 000000000..b2359e40a --- /dev/null +++ b/lib/debug/limited_pp.rb @@ -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 diff --git a/lib/debug/server_cdp.rb b/lib/debug/server_cdp.rb index ef556febd..955af4003 100644 --- a/lib/debug/server_cdp.rb +++ b/lib/debug/server_cdp.rb @@ -9,6 +9,8 @@ require 'tmpdir' require 'tempfile' require 'timeout' +require_relative 'variable' +require_relative 'variable_inspector' module DEBUGGER__ module UI_CDP @@ -1112,46 +1114,29 @@ 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), - ] + 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))] + 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 diff --git a/lib/debug/server_dap.rb b/lib/debug/server_dap.rb index 8dbefda8a..60f7f2a4b 100644 --- a/lib/debug/server_dap.rb +++ b/lib/debug/server_dap.rb @@ -4,6 +4,8 @@ require 'irb/completion' require 'tmpdir' require 'fileutils' +require_relative 'variable' +require_relative 'variable_inspector' module DEBUGGER__ module UI_DAP @@ -765,18 +767,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 @@ -867,56 +862,28 @@ def process_dap args 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 - 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.has_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 @@ -973,7 +940,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) when :completions fid, text = args @@ -1035,72 +1008,48 @@ def search_const b, expr false end - def evaluate_result r - variable nil, r - end - - def type_name obj - klass = M_CLASS.bind_call(obj) - - begin - M_NAME.bind_call(klass) || klass.to_s - rescue Exception => e - "" + # 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 + [0, VariableInspector.new.named_members_of(member.value).count] end - end - def variable_ name, obj, indexedVariables: 0, 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] = obj + @var_map[vid] = member.value else + # This object has no children, so we don't need to remember it in the `@var_map` vid = 0 end - namedVariables += M_INSTANCE_VARIABLES.bind_call(obj).size - - if NaiveString === obj - str = obj.str.dump - vid = indexedVariables = namedVariables = 0 - else - str = value_inspect(obj) - end - - if name - { 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 - 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[:indexedVariables] = indexedVariables unless indexedVariables == 0 + variable[:namedVariables] = namedVariables unless namedVariables == 0 + + variable end end end diff --git a/lib/debug/session.rb b/lib/debug/session.rb index e9d95775c..50c9d8bf4 100644 --- a/lib/debug/session.rb +++ b/lib/debug/session.rb @@ -34,6 +34,7 @@ require_relative 'source_repository' require_relative 'breakpoint' require_relative 'tracer' +require_relative 'limited_pp' # To prevent loading old lib/debug.rb in Ruby 2.6 to 3.0 $LOADED_FEATURES << 'debug.rb' @@ -2302,53 +2303,8 @@ def self.load_rc end end - # Inspector - - SHORT_INSPECT_LENGTH = 40 - - class LimitedPP - 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 - 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}>" + def self.safe_inspect obj, max_length: LimitedPP::SHORT_INSPECT_LENGTH, short: false + LimitedPP.safe_inspect(obj, max_length: max_length, short: short) end def self.warn msg diff --git a/lib/debug/variable.rb b/lib/debug/variable.rb new file mode 100644 index 000000000..b3c812343 --- /dev/null +++ b/lib/debug/variable.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +require_relative 'variable_inspector' + +module DEBUGGER__ + class Variable + attr_reader :name, :value + + def initialize(name:, value:, internal: false) + @name = name + @value = value + @is_internal = internal + end + + def internal? + @is_internal + end + + def self.internal name:, value: + new(name: name, value: value, internal: true) + end + + def inspect_value + @inspect_value ||= if VariableInspector::NaiveString === @value + @value.str.dump + else + VariableInspector.value_inspect(@value) + end + end + + def value_type_name + klass = M_CLASS.bind_call(@value) + + begin + M_NAME.bind_call(klass) || klass.to_s + rescue Exception => e + "" + end + end + + def ==(other) + other.instance_of?(self.class) && + @name == other.name && + @value == other.value && + @is_internal == other.internal? + end + + def inspect + "#" + end + + # TODO: Replace with Reflection helpers once they are merged + # https://github.com/ruby/debug/pull/1002 + M_CLASS = method(:class).unbind + end +end diff --git a/lib/debug/variable_inspector.rb b/lib/debug/variable_inspector.rb new file mode 100644 index 000000000..c0254141b --- /dev/null +++ b/lib/debug/variable_inspector.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +require_relative 'variable' +require_relative 'limited_pp' + +module DEBUGGER__ + class VariableInspector + def indexed_members_of(obj, start:, count:) + return [] if start > (obj.length - 1) + + capped_count = [count, obj.length - start].min + + (start...(start + capped_count)).map do |i| + Variable.new(name: i.to_s, value: obj[i]) + end + end + + def named_members_of(obj) + return [] if NaiveString === obj + + members = case obj + when Hash then obj.map { |k, v| Variable.new(name: value_inspect(k), value: v) } + when Struct then obj.members.map { |name| Variable.new(name: name, value: obj[name]) } + when String + members = [ + Variable.internal(name: '#length', value: obj.length), + Variable.internal(name: '#encoding', value: obj.encoding), + ] + + printed_str = value_inspect(obj) + members << Variable.internal(name: "#dump", value: NaiveString.new(obj)) if printed_str.end_with?('...') + + members + when Class, Module then [Variable.internal(name: "%ancestors", value: obj.ancestors[1..])] + when Range then [ + Variable.internal(name: "#begin", value: obj.begin), + Variable.internal(name: "#end", value: obj.end), + ] + else [] + end + + ivars_members = M_INSTANCE_VARIABLES.bind_call(obj).sort.map do |iv| + Variable.new(name: iv, value: M_INSTANCE_VARIABLE_GET.bind_call(obj, iv)) + end + + members.unshift Variable.internal(name: '#class', value: M_CLASS.bind_call(obj)) + members.concat(ivars_members) + + members + end + + private + + def value_inspect(obj, short: true) + self.class.value_inspect(obj, short: short) + end + + def self.value_inspect(obj, short: true) + # TODO: max length should be configurable? + str = LimitedPP.safe_inspect obj, short: short, max_length: MAX_LENGTH + + if str.encoding == Encoding::UTF_8 + str.scrub + else + str.encode(Encoding::UTF_8, invalid: :replace, undef: :replace) + end + end + + MAX_LENGTH = 180 + + # TODO: Replace with Reflection helpers once they are merged + # https://github.com/ruby/debug/pull/1002 + M_INSTANCE_VARIABLES = method(:instance_variables).unbind + M_INSTANCE_VARIABLE_GET = method(:instance_variable_get).unbind + M_CLASS = method(:class).unbind + + class NaiveString + attr_reader :str + def initialize str + @str = str + end + + def == other + other.instance_of?(self.class) && @str == other.str + end + end + end +end diff --git a/test/debug/variable_inspector_test.rb b/test/debug/variable_inspector_test.rb new file mode 100644 index 000000000..33c833685 --- /dev/null +++ b/test/debug/variable_inspector_test.rb @@ -0,0 +1,215 @@ +# frozen_string_literal: true + +require 'test/unit' +require_relative '../../lib/debug/variable_inspector' + +module DEBUGGER__ + class VariableInspectorTest < Test::Unit::TestCase + def setup + @inspector = VariableInspector.new + end + + def test_array_indexed_members + a = ['a', 'b', 'c'] + + # Test correct truncation + assert_equal [], @inspector.indexed_members_of(a, start: 0, count: 0).map(&:value) + assert_equal ['a'], @inspector.indexed_members_of(a, start: 0, count: 1).map(&:value) + assert_equal ['a', 'b'], @inspector.indexed_members_of(a, start: 0, count: 2).map(&:value) + assert_equal ['a', 'b', 'c'], @inspector.indexed_members_of(a, start: 0, count: 3).map(&:value) + assert_equal ['a', 'b', 'c'], @inspector.indexed_members_of(a, start: 0, count: 4).map(&:value) + assert_equal ['b'], @inspector.indexed_members_of(a, start: 1, count: 1).map(&:value) + assert_equal ['b', 'c'], @inspector.indexed_members_of(a, start: 1, count: 2).map(&:value) + assert_equal ['b', 'c'], @inspector.indexed_members_of(a, start: 1, count: 3).map(&:value) + assert_equal ['b', 'c'], @inspector.indexed_members_of(a, start: 1, count: 4).map(&:value) + + # Test starting off the end + assert_equal [], @inspector.indexed_members_of(a, start: 999, count: 1).map(&:value) + + assert_equal [], @inspector.indexed_members_of([], start: 0, count: 999) + assert_equal [Variable.new(name: '0', value: 'a')], @inspector.indexed_members_of(['a'], start: 0, count: 999) + + expected = [ + Variable.new(name: '5', value: 'f'), + Variable.new(name: '6', value: 'g'), + Variable.new(name: '7', value: 'h'), + ] + assert_equal expected, @inspector.indexed_members_of(Array('a'...'z'), start: 5, count: 3) + end + + def test_named_members_of_hash + actual = @inspector.named_members_of( + { + sym: 'has Symbol key', + "str" => 'has String key', + 1 => 'has Integer key', + } + ) + + expected = [ + Variable.internal(name: '#class', value: Hash), + Variable.new(name: ':sym', value: "has Symbol key"), + Variable.new(name: '"str"', value: "has String key"), + Variable.new(name: '1', value: "has Integer key"), + ] + + assert_equal expected, actual + end + + def test_named_members_of_struct + expected = [ + Variable.internal(name: '#class', value: PointStruct), + # Struct members are stored separately from ivars + Variable.new(name: :x, value: 1), + Variable.new(name: :y, value: 2), + # If there are any other other ivars, they should also be included + Variable.new(name: :@ivar, value: "some other ivar"), + ] + + point = PointStruct.new(x: 1, y: 2) + + assert_equal expected, @inspector.named_members_of(point) + end + + def test_named_members_of_string + expected = [ + Variable.internal(name: '#class', value: String), + Variable.internal(name: '#length', value: 5), + Variable.internal(name: '#encoding', value: Encoding::UTF_8), + # skip #dump member for short strings + ] + + assert_equal expected, @inspector.named_members_of("hello") + + + long_string = "A long string " + ('*' * 1000) + + expected = [ + Variable.internal(name: '#class', value: String), + Variable.internal(name: '#length', value: long_string.length), + Variable.internal(name: '#encoding', value: Encoding::UTF_8), + Variable.internal(name: '#dump', value: VariableInspector::NaiveString.new(long_string)), + ] + + assert_equal expected, @inspector.named_members_of(long_string) + end + + def test_named_members_of_class + expected = [ + Variable.internal(name: '#class', value: Class), + Variable.internal(name: '%ancestors', value: PointStruct.ancestors.drop(1)), + ] + + assert_equal expected, @inspector.named_members_of(PointStruct) + end + + def test_named_members_of_module + ancestors = [Module.new, Module.new, Module.new] + mod = Module.new do + include *ancestors + end + + expected = [ + Variable.internal(name: '#class', value: Module), + Variable.internal(name: '%ancestors', value: ancestors), + ] + + assert_equal expected, @inspector.named_members_of(mod) + end + + def test_named_members_of_range + # Ranges that include end + assert_equal( + [ + Variable.internal(name: "#class", value: Range), + Variable.internal(name: "#begin", value: 1), + Variable.internal(name: "#end", value: 2), + ], + @inspector.named_members_of(1..2) + ) + assert_equal( + [ + Variable.internal(name: "#class", value: Range), + Variable.internal(name: "#begin", value: 1), + Variable.internal(name: "#end", value: nil), + ], + @inspector.named_members_of(1..) + ) + assert_equal( + [ + Variable.internal(name: "#class", value: Range), + Variable.internal(name: "#begin", value: nil), + Variable.internal(name: "#end", value: 2), + ], + @inspector.named_members_of(..2) + ) + + # Ranges that exclude end + assert_equal( + [ + Variable.internal(name: "#class", value: Range), + Variable.internal(name: "#begin", value: 1), + Variable.internal(name: "#end", value: 2), + ], + @inspector.named_members_of(1...2) + ) + assert_equal( + [ + Variable.internal(name: "#class", value: Range), + Variable.internal(name: "#begin", value: 1), + Variable.internal(name: "#end", value: nil), + ], + @inspector.named_members_of(1...) + ) + assert_equal( + [ + Variable.internal(name: "#class", value: Range), + Variable.internal(name: "#begin", value: nil), + Variable.internal(name: "#end", value: 2) + ], + @inspector.named_members_of(...2) + ) + + # Range with nil bounds + assert_equal( + [ + Variable.internal(name: "#class", value: Range), + Variable.internal(name: "#begin", value: nil), + Variable.internal(name: "#end", value: nil), + ], + @inspector.named_members_of(Range.new(nil, nil)) + ) + end + + def test_named_members_of_other_objects + assert_equal [Variable.internal(name: '#class', value: Object)], @inspector.named_members_of(Object.new) + + expected = [ + Variable.internal(name: '#class', value: Point), + # Struct members are stored separately from ivars + Variable.new(name: :@x, value: 1), + Variable.new(name: :@y, value: 2), + ] + + point = Point.new(x: 1, y: 2) + + assert_equal expected, @inspector.named_members_of(point) + end + + private + + class PointStruct < Struct.new(:x, :y, keyword_init: true) + def initialize(x:, y:) + super + @ivar = "some other ivar" + end + end + + class Point # A "plain ol' Ruby object" + def initialize(x:, y:) + @x = x + @y = y + end + end + end +end