Share variable inspection logic between CDP and DAP #1001

amomchilov · 2023-07-26T22:36:29Z

The logic that decides how variables are presented is repeated between the CDP and DAP implementations. This is error prone and leads to inconsistencies, where improvements are made to one or the other, but not both. E.g.:

The special #dump member for String only exists in VSCode DAP: show #dump when pp'ed string is too long #869
This feature to hide special #class members only exists for VSCode DAP: add the ability to hide the #class item for each object #986
VSCode formats hash keys with value_inspect, but not Chrome
CDP adds #class appears at the end, whereas DAP adds #class to the start.

This PR extracts the variable inspection logic into a new VariableInspector class, which can be shared between VSCode and Chrome. This gives a central place to make future improvements.

I tried to make only the minimal behaviour changes necessary to unify the two implementations.

This PR then unlocks the ability to make some changes really nice/easily:

amomchilov · 2023-07-26T22:42:46Z

lib/debug/server_dap.rb

          end
+
+          vars = members.map { |member| variable(member.name, member.value) }


There's nothing implemented like CDP's internalProperty yet, but I think support for presentationHint can be added here eventually to achieve a similar effect.

amomchilov · 2023-07-26T22:43:22Z

lib/debug/variable_inspector.rb

+    MAX_LENGTH = 180
+
+    def value_inspect obj, short: true
+      # TODO: max length should be configurable?
+      str = DEBUGGER__.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


I just copied this snippet for now, but my eventual goal is to move all presentation logic out of ThreadClient and into a shared class like this.

amomchilov · 2023-07-26T22:44:03Z

lib/debug/variable_inspector.rb

+    def named_members_of obj
+      members = case obj
+      when Array then obj.map.with_index { |o, i| Member.new(name: i.to_s, value: o) }
+      when Hash then obj.map { |k, v| Member.new(name: value_inspect(k), value: v) }


This used to be inconsistent before. VSCode would format Hash keys with value_inspect, but not Chrome. Now they will both us it.

amomchilov · 2023-07-26T22:46:05Z

lib/debug/server_cdp.rb

-          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))]


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

amomchilov · 2023-07-28T12:34:38Z

lib/debug/server_dap.rb

-        obj = @var_map[vid]
-        if obj


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

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:

... 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

amomchilov · 2023-07-28T12:35:15Z

lib/debug/server_dap.rb

+               result: result_variable.inspect_value,
+               **render_variable(result_variable)


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

amomchilov · 2023-07-28T12:36:08Z

lib/debug/server_dap.rb

-      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


This just becomes a natural part of VariableInspecto#named_members_of, so we don't have to worry about it here anymore.

amomchilov · 2023-07-28T12:39:20Z

lib/debug/server_dap.rb

-        str = value_inspect(obj)
-      end
-
-      if name


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.

amomchilov · 2023-07-28T12:39:51Z

lib/debug/server_dap.rb

+      variable[:indexedVariables] = indexedVariables unless indexedVariables == 0
+      variable[:namedVariables] = namedVariables unless namedVariables == 0


This is kind of nice, it declutters the response logging a lot, so it's easier to debug.

amomchilov · 2023-07-28T12:41:43Z

lib/debug/server_dap.rb

-      when String
-        variable_ name, obj, namedVariables: 3 # #length, #encoding, #to_str


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.

amomchilov · 2023-07-28T12:42:28Z

lib/debug/session.rb

-  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)


I extracted this out so it can be used in VariableInspector (without needing to require the whole session.rb, such as in the unit test).

I left this stub to minimize the churn here

ko1 · 2023-09-25T07:54:47Z

I like an idea to unify CDP and DAP because of implementation.
However I'm not sure we can merge it because of difference between CDP and DAP (I don't know the CDP details). Do you have a time to trouble shooting for CDP?
The name "variable.rb" doesn't make sense for me because it's only for the representation (the name is too general). debug.gem has a policy to reduce the number of "require" to reduce the boot time. Can we merge it with variable_inspector.rb?

amomchilov · 2023-09-26T22:02:58Z

Do you have a time to trouble shooting for CDP?

I've already run this with CDP, it works great.

Can we merge it with variable_inspector.rb?

@ko1 Done. I still call it class Variable. It's a general name, but I don't have any better ideas. Please suggest if you have any.

ono-max · 2023-12-10T07:31:28Z

lib/debug/variable_inspector.rb

+module DEBUGGER__
+  class VariableInspector
+    def indexed_members_of(obj, start:, count:)
+      return [] if start > (obj.length - 1)


In this implementation, the #class will disappear when displaying Array. Is this intentional behavior?

That makes sense (in the same spirit as #1004), though perhaps we should keep the #class if it's a non-standard subclass of Array. I'll make that in a separate PR.

ono-max · 2023-12-10T10:17:26Z

Also, some tests are failed in protocol part. Can you fix them?
FYI: https://github.com/ruby/debug/blob/master/CONTRIBUTING.md

amomchilov · 2023-12-14T03:23:30Z

@ono-max I can have a look over the holiday break next week :)

amomchilov · 2024-01-04T04:33:27Z

Hey @ono-max, Happy new year!

I've fixed the protocol tests, and rebased onto the latest master.

There's 2 test failures in test/console/nested_break_test.rb left, but I don't see how they could be related.

These values are assumed to be 0 if they're missing.

ono-max · 2024-01-04T08:35:57Z

lib/debug/server_cdp.rb

-              internalProperty('#begin', obj.begin),
-              internalProperty('#end', obj.end),
-            ]
+        if @obj_map.key?(oid)


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

ono-max · 2024-01-04T08:55:33Z

lib/debug/variable_inspector.rb

+    def indexed_members_of(obj, start:, count:)
+      return [] if start > (obj.length - 1)
+
+      capped_count = [count, obj.length - start].min


Can you add the comment why we need to compare count and obj.length - start and choose the minimum one? Sorry, I could not understand the reason.

It's to keep the iteration from running off the end of the array. This used to give extra nils at the end. See 6837459

I can add a comment.

Could you please run the CI?

ono-max · 2024-01-05T03:49:34Z

It seems to be that some protocol tests are still failed. Can you please check them? If you need help, please let me know 👍

I believe that https://github.com/ruby/debug/actions/runs/7405610000/job/20183471986?pr=1001 and https://github.com/ruby/debug/actions/runs/7405610000/job/20183471690?pr=1001 are not flakey tests.

amomchilov · 2024-01-06T19:35:58Z

@ono-max Hmmm I think we have one of those "it works on my machine" situations

$ bundle exec rake test_protocol

Loaded suite /Users/alex/.gem/ruby/3.3.0/gems/rake-13.0.6/lib/rake/rake_test_loader
Started
Finished in 1.438602 seconds.
--------------------------------------------------------------------------------
2 tests, 690 assertions, 0 failures, 0 errors, 0 pendings, 0 omissions, 0 notifications
100% passed
--------------------------------------------------------------------------------
1.39 tests/s, 479.63 assertions/s

I'm running on Ruby 3.3.0 on macOS Sonoma 14.2.1.

I suspect these errors are coming from the ancestors chains being in different in CI, but I don't know why that would be. Could you please help me investigate this? I don't have access to a Linux machine to compare against.

ono-max · 2024-01-07T11:17:47Z

I investigated the cause of the failed tests. I believe that the reason is because ancestor classes are changed from Ruby 3.1. Thus, we can skip the test if the Ruby version is lower than 3.1, e.g. 6f154ac

amomchilov · 2024-01-08T05:05:45Z

@ono-max Fantastic find! I pulled that commit into this PR, so you get credit for it :)

Is this ready to merge?

ono-max · 2024-01-08T05:10:38Z

I appreciate your dedicated efforts and hard work. @ko1-san will merge this PR 👍

amomchilov · 2024-01-08T14:07:39Z

@ono-max Thanks!

Could you run the CI one more time, so we're sure it's green before merging?

amomchilov · 2024-01-08T19:35:36Z

@ono-max I think the remaining test failures are just flakey tests. I can reproduce them Ruby 3.1.4, but they fail less than 1/10 times.

amomchilov · 2024-01-19T04:33:51Z

@ko1 Can you please review and merge this?

amomchilov commented Jul 26, 2023

View reviewed changes

amomchilov force-pushed the share-variable-inspection-logic branch 4 times, most recently from 1295842 to 6119aa8 Compare July 27, 2023 20:41

amomchilov marked this pull request as ready for review July 27, 2023 20:50

amomchilov mentioned this pull request Jul 27, 2023

Remove double colon in Hash keys (:sym: "value" → sym: "value") #1003

Open

amomchilov force-pushed the share-variable-inspection-logic branch 2 times, most recently from 34b1210 to de6411b Compare July 28, 2023 12:43

amomchilov commented Jul 28, 2023

View reviewed changes

amomchilov force-pushed the share-variable-inspection-logic branch from de6411b to ee133fd Compare July 28, 2023 13:36

This was referenced Jul 28, 2023

Omit #class for simple values #1004

Open

DAP: add the ability to hide the #class item for each object #986

Closed

amomchilov force-pushed the share-variable-inspection-logic branch from ee133fd to fb05fe3 Compare July 28, 2023 20:33

ono-max reviewed Dec 10, 2023

View reviewed changes

ono-max self-requested a review December 10, 2023 07:31

amomchilov force-pushed the share-variable-inspection-logic branch 2 times, most recently from a0a8b3b to 2e850bc Compare January 4, 2024 04:31

amomchilov added 6 commits January 3, 2024 23:42

Improve existing tests

e42c994

Share variable inspection logic between CDP and DAP

e2f84ee

Inline variable.rb

eca9d80

Fix edge case for falsy values

b24af44

Skip unnecessary values when 0

54b9a5e

These values are assumed to be 0 if they're missing.

Fix out-of-bound index access

6837459

amomchilov force-pushed the share-variable-inspection-logic branch from 2e850bc to 6837459 Compare January 4, 2024 04:42

ono-max reviewed Jan 4, 2024

View reviewed changes

amomchilov requested a review from ono-max January 4, 2024 14:30

Test for ruby#1001

6f154ac

ono-max approved these changes Jan 8, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Share variable inspection logic between CDP and DAP #1001

Share variable inspection logic between CDP and DAP #1001

amomchilov commented Jul 26, 2023 •

edited

Loading

amomchilov Jul 26, 2023

amomchilov Jul 26, 2023

amomchilov Jul 26, 2023

amomchilov Jul 26, 2023

amomchilov Jul 28, 2023

amomchilov Jan 4, 2024 •

edited

Loading

amomchilov Jul 28, 2023

amomchilov Jul 28, 2023

amomchilov Jul 28, 2023

amomchilov Jul 28, 2023

amomchilov Jul 28, 2023

amomchilov Jul 28, 2023

ko1 commented Sep 25, 2023

amomchilov commented Sep 26, 2023 •

edited

Loading

ono-max Dec 10, 2023

amomchilov Dec 14, 2023

ono-max commented Dec 10, 2023 •

edited

Loading

amomchilov commented Dec 14, 2023

amomchilov commented Jan 4, 2024

ono-max Jan 4, 2024

ono-max Jan 4, 2024 •

edited

Loading

amomchilov Jan 4, 2024

ono-max Jan 5, 2024

ono-max commented Jan 5, 2024 •

edited

Loading

amomchilov commented Jan 6, 2024

ono-max commented Jan 7, 2024

amomchilov commented Jan 8, 2024

ono-max commented Jan 8, 2024

amomchilov commented Jan 8, 2024

amomchilov commented Jan 8, 2024 •

edited

Loading

amomchilov commented Jan 19, 2024

		end

		vars = members.map { \|member\| variable(member.name, member.value) }

		result: result_variable.inspect_value,
		**render_variable(result_variable)

		variable[:indexedVariables] = indexedVariables unless indexedVariables == 0
		variable[:namedVariables] = namedVariables unless namedVariables == 0

		when String
		variable_ name, obj, namedVariables: 3 # #length, #encoding, #to_str

Share variable inspection logic between CDP and DAP #1001

Are you sure you want to change the base?

Share variable inspection logic between CDP and DAP #1001

Conversation

amomchilov commented Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amomchilov Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ko1 commented Sep 25, 2023

amomchilov commented Sep 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ono-max commented Dec 10, 2023 • edited Loading

amomchilov commented Dec 14, 2023

amomchilov commented Jan 4, 2024

Choose a reason for hiding this comment

ono-max Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ono-max commented Jan 5, 2024 • edited Loading

amomchilov commented Jan 6, 2024

ono-max commented Jan 7, 2024

amomchilov commented Jan 8, 2024

ono-max commented Jan 8, 2024

amomchilov commented Jan 8, 2024

amomchilov commented Jan 8, 2024 • edited Loading

amomchilov commented Jan 19, 2024

amomchilov commented Jul 26, 2023 •

edited

Loading

amomchilov Jan 4, 2024 •

edited

Loading

amomchilov commented Sep 26, 2023 •

edited

Loading

ono-max commented Dec 10, 2023 •

edited

Loading

ono-max Jan 4, 2024 •

edited

Loading

ono-max commented Jan 5, 2024 •

edited

Loading

amomchilov commented Jan 8, 2024 •

edited

Loading