Conversation
…add corresponding tests
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The interactive debugger currently can’t pass arguments to restarts and the new interactive CLI test assertions are fragile against prompt/EOF output, so behavior and tests need tightening before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces an interactive debugger for ListTalk conditions, adds a per-thread debugger hook mechanism, and expands the restart/correctable-error facilities so evaluation and REPL flows can recover from certain errors (e.g., unbound symbols) instead of hard-aborting.
Changes:
- Add a new
ListTalkDebuggerlibrary with an interactive debugger REPL and object inspector, plus CLI integration (-d/--debugand auto-enable for interactive top-level). - Introduce debugger hook support in thread state and condition signaling paths, and extend stack trace printing with a “skip frames” variant for cleaner debugger backtraces.
- Add new restarts/correctable error support (
LT_cerror, unbound-symbol restarts), adjust static restart naming to keyword symbols, and expand tests accordingly.
File summaries
| File | Description |
|---|---|
| tests/listtalk_cli_test.py | Adds CLI coverage for -d/--debug and an interactive REPL syntax-error recovery case. |
| tests/conditions_test.c | Adds tests for debugger hook scoping/disablement, unbound-symbol restarts, and LT_cerror continue restart behavior. |
| tests/c_api_test.c | Adds environment slot behavior test and expands static restart behavior checks (keyword naming + invocation). |
| src/vm/thread_state.c | Initializes new per-thread debugger_hook state. |
| src/vm/stack_trace.c | Adds LT_stack_trace_print_skipping and keeps LT_stack_trace_print as a wrapper. |
| src/vm/reader.c | Invokes debugger on unrecoverable reader errors before aborting. |
| src/vm/eval.c | Adds unbound-symbol restarts (use-value, define-variable) and catch/throw-based resumption logic. |
| src/vm/error.c | Invokes debugger for fatal errors and adds correctable error API (LT_cerror) with a :continue restart. |
| src/vm/conditions.c | Adds debugger hook storage + LT_invoke_debugger implementation. |
| src/debugger/debugger.c | New interactive debugger/inspector implementation and debugger hook primitive. |
| src/classes/Restart.c | Changes static restart naming to keyword symbols and defers materialization. |
| src/classes/Package.c | Adds predefined ListTalk-debug package and wires package use relationships. |
| src/classes/Environment.c | Exposes parent slot via a native-pointer slot type (NULL → NIL). |
| src/classes/Class.c | Introduces LT_SlotType_ReadonlyNativeObjectPointer. |
| src/bin/listtalk/main.c | Adds -d/--debug, enables debugger at interactive top-level, and updates REPL loop control flow with a toplevel restart. |
| meson.build | Builds/installs ListTalkDebugger and links it into the listtalk executable; generates pkg-config metadata. |
| ListTalk/vm/thread_state.h | Adds debugger_hook field to thread state struct. |
| ListTalk/vm/stack_trace.h | Declares LT_stack_trace_print_skipping. |
| ListTalk/vm/error.h | Declares LT_cerror public API. |
| ListTalk/vm/conditions.h | Declares debugger hook APIs and adds LT_WITH_DEBUGGER_HOOK macro. |
| ListTalk/debugger/debugger.h | New public debugger API header. |
| ListTalk/classes/Restart.h | Updates static-restart macro to register a materialization constructor. |
| ListTalk/classes/Package.h | Exposes LT_PACKAGE_LISTTALK_DEBUG. |
| ListTalk/classes/Class.h | Declares new readonly native-pointer slot type. |
Review details
- Files reviewed: 23/24 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+260
to
+284
| static void debugger_repl_object(LT_Value object, void* opaque){ | ||
| DebuggerContext* context = opaque; | ||
| LT_Value restart_value = debugger_restart_selected(object, context->restarts); | ||
| LT_Value returned_to_debugger = LT_NIL; | ||
|
|
||
| if (debugger_keyword_selected(object, "inspect-condition")){ | ||
| LT_Debugger_inspect(context->condition); | ||
| return; | ||
| } | ||
| if (debugger_keyword_selected(object, "inspect-backtrace")){ | ||
| LT_Debugger_inspect(context->backtrace); | ||
| return; | ||
| } | ||
|
|
||
| if (restart_value != LT_INVALID){ | ||
| LT_Restart* restart = LT_Restart_from_value(restart_value); | ||
| LT_apply( | ||
| LT_Restart_callable(restart), | ||
| LT_NIL, | ||
| LT_NIL, | ||
| LT_NIL, | ||
| NULL | ||
| ); | ||
| return; | ||
| } |
Comment on lines
+88
to
+94
| if not completed.stdout.rstrip().endswith("9"): | ||
| sys.stderr.write( | ||
| "FAIL: top-level REPL did not recover from syntax error\n{0}".format( | ||
| completed.stdout | ||
| ) | ||
| ) | ||
| return 1 |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…gement; add tests for restart behavior and banner display
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
The new interactive CLI tests use brittle stdout.rstrip().endswith(...) assertions that are likely to fail because the REPL prints prompts even when stdin is a pipe.
Review details
Suppressed comments (2)
tests/listtalk_cli_test.py:92
- The REPL prints prompts even when stdin is a pipe (see src/repl/repl.c:429-439), so
completed.stdout.rstrip().endswith("9")can fail because output often ends with a finallisttalk>prompt on EOF. Prefer asserting that the expected result appears somewhere in stdout (or strip trailing prompts) instead of requiring it to be the very last character.
if not completed.stdout.rstrip().endswith("9"):
tests/listtalk_cli_test.py:120
- Similar to the syntax-error case, the interactive REPL session will typically emit a trailing
listtalk>prompt before exiting on EOF, so checkingendswith(expected_result)is brittle and can fail depending on the REPL implementation/back-end. Consider checking for the result token/line within stdout instead.
if not completed.stdout.rstrip().endswith(expected_result):
- Files reviewed: 23/24 changed files
- Comments generated: 0 new
- Review effort level: Lite
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
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.
No description provided.