Fix crash inspecting indexed values with an unfiltered variables request - #93
Open
tmcgilchrist wants to merge 2 commits into
Open
Fix crash inspecting indexed values with an unfiltered variables request#93tmcgilchrist wants to merge 2 commits into
tmcgilchrist wants to merge 2 commits into
Conversation
Add four scenarios on top of the existing scopes/frames tests: - test_stepping drives next/stepIn/stepOut and records where the debuggee lands, using stack depth as the stable signal so the test does not depend on exactly which line each step snaps to. - test_values renders one local per shape of value (ints, floats, chars, strings, bools, unit, boxed ints, lists, arrays, tuples, options, records, variants, closures, lazies) and expands the structured ones a level. The fixture references every local so none is dropped as unused. - test_heap stops inside a closure and reads the Heap scope, i.e. the variables captured from the enclosing scope. This exercises Value_scope.iter_compenv_heap, which is version-conditional in the same way the Globals scope was in hackwaly#74. - test_events exercises Code_module.find_event/find_events directly, without a debuggee, and records how each source line maps to a debug event. It documents the current mapping, including the lines that are not breakpointable and the backward snap noted in KNOWN_ISSUES.md. The dap_client helper gains stepping commands and value expansion that fetches named and indexed children separately, with a filter, the way an editor does. debugger.ml re-exports the symbol-table modules so the event-mapping test can reach them.
This was referenced Jul 22, 2026
The variables request handler asserted that a value had no indexed children when the request omitted the filter field, and raised Assert_failure for an array. The DAP spec says an omitted filter returns both named and indexed children, so this is a valid request.
tmcgilchrist
force-pushed
the
fix_variable_crash
branch
from
July 23, 2026 22:09
09f4465 to
ac58f73
Compare
sim642
reviewed
Jul 25, 2026
Comment on lines
+129
to
+133
| let%lwt named = value#list_named in | ||
| let%lwt indexed = indexed () in | ||
| Lwt.return (named @ indexed) | ||
| | Some Named -> value#list_named | ||
| | Some Indexed -> | ||
| let start = arg.start |> Option.value ~default:0 in | ||
| let end_ = | ||
| (match arg.count with | ||
| | Some count -> start + count | ||
| | None -> value#num_indexed) | ||
| - 1 | ||
| in | ||
| Seq.int_range ~start ~end_ () | ||
| |> List.of_seq | ||
| |> Lwt_list.map_s (fun i -> | ||
| let%lwt obj = value#get_indexed i in | ||
| Lwt.return (string_of_int i, obj))) | ||
| | Some Indexed -> indexed ?start:arg.start ?count:arg.count () |
Collaborator
There was a problem hiding this comment.
Why are start and count not used when there's no filter?
The specification isn't super clear about this, but it seems like they should also apply to named, a different part of the specification explicitly talks about paging named variables:
/**
* The number of named child variables.
* The client can use this information to present the variables in a paged
* UI and fetch them in chunks.
* The value should be less than or equal to 2147483647 (2^31-1).
*/
namedVariables?: number;No idea how the paging is supposed to work without a filter though. That seems underspecified (microsoft/debug-adapter-protocol#633).
But perhaps this is better tackled in a follow-up PR because this currently leaves that behavior unchanged.
sim642
reviewed
Jul 25, 2026
Comment on lines
+4
to
+13
| 4 snaps to none contains [] | let greet name = | ||
| 5 snaps to 5:3 contains [] | let greeting = "Hello, " ^ name in | ||
| 6 snaps to 6:3 contains [6:3] | greeting | ||
| 7 snaps to none contains [] | | ||
| 8 snaps to none contains [] | let () = | ||
| 9 snaps to none contains [] | let x = 41 in | ||
| 10 snaps to 10:3 contains [10:3; 11:3] | let y = x + 1 in | ||
| 11 snaps to 11:3 contains [] | let msg = greet "world" in | ||
| 12 snaps to 12:3 contains [12:3; 12:3] | print_endline msg; | ||
| 13 snaps to 12:3 contains [] | print_endline (string_of_int y) |
Collaborator
There was a problem hiding this comment.
This seems all over the place:
- Line 5 snaps but line 9 doesn't, even though both are the first
letin a definition. - Line 10 contains something on line 11.
- Line 12 contains the same thing twice.
- Line 13 snaps to line 12, although there's multiple function calls on line 13.
I guess the test is passing but this doesn't seem like what I'd expect at all. Is the logic in earlybird really so strange?
| @@ -0,0 +1,3 @@ | |||
| list_ (:: (‹1›, ‹2›)) -> ‹1› = 1, ‹2› = :: (‹1›, ‹2›) | |||
Collaborator
There was a problem hiding this comment.
So is this broken: https://github.com/hackwaly/ocamlearlybird/blob/1f77823fbb3f8b65b1ac4e63d70878c9b270d605/src/debugger/inspect/value_list.ml?
According to that, lists should show up differently from generic variant constructors.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The variables request handler asserted a value had no indexed children when the request omitted the filter field, and raised
Assert_failurefor an array. Per the DAP spec an omitted filter returns both named and indexed children, so this is a valid request.Editors that page variables always send the filter and are unaffected (VS Code, and emacs dap-mode which sends
:filter "indexed"/"named"). nvim-dap has no variable paging (every variables request it sends carries only variablesReference) so expanding an array in nvim-dap sends an unfiltered request and crashes the adapter. This is the same array-inspection path first reported in #31 (whose VS Code cause was fixed separately in 2021)Also adds an integration-test suite driving the real adapter over DAP. test_indexed is the regression test for this fix and aborts the old adapter with the assertion.
Sources:
The first commit introduces more integration style tests that exercise other functionality. I didn't feel confident making more changes to the project without some tests to check current behaviour and to demonstrate future fixes. I'm unsure whether I like the style and verbosity of them, but I haven't thought of a better way to write them. Open to ideas on how the integration tests should work @sim642