🚀 Clear assertion greyed-out lines on restart and highlight issue locations #232
Conversation
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughQueues parse errors and emits structured highlight events from the DAP server; adds HighlightError DAP message type and helpers; launch/restart now queue parse errors instead of failing inline; Python bindings capture loader stderr; C++ parser adds validation/position utilities and simulator resets; tests for coordinate/highlight behavior added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DAPServer
participant ParserLoader as Parser/Loader
participant Simulation
participant Editor
Client->>DAPServer: launch/restart request
DAPServer->>ParserLoader: load_code(program)
alt parse success
ParserLoader-->>DAPServer: OK
DAPServer->>Simulation: run (unless stopOnEntry)
Simulation-->>DAPServer: diagnostics / assertion events
DAPServer->>DAPServer: collect_highlight_entries()
Note right of DAPServer: pending_highlights populated
DAPServer-->>Editor: HighlightError (highlights + source)
DAPServer->>Client: send_state / messages
else parse failure
ParserLoader-->>DAPServer: raises RuntimeError (detail)
DAPServer->>DAPServer: queue_parse_error(detail)
DAPServer-->>Editor: HighlightError (from queued parse error)
DAPServer->>Simulation: reset (exceptions suppressed)
DAPServer->>Client: send_state (Finished/Assertion failed)
end
Note over Editor,DAPServer: _prevent_exit controls ExitedDAPEvent emission
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/mqt/debugger/dap/messages/restart_dap_message.py (1)
57-88: Consider extracting common parsing logic shared with LaunchDAPMessage.The
handle()method shares significant logic withLaunchDAPMessage.handle()(file reading, error handling, conditional simulation run). Consider extracting a shared helper to reduce duplication.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
bindings/InterfaceBindings.cpppython/mqt/debugger/dap/dap_server.pypython/mqt/debugger/dap/messages/__init__.pypython/mqt/debugger/dap/messages/highlight_error_dap_message.pypython/mqt/debugger/dap/messages/launch_dap_message.pypython/mqt/debugger/dap/messages/restart_dap_message.pysrc/common/parsing/CodePreprocessing.cpptest/python/test_dap_server.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-15T01:54:22.129Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1383
File: bindings/dd/register_matrix_dds.cpp:64-109
Timestamp: 2025-12-15T01:54:22.129Z
Learning: In the munich-quantum-toolkit/core repository, after migrating to nanobind, docstrings for Python bindings are now added directly in the C++ binding code (using R"pb(...)pb" syntax) and stub files (.pyi) are auto-generated using the `bindings/generate-stubs.sh` script. This replaces the previous pybind11 approach where docstrings were manually maintained in stub files.
Applied to files:
bindings/InterfaceBindings.cpp
🧬 Code graph analysis (6)
python/mqt/debugger/dap/messages/launch_dap_message.py (1)
python/mqt/debugger/pydebugger.pyi (3)
load_code(130-135)run_simulation(183-188)reset_simulation(193-198)
src/common/parsing/CodePreprocessing.cpp (3)
include/common/parsing/ParsingError.hpp (1)
ParsingError(32-32)src/common/parsing/ParsingError.cpp (1)
ParsingError(23-23)include/common/parsing/CodePreprocessing.hpp (1)
isVariableDeclaration(296-296)
test/python/test_dap_server.py (2)
python/mqt/debugger/dap/dap_server.py (2)
code_pos_to_coordinates(376-402)collect_highlight_entries(444-470)python/mqt/debugger/pydebugger.pyi (3)
potential_error_causes(527-535)get_instruction_position(274-284)get_diagnostics(419-424)
python/mqt/debugger/dap/messages/highlight_error_dap_message.py (4)
python/mqt/debugger/dap/messages/dap_event.py (1)
DAPEvent(17-41)python/mqt/debugger/dap/messages/launch_dap_message.py (1)
validate(44-55)python/mqt/debugger/dap/messages/restart_dap_message.py (1)
validate(44-55)python/mqt/debugger/dap/dap_server.py (1)
start(122-142)
python/mqt/debugger/dap/messages/__init__.py (1)
python/mqt/debugger/dap/messages/highlight_error_dap_message.py (1)
HighlightError(22-140)
python/mqt/debugger/dap/messages/restart_dap_message.py (1)
python/mqt/debugger/pydebugger.pyi (3)
load_code(130-135)run_simulation(183-188)reset_simulation(193-198)
🪛 Cppcheck (2.19.0)
src/common/parsing/CodePreprocessing.cpp
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
bindings/InterfaceBindings.cpp
[information] 26-26: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
🪛 Ruff (0.14.10)
python/mqt/debugger/dap/messages/launch_dap_message.py
69-69: Boolean positional value in function call
(FBT003)
python/mqt/debugger/dap/messages/restart_dap_message.py
70-70: Boolean positional value in function call
(FBT003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Debug) / 🏁 windows-2022 msvc Debug
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Release) / 🏁 windows-2022 msvc Release
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🇨 Test 🏁 (windows-11-arm, msvc, Release) / 🏁 windows-11-arm msvc Release
- GitHub Check: 🇨 Test 🍎 (macos-15-intel, clang, Release) / 🍎 macos-15-intel clang Release
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Release) / 🐧 ubuntu-24.04 gcc Release
- GitHub Check: 🇨 Lint / 🚨 Lint
🔇 Additional comments (20)
bindings/InterfaceBindings.cpp (1)
179-192: LGTM! The error capture pattern is well-implemented.The use of
py::scoped_ostream_redirectfor RAII-style stderr capture and the fallback to a generic error message when the captured output is empty is a clean approach for surfacing C++ parsing errors to Python callers.src/common/parsing/CodePreprocessing.cpp (5)
100-145: Well-structured validation with clear error messages.The centralized
validateTargetsfunction properly validates qubit target syntax and bounds, with informative error messages that include the problematic target and context. Good use of early returns for efficiency.
476-490: LGTM! Robust register size parsing.The combination of
isDigitsvalidation andtry/catcharoundstd::stoulprovides defense-in-depth against malformed input and overflow conditions.
446-450: Improved position accuracy for diagnostics.The updated
trueStartcalculation correctly identifies the first non-whitespace character for more precise error reporting.
554-563: LGTM! Proper integration of target validation.The validation is correctly applied to assertions and regular instructions while appropriately skipping variable declarations. The context strings provide meaningful differentiation in error messages.
50-64: No action needed—lineColumnForOffsetcorrectly handles offset values at and near code boundaries.The function uses
std::string::rfind('\n', offset)which has well-defined behavior even whenoffset == code.size(). The C++ standard guarantees this is safe, and the subsequent column calculation works correctly for valid positions. No bounds checking or code changes are required.Likely an incorrect or invalid review comment.
python/mqt/debugger/dap/messages/__init__.py (1)
24-24: LGTM! Clean module export.The new
HighlightErrorclass is properly imported and added to__all__following the existing alphabetical ordering convention.Also applies to: 61-61
python/mqt/debugger/dap/messages/restart_dap_message.py (1)
73-82: Verify: Response returns success=True even when parsing fails.The handler queues the parse error and returns
"success": Trueeven whenload_coderaises aRuntimeError. This defers error reporting to the highlight event mechanism. Confirm this aligns with the DAP protocol expectations for the restart command.python/mqt/debugger/dap/messages/launch_dap_message.py (1)
67-81: LGTM! Error handling aligns with restart handler pattern.The parse error handling with
queue_parse_errorand the use ofcontextlib.suppressfor the reset fallback path is consistent and well-implemented.test/python/test_dap_server.py (2)
18-25: LGTM! Good test for coordinate boundary handling.This test correctly validates that the column calculation for a newline position returns the correct 1-based column value for the end of the line.
28-44: Good regression test for the highlight boundary fix.This test validates the core fix by ensuring highlight entries don't incorrectly span to the next instruction. The use of
SimpleNamespacefor lightweight mocking is appropriate for unit tests.python/mqt/debugger/dap/messages/highlight_error_dap_message.py (2)
102-140: LGTM! Position handling is correct.The
_normalize_positionmethod properly validates and extracts line/column values, and_later_thancorrectly implements lexicographic position comparison.
22-39: LGTM! Clean implementation of the custom DAP event.The
HighlightErrorclass follows the establishedDAPEventpattern, properly normalizing inputs in the constructor before callingsuper().__init__()which triggers validation.python/mqt/debugger/dap/dap_server.py (7)
317-323: LGTM! reset_gray_out correctly clears highlights on new execution.This directly addresses the PR objective by sending an empty gray-out event on launch/restart, ensuring a clean editor state.
385-402: LGTM! Improved boundary handling in coordinate calculation.The
pos <= len(line_code)condition and theelseclause for handling positions beyond the text correctly address edge cases for newline positions and end-of-file scenarios.
535-544: Verify: Backward search terminates correctly.The
whileloop searching for a previous non-empty line correctly decrementsprev_indexand has a proper termination condition (prev_index >= 0). The logic is sound.
444-470: LGTM! Robust highlight collection with appropriate fallback.The method correctly handles the case where no specific error causes are found by falling back to a generic assertion failure highlight. The
RuntimeErrorcatch ensures resilience against diagnostics failures.
262-280: LGTM! Proper coordination of highlight emission and exit events.The
_prevent_exitflag correctly prevents prematureExitedDAPEventemission when highlights are pending, and is reset on subsequent stepping commands to restore normal behavior.
559-582: LGTM! Clean recursive flattening of message structures.The helper correctly handles various nested structures while filtering empty strings and appropriately converting other types to strings.
653-668:send_statemethod is unused in the codebase.This method is well-implemented but has no internal calls. Verify whether it's intended as part of the public DAP API, required for external clients, or should be removed.
| def validate(self) -> None: | ||
| """Validate the 'highlightError' DAP event message after creation.""" | ||
| if not self.highlights: | ||
| msg = "At least one highlight entry is required to show the issue location." | ||
| raise ValueError(msg) | ||
|
|
||
| for highlight in self.highlights: | ||
| if "message" not in highlight or not str(highlight["message"]).strip(): | ||
| msg = "Each highlight entry must contain a descriptive 'message'." | ||
| raise ValueError(msg) | ||
| highlight_range = highlight.get("range") | ||
| if not isinstance(highlight_range, dict): | ||
| msg = "Each highlight entry must provide a 'range' dictionary." | ||
| raise TypeError(msg) | ||
| start = highlight_range.get("start") | ||
| end = highlight_range.get("end") | ||
| if not isinstance(start, dict) or not isinstance(end, dict): | ||
| msg = "Highlight ranges must define 'start' and 'end' coordinates." | ||
| raise TypeError(msg) | ||
| if self._later_than(start, end): | ||
| msg = "Highlight range 'end' must not precede 'start'." | ||
| raise ValueError(msg) | ||
|
|
||
| if "name" not in self.source or "path" not in self.source: | ||
| msg = "Source information must at least expose 'name' and 'path'." | ||
| raise ValueError(msg) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant validation between normalize and validate methods.
The _normalize_highlight method already validates ranges and throws on invalid input (lines 77-89), but validate() re-checks similar conditions (lines 51-62). Consider consolidating to avoid duplicate validation logic.
🤖 Prompt for AI Agents
In python/mqt/debugger/dap/messages/highlight_error_dap_message.py around lines
41-66, validate() duplicates range and message checks already performed in
_normalize_highlight (lines ~77-89); remove the redundant per-highlight
range/message checks from validate() and either call/ensure _normalize_highlight
has been run before validate() (or invoke it at the start of validate()),
keeping only high-level checks in validate() (e.g., non-empty highlights and
required source fields). Ensure error types/messages remain consistent by
centralizing the detailed validation in _normalize_highlight and leaving
validate() to assert overall structure.
…-quantum-toolkit#234) Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
svenjeschmitt-ops
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
python/mqt/debugger/dap/dap_server.py (1)
373-389: Handle empty source code edge case incode_pos_to_coordinates.When
source_codeis empty,scopes_dap_messagecalls this method withend_pos = -1, causing it to returnline = -1whenlines_start_at_oneisFalse. This produces invalid line numbers in DAP responses. Add an early check to handle empty source code explicitly or validate thatpos >= 0.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
python/mqt/debugger/dap/dap_server.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: burgholzer
Repo: munich-quantum-toolkit/yaqs PR: 212
File: CHANGELOG.md:12-15
Timestamp: 2025-10-14T14:37:38.047Z
Learning: In the munich-quantum-toolkit/yaqs project, changelog entries follow the template: "- $TITLE ([#$NUMBER]($URL)) ([**AUTHOR**](https://github.com/$AUTHOR))". Issue references should not be included in changelog entries; the PR number is sufficient for traceability.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Debug) / 🏁 windows-2022 msvc Debug
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Release) / 🐧 ubuntu-24.04 gcc Release
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Release) / 🍎 macos-14 clang Release
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🇨 Test 🏁 (windows-11-arm, msvc, Release) / 🏁 windows-11-arm msvc Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04-arm, gcc, Release) / 🐧 ubuntu-24.04-arm gcc Release
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Release) / 🏁 windows-2022 msvc Release
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Debug) / 🍎 macos-14 clang Debug
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Debug) / 🐧 ubuntu-24.04 gcc Debug
- GitHub Check: 🇨 Test 🍎 (macos-15-intel, clang, Release) / 🍎 macos-15-intel clang Release
- GitHub Check: 🇨 Lint / 🚨 Lint
🔇 Additional comments (13)
python/mqt/debugger/dap/dap_server.py (13)
118-119: LGTM! Clean state management additions.The new fields
pending_highlightsand_prevent_exitare properly initialized and serve the PR objective of managing highlight lifecycle and preventing premature exit when errors need to be displayed.
172-186: LGTM! Proper reset of exit prevention flag.Resetting
_prevent_exittoFalsewhen execution commands are issued ensures that the debugger can exit normally after the user takes action, aligning with the PR objective of preventing exit only while highlights are being displayed.
257-261: LGTM! Proper highlight emission and exit prevention.The logic correctly sends queued highlights, clears the pending list, and sets the exit prevention flag to ensure users can review the highlighted errors before the debugger exits.
271-275: LGTM! Exit gated by highlight state.The modified condition correctly prevents the debugger from exiting when
_prevent_exitisTrue, ensuring highlights remain visible until the user continues or steps.
355-361: LGTM! Proper error categorization and highlight emission.Using
"stderr"for assertion failure messages is appropriate, and the highlight collection logic ensures error causes are properly displayed to the user.
431-457: LGTM! Robust error cause collection with fallback.The method properly handles missing diagnostics and ensures at least the failing instruction is highlighted when no specific error causes are identified.
459-478: LGTM! Well-structured highlight entry construction.The method correctly handles position-to-coordinate conversion with proper clamping at Line 466 to prevent out-of-bounds coordinates. The snippet extraction at Line 468 relies on Python's graceful slice handling, which is acceptable since the snippet is for display purposes only.
480-487: LGTM! Clean error type mapping.Simple, focused method with proper default handling for unknown error types.
489-494: LGTM! Parse error queuing for deferred display.The method correctly parses and queues parse errors. Note that Line 494 replaces any existing pending highlights rather than appending; this is likely intentional for parse errors which typically occur individually during launch.
496-508: LGTM! Robust parse error location extraction.The regex pattern correctly matches typical compiler error format
<input>:line:column: detailwith appropriate fallback values when the pattern doesn't match.
546-569: LGTM! Comprehensive message flattening logic.The recursive flattening correctly handles nested message structures (strings, dicts, lists). For typical DAP message hierarchies, the recursion depth should be shallow and safe. If deeply nested messages become a concern, consider adding a depth limit parameter.
571-638: LGTM! Clean message sending refactor.The introduction of
send_message_simpleand the addition of thecategoryparameter tosend_message_hierarchyimproves code organization and allows proper categorization of messages (console/stdout/stderr).
640-656: LGTM! Clear execution state reporting.The method correctly reports execution state with appropriate priority (assertion failure > breakpoint > finished > running) and uses the new simple message pathway for clean output.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/dd/DDSimDebug.cpp (1)
591-627: Consolidate redundant state clearing into ddsimLoadCode.Lines 597–608 clear the same state containers that
preprocessAssertionCodeclears again at lines 1654–1665. SincepreprocessAssertionCodeis only called from this function (line 611) after the initial clearing, the duplicate clearing inpreprocessAssertionCodeis redundant and obscures where state initialization actually occurs.Consider removing the clearing loops from
preprocessAssertionCode(lines 1654–1665) and documenting that callers must ensure a clean state beforehand, or makepreprocessAssertionCodemore self-contained if it may be called independently in the future.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/backend/dd/DDSimDebug.cpp
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: burgholzer
Repo: munich-quantum-toolkit/yaqs PR: 212
File: CHANGELOG.md:12-15
Timestamp: 2025-10-14T14:37:38.047Z
Learning: In the munich-quantum-toolkit/yaqs project, changelog entries follow the template: "- $TITLE ([#$NUMBER]($URL)) ([**AUTHOR**](https://github.com/$AUTHOR))". Issue references should not be included in changelog entries; the PR number is sufficient for traceability.
🧬 Code graph analysis (1)
src/backend/dd/DDSimDebug.cpp (2)
test/utils/common_fixtures.hpp (6)
code(211-215)code(211-213)code(236-244)code(236-237)code(254-256)code(254-254)include/frontend/cli/CliFrontEnd.hpp (2)
code(46-46)code(68-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04-arm, gcc, Release) / 🐧 ubuntu-24.04-arm gcc Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Debug) / 🐧 ubuntu-24.04 gcc Debug
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Release) / 🍎 macos-14 clang Release
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Debug) / 🍎 macos-14 clang Debug
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Release) / 🐧 ubuntu-24.04 gcc Release
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Debug) / 🏁 windows-2022 msvc Debug
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🇨 Test 🍎 (macos-15-intel, clang, Release) / 🍎 macos-15-intel clang Release
- GitHub Check: 🇨 Test 🏁 (windows-11-arm, msvc, Release) / 🏁 windows-11-arm msvc Release
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Release) / 🏁 windows-2022 msvc Release
- GitHub Check: 🇨 Lint / 🚨 Lint
🔇 Additional comments (1)
src/backend/dd/DDSimDebug.cpp (1)
1134-1151: LGTM: Readiness guard prevents stale state execution.The check on line 1136–1138 properly gates execution to ensure code is loaded and state is initialized. Combined with the
readyflag management (setfalsewhen loading code,trueafter successful processing), this ensures each execution starts with a clean slate and prevents undefined behavior from uninitialized state.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/backend/dd/DDSimDebug.cpp
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: burgholzer
Repo: munich-quantum-toolkit/yaqs PR: 212
File: CHANGELOG.md:12-15
Timestamp: 2025-10-14T14:37:38.047Z
Learning: In the munich-quantum-toolkit/yaqs project, changelog entries follow the template: "- $TITLE ([#$NUMBER]($URL)) ([**AUTHOR**](https://github.com/$AUTHOR))". Issue references should not be included in changelog entries; the PR number is sufficient for traceability.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04-arm, gcc, Release) / 🐧 ubuntu-24.04-arm gcc Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Debug) / 🐧 ubuntu-24.04 gcc Debug
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Release) / 🐧 ubuntu-24.04 gcc Release
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🇨 Lint / 🚨 Lint
🔇 Additional comments (1)
src/backend/dd/DDSimDebug.cpp (1)
593-608: LGTM: Proper state reset on code load.The explicit clearing of all instruction-related metadata and setting
ready = falseensures each execution starts with a clean state. This aligns with the PR's objective to reset greyed-out highlights between executions.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
bindings/InterfaceBindings.cppsrc/common/parsing/CodePreprocessing.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-12-15T01:54:22.129Z
Learnt from: denialhaag
Repo: munich-quantum-toolkit/core PR: 1383
File: bindings/dd/register_matrix_dds.cpp:64-109
Timestamp: 2025-12-15T01:54:22.129Z
Learning: In the munich-quantum-toolkit/core repository, after migrating to nanobind, docstrings for Python bindings are now added directly in the C++ binding code (using R"pb(...)pb" syntax) and stub files (.pyi) are auto-generated using the `bindings/generate-stubs.sh` script. This replaces the previous pybind11 approach where docstrings were manually maintained in stub files.
Applied to files:
bindings/InterfaceBindings.cpp
🧬 Code graph analysis (1)
bindings/InterfaceBindings.cpp (1)
test/utils/common_fixtures.hpp (6)
code(211-215)code(211-213)code(236-244)code(236-237)code(254-256)code(254-254)
🪛 Cppcheck (2.19.0)
src/common/parsing/CodePreprocessing.cpp
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
bindings/InterfaceBindings.cpp
[information] 26-26: Include file
(missingIncludeSystem)
[information] 27-27: Include file
(missingIncludeSystem)
[information] 28-28: Include file
(missingIncludeSystem)
[information] 29-29: Include file
(missingIncludeSystem)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Release) / 🏁 windows-2022 msvc Release
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🇨 Test 🏁 (windows-11-arm, msvc, Release) / 🏁 windows-11-arm msvc Release
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Debug) / 🍎 macos-14 clang Debug
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Debug) / 🏁 windows-2022 msvc Debug
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Debug) / 🐧 ubuntu-24.04 gcc Debug
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Release) / 🍎 macos-14 clang Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04-arm, gcc, Release) / 🐧 ubuntu-24.04-arm gcc Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Release) / 🐧 ubuntu-24.04 gcc Release
- GitHub Check: 🇨 Lint / 🚨 Lint
🔇 Additional comments (14)
bindings/InterfaceBindings.cpp (2)
26-26: LGTM! Includes are necessary for error capture.The
<iostream>header providesstd::cerrand<pybind11/iostream.h>providespy::scoped_ostream_redirect, both of which are used in the updatedload_codebinding.Also applies to: 29-29
177-193: Excellent improvement to error reporting!The new implementation captures stderr output during
loadCodeexecution via Python'sStringIO, providing users with actual parse error messages instead of generic error text. The use ofpy::scoped_ostream_redirectensures proper cleanup through RAII, and the fallback message prevents empty errors.src/common/parsing/CodePreprocessing.cpp (12)
23-23: LGTM! Necessary includes for new functionality.The includes support
std::isdigit(line 43) andstd::exception(lines 146, 501).Also applies to: 25-25
38-44: LGTM! Robust digit validation.The empty-string check and unsigned char cast properly handle edge cases for
std::isdigit.
46-49: LGTM! Clear structure for line/column positions.The 1-based initialization aligns with standard editor conventions.
67-91: LGTM! Well-designed target position fallback.The function gracefully falls back to the first non-space character when the target is not found, providing helpful error positioning.
93-99: LGTM! Standard error message formatting.The
<input>:line:column:format follows common compiler conventions for error reporting.
101-108: LGTM! Clear error message construction.
110-115: LGTM! Clear error message construction.
117-162: LGTM! Comprehensive target validation with robust error handling.The validation properly handles:
- Format checking (register name must be non-empty, proper bracket nesting)
- Numeric index validation with overflow protection
- Shadowed register handling
- Bounds checking
The try-catch around
std::stoul(lines 144-150) correctly guards against numeric overflow even afterisDigitsvalidation.
463-467: LGTM! Improved error positioning accuracy.Skipping leading whitespace ensures error highlights point to the actual instruction start rather than preceding spaces, improving the user experience.
493-504: LGTM! Robust register validation with detailed error reporting.The strengthened validation correctly handles:
- Missing sizes
- Non-numeric sizes
- Negative sizes (via
isDigitsrejecting '-')- Overflow (via exception handling)
The formatted error messages with line/column info significantly improve the debugging experience.
569-570: LGTM! Consolidated validation for assertion targets.Delegating to
validateTargetseliminates code duplication and provides consistent error formatting across all validation paths.
575-578: LGTM! Consolidated validation for regular instruction targets.The conditional check properly excludes variable declarations from target validation, as they have distinct syntax rules validated separately (lines 493-504).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
python/mqt/debugger/dap/messages/highlight_error_dap_message.py (1)
58-84: Inconsistent message validation between normalization and validation.At line 79,
_normalize_highlightsetsmessageto""as a default for missing or whitespace-only messages. However,validate()(lines 48-50) later requires each message to be non-empty. This defers the error from normalization to validation.Consider failing immediately during normalization if a non-empty message is required, so callers receive earlier feedback.
🔎 Proposed fix
Validate the message during normalization:
normalized["instruction"] = int(normalized.get("instruction", -1)) normalized["reason"] = str(normalized.get("reason", "unknown")) normalized["code"] = str(normalized.get("code", "")) normalized["message"] = str(normalized.get("message", "")).strip() + if not normalized["message"]: + msg = "Each highlight entry must contain a descriptive 'message'." + raise ValueError(msg) normalized["range"] = { "start": start, "end": end, }Then simplify
validate()to only check that highlights is non-empty, since per-highlight validation is already done:def validate(self) -> None: """Validate the 'highlightError' DAP event message after creation.""" if not self.highlights: msg = "At least one highlight entry is required to show the issue location." raise ValueError(msg) - - for highlight in self.highlights: - if "message" not in highlight or not str(highlight["message"]).strip(): - msg = "Each highlight entry must contain a descriptive 'message'." - raise ValueError(msg)src/common/parsing/CodePreprocessing.cpp (1)
51-65: Bounds validation still missing.The previous review comment regarding the lack of bounds validation for
offsetremains applicable. Ifoffsetexceedscode.size(), the column calculation on line 63 will produce incorrect results.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
python/mqt/debugger/dap/messages/highlight_error_dap_message.pysrc/common/parsing/CodePreprocessing.cpp
🧰 Additional context used
🧬 Code graph analysis (1)
src/common/parsing/CodePreprocessing.cpp (3)
include/common/parsing/ParsingError.hpp (1)
ParsingError(32-32)src/common/parsing/ParsingError.cpp (1)
ParsingError(23-23)include/common/parsing/CodePreprocessing.hpp (1)
isVariableDeclaration(296-296)
🪛 Cppcheck (2.19.0)
src/common/parsing/CodePreprocessing.cpp
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
[information] 23-23: Include file
(missingIncludeSystem)
[information] 24-24: Include file
(missingIncludeSystem)
[information] 25-25: Include file
(missingIncludeSystem)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: 🇨 Lint / 🚨 Lint
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Debug) / 🍎 macos-14 clang Debug
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04-arm, gcc, Release) / 🐧 ubuntu-24.04-arm gcc Release
- GitHub Check: 🇨 Test 🏁 (windows-11-arm, msvc, Release) / 🏁 windows-11-arm msvc Release
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Release) / 🍎 macos-14 clang Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Debug) / 🐧 ubuntu-24.04 gcc Debug
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Release) / 🐧 ubuntu-24.04 gcc Release
- GitHub Check: 🇨 Test 🍎 (macos-15-intel, clang, Release) / 🍎 macos-15-intel clang Release
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Release) / 🏁 windows-2022 msvc Release
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Debug) / 🏁 windows-2022 msvc Debug
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
🔇 Additional comments (7)
src/common/parsing/CodePreprocessing.cpp (7)
38-44: LGTM!The
isDigitsimplementation correctly handles the empty string case and properly casts tounsigned charbefore callingstd::isdigitto avoid undefined behavior with negative char values.
67-91: LGTM!The function correctly calculates line/column positions for error reporting, with appropriate fallback to the first non-whitespace character when the target is not found.
93-115: LGTM!Clean utility functions for consistent error message formatting.
117-162: LGTM!The
validateTargetsfunction is well-implemented with:
- Proper bracket syntax validation
- Guarded numeric parsing with
isDigitscheck followed by try/catch for overflow protection- Correct handling of shadowed registers (function parameters)
- Proper bounds checking against defined register sizes
463-467: LGTM!The improved
trueStartcalculation correctly skips leading whitespace for more accurate error position reporting while properly maintaining bounds within the current instruction.
493-504: LGTM!The strengthened register declaration validation provides:
- Proper rejection of declarations missing size specifications
- Guarded numeric parsing with both
isDigitsand try/catch for overflow edge cases- Descriptive error messages with accurate position information
569-578: LGTM!Proper integration of
validateTargetsfor both assertions and regular instructions, with the correct guard to skip variable declarations which don't have qubit targets to validate.
| def __init__(self, highlights: Sequence[Mapping[str, Any]], source: Mapping[str, Any] | None) -> None: | ||
| """Create a new 'highlightError' DAP event message. | ||
| Args: | ||
| highlights (Sequence[Mapping[str, Any]]): Highlight entries describing the problematic ranges. | ||
| source (Mapping[str, Any] | None): Information about the current source file. | ||
| """ | ||
| self.highlights = [self._normalize_highlight(entry) for entry in highlights] | ||
| self.source = self._normalize_source(source) | ||
| super().__init__() |
There was a problem hiding this comment.
Type signature allows None but implementation rejects it.
The constructor signature permits source: Mapping[str, Any] | None (line 30), but _normalize_source (line 106) raises TypeError if source is not a Mapping. Passing None will cause a runtime exception.
🔎 Proposed fix
Either handle None in _normalize_source:
@staticmethod
def _normalize_source(source: Mapping[str, Any] | None) -> dict[str, Any]:
"""Create a defensive copy of the provided DAP Source information."""
+ if source is None:
+ msg = "Source information is required."
+ raise ValueError(msg)
if not isinstance(source, Mapping):
msg = "Source information must be provided as a mapping."
raise TypeError(msg)Or remove | None from the constructor signature if source is always required:
-def __init__(self, highlights: Sequence[Mapping[str, Any]], source: Mapping[str, Any] | None) -> None:
+def __init__(self, highlights: Sequence[Mapping[str, Any]], source: Mapping[str, Any]) -> None:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def __init__(self, highlights: Sequence[Mapping[str, Any]], source: Mapping[str, Any] | None) -> None: | |
| """Create a new 'highlightError' DAP event message. | |
| Args: | |
| highlights (Sequence[Mapping[str, Any]]): Highlight entries describing the problematic ranges. | |
| source (Mapping[str, Any] | None): Information about the current source file. | |
| """ | |
| self.highlights = [self._normalize_highlight(entry) for entry in highlights] | |
| self.source = self._normalize_source(source) | |
| super().__init__() | |
| def __init__(self, highlights: Sequence[Mapping[str, Any]], source: Mapping[str, Any]) -> None: | |
| """Create a new 'highlightError' DAP event message. | |
| Args: | |
| highlights (Sequence[Mapping[str, Any]]): Highlight entries describing the problematic ranges. | |
| source (Mapping[str, Any] | None): Information about the current source file. | |
| """ | |
| self.highlights = [self._normalize_highlight(entry) for entry in highlights] | |
| self.source = self._normalize_source(source) | |
| super().__init__() |
🤖 Prompt for AI Agents
python/mqt/debugger/dap/messages/highlight_error_dap_message.py lines 30-39: the
constructor type allows source: Mapping[str, Any] | None but calling
self._normalize_source(source) will raise when source is None; update the code
so None is handled consistently—either remove the optional from the constructor
signature if source must always be provided, or (preferred) keep the optional
and modify _normalize_source to accept None (returning None or a normalized
empty/default mapping) and adjust the constructor's annotated type to
typing.Optional[Mapping[str, Any]] so the runtime and type annotations match.
There was a problem hiding this comment.
I agree with @coderabbitai here, if the source field is allowed to be None, then the _normalize_source method and its signature should be updated.
If it really should not be None, then the __init__ signature should be updated.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for HighlightError instantiation to see how source parameter is used
rg -n "HighlightError\(" --type=py -A 3 -B 1
Length of output: 1443
🏁 Script executed:
#!/bin/bash
# Search for source_file in DAPServer to see its initialization and usage
rg -n "source_file" --type=py python/mqt/debugger/dap/dap_server.py -C 2
Length of output: 1276
🏁 Script executed:
#!/bin/bash
# Search for source_file assignments to see if it's always initialized
rg -n "self\.source_file\s*=" --type=py python/mqt/debugger/dap/ -C 2
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for source_file assignments with broader pattern
rg -n "source_file =" --type=py python/mqt/debugger/dap/ -C 2
Length of output: 1136
@DRovara, thank you for the confirmation! Based on my analysis of the codebase, I recommend removing | None from the constructor signature.
Here's why:
-
Actual usage never passes
None: Both instantiations ofHighlightErrorindap_server.py(lines 258 and 359) passself.source_file, which is always initialized as a dict with"name"and"path"keys before these calls. -
Field declaration expects a dict: In
DAPServer,source_fileis declared assource_file: dict[str, Any](not optional), and it's set in bothlaunch_dap_message.pyandrestart_dap_message.pybefore anyHighlightErrorinstances are created. -
_normalize_sourcevalidates required fields: The method checks for"name"and"path"keys, which indicates source information is essential for the event.
Recommended fix:
def __init__(self, highlights: Sequence[Mapping[str, Any]], source: Mapping[str, Any]) -> None:
"""Create a new 'highlightError' DAP event message.
Args:
highlights (Sequence[Mapping[str, Any]]): Highlight entries describing the problematic ranges.
source (Mapping[str, Any]): Information about the current source file.
"""
self.highlights = [self._normalize_highlight(entry) for entry in highlights]
self.source = self._normalize_source(source)
super().__init__()This makes the signature consistent with the implementation and actual usage patterns.
svenjeschmitt-ops
changed the title
svenjeschmitt-ops
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
python/mqt/debugger/dap/messages/highlight_error_dap_message.py (2)
36-45: Critical: Type signature mismatch -Nonenot handled.The constructor allows
source: Mapping[str, Any] | None, but_normalize_source(line 153) raisesTypeErrorwhensourceis not aMapping. PassingNonecauses a runtime exception.🔎 Recommended fix
Either handle
Noneexplicitly in_normalize_source:@staticmethod def _normalize_source(source: Mapping[str, Any] | None) -> dict[str, Any]: """Create a defensive copy of the provided DAP Source information.""" + if source is None: + msg = "Source information is required." + raise ValueError(msg) if not isinstance(source, Mapping): msg = "Source information must be provided as a mapping." raise TypeError(msg)Or remove
| Nonefrom the constructor ifsourceis always required:-def __init__(self, highlights: Sequence[Mapping[str, Any]], source: Mapping[str, Any] | None) -> None: +def __init__(self, highlights: Sequence[Mapping[str, Any]], source: Mapping[str, Any]) -> None:
47-61: Validation partially duplicates normalization checks.
validate()checks for non-empty highlights and messages, while_normalize_highlight(lines 86-98) validates range structure. There's overlap in that both methods touch the highlights list and message field, thoughvalidate()enforces non-empty messages post-normalization while_normalize_highlightonly normalizes. Consider consolidating to reduce duplication.python/mqt/debugger/dap/dap_server.py (1)
513-547: Minor: Redundant condition on Line 542.Line 542's ternary
end_column if end_column > 0 else columnis redundant. Sincecolumn >= 1(line 521) andend_column = max(column, len(line_text) + 1)(line 536),end_columnis always positive.Otherwise, the empty-line heuristic (lines 525-534) is a thoughtful UX improvement for locating errors on blank lines.
🔎 Optional simplification
return { "instruction": -1, "range": { "start": {"line": line, "column": column}, - "end": {"line": line, "column": end_column if end_column > 0 else column}, + "end": {"line": line, "column": end_column}, }, "reason": "parseError", "code": snippet, "message": detail, }
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
python/mqt/debugger/dap/dap_server.pypython/mqt/debugger/dap/messages/highlight_error_dap_message.py
🧰 Additional context used
🧬 Code graph analysis (2)
python/mqt/debugger/dap/messages/highlight_error_dap_message.py (1)
python/mqt/debugger/dap/messages/dap_event.py (1)
DAPEvent(17-41)
python/mqt/debugger/dap/dap_server.py (2)
python/mqt/debugger/dap/messages/highlight_error_dap_message.py (2)
HighlightError(22-179)encode(62-70)python/mqt/debugger/dap/messages/output_dap_event.py (2)
encode(54-69)OutputDAPEvent(18-69)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Release) / 🏁 windows-2022 msvc Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04-arm, gcc, Release) / 🐧 ubuntu-24.04-arm gcc Release
- GitHub Check: 🇨 Test 🏁 (windows-2022, msvc, Debug) / 🏁 windows-2022 msvc Debug
- GitHub Check: 🇨 Test 🏁 (windows-11-arm, msvc, Release) / 🏁 windows-11-arm msvc Release
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Debug) / 🐧 ubuntu-24.04 gcc Debug
- GitHub Check: 🐍 Test (ubuntu-24.04-arm) / 🐍 ubuntu-24.04-arm
- GitHub Check: 🇨 Test 🐧 (ubuntu-24.04, gcc, Release) / 🐧 ubuntu-24.04 gcc Release
- GitHub Check: 🐍 Test (macos-15-intel) / 🐍 macos-15-intel
- GitHub Check: 🇨 Test 🍎 (macos-15-intel, clang, Release) / 🍎 macos-15-intel clang Release
- GitHub Check: 🐍 Test (ubuntu-24.04) / 🐍 ubuntu-24.04
- GitHub Check: 🐍 Test (macos-14) / 🐍 macos-14
- GitHub Check: 🐍 Test (windows-2022) / 🐍 windows-2022
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Debug) / 🍎 macos-14 clang Debug
- GitHub Check: 🇨 Test 🍎 (macos-14, clang, Release) / 🍎 macos-14 clang Release
- GitHub Check: 🇨 Lint / 🚨 Lint
🔇 Additional comments (18)
python/mqt/debugger/dap/messages/highlight_error_dap_message.py (4)
62-70: LGTM: Encoding follows DAP event pattern.The
encode()method correctly extends the base event structure and wraps the normalized data underbody, consistent with other DAP event implementations.
72-109: LGTM: Robust normalization with validation.
_normalize_highlightprovides thorough validation of range structure and ordering, with sensible defaults for optional fields. The defensive checks and error messages are clear.
111-137: LGTM: Position normalization is defensive.The type check and KeyError handling ensure required fields are present and convertible to integers, with clear error messages for debugging.
139-162: Source normalization is correct but see constructor issue.The validation and normalization logic is sound for non-
Nonesources. However, this method's rejection ofNonecreates the type mismatch issue flagged in the constructor review (lines 36-45).python/mqt/debugger/dap/dap_server.py (14)
14-14: LGTM: Import supports parse error handling.The
remodule import is used for parsing error message locations in_parse_error_location(line 502).
118-119: LGTM: State variables properly initialized.
pending_highlightsand_prevent_exitare correctly typed and initialized to support the highlight queuing and exit prevention logic.
172-186: LGTM: Exit prevention reset on execution commands.Resetting
_prevent_exitfor execution-resuming commands (Next, Step*, Continue, Restart, Launch) correctly allows exit events after clean execution while preventing them after parse/assertion errors.
257-261: LGTM: Highlight emission and exit prevention.The logic correctly emits queued highlights, clears the queue, and sets
_prevent_exitto prevent premature exit events after parse errors.
271-275: LGTM: Exit condition respects prevention flag.The condition correctly prevents
ExitedDAPEventwhen_prevent_exitisTrue, allowing the session to remain active after parse or assertion errors for user review.
357-361: LGTM: Assertion failure highlights emitted.Collecting and emitting highlight entries for assertion failures with
_prevent_exitcorrectly prevents premature session termination while displaying error context.
363-389: LGTM: Position calculation handles edge cases.The logic correctly handles empty source, positions exceeding the source length, and normal cases. The
for-elseclause appropriately handles overflow by using the last line's bounds.
431-457: LGTM: Highlight collection with defensive fallback.The method defensively handles diagnostic errors and provides a sensible fallback to highlight the failing instruction when no specific error causes are identified.
459-481: LGTM: Highlight entry construction is defensive.The method safely handles runtime errors from position lookups and appropriately calculates end positions to exclude trailing newlines for cleaner highlighting.
483-490: LGTM: Error cause mapping is clear.The mapping from
ErrorCauseTypeto string identifiers is straightforward, with a sensible default for unknown types.
492-497: LGTM: Parse error queuing is appropriate.Extracting location from the error message and replacing
pending_highlightsis correct for parse errors, which occur once per launch/restart.
499-511: LGTM: Error location parsing with fallback.The regex pattern correctly parses compiler error format, with a sensible fallback to line 1, column 1 when the format doesn't match.
549-641: LGTM: Message sending refactored for modularity.The refactoring into
_flatten_message_partsandsend_message_simpleimproves code organization. The recursive flattening correctly handles nested message structures (strings, dicts, lists), and the delegation fromsend_message_hierarchytosend_message_simplereduces duplication.
643-659: LGTM: State reporting is comprehensive.The method checks all relevant execution states (assertion failed, breakpoint, finished, running) and sends clear status messages to the client using the appropriate messaging infrastructure.
| @staticmethod | ||
| def _later_than(start: Mapping[str, Any], end: Mapping[str, Any]) -> bool: | ||
| """Return True if 'end' describes a position before 'start'. | ||
| Args: | ||
| start (Mapping[str, Any]): The start position mapping. | ||
| end (Mapping[str, Any]): The end position mapping. | ||
| Returns: | ||
| bool: True when the end position is before the start position. | ||
| """ | ||
| start_line = int(start.get("line", 0)) | ||
| start_column = int(start.get("column", 0)) | ||
| end_line = int(end.get("line", 0)) | ||
| end_column = int(end.get("column", 0)) | ||
| return (end_line, end_column) < (start_line, start_column) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider renaming for clarity.
The method returns True when end is before start, but the name _later_than might suggest the opposite. Consider renaming to _is_before or _end_before_start for clarity.
🤖 Prompt for AI Agents
In python/mqt/debugger/dap/messages/highlight_error_dap_message.py around lines
164 to 179, the private method name _later_than is misleading because it returns
True when the end position is before the start; rename the method to a clearer
name such as _end_before_start (or _is_before) and update its docstring to match
the new name/semantics, then refactor all internal references/usages in this
module (and any tests) to call the new name so behavior stays the same.
There was a problem hiding this comment.
Agree with @coderabbitai, maybe something like _start_comes_after_end or so?
There was a problem hiding this comment.
@DRovara, excellent suggestion! _start_comes_after_end reads very clearly. Here's the refactoring with your suggested name:
Updated method definition:
@staticmethod
def _start_comes_after_end(start: Mapping[str, Any], end: Mapping[str, Any]) -> bool:
"""Return True if 'start' describes a position after 'end'.
Args:
start (Mapping[str, Any]): The start position mapping.
end (Mapping[str, Any]): The end position mapping.
Returns:
bool: True when the start position is after the end position.
"""
start_line = int(start.get("line", 0))
start_column = int(start.get("column", 0))
end_line = int(end.get("line", 0))
end_column = int(end.get("column", 0))
return (end_line, end_column) < (start_line, start_column)Updated usage in _normalize_highlight:
start = HighlightError._normalize_position(highlight_range.get("start"))
end = HighlightError._normalize_position(highlight_range.get("end"))
if HighlightError._start_comes_after_end(start, end):
msg = "Highlight range 'end' must be after 'start'."
raise ValueError(msg)All three suggestions (_end_before_start, _is_before, _start_comes_after_end) are significant improvements over _later_than. Your choice is nicely descriptive!
DRovara
left a comment
DRovara
left a comment
There was a problem hiding this comment.
Thanks a lot for the work and sorry for the late review. This ended up becoming quite a big PR, so I hope I have covered everything. Either way, at some point I would probably also like to manually try everything out myself, but I guess that can wait until after we merge this.
I have a couple of requested changes. Some of them should be quite simple, but there are also some bigger ones that might need larger restructuring. I hope that works out fine for you.
| checkOrThrow(self->loadCode(self, code)); | ||
| const py::module io = py::module::import("io"); | ||
| const py::object stringIo = io.attr("StringIO")(); | ||
| Result result = OK; | ||
| { | ||
| const py::scoped_ostream_redirect redirect(std::cerr, stringIo); | ||
| result = self->loadCode(self, code); | ||
| } | ||
| if (result != OK) { | ||
| auto message = stringIo.attr("getvalue")().cast<std::string>(); | ||
| if (message.empty()) { | ||
| message = "An error occurred while executing the operation"; | ||
| } | ||
| throw std::runtime_error(message); | ||
| } |
There was a problem hiding this comment.
This seems like a really hacky solution.
I'd rather prefer if there was some other way of getting the error message that does not involve redirecting STDERR. Here are some potential solutions I can imagine:
- rather than returning a
Result,loadCodecould be updated to return some other enum value that has a value for each type of error that could come up when parsing and pre-defined strings are defined for each error type. loadCodereceives an additionalchar* errorMessageparameter that expects a fixed-size (e.g. 255)charbuffer to which the error message can be written.- We define a new
structcalledLoadResultthat contains all important information that could come up with a parsing error (uint errorCode,char errorMessage[255],uint line...) and use that as a return type forloadCode
I personally feel like the third suggestion might be the best one.
| def __init__(self, highlights: Sequence[Mapping[str, Any]], source: Mapping[str, Any] | None) -> None: | ||
| """Create a new 'highlightError' DAP event message. | ||
| Args: | ||
| highlights (Sequence[Mapping[str, Any]]): Highlight entries describing the problematic ranges. | ||
| source (Mapping[str, Any] | None): Information about the current source file. | ||
| """ | ||
| self.highlights = [self._normalize_highlight(entry) for entry in highlights] | ||
| self.source = self._normalize_source(source) | ||
| super().__init__() |
There was a problem hiding this comment.
I agree with @coderabbitai here, if the source field is allowed to be None, then the _normalize_source method and its signature should be updated.
If it really should not be None, then the __init__ signature should be updated.
| def collect_highlight_entries(self, failing_instruction: int) -> list[dict[str, Any]]: | ||
| """Collect highlight entries for the current assertion failure.""" | ||
| highlights: list[dict[str, Any]] = [] | ||
| if getattr(self, "source_code", ""): |
There was a problem hiding this comment.
Why do we need this check? Is there no cleaner way to do it?
| diagnostics = self.simulation_state.get_diagnostics() | ||
| error_causes = diagnostics.potential_error_causes() |
There was a problem hiding this comment.
From my understanding, this method is always called when handle_assertion_fail happens. However, in that method, there is already a call to potential_error_causes(). Since that call could, in some cases, be rather small, I'm not sure if we really want to do it twice.
Is there any way to instead pass the error causes on as parameters to this method?
| def _format_highlight_reason(cause_type: mqt.debugger.ErrorCauseType | None) -> str: | ||
| """Return a short identifier for the highlight reason.""" | ||
| if cause_type == mqt.debugger.ErrorCauseType.MissingInteraction: | ||
| return "missingInteraction" |
There was a problem hiding this comment.
Can we maybe use a "cleaner" datatype - i.e., an enum - for these indicators?
| auto* ddsim = toDDSimulationState(self); | ||
| if (!ddsim->ready) { | ||
| return ERROR; | ||
| } |
There was a problem hiding this comment.
Just out of interest - is there any particular reason this check is needed here? According to coderabbit, we don't have similar checks in other run methods.
| if (target.empty()) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Why do we allow a target to be an empty string here?
| def send_state(self, connection: socket.socket) -> None: | ||
| """Send the state of the current execution to the client. | ||
| Args: | ||
| connection (socket.socket): The client socket. | ||
| """ | ||
| output_lines = [] | ||
| if self.simulation_state.did_assertion_fail(): | ||
| output_lines.append("Assertion failed") | ||
| if self.simulation_state.was_breakpoint_hit(): | ||
| output_lines.append("Breakpoint hit") | ||
| if self.simulation_state.is_finished(): | ||
| output_lines.append("Finished") | ||
| if not output_lines: | ||
| output_lines.append("Running") | ||
| for line_text in output_lines: | ||
| self.send_message_simple(line_text, None, None, 0, 0, connection) |
There was a problem hiding this comment.
What's the idea of this method? Where is it used (I can't seem to find any uses of it in the code)
| @staticmethod | ||
| def _later_than(start: Mapping[str, Any], end: Mapping[str, Any]) -> bool: | ||
| """Return True if 'end' describes a position before 'start'. | ||
| Args: | ||
| start (Mapping[str, Any]): The start position mapping. | ||
| end (Mapping[str, Any]): The end position mapping. | ||
| Returns: | ||
| bool: True when the end position is before the start position. | ||
| """ | ||
| start_line = int(start.get("line", 0)) | ||
| start_column = int(start.get("column", 0)) | ||
| end_line = int(end.get("line", 0)) | ||
| end_column = int(end.get("column", 0)) | ||
| return (end_line, end_column) < (start_line, start_column) |
There was a problem hiding this comment.
Agree with @coderabbitai, maybe something like _start_comes_after_end or so?
There was a problem hiding this comment.
The "best practice" we have built so far in this project is for every C++ function to have a docstring either with the header declaration in the hpp file, or, if no such header declaration exists, when the actual function is defined in the cpp file, even for private namespace functions.
Therefore, it would be nice if you could also add docstrings to the functions you defined here in the same way.
2 participants
Description
This pull request improves the debugging highlight functionality and fixes an issue where greyed-out highlights were not correctly reset between executions.
When an issue is detected in the code, the extension highlights the affected line in red and displays a descriptive error message explaining the nature of the problem. This visual feedback helps users quickly identify both the location of the error and its underlying cause.
A second part of the implementation concerns the assertion functionality. After an assertion failure, the extension greys out all lines of code that cannot have caused the error. While this behaviour is intended, the greyed-out highlights were previously not cleared when a new execution started. As a result, lines remained greyed out until the editor was closed and reopened.
Overall, the highlighting functionality provides an efficient way to show where an error occurred and what caused it. This change introduces an explicit reset of all greyed-out highlights on launch and restart events, ensuring that each execution starts with a clean editor state.
Fixes #(issue)
Checklist: