Repository navigation
feat: brace-mode multi-statement support for expression evaluator (#8) - #11
Merged
Merged
Conversation
Closes #8 User input wrapped in `{ ... }` is now spliced into the wrapper's method body verbatim instead of being treated as a single expression. The mode is detected by a tokenizer-aware brace-match that ignores braces inside string/char/text-block literals, so `"{x}".length()` still routes through expression mode while `{ try { return foo(); } catch (...) }` routes through block mode. In block mode the user is responsible for `return X;` statements; a trailing `return null;` fallthrough guard keeps the wrapper type-correct if the user block doesn't end with a return. This automatically extends to every eval call site routing through `evaluate()` -- jdwp_evaluate_expression, jdwp_assert_expression, breakpoint conditions, logpoint expressions and conditions, exception logpoints, field watchpoint conditions/expressions, and watchers. Tool descriptions on all 8 eval-bearing tool methods/params updated with a `(supports `{ ...; return X; }` block syntax)` suffix so the feature is discoverable from any eval-shaped param in isolation. Tests added: 9 in JdiExpressionEvaluatorRewriteTest covering block-mode detection, nested braces, brace-inside-string-literal escapes, and expression-mode fallback for `{x}.foo()`-style inputs.
… warn on multi-Location lines Closes #9 Two cheap diagnostics that don't change behaviour but make install-time mysteries easier to debug: 1) CLASS_PREPARE event recorded in EventHistory. Without this, jdwp_get_events shows only [VM_START, VM_DEATH] for sessions that sit on a deferred BP — agents can't tell "class never loaded" from "class loaded but BP did not fire". 2) describeLocation(method+bci) logged at every BP install. When locationsOfLine(line) returns more than one Location (typical for lambdas: one in the enclosing method, one inside the synthetic `lambda$...$N`), we now emit a BP_MULTI_LOCATION event + WARN log + WARNING suffix in the tool response so the "BP installed but silently misses one of the code paths" case is impossible to miss. No behavioural change — the BP-bind logic still uses .get(0). A proper multi-location bind is tracked as a separate concern; this issue is diagnostics-only. Touched: - JdiEventListener: describeLocation helper, CLASS_PREPARE event in EventHistory, BP_MULTI_LOCATION event + WARN log on multi-location lines. - JDWPTools.jdwp_set_breakpoint / jdwp_set_logpoint: WARNING suffix in the synchronous tool response when locations.size() > 1.
There was a problem hiding this comment.
Pull request overview
Adds IntelliJ-style { ... } “block mode” to the JDI expression evaluator so users can submit multi-statement method-body snippets (e.g., try/catch, intermediate locals, early return) instead of being limited to a single Java expression. This flows through all evaluation call sites routed via JdiExpressionEvaluator.evaluate() and updates the relevant MCP tool descriptions to advertise the feature.
Changes:
- Add
isBlockMode(input)to detect top-level brace-wrapped inputs while skipping string/char/text-block contents. - Update wrapper source generation to splice block bodies into the generated method and keep expression mode unchanged.
- Extend unit tests to cover block-mode detection edge cases and update tool/param descriptions to mention
{ ... }syntax.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java |
Adds block-mode detection + block-mode source generation path for evaluation wrappers. |
jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JDWPTools.java |
Updates MCP tool/param descriptions to document block syntax for eval-bearing tools. |
jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java |
Adds unit tests for isBlockMode detection behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Javadoc on generateSourceCode and isBlockMode: replace malformed
`{@code {}` and `{@code }}` inline tags (the `}` closes the inline
tag prematurely) with plain '{' and '}' references in prose.
- isBlockMode now skips Java line (`//…`) and block (`/*…*/`) comments
alongside string/char/text-block literals. Without this, valid blocks
like `{ // }` + body or `{ /* } */ … }` would be misclassified as
expression-mode and fail compilation.
- Unterminated comment that swallows the trailing `}` is detected
(skip past `n`) and reported as not-block-mode rather than left
silently miscounted.
- generateSourceCode block-mode body now wraps the user body in
`if (__mcpFallthroughGuard) { ... } return null;` where the guard
is a non-final local set to true. JLS §15.29 says only final
variables initialised with constant expressions are constant
expressions, so the compiler cannot prove the post-if `return null;`
unreachable. Fixes the "unreachable code" compile error when the
user's block ends with an explicit `return X;` (the previous trailing
`return null;` was statically unreachable in that case).
- evaluate() now SKIPS the bare-field-reference rewrite when the input
is in block mode. The rewriter is identifier-level and cannot tell
a field reference from a local-variable declaration; in block mode
`int count = 1; ...` would otherwise become
`int _this.count = 1; ...`. Block-mode users are expected to use
explicit `this.field` / `_this.field` references; the keyword
rewrite in generateSourceCode still handles `this.field` for them.
Tests added (4 in JdiExpressionEvaluatorRewriteTest):
- isBlockMode skips `}` inside a `/* ... */` block comment
- isBlockMode skips `}` inside a `//` line comment
- isBlockMode handles mixed nested braces + comment-buried braces
- isBlockMode tolerates unterminated block comment (returns false
because the comment swallows the trailing `}`)
- Race-guard install path in jdwp_set_breakpoint AND jdwp_set_logpoint now emits the multi-Location WARNING suffix too, not just the eager path. A lambda-bearing line that loads BETWEEN the initial classesByName check and the ClassPrepareRequest registration goes through the race-guard branch; previously it bound .get(0) and returned without warning, so the lambda silently missed depending on timing. Factored the formatter into a shared `multiLocationSuffix` helper so both paths in both tools use one definition. - describeLocation is now no-throw end-to-end. Even the toString() fallback can raise JDI runtime exceptions (e.g. VMDisconnectedException during VM tear-down); wrapping it ensures a single diagnostic call cannot abort the surrounding BP install path. Final fallback returns the literal "<location-unavailable>" so the diagnostic still emits. - ClassPrepareEvent diagnostic now distinguishes line breakpoints from logpoints. Logpoints share the pending-line path, so the previous always-"BP" labels would mislead an agent grepping for "logpoint". Uses breakpointTracker.isLogpoint(id) to pick between "BP"/"LP" in the log message and EventHistory entry, and adds a `kind` key to the event metadata. Tests added (4 in JdiEventListenerClassPrepareTest): - CLASS_PREPARE event recorded when a CPE activates pending items - CLASS_PREPARE NOT recorded when no pending items reference the class (short-circuit path) - BP_MULTI_LOCATION recorded for a lambda-bearing line (multiple Locations) with the new "BP " label prefix - BP_MULTI_LOCATION uses "LP " label when the pending entry is a logpoint (has a logpoint expression attached)
…branch
The literal was `{a;}+b;` which has no trailing `}`, so isBlockMode rejected
it at the cheap end-char guard and never reached the early `depth == 0`
branch the test claims to cover. Use `{a;}+b;}` so the input clears the
end-char guard and the rejection comes from the brace-balance walk.
…scribeLocation Addresses second-round PR #12 review: - Document that CLASS_PREPARE is recorded only when a prepare activates pending work (never for every class load) so the contract is explicit. - Split the line-pending count into BP(s) vs logpoint(s) — both ride the same deferred-line path, so reporting all as "line BP(s)" misattributed logpoints. - Compute describeLocation(location) once and reuse across log/summary/details; it is best-effort and can change/throw during a VM tear-down race. - Add CLASS_PREPARE, BP_MULTI_LOCATION, BP_PROMOTION_FAILED to EventHistory's documented event-type list. - Add test pinning the BP/LP count split.
… exhaustive Previously only the deferred (ClassPrepare) activation path recorded a BP_MULTI_LOCATION event + WARN log; the eager and race-guard install paths in jdwp_set_breakpoint / jdwp_set_logpoint only appended a synchronous WARNING suffix. As a result jdwp_get_events showed the diagnostic for deferred binds only. Replace the pure multiLocationSuffix with an instance multiLocationDiagnostic that also logs and records the same BP_MULTI_LOCATION event (identical type and detail keys as the deferred path), so the diagnostic surfaces uniformly regardless of bind timing. The response suffix text is unchanged. Also make EventHistory's documented event-type list exhaustive — it omitted STEP_SUPPRESSED and RECONNECT — and note it must be kept in sync.
…oc producers Thread 2 & 3 (real bug): both jdwp_set_breakpoint and jdwp_set_logpoint race-guard recheck paths emitted multiLocationDiagnostic (WARN log + BP_MULTI_LOCATION event + response suffix) unconditionally — even when promotePendingToActive lost the race and the just-created request was deleted. The winning path (the ClassPrepare listener) already records its own BP_MULTI_LOCATION, so this double-counted. Move the diagnostic into the promotion-won branch; the lost-race return now notes "(bound by a concurrent activation path)" without re-warning. +2 tests (win/lose race). Thread 1: EventHistory is not single-producer — reword the class javadoc to acknowledge install-time records from MCP worker threads (BP_MULTI_LOCATION from JDWPTools, RECONNECT from JDIConnectionService) and that no global ordering is implied across producers.
…; doc fixes - record() method javadoc no longer claims it is called exclusively from JdiEventListener — it documents the multi-producer, any-thread contract to match the class-level javadoc. - Scope the "exhaustive" event-type list to production code (tests record ad-hoc types like "TEST" that are intentionally not listed). - Drop the @link to the private JdiEventListener#handleClassPrepareEvent (doclint can't resolve a private member) — link the class and describe the deferred path in words instead.
feat: diagnostics for deferred BP/LP install — surface CLASS_PREPARE + warn on multi-Location lines (#9)
fix: emit eval wrapper into target's package when this is non-public (#7)
# Conflicts: # jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java
novoj
added a commit
that referenced
this pull request
May 24, 2026
Every eval-bearing surface (jdwp_evaluate_expression, jdwp_assert_expression,
breakpoint conditions, logpoint expressions/conditions, exception logpoints,
field-watchpoint conditions/expressions, and watchers) now accepts a
brace-wrapped statement body in addition to a single expression. An input that
starts with `{` and ends with the matching `}` is spliced into the wrapper
method verbatim; the user writes `return X;` and a trailing `return null;`
fallthrough guard keeps the wrapper type-correct. This unlocks intermediate
locals, try/catch, early returns, and loops at a breakpoint. Resolves #8.
Mode detection is tokenizer-aware: braces inside string/char/text-block
literals don't trigger block mode, so `"{x}".length()` stays in expression
mode. All eight eval-bearing tool params advertise the block syntax in their
descriptions so it's discoverable in isolation. The bare-field `this.field`
auto-rewrite is skipped in block mode (the identifier-level rewriter can't
distinguish a field reference from a local declaration); block-mode users use
explicit this.field / _this.field.
Lands the work from PR #11, which had merged into its stacked base
(fix/eval-target-package-wrapper) rather than main and so missed 2.3.0;
re-targeted to main via PR #13.
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.
Closes #8.
Summary
Adds IntelliJ-style
{ … }block syntax to the expression evaluator. Wrap your input in braces to get a method-body context with try/catch, intermediate locals, and earlyreturn.How
isBlockMode(input)is a tokenizer-aware brace matcher — string / char / text-block contents are ignored so"{x}".length()doesn't trip it.generateSourceCodesplices the inside of the braces verbatim into the wrapper's method body and appends areturn null;fallthrough guard.jdwp_evaluate_expression,jdwp_assert_expression, BP conditions, LP expressions and conditions, exception logpoints, field watchpoints, watchers) automatically inherit this — they all route throughJdiExpressionEvaluator.evaluate().@McpTool/@McpToolParamupdated with a short(supports \{ ...; return X; }` block syntax)` suffix so the feature is discoverable in isolation.Examples
Test plan
./mvnw -pl jdwp-mcp-server test— 912 tests pass (9 new inJdiExpressionEvaluatorRewriteTest){x}.foo()(block-closer not at top level){ return "}"; }(close brace inside string literal ignored)