From 7a0fec0d59a0204cda02f93f43db71d1d0b06db3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Novotn=C3=BD?= Date: Sat, 23 May 2026 14:22:09 +0200 Subject: [PATCH 1/3] feat: brace-mode multi-statement support for expression evaluator 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. --- .../java/one/edee/mcp/jdwp/JDWPTools.java | 45 ++++----- .../evaluation/JdiExpressionEvaluator.java | 93 +++++++++++++++++-- .../JdiExpressionEvaluatorRewriteTest.java | 63 +++++++++++++ 3 files changed, 174 insertions(+), 27 deletions(-) diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JDWPTools.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JDWPTools.java index ccabf09..2e97649 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JDWPTools.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/JDWPTools.java @@ -806,10 +806,10 @@ private static boolean isDefaultObjectToString(String value) { } - @McpTool(description = "Evaluate a Java expression in the context of a suspended thread's stack frame. Returns '[VM_DEATH] ...' if the target VM disconnects mid-call.") + @McpTool(description = "Evaluate a Java expression in the context of a suspended thread's stack frame. Two input modes: (1) single expression — `order.getTotal()`, `x + y`. (2) block mode — wrap the input in `{ ... }` to send a statement body with try/catch, intermediate locals, or early `return`; the user writes explicit `return X;` statements to yield a value. Returns '[VM_DEATH] ...' if the target VM disconnects mid-call.") public String jdwp_evaluate_expression( @McpToolParam(description = "Thread unique ID") long threadId, - @McpToolParam(description = "Java expression to evaluate (e.g., 'order.getTotal()', 'x + y', 'name.length()')") String expression, + @McpToolParam(description = "Java expression OR `{ ... }` block to evaluate. Examples: `order.getTotal()`, `x + y`, `{ try { return risky(); } catch (Exception e) { return e.getMessage(); } }`") String expression, @McpToolParam(required = false, description = "Frame index (0 = current frame, default: 0)") @Nullable Integer frameIndex) { try { final VirtualMachine vm = jdiService.getVM(); @@ -839,9 +839,9 @@ public String jdwp_evaluate_expression( } } - @McpTool(description = "Evaluate a Java expression and compare its result against an expected value. Returns 'OK' on match, 'MISMATCH' with actual vs expected on failure, or '[VM_DEATH] ...' if the target VM disconnects mid-call. Comparison is string-based against the same formatting jdwp_evaluate_expression uses (so primitives auto-unbox, strings strip surrounding quotes).") + @McpTool(description = "Evaluate a Java expression and compare its result against an expected value. Returns 'OK' on match, 'MISMATCH' with actual vs expected on failure, or '[VM_DEATH] ...' if the target VM disconnects mid-call. Comparison is string-based against the same formatting jdwp_evaluate_expression uses (so primitives auto-unbox, strings strip surrounding quotes). Supports `{ block }` syntax for multi-statement.") public String jdwp_assert_expression( - @McpToolParam(description = "Java expression (e.g., 'order.getTotal()', 'session.getRole()', 'list.size() == 5')") String expression, + @McpToolParam(description = "Java expression (e.g., 'order.getTotal()', 'session.getRole()', 'list.size() == 5') — supports `{ ...; return X; }` block syntax") String expression, @McpToolParam(description = "Expected value (string-compared against the formatted expression result)") String expected, @McpToolParam(required = false, description = "Thread ID — defaults to the last breakpoint thread") @Nullable Long threadId, @McpToolParam(required = false, description = "Frame index (default: 0)") @Nullable Integer frameIndex) { @@ -1956,12 +1956,12 @@ private String doStep(@Nullable Long threadId, int stepDepth, String label) { } } - @McpTool(description = "Set a breakpoint at a specific line in a class. Supports conditional breakpoints. By default the breakpoint is registered passively: if the class is not yet loaded, the BP is deferred via a ClassPrepareRequest and activates when the JVM loads the class on its own — same behaviour as IntelliJ / Eclipse / jdb. Pass forceLoad=true only when you need the breakpoint to bind immediately and accept that loading the class will run its `` (early static init, eager dependency loads, masked lazy-load diagnostics).") + @McpTool(description = "Set a breakpoint at a specific line in a class. Supports conditional breakpoints — conditions support `{ block }` syntax for multi-statement. By default the breakpoint is registered passively: if the class is not yet loaded, the BP is deferred via a ClassPrepareRequest and activates when the JVM loads the class on its own — same behaviour as IntelliJ / Eclipse / jdb. Pass forceLoad=true only when you need the breakpoint to bind immediately and accept that loading the class will run its `` (early static init, eager dependency loads, masked lazy-load diagnostics).") public String jdwp_set_breakpoint( @McpToolParam(description = "Fully qualified class name (e.g. 'com.example.MyClass')") String className, @McpToolParam(description = "Line number") int lineNumber, @McpToolParam(required = false, description = "Suspend policy: 'all' (default), 'thread', 'none'") @Nullable String suspendPolicy, - @McpToolParam(required = false, description = "Optional condition — only suspend when this evaluates to true (e.g., 'i > 100')") @Nullable String condition, + @McpToolParam(required = false, description = "Optional condition — only suspend when this evaluates to true (e.g., 'i > 100') — supports `{ ...; return X; }` block syntax") @Nullable String condition, @McpToolParam(required = false, description = "Optional ID of a trigger breakpoint — this BP stays disarmed until the trigger fires. Sticky by default: once armed it stays so unless re-disarmed via jdwp_disarm_until_trigger or oneShot=true.") @Nullable Integer triggerBreakpointId, @McpToolParam(required = false, description = "If true, re-disarm this BP after each hit so the next trigger fire re-arms it (IntelliJ-style). Default: false (sticky).") @Nullable Boolean oneShot, @McpToolParam(required = false, description = "If true, force-load the class via Class.forName in the target VM when it isn't already loaded. Default: false (passive — defer via ClassPrepareRequest). Force-load triggers ``, cascades dependent loads, and can mask lazy-init / classloader-leak diagnostics — use sparingly.") @Nullable Boolean forceLoad) { @@ -2097,12 +2097,12 @@ public String jdwp_set_breakpoint( } } - @McpTool(description = "Set a logpoint (non-stopping breakpoint) that evaluates an expression and logs the result without pausing execution. Supports an optional condition — the expression is only logged when the condition evaluates to true. By default the logpoint is registered passively: if the class is not yet loaded, it is deferred via a ClassPrepareRequest and activates on natural load. Pass forceLoad=true to force the class load (runs ``, may mask lazy-init diagnostics).") + @McpTool(description = "Set a logpoint (non-stopping breakpoint) that evaluates an expression and logs the result without pausing execution. Supports an optional condition — the expression is only logged when the condition evaluates to true. Both expression and condition support `{ block }` syntax for multi-statement. By default the logpoint is registered passively: if the class is not yet loaded, it is deferred via a ClassPrepareRequest and activates on natural load. Pass forceLoad=true to force the class load (runs ``, may mask lazy-init diagnostics).") public String jdwp_set_logpoint( @McpToolParam(description = "Fully qualified class name") String className, @McpToolParam(description = "Line number") int lineNumber, - @McpToolParam(description = "Java expression to evaluate and log (e.g., '\"x=\" + x', 'order.getTotal()')") String expression, - @McpToolParam(required = false, description = "Optional condition — only log when this evaluates to true (e.g., 'i > 100')") @Nullable String condition, + @McpToolParam(description = "Java expression to evaluate and log (e.g., '\"x=\" + x', 'order.getTotal()') — supports `{ ...; return X; }` block syntax") String expression, + @McpToolParam(required = false, description = "Optional condition — only log when this evaluates to true (e.g., 'i > 100') — supports `{ ...; return X; }` block syntax") @Nullable String condition, @McpToolParam(required = false, description = "If true, force-load the class via Class.forName in the target VM when it isn't already loaded. Default: false (passive — defer via ClassPrepareRequest). Force-load triggers `` and can mask lazy-init / classloader-leak diagnostics — use sparingly.") @Nullable Boolean forceLoad) { // Track the pending ID outside the try so the catch can clean it up if locationsOfLine // (or any later step) throws after the pending entry has already been registered. @@ -2464,7 +2464,8 @@ public String jdwp_set_exception_breakpoint( "throw of the given exception type. The expression is evaluated against the throwing frame with " + "$exception bound to the thrown object (e.g., \"$exception.getMessage()\"); its result is attached " + "to the event. The optional condition is evaluated with the same $exception binding — the log is " + - "recorded only when the condition is true. Use this for tracing exception flows in long-running " + + "recorded only when the condition is true. Both expression and condition support `{ block }` syntax " + + "for multi-statement. Use this for tracing exception flows in long-running " + "services without halting traffic. By default the logpoint is registered passively: if the exception " + "class is not yet loaded the logpoint is deferred via a ClassPrepareRequest and activates on natural " + "load. Pass forceLoad=true when targeting an exception class the application hasn't referenced yet " + @@ -2472,8 +2473,8 @@ public String jdwp_set_exception_breakpoint( "running `` in the target VM.") public String jdwp_set_exception_logpoint( @McpToolParam(description = "Exception class name (e.g., 'java.sql.SQLException')") String exceptionClass, - @McpToolParam(description = "Java expression evaluated on each hit; $exception is bound to the thrown object") String expression, - @McpToolParam(required = false, description = "Optional condition with $exception bound — only log when this evaluates to true") @Nullable String condition, + @McpToolParam(description = "Java expression evaluated on each hit; $exception is bound to the thrown object — supports `{ ...; return X; }` block syntax") String expression, + @McpToolParam(required = false, description = "Optional condition with $exception bound — only log when this evaluates to true — supports `{ ...; return X; }` block syntax") @Nullable String condition, @McpToolParam(required = false, description = "Log caught exceptions (default: true)") @Nullable Boolean caught, @McpToolParam(required = false, description = "Log uncaught exceptions (default: true)") @Nullable Boolean uncaught, @McpToolParam(required = false, description = "Optional ID of a trigger breakpoint — this logpoint stays disarmed until the trigger fires. Sticky by default.") @Nullable Integer triggerBreakpointId, @@ -2600,15 +2601,16 @@ Exception breakpoint deferred (ID: %d) @McpTool(description = "Set a field watchpoint that suspends the firing thread when the field is read " + "(mode='access'), written (mode='modification'), or both. Conditions are evaluated against the firing " + "frame with $oldValue, $newValue (modification only), $object (null for static), $fieldName, and $mode " + - "bound. By default the BP is registered passively: if the class is not yet loaded it is deferred via " + - "a ClassPrepareRequest and activates on natural load. Pass forceLoad=true to bind immediately and " + - "accept the side effects of running `` in the target VM. ERROR if the field is ambiguous on the " + - "class, missing, or static when objectFilterId is supplied.") + "bound; conditions support `{ block }` syntax for multi-statement. By default the BP is registered " + + "passively: if the class is not yet loaded it is deferred via a ClassPrepareRequest and activates on " + + "natural load. Pass forceLoad=true to bind immediately and accept the side effects of running " + + "`` in the target VM. ERROR if the field is ambiguous on the class, missing, or static when " + + "objectFilterId is supplied.") public String jdwp_set_field_breakpoint( @McpToolParam(description = "Fully-qualified class declaring the field (e.g., 'com.example.Order')") String className, @McpToolParam(description = "Field name (must be unambiguous on the class)") String fieldName, @McpToolParam(description = "Watch mode: 'access', 'modification', or 'both' (case-insensitive)") String mode, - @McpToolParam(required = false, description = "Optional condition with $oldValue, $newValue, $object, $fieldName, $mode bound — fire only when this evaluates to true") @Nullable String condition, + @McpToolParam(required = false, description = "Optional condition with $oldValue, $newValue, $object, $fieldName, $mode bound — fire only when this evaluates to true — supports `{ ...; return X; }` block syntax") @Nullable String condition, @McpToolParam(required = false, description = "Optional thread filter — only fire when this thread (uniqueID) hits the field") @Nullable Long threadFilterId, @McpToolParam(required = false, description = "Optional object filter — only fire on the given instance (object ID from jdwp_get_locals/jdwp_get_fields). Must be omitted for static fields.") @Nullable Long objectFilterId, @McpToolParam(required = false, description = "Optional ID of a trigger breakpoint — this field BP stays disarmed until the trigger fires. Sticky by default.") @Nullable Integer triggerBreakpointId, @@ -2625,6 +2627,7 @@ public String jdwp_set_field_breakpoint( "access or modification of the field. The expression is evaluated against the firing frame with " + "$oldValue, $newValue (modification only), $object (null for static), $fieldName, and $mode bound; " + "its result is attached to the event. Optional condition gates whether the log is recorded. " + + "Both expression and condition support `{ block }` syntax for multi-statement. " + "Use this to trace state transitions in long-running services without halting traffic. By default the " + "logpoint is registered passively: if the class is not yet loaded it is deferred via a " + "ClassPrepareRequest and activates on natural load. Pass forceLoad=true to bind immediately and " + @@ -2633,8 +2636,8 @@ public String jdwp_set_field_logpoint( @McpToolParam(description = "Fully-qualified class declaring the field") String className, @McpToolParam(description = "Field name (must be unambiguous on the class)") String fieldName, @McpToolParam(description = "Watch mode: 'access', 'modification', or 'both' (case-insensitive)") String mode, - @McpToolParam(description = "Java expression evaluated on each hit (e.g., '$oldValue + \" -> \" + $newValue')") String expression, - @McpToolParam(required = false, description = "Optional condition with the same synthetic bindings — log only when true") @Nullable String condition, + @McpToolParam(description = "Java expression evaluated on each hit (e.g., '$oldValue + \" -> \" + $newValue') — supports `{ ...; return X; }` block syntax") String expression, + @McpToolParam(required = false, description = "Optional condition with the same synthetic bindings — log only when true — supports `{ ...; return X; }` block syntax") @Nullable String condition, @McpToolParam(required = false, description = "Optional thread filter — only fire when this thread (uniqueID) hits the field") @Nullable Long threadFilterId, @McpToolParam(required = false, description = "Optional object filter — only fire on the given instance. Must be omitted for static fields.") @Nullable Long objectFilterId, @McpToolParam(required = false, description = "Optional ID of a trigger breakpoint — this logpoint stays disarmed until the trigger fires. Sticky by default.") @Nullable Integer triggerBreakpointId, @@ -3682,11 +3685,11 @@ public String jdwp_get_current_thread() { } } - @McpTool(description = "Attach a watcher to a breakpoint to evaluate a Java expression when hit. Returns the watcher ID.") + @McpTool(description = "Attach a watcher to a breakpoint to evaluate a Java expression when hit. Returns the watcher ID. Supports `{ block }` syntax for multi-statement.") public String jdwp_attach_watcher( @McpToolParam(description = "Breakpoint request ID (from jdwp_overview)") int breakpointId, @McpToolParam(description = "Descriptive label for this watcher (e.g., 'Trace entity ID', 'Check user name')") String label, - @McpToolParam(description = "Java expression to evaluate (e.g., 'entity.id', 'user.name', 'items.size()')") String expression) { + @McpToolParam(description = "Java expression to evaluate (e.g., 'entity.id', 'user.name', 'items.size()') — supports `{ ...; return X; }` block syntax") String expression) { try { if (expression.trim().isEmpty()) { return "Error: No expression provided"; diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java index e6617ac..e0ed308 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java @@ -371,10 +371,24 @@ private static String resolveLocalVarType(LocalVariable var, String wrapperPacka /** * Generates a wrapper class with a static `evaluate()` method that accepts the context - * variables as parameters and returns the result of the user expression. The user expression - * is wrapped in `(Object)(...)` so any value type — including primitives via autoboxing — - * can be returned. The bare `this` keyword in the expression is tokenizer-rewritten to `_this` - * because the wrapper class doesn't have a `this` reference (it's a static method). + * variables as parameters and returns the result of the user input. + * + *

Two input modes, picked by {@link #isBlockMode}: + *

    + *
  • Expression mode (default). The input is treated as a single Java expression and + * wrapped as {@code return (Object) ();} so any value type — including primitives + * via autoboxing — flows back as the method's return value.
    • + *
  • Block mode. The trimmed input starts with {@code {} and ends with {@code }}; + * the inside is spliced into the method body verbatim, with a trailing + * {@code return null;} appended so the method is reachable-fall-through-safe even if + * the user forgot a {@code return}. The user is responsible for writing {@code return X;} + * statements to yield a value — block mode supports {@code try/catch}, intermediate + * locals, early {@code return}, and other statement-level constructs that the single- + * expression mode could not express.
    • + *
+ * + *

In both modes, the bare {@code this} keyword is tokenizer-rewritten to {@code _this} + * because the wrapper is a static method. * *

The package portion of {@code className} drives the wrapper's {@code package} declaration. * When the caller routes a non-public {@code this} type into its own package via @@ -395,17 +409,84 @@ private static String generateSourceCode(String className, EvaluationContext con // literals are NOT rewritten — see {@link #rewriteThisKeyword(String)}. final String safeExpression = rewriteThisKeyword(expression); + final String methodBody; + if (isBlockMode(safeExpression)) { + // Extract the body inside the outermost braces — already balanced because + // isBlockMode verified start with `{` and end with `}` and the tokenizer skips + // string/char/text-block content. The trailing `return null;` keeps the method + // type-correct when the user's block falls through without a return; an explicit + // `return X;` inside the user block wins, and `unreachable code` does not occur + // because the unconditional return is a separate statement, not part of the user + // body. + final String trimmed = safeExpression.trim(); + final String userBody = trimmed.substring(1, trimmed.length() - 1); + methodBody = + " // User block:\n" + + userBody + '\n' + + " // Fallthrough guard — block reached end without a `return`.\n" + + " return null;\n"; + } else { + methodBody = + " // User expression:\n" + + " return (Object) (" + safeExpression + ");\n"; + } + final String packageDecl = packageName.isEmpty() ? "" : "package " + packageName + ";\n\n"; return packageDecl + "// Automatically generated class for JDI expression evaluation\n" + "public class " + simpleClassName + " {\n" + " public static Object " + EVALUATION_METHOD_NAME + '(' + methodParameters + ") {\n" + - " // User expression:\n" + - " return (Object) (" + safeExpression + ");\n" + + methodBody + " }\n" + "}\n"; } + /** + * Returns {@code true} when the user input is in block mode — i.e. its trimmed form starts + * with {@code {} and ends with the matching {@code }}. Tokenizer-aware: it makes sure the + * trailing {@code }} is at the top level and not buried inside a string / char / text-block + * literal, so an input like {@code "{x}".length()} (a method call on a string literal that + * happens to start with {@code {}) stays in expression mode. + * + *

Static + package-private so it can be unit-tested without a real {@link StackFrame}. + */ + static boolean isBlockMode(String expression) { + final String trimmed = expression.trim(); + if (trimmed.length() < 2 || trimmed.charAt(0) != '{' || trimmed.charAt(trimmed.length() - 1) != '}') { + return false; + } + // Walk the inside and confirm brace balance returns to zero at exactly the closing `}`. + // Tokenizer-aware so braces inside string / char / text-block literals are ignored. + final int n = trimmed.length() - 1; + int depth = 1; + int i = 1; + while (i < n) { + final char c = trimmed.charAt(i); + if (c == '"') { + i = skipStringLiteral(trimmed, i); + continue; + } + if (c == '\'') { + i = skipCharLiteral(trimmed, i); + continue; + } + if (c == '{') { + depth++; + } else if (c == '}') { + depth--; + if (depth == 0) { + // Hit a top-level `}` before the trailing one — input is something like + // `{stmt;} + foo` so the trailing `}` of the trimmed form is a different + // closer and block-mode does not apply. + return false; + } + } + i++; + } + // Trailing `}` at index n closes the outer brace. + return depth == 1; + } + /** * Get a name suitable to use in the generated wrapper class for the given runtime type. * Handles three cases the wrapper compiler cannot otherwise express: diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java index 7dad664..7ad15cb 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java @@ -383,4 +383,67 @@ void shouldTolerateUnterminatedStringInThisKeyword() { // The bare `this` before the literal should still be rewritten assertThat(result).isEqualTo("_this + \"unterminated"); } + + // ── isBlockMode: detect `{ ... }` block input vs expression input ──────────────────── + + @Test + @DisplayName("isBlockMode: plain expression is NOT a block") + void shouldNotDetectBlockForPlainExpression() { + assertThat(JdiExpressionEvaluator.isBlockMode("foo.bar() + baz")).isFalse(); + } + + @Test + @DisplayName("isBlockMode: `{ stmt; }` IS a block") + void shouldDetectSingleStatementBlock() { + assertThat(JdiExpressionEvaluator.isBlockMode("{ return x; }")).isTrue(); + } + + @Test + @DisplayName("isBlockMode: leading/trailing whitespace is allowed") + void shouldDetectBlockWithSurroundingWhitespace() { + assertThat(JdiExpressionEvaluator.isBlockMode(" \n { return x; }\n ")).isTrue(); + } + + @Test + @DisplayName("isBlockMode: nested braces are matched correctly") + void shouldDetectBlockWithNestedBraces() { + assertThat(JdiExpressionEvaluator.isBlockMode( + "{ if (x > 0) { return x; } else { return 0; } }")).isTrue(); + } + + @Test + @DisplayName("isBlockMode: braces inside a string literal do NOT terminate the outer block") + void shouldNotConfuseBracesInsideStringLiteral() { + // The `}` inside the string would mislead a naive matcher; the tokenizer must skip it. + assertThat(JdiExpressionEvaluator.isBlockMode("{ return \"}\"; }")).isTrue(); + } + + @Test + @DisplayName("isBlockMode: `{x}.foo()` is NOT a block (trailing `}` is not the outer closer)") + void shouldNotDetectBlockWhenClosingBraceIsMidExpression() { + // `{x}` opens-and-closes before `.foo()`, so the trailing closer of the trimmed input + // is `)` — block mode should not apply. + assertThat(JdiExpressionEvaluator.isBlockMode("{x}.foo()")).isFalse(); + } + + @Test + @DisplayName("isBlockMode: brace-balanced internals followed by expression text are NOT a block") + void shouldNotDetectBlockWhenInternalBraceClosesEarly() { + // `{a;}+b;` — the first `}` closes the outer brace at i=4, depth goes to 0 before the + // trailing `}` at i=7. Block detection must reject this. + assertThat(JdiExpressionEvaluator.isBlockMode("{a;}+b;")).isFalse(); + } + + @Test + @DisplayName("isBlockMode: an empty input is NOT a block") + void shouldNotDetectBlockForEmptyInput() { + assertThat(JdiExpressionEvaluator.isBlockMode("")).isFalse(); + assertThat(JdiExpressionEvaluator.isBlockMode(" ")).isFalse(); + } + + @Test + @DisplayName("isBlockMode: a lone `{}` is a (trivial, empty) block") + void shouldDetectEmptyBlock() { + assertThat(JdiExpressionEvaluator.isBlockMode("{}")).isTrue(); + } } From 3fa17d89f3357249f60f532e580da5762122b27d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Novotn=C3=BD?= Date: Sat, 23 May 2026 14:46:22 +0200 Subject: [PATCH 2/3] review: address PR #11 feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 `}`) --- .../evaluation/JdiExpressionEvaluator.java | 110 ++++++++++++++---- .../JdiExpressionEvaluatorRewriteTest.java | 30 +++++ 2 files changed, 120 insertions(+), 20 deletions(-) diff --git a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java index e0ed308..9b580da 100644 --- a/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java +++ b/jdwp-mcp-server/src/main/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluator.java @@ -378,13 +378,13 @@ private static String resolveLocalVarType(LocalVariable var, String wrapperPacka *

  • Expression mode (default). The input is treated as a single Java expression and * wrapped as {@code return (Object) ();} so any value type — including primitives * via autoboxing — flows back as the method's return value.
    • - *
  • Block mode. The trimmed input starts with {@code {} and ends with {@code }}; - * the inside is spliced into the method body verbatim, with a trailing - * {@code return null;} appended so the method is reachable-fall-through-safe even if - * the user forgot a {@code return}. The user is responsible for writing {@code return X;} - * statements to yield a value — block mode supports {@code try/catch}, intermediate - * locals, early {@code return}, and other statement-level constructs that the single- - * expression mode could not express.
    • + *
  • Block mode. The trimmed input starts with '{' and ends with the matching '}'; + * the inside is spliced into the method body verbatim, guarded by a runtime-only branch + * so that the trailing fallthrough {@code return null;} stays reachable even when the + * user body ends with an explicit {@code return X;}. The user is responsible for writing + * {@code return X;} statements to yield a value — block mode supports {@code try/catch}, + * intermediate locals, early {@code return}, and other statement-level constructs that + * the single-expression mode could not express.
    • * * *

    In both modes, the bare {@code this} keyword is tokenizer-rewritten to {@code _this} @@ -412,18 +412,25 @@ private static String generateSourceCode(String className, EvaluationContext con final String methodBody; if (isBlockMode(safeExpression)) { // Extract the body inside the outermost braces — already balanced because - // isBlockMode verified start with `{` and end with `}` and the tokenizer skips - // string/char/text-block content. The trailing `return null;` keeps the method - // type-correct when the user's block falls through without a return; an explicit - // `return X;` inside the user block wins, and `unreachable code` does not occur - // because the unconditional return is a separate statement, not part of the user - // body. + // isBlockMode verified the structure (start with `{`, balanced end with `}`, + // string/char/text-block/comment regions skipped). + // + // The user body is wrapped in `if (__mcpFallthroughGuard) { ... }` where the guard + // is a non-final local set to true. The compiler can't treat it as a compile-time + // constant (JLS §15.29 — only `final` variables initialised with constant + // expressions qualify), so it does NOT prove the post-if `return null;` unreachable. + // That keeps the wrapper compiling whether or not the user's body ends with an + // explicit `return X;`. At runtime the guard is always true → user body always runs; + // `return null;` is the safety net for blocks that fall through without returning. final String trimmed = safeExpression.trim(); final String userBody = trimmed.substring(1, trimmed.length() - 1); methodBody = " // User block:\n" + + " boolean __mcpFallthroughGuard = true;\n" + + " if (__mcpFallthroughGuard) {\n" + userBody + '\n' + - " // Fallthrough guard — block reached end without a `return`.\n" + + " }\n" + + " // Fallthrough guard — only reached when the user's block doesn't return.\n" + " return null;\n"; } else { methodBody = @@ -443,10 +450,11 @@ private static String generateSourceCode(String className, EvaluationContext con /** * Returns {@code true} when the user input is in block mode — i.e. its trimmed form starts - * with {@code {} and ends with the matching {@code }}. Tokenizer-aware: it makes sure the - * trailing {@code }} is at the top level and not buried inside a string / char / text-block - * literal, so an input like {@code "{x}".length()} (a method call on a string literal that - * happens to start with {@code {}) stays in expression mode. + * with '{' and ends with the matching '}'. Tokenizer-aware: braces inside string / char / + * text-block literals and inside line ({@code //…}) or block ({@code /*…*}{@code /}) comments + * are ignored, so inputs like {@code "{x}".length()} (a method call on a string literal that + * happens to start with '{') stay in expression mode, and a comment containing '}' inside an + * otherwise-block input does not prematurely close the outer block. * *

    Static + package-private so it can be unit-tested without a real {@link StackFrame}. */ @@ -456,7 +464,8 @@ static boolean isBlockMode(String expression) { return false; } // Walk the inside and confirm brace balance returns to zero at exactly the closing `}`. - // Tokenizer-aware so braces inside string / char / text-block literals are ignored. + // Tokenizer-aware so braces inside string / char / text-block literals and Java comments + // are ignored. final int n = trimmed.length() - 1; int depth = 1; int i = 1; @@ -470,6 +479,27 @@ static boolean isBlockMode(String expression) { i = skipCharLiteral(trimmed, i); continue; } + if (c == '/' && i + 1 < n) { + final char next = trimmed.charAt(i + 1); + if (next == '/') { + i = skipLineComment(trimmed, i); + // A line comment that runs to end-of-string would have swallowed the + // trailing `}` (no newline before it) — there is no real closer, so the + // input is not block-mode. + if (i > n) { + return false; + } + continue; + } + if (next == '*') { + i = skipBlockComment(trimmed, i); + // Unterminated block comment swallows the trailing `}` too. Same rationale. + if (i > n) { + return false; + } + continue; + } + } if (c == '{') { depth++; } else if (c == '}') { @@ -487,6 +517,39 @@ static boolean isBlockMode(String expression) { return depth == 1; } + /** + * Returns the index just past the end of a {@code //…} line comment that starts at + * {@code start}. Terminator is the next newline; if no newline is found, returns the end of + * the string (best-effort tolerance — never throws). + */ + private static int skipLineComment(String s, int start) { + final int n = s.length(); + int i = start + 2; + while (i < n && s.charAt(i) != '\n') { + i++; + } + // Caller resumes scanning AT the newline (or end). The newline itself is not part of the + // comment, so we don't advance past it here. + return i; + } + + /** + * Returns the index just past the end of a {@code /*…*}{@code /} block comment that starts at + * {@code start}. If the closing {@code *}{@code /} is missing, returns the end of the string + * (best-effort tolerance — never throws). + */ + private static int skipBlockComment(String s, int start) { + final int n = s.length(); + int i = start + 2; + while (i + 1 < n) { + if (s.charAt(i) == '*' && s.charAt(i + 1) == '/') { + return i + 2; + } + i++; + } + return n; + } + /** * Get a name suitable to use in the generated wrapper class for the given runtime type. * Handles three cases the wrapper compiler cannot otherwise express: @@ -756,13 +819,20 @@ private static ClassLoaderReference findClassLoader(StackFrame frame) throws Jdi // Auto-rewrite bare references to fields of `this` so users can write // `sessions.containsKey(session)` instead of `_this.sessions.containsKey(session)`. + // SKIPPED in block mode — the rewriter is identifier-level and cannot distinguish + // a field reference from a local-variable declaration. Rewriting a statement-body + // input like `int count = 1; ...` would corrupt the declaration into + // `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` → `_this.field`). // The set of rewritable fields depends on what the wrapper can actually see: // - When the wrapper lives in the default isolated package, only public fields on a // public declaring type are reachable. // - When the wrapper lives in `this`'s own package, public/protected/package-private // fields are all reachable. Private fields still aren't, so they stay un-rewritten // and produce a compile error the user can fix (or work around via reflection). - if (thisObject != null && thisObject.referenceType() instanceof ClassType thisClass) { + if (!isBlockMode(expression) + && thisObject != null && thisObject.referenceType() instanceof ClassType thisClass) { final boolean sharingPackage = !wrapperPackage.equals(DEFAULT_EVALUATION_PACKAGE) && wrapperPackage.equals(packageOf(thisClass.name())); if (thisClass.isPublic() || sharingPackage) { diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java index 7ad15cb..175f5ed 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java @@ -446,4 +446,34 @@ void shouldNotDetectBlockForEmptyInput() { void shouldDetectEmptyBlock() { assertThat(JdiExpressionEvaluator.isBlockMode("{}")).isTrue(); } + + @Test + @DisplayName("isBlockMode: `}` inside a `/* ... */` block comment does NOT close the outer block") + void shouldNotConfuseBracesInsideBlockComment() { + // Without comment-skip, the `}` inside the comment would drop depth to 0 and the + // outer block would be rejected as non-block input. + assertThat(JdiExpressionEvaluator.isBlockMode("{ /* close: } */ return 1; }")).isTrue(); + } + + @Test + @DisplayName("isBlockMode: `}` inside a `//` line comment does NOT close the outer block") + void shouldNotConfuseBracesInsideLineComment() { + assertThat(JdiExpressionEvaluator.isBlockMode("{ // }\n return 1; }")).isTrue(); + } + + @Test + @DisplayName("isBlockMode: `/* nested } */` followed by real `}` still balances") + void shouldHandleBlockCommentWithBraceAtVariousDepths() { + // Nested braces in code, plus a `}` hidden inside a comment, plus the real closer. + assertThat(JdiExpressionEvaluator.isBlockMode( + "{ if (x) { /* not a closer: } */ return 1; } return 0; }")).isTrue(); + } + + @Test + @DisplayName("isBlockMode: unterminated block comment is tolerated, treats remainder as comment") + void shouldTolerateUnterminatedBlockComment() { + // `/* …` without a closing `*/` swallows the rest of the input including the trailing `}`. + // The outer brace then never closes → not block-mode. Must not throw. + assertThat(JdiExpressionEvaluator.isBlockMode("{ /* unterminated }")).isFalse(); + } } From 0c3de5970f1f9cc354a0277c4b222188ca6816c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jan=20Novotn=C3=BD?= Date: Sun, 24 May 2026 09:55:11 +0200 Subject: [PATCH 3/3] review: make internal-brace-closes-early test actually exercise that 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. --- .../evaluation/JdiExpressionEvaluatorRewriteTest.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java index 175f5ed..348fbe7 100644 --- a/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java +++ b/jdwp-mcp-server/src/test/java/one/edee/mcp/jdwp/evaluation/JdiExpressionEvaluatorRewriteTest.java @@ -429,9 +429,12 @@ void shouldNotDetectBlockWhenClosingBraceIsMidExpression() { @Test @DisplayName("isBlockMode: brace-balanced internals followed by expression text are NOT a block") void shouldNotDetectBlockWhenInternalBraceClosesEarly() { - // `{a;}+b;` — the first `}` closes the outer brace at i=4, depth goes to 0 before the - // trailing `}` at i=7. Block detection must reject this. - assertThat(JdiExpressionEvaluator.isBlockMode("{a;}+b;")).isFalse(); + // `{a;}+b;}` — input both starts and ends with a brace (so it clears the cheap + // first/last-char guard), but the FIRST `}` at i=3 drops depth to 0 before the trailing + // `}` at i=7. That trailing brace is therefore a separate closer, not the match for the + // opening one, so this exercises the early `depth == 0` rejection branch — not the + // end-char guard. Block detection must reject it. + assertThat(JdiExpressionEvaluator.isBlockMode("{a;}+b;}")).isFalse(); } @Test