Added Debugger Support with ExecutionContext and Web API Integration and implemented bot's suggestions with synced main #55
Conversation
WalkthroughIntroduces a step-by-step debugger: adds ExecutionContext lifecycle, LocatedStatement for source line tracking, parser changes to emit locations, Evaluator breakpoint- and pause-aware execution with stepping, KidCodeEngine wiring to the context, REST debugger endpoints, and a local DebuggerTest utility. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Web UI
participant DC as DebuggerController
participant EC as ExecutionContext
participant Eval as Evaluator
participant Stmt as LocatedStatement
Note over Eval,Stmt: Evaluator iterates statements (LocatedStatement)
Eval->>Stmt: inspect lineNumber
alt lineNumber in breakpoints
Eval->>EC: set state PAUSED (via context) / notify UI
end
Eval->>EC: call waitIfPaused() (blocks while PAUSED)
alt EC state = STEPPING
Note right of EC: after single execute, context moves to PAUSED
end
UI->>DC: POST /debugger/pause
DC->>EC: pause()
UI->>DC: POST /debugger/step
DC->>EC: step() -> notify() (unblocks one statement)
UI->>DC: POST /debugger/resume
DC->>EC: resume() -> notifyAll() (continue execution)
UI->>DC: POST /debugger/terminate
DC->>EC: terminate() -> notifyAll()
Eval->>Eval: stop when EC.isTerminated() or stopSignal true
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@Sansi-28 Hi, I’ve made the necessary changes as suggested. Kindly have a look. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
kidcode-core/src/main/java/com/kidcode/core/ast/LocatedStatement.java (1)
7-10: Validate constructor inputs and make wrapper final.Guard against null and invalid line numbers; declare the wrapper final for immutability.
Apply:
+import java.util.Objects; -public class LocatedStatement implements Statement { +public final class LocatedStatement implements Statement { @@ - public LocatedStatement(Statement inner, int lineNumber) { - this.inner = inner; - this.lineNumber = lineNumber; - } + public LocatedStatement(Statement inner, int lineNumber) { + this.inner = Objects.requireNonNull(inner, "inner"); + if (lineNumber < 1) throw new IllegalArgumentException("lineNumber must be >= 1"); + this.lineNumber = lineNumber; + }Optionally add a concise toString() for better logs while debugging.
kidcode-web/src/main/java/com/kidcode/web/DebuggerTest.java (1)
15-48: Avoid race; wait until actually paused. Move to test sources.The print/sleep assumes the VM hit the breakpoint; it may resume before Line 41. Poll for PAUSED (or add isPaused/getState), and place this utility under src/test/java to avoid shipping it.
Example minimal changes:
- System.out.println("Program paused at breakpoint. Waiting 2 seconds..."); - Thread.sleep(2000); + // Wait until paused at breakpoint (avoid race) + while (!contextIsPaused(context)) { Thread.sleep(20); } + System.out.println("Paused. Waiting 2 seconds..."); + Thread.sleep(2000);Add helper (or add ExecutionContext#isPaused()):
private static boolean contextIsPaused(ExecutionContext ctx) { try { var m = ExecutionContext.class.getDeclaredMethod("isStepping"); // placeholder; replace with isPaused() return false; // implement once isPaused() exists } catch (Exception e) { return false; } }Prefer: add
public synchronized boolean isPaused()to ExecutionContext and use it here.kidcode-core/src/main/java/com/kidcode/core/KidCodeEngine.java (1)
12-14: Accept breakpoints from caller.To support UI‑provided breakpoints (issue #16), overload/extend execute to accept a Set and pass it through.
Example:
- public List<ExecutionEvent> execute(String sourceCode) { + public List<ExecutionEvent> execute(String sourceCode, Set<Integer> breakpoints) {kidcode-core/src/main/java/com/kidcode/core/evaluator/Evaluator.java (2)
61-67: Prevent repeated timeout errors and stop promptly.When the instruction limit is exceeded, an ErrorEvent may be added for every subsequent statement (esp. inside loops). Emit once and terminate execution to short‑circuit nested flows.
- if (stopSignal.get() || ++instructionCount > INSTRUCTION_LIMIT) { - if (instructionCount > INSTRUCTION_LIMIT) { - events.add(new ExecutionEvent.ErrorEvent("Execution timed out! Possible infinite loop.")); - } - return; - } + if (stopSignal.get() || ++instructionCount > INSTRUCTION_LIMIT) { + if (instructionCount > INSTRUCTION_LIMIT && !timedOut) { + timedOut = true; + events.add(new ExecutionEvent.ErrorEvent("Execution timed out! Possible infinite loop.")); + // Optional: notify controller/engine to stop + executionContext.terminate(); + } + return; + }Add field:
private int instructionCount = 0; +private boolean timedOut = false;Reset in evaluate():
instructionCount = 0; +timedOut = false;
53-60: Expose pause metadata (optional).Consider emitting an event when pausing at a breakpoint (with lineNumber) so UIs can highlight current line.
kidcode-core/src/main/java/com/kidcode/core/evaluator/ExecutionContext.java (2)
4-4: Remove unused import.
AtomicBooleanis imported but never used in the class.Apply this diff:
-import java.util.concurrent.atomic.AtomicBoolean; -
15-15: Consider removing redundantvolatilemodifier.The
statefield is markedvolatile, but all methods that access it aresynchronized, which already provides memory visibility guarantees. Thevolatilemodifier is redundant and can be removed for cleaner code.Apply this diff:
- private volatile State state = State.RUNNING; + private State state = State.RUNNING;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
kidcode-core/src/main/java/com/kidcode/core/KidCodeEngine.java(2 hunks)kidcode-core/src/main/java/com/kidcode/core/ast/LocatedStatement.java(1 hunks)kidcode-core/src/main/java/com/kidcode/core/evaluator/Evaluator.java(7 hunks)kidcode-core/src/main/java/com/kidcode/core/evaluator/ExecutionContext.java(1 hunks)kidcode-core/src/main/java/com/kidcode/core/parser/Parser.java(2 hunks)kidcode-web/src/main/java/com/kidcode/web/DebuggerTest.java(1 hunks)kidcode-web/src/main/java/com/kidcode/web/controller/DebuggerController.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
kidcode-web/src/main/java/com/kidcode/web/DebuggerTest.java (3)
kidcode-core/src/main/java/com/kidcode/core/evaluator/Evaluator.java (1)
Evaluator(12-263)kidcode-core/src/main/java/com/kidcode/core/evaluator/Environment.java (1)
Environment(8-94)kidcode-core/src/main/java/com/kidcode/core/evaluator/ExecutionContext.java (1)
ExecutionContext(6-53)
kidcode-core/src/main/java/com/kidcode/core/evaluator/Evaluator.java (1)
kidcode-core/src/main/java/com/kidcode/core/evaluator/ExecutionContext.java (1)
ExecutionContext(6-53)
kidcode-core/src/main/java/com/kidcode/core/KidCodeEngine.java (2)
kidcode-core/src/main/java/com/kidcode/core/evaluator/ExecutionContext.java (1)
ExecutionContext(6-53)kidcode-core/src/main/java/com/kidcode/core/evaluator/Environment.java (1)
Environment(8-94)
kidcode-web/src/main/java/com/kidcode/web/controller/DebuggerController.java (1)
kidcode-core/src/main/java/com/kidcode/core/evaluator/ExecutionContext.java (1)
ExecutionContext(6-53)
🔇 Additional comments (6)
kidcode-core/src/main/java/com/kidcode/core/evaluator/Evaluator.java (1)
117-118: Good change: error events for SayStatement failures.Using ErrorEvent for evaluation failures improves clarity for UIs.
kidcode-web/src/main/java/com/kidcode/web/controller/DebuggerController.java (1)
3-7: Jakarta/Boot 3 compatibility looks fine.Using org.springframework.web.bind.annotation is correct for Boot 3/Jakarta. No javax.* usage.
kidcode-core/src/main/java/com/kidcode/core/parser/Parser.java (2)
70-101: LGTM! Well-designed safeguard against parser stalls.The refactored
parseStatement()correctly captures line numbers and wraps results inLocatedStatement. The safeguard at lines 92-98 is particularly well-implemented: it ensures the parser advances at least one token when a parse method returnsnullwithout consuming tokens, preventing infinite loops. The switch expression cleanly dispatches to appropriate parsers, and the error handling in the default case properly advances the token stream.
136-137: Good catch on error recovery.Adding
advanceToNextStatement()in the error case prevents the parser from getting stuck when the expected=token is missing. This aligns with the overall token advancement refactoring and improves error recovery.kidcode-core/src/main/java/com/kidcode/core/evaluator/ExecutionContext.java (2)
36-44: Correct wait/notify pattern implementation.The
waitIfPaused()method correctly implements the standard wait pattern:
- Uses a
whileloop (notif) to guard against spurious wakeups- Properly handles
InterruptedExceptionby re-interrupting the current thread- Works correctly with the notify/notifyAll calls in
step(),resume(), andterminate()
17-34: Thread-safe state transitions with proper coordination.The state transition methods (
pause(),step(),resume(),terminate()) are correctly synchronized and usenotifyAll()appropriately to wake waiting threads. The use ofnotifyAll()rather thannotify()is the right choice here to ensure all waiting threads are woken when state changes.
kidcode-web/src/main/java/com/kidcode/web/controller/DebuggerController.java
Show resolved
Hide resolved
kidcode-web/src/main/java/com/kidcode/web/controller/DebuggerController.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
kidcode-core/src/main/java/com/kidcode/core/evaluator/Evaluator.java (2)
58-66: Breakpoint pause flow looks good; add a cheap fast‑path.Minor: skip pause() if already paused to avoid redundant state writes (optional).
- if (lineNumber != -1 && breakpoints.contains(lineNumber)) { - executionContext.pause(); - } + if (lineNumber != -1 && breakpoints.contains(lineNumber) && !executionContext.isStepping()) { + // If stepping, we'll pause right after executing the statement. + executionContext.pause(); + }
120-125: Standardize error event types for consistency.Here SayStatement errors emit ErrorEvent, but other error paths (e.g., Line 141 in IfStatement) still emit SayEvent. Unify to ErrorEvent so the UI can style/handle errors consistently.
Example minimal fix:
- if (isError(cond)) { - events.add(new ExecutionEvent.SayEvent((String) cond)); + if (isError(cond)) { + events.add(new ExecutionEvent.ErrorEvent((String) cond)); return; }Optionally, adopt a single helper to translate any "Error:" string into ErrorEvent at the call sites.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
kidcode-core/src/main/java/com/kidcode/core/evaluator/Evaluator.java(7 hunks)kidcode-core/src/main/java/com/kidcode/core/parser/Parser.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- kidcode-core/src/main/java/com/kidcode/core/parser/Parser.java
🧰 Additional context used
🧬 Code graph analysis (1)
kidcode-core/src/main/java/com/kidcode/core/evaluator/Evaluator.java (1)
kidcode-core/src/main/java/com/kidcode/core/evaluator/ExecutionContext.java (1)
ExecutionContext(6-53)
🔇 Additional comments (6)
kidcode-core/src/main/java/com/kidcode/core/evaluator/Evaluator.java (6)
50-57: LGTM: clean unwrap of LocatedStatement for line mapping.Pattern matching keeps code concise and prepares for breakpoint checks.
162-165: Stepping semantics implemented correctly (step → execute one → pause).This aligns with the described UX.
35-37: LGTM: initial Clear + Move event initialization is consistent with prior state emission.
201-202: LGTM: explicit 'variable not found' error improves debuggability.
246-251: LGTM: strong type and bounds checks for list indexing with clear messages.
53-56: Java compiler configuration correctly set to Java 17 across all modules.The parent POM (
pom.xml) establishes<maven.compiler.source>17</maven.compiler.source>and<maven.compiler.target>17</maven.compiler.target>, which all child modules (kidcode-core, kidcode-desktop, kidcode-web) inherit. The instanceof pattern variable syntax used in Evaluator.java (lines 53-56) is fully supported by Java 16+ and will compile correctly under Java 17. The Spring Boot 3.0.0 dependency in kidcode-web also confirms Java 17+ alignment.
kidcode-core/src/main/java/com/kidcode/core/evaluator/Evaluator.java
Outdated
Show resolved
Hide resolved
@sakina1303 make the changes suggested by coderabbitsai and we are good to go |
sakina1303
force-pushed
the
feature/debugger-from-main
branch
from
196a4d5 to
ae163b9
Compare
done |
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)
kidcode-core/src/main/java/com/kidcode/core/parser/Parser.java (1)
65-94: parseStatement is malformed and won’t compile; wrapper logic incomplete.
- Stray closing brace after switch; dangling block referencing undeclared variables: inner, startPos, line.
- Early returns in switch prevent wrapping in LocatedStatement.
Fix by restructuring parseStatement to capture line/start, select inner via switch, recover on null, then wrap:
- private Statement parseStatement() { - switch (currentToken().type()) { - case MOVE: return parseMoveStatement(); - case TURN: return parseTurnStatement(); - case SAY: return parseSayStatement(); - case REPEAT: return parseRepeatStatement(); - case SET: return parseSetStatement(); - case IF: return parseIfStatement(); - case PEN: return parsePenStatement(); - case COLOR: return parseSetColorStatement(); - case DEFINE: return parseFunctionDefinitionStatement(); - case IDENTIFIER: return parseFunctionCallStatement(); - default: - errors.add("Error line " + currentToken().lineNumber() + ": Invalid start of a statement: '" + currentToken().literal() + "'"); - advanceToNextStatement(); - return null; - } - }; - - if (inner == null) { - // Some parse... methods return null on error without consuming tokens. - // Advance at least one token to avoid infinite loops. - if (position == startPos) { - advanceToNextStatement(); - } - return null; - } - return new LocatedStatement(inner, line); // wrap it with line number -} + private Statement parseStatement() { + final int line = currentToken().lineNumber(); + final int startPos = position; + Statement inner = null; + switch (currentToken().type()) { + case MOVE: inner = parseMoveStatement(); break; + case TURN: inner = parseTurnStatement(); break; + case SAY: inner = parseSayStatement(); break; + case REPEAT: inner = parseRepeatStatement(); break; + case SET: inner = parseSetStatement(); break; + case IF: inner = parseIfStatement(); break; + case PEN: inner = parsePenStatement(); break; + case COLOR: inner = parseSetColorStatement(); break; + case DEFINE: inner = parseFunctionDefinitionStatement(); break; + case IDENTIFIER: inner = parseFunctionCallStatement(); break; + default: + errors.add("Error line " + line + ": Invalid start of a statement: '" + currentToken().literal() + "'"); + advanceToNextStatement(); + return null; + } + if (inner == null) { + if (position == startPos) { + advanceToNextStatement(); + } + return null; + } + return new LocatedStatement(inner, line); + }
♻️ Duplicate comments (3)
kidcode-core/src/main/java/com/kidcode/core/KidCodeEngine.java (1)
39-46: Compile breakers and isolated context; inject shared ExecutionContext.
- stopSignal declared twice (Line 39 and Line 42).
- Local
new ExecutionContext()isolates from web controller; HTTP controls won’t affect execution.environmentis undefined at return.Apply minimal fix + constructor injection:
- Supplier<Boolean> stopSignal = () -> executionStopped; - ExecutionContext context = new ExecutionContext(); - Set<Integer> breakpoints = new HashSet<>(); // empty set for now, add breakpoints as needed - Supplier<Boolean> stopSignal = () -> executionStopped || context.isTerminated(); + Set<Integer> breakpoints = new HashSet<>(); + Supplier<Boolean> stopSignal = () -> executionStopped || context.isTerminated(); - Evaluator evaluator = new Evaluator(stopSignal, context, breakpoints); - return evaluator.evaluate(program, environment); + Evaluator evaluator = new Evaluator(stopSignal, context, breakpoints); + Environment env = new Environment(); + return evaluator.evaluate(program, env);And refactor class to receive shared context (bean-injected):
public class KidCodeEngine { - private volatile boolean executionStopped = false; + private volatile boolean executionStopped = false; + private final ExecutionContext context; + + public KidCodeEngine(ExecutionContext context) { + this.context = context; + }Provide the bean in web config to share the same instance with DebuggerController.
kidcode-core/src/main/java/com/kidcode/core/evaluator/Evaluator.java (2)
24-28: Constructor lacks null-safety and defensive copy for breakpoints.Avoid NPE and external mutation:
+import java.util.Objects; @@ - public Evaluator(Supplier<Boolean> stopSignal, ExecutionContext context, Set<Integer> breakpoints) { - this.stopSignal = stopSignal; - this.executionContext = context; - this.breakpoints = breakpoints; + public Evaluator(Supplier<Boolean> stopSignal, ExecutionContext context, Set<Integer> breakpoints) { + this.stopSignal = Objects.requireNonNull(stopSignal, "stopSignal"); + this.executionContext = Objects.requireNonNull(context, "executionContext"); + this.breakpoints = (breakpoints == null) ? Set.of() : Set.copyOf(breakpoints); }
67-76: Terminate isn’t fully honored; add checks after waits and in loops.Ensure immediate stop when terminated:
@@ - executionContext.waitIfPaused(); + executionContext.waitIfPaused(); + if (executionContext.isTerminated()) { + return; + } @@ - for (Statement statement : program) { - if (stopSignal.get()) break; + for (Statement statement : program) { + if (stopSignal.get() || executionContext.isTerminated()) break; evaluateStatement(statement, env); + if (executionContext.isTerminated()) break; } @@ - for (int i = 0; i < times; i++) { - if (stopSignal.get()) return; + for (int i = 0; i < times; i++) { + if (stopSignal.get() || executionContext.isTerminated()) return; for (Statement bodyStatement : repeatStmt.body()) { - evaluateStatement(bodyStatement, env); + if (stopSignal.get() || executionContext.isTerminated()) return; + evaluateStatement(bodyStatement, env); + if (executionContext.isTerminated()) return; } } @@ - for (Statement bodyStmt : func.body()) { - evaluateStatement(bodyStmt, localEnv); - } + for (Statement bodyStmt : func.body()) { + if (stopSignal.get() || executionContext.isTerminated()) return; + evaluateStatement(bodyStmt, localEnv); + if (executionContext.isTerminated()) return; + }Also include
executionContext.isTerminated()in the per-statement guard:- if (stopSignal.get() || ++instructionCount > INSTRUCTION_LIMIT) { + if (stopSignal.get() || executionContext.isTerminated() || ++instructionCount > INSTRUCTION_LIMIT) {Also applies to: 38-43, 136-141, 300-303
🧹 Nitpick comments (3)
kidcode-core/src/main/java/com/kidcode/core/parser/Parser.java (1)
219-221: Double-advance semantics aroundend if.The sequence “if (peek==IF) nextToken(); advanceToNextStatement();” effectively consumes both tokens of “end if”, but the comment says it consumes ‘if’. Consider clarifying the comment or explicitly consuming END then IF for readability. Behavior is fine.
kidcode-core/src/main/java/com/kidcode/core/evaluator/Evaluator.java (1)
58-66: Duplicate breakpoint pause block.Lines 58-61 and 63-65 are identical. Remove one.
- // Pause if breakpoint - if (lineNumber != -1 && breakpoints.contains(lineNumber)) { - executionContext.pause(); - } - // Pause if breakpoint - if (lineNumber != -1 && breakpoints.contains(lineNumber)) { - executionContext.pause(); - } + // Pause if breakpoint + if (lineNumber != -1 && breakpoints.contains(lineNumber)) { + executionContext.pause(); + }kidcode-core/src/main/java/com/kidcode/core/evaluator/ExecutionContext.java (1)
15-53: Add await/isPaused helpers to support external synchronization.To make tests and web UI robust, expose non-blocking query and timed await:
@@ public synchronized boolean isStepping() { return state == State.STEPPING; } + + public synchronized boolean isPaused() { + return state == State.PAUSED; + } + + public synchronized boolean awaitPaused(long timeoutMs) throws InterruptedException { + long end = System.currentTimeMillis() + timeoutMs; + while (state != State.PAUSED && state != State.TERMINATED) { + long waitFor = end - System.currentTimeMillis(); + if (waitFor <= 0) break; + wait(waitFor); + } + return state == State.PAUSED; + }Optional: remove unused
AtomicBooleanimport.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
kidcode-core/src/main/java/com/kidcode/core/KidCodeEngine.java(2 hunks)kidcode-core/src/main/java/com/kidcode/core/ast/LocatedStatement.java(1 hunks)kidcode-core/src/main/java/com/kidcode/core/evaluator/Evaluator.java(7 hunks)kidcode-core/src/main/java/com/kidcode/core/evaluator/ExecutionContext.java(1 hunks)kidcode-core/src/main/java/com/kidcode/core/parser/Parser.java(2 hunks)kidcode-web/src/main/java/com/kidcode/web/DebuggerTest.java(1 hunks)kidcode-web/src/main/java/com/kidcode/web/controller/DebuggerController.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- kidcode-core/src/main/java/com/kidcode/core/ast/LocatedStatement.java
- kidcode-web/src/main/java/com/kidcode/web/controller/DebuggerController.java
🧰 Additional context used
🧬 Code graph analysis (3)
kidcode-core/src/main/java/com/kidcode/core/evaluator/Evaluator.java (1)
kidcode-web/src/main/resources/static/app.js (1)
events(193-193)
kidcode-core/src/main/java/com/kidcode/core/KidCodeEngine.java (2)
kidcode-core/src/main/java/com/kidcode/core/evaluator/ExecutionContext.java (1)
ExecutionContext(6-53)kidcode-core/src/main/java/com/kidcode/core/evaluator/Environment.java (1)
Environment(8-94)
kidcode-web/src/main/java/com/kidcode/web/DebuggerTest.java (3)
kidcode-core/src/main/java/com/kidcode/core/evaluator/Evaluator.java (1)
Evaluator(12-304)kidcode-core/src/main/java/com/kidcode/core/evaluator/Environment.java (1)
Environment(8-94)kidcode-core/src/main/java/com/kidcode/core/evaluator/ExecutionContext.java (1)
ExecutionContext(6-53)
🔇 Additional comments (2)
kidcode-core/src/main/java/com/kidcode/core/parser/Parser.java (2)
126-127: Good recovery on missing '=' in set.Advancing on error avoids infinite loops. LGTM.
223-234: parseBlock entrynextToken()changes contract; verify all callers.parseBlock now consumes the starter/else token. Callers (if/define/repeat) appear aligned, but this is brittle. Consider documenting this contract in the method Javadoc to avoid future double-consume bugs.
|
@sakina1303 Their is the last one. |
sakina1303
force-pushed
the
feature/debugger-from-main
branch
from
d7ac4e9 to
6c8c2f6
Compare
|
@Sansi-28 done. |
|
@coderabbitai review |
✅ Actions performedFull review triggered. |
✅ Actions performedReview triggered.
|
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)
kidcode-core/src/main/java/com/kidcode/core/parser/Parser.java (1)
65-93: Critical syntax error: malformed parseStatement method.The method has multiple compile-breaking issues:
- Line 82: Stray
};closing brace after the switch statement. Each case already returns directly, so this brace is unmatched.- Lines 84-93: References undeclared variables
inner,line, andstartPos. These don't exist in the method scope.- Missing wrapper logic: The intent to wrap statements in
LocatedStatementis clear from the AI summary, but the implementation is incomplete.The method won't compile as-is.
Fix by capturing the line number, storing the result, and wrapping properly:
private Statement parseStatement() { + int line = currentToken().lineNumber(); + int startPos = position; + Statement inner; + switch (currentToken().type()) { - case MOVE: return parseMoveStatement(); - case TURN: return parseTurnStatement(); - case SAY: return parseSayStatement(); - case REPEAT: return parseRepeatStatement(); - case SET: return parseSetStatement(); - case IF: return parseIfStatement(); - case PEN: return parsePenStatement(); - case COLOR: return parseSetColorStatement(); - case DEFINE: return parseFunctionDefinitionStatement(); - case IDENTIFIER: return parseFunctionCallStatement(); + case MOVE: inner = parseMoveStatement(); break; + case TURN: inner = parseTurnStatement(); break; + case SAY: inner = parseSayStatement(); break; + case REPEAT: inner = parseRepeatStatement(); break; + case SET: inner = parseSetStatement(); break; + case IF: inner = parseIfStatement(); break; + case PEN: inner = parsePenStatement(); break; + case COLOR: inner = parseSetColorStatement(); break; + case DEFINE: inner = parseFunctionDefinitionStatement(); break; + case IDENTIFIER: inner = parseFunctionCallStatement(); break; default: errors.add("Error line " + currentToken().lineNumber() + ": Invalid start of a statement: '" + currentToken().literal() + "'"); advanceToNextStatement(); return null; - } - }; + } if (inner == null) { - // Some parse... methods return null on error without consuming tokens. - // Advance at least one token to avoid infinite loops. if (position == startPos) { advanceToNextStatement(); } return null; } - return new LocatedStatement(inner, line); // wrap it with line number + return new LocatedStatement(inner, line); }
♻️ Duplicate comments (6)
kidcode-web/src/main/java/com/kidcode/web/controller/DebuggerController.java (2)
14-37: Add state endpoint and security hardening.UI needs a way to poll debugger state. Additionally, these endpoints lack authentication—anyone can control execution.
Add state endpoint:
@PostMapping("/terminate") public String terminate() { context.terminate(); return "Terminated"; } + + @GetMapping("/state") + public String state() { + if (context.isTerminated()) return "TERMINATED"; + if (context.isStepping()) return "STEPPING"; + // Add isPaused() to ExecutionContext if needed, or return generic state + return "RUNNING"; + }For dev security, bind to localhost in
application.properties:server.address=127.0.0.1Or add Spring Security for production use.
11-13: Missing Spring bean configuration for ExecutionContext.Constructor requires
ExecutionContextbut no@Beanor@Componentprovides it. Application will fail to start with "No qualifying bean" error.Create configuration:
// kidcode-web/src/main/java/com/kidcode/web/config/DebuggerConfig.java package com.kidcode.web.config; import com.kidcode.core.evaluator.ExecutionContext; import com.kidcode.core.KidCodeEngine; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; @Configuration public class DebuggerConfig { @Bean public ExecutionContext executionContext() { return new ExecutionContext(); } @Bean public KidCodeEngine kidCodeEngine(ExecutionContext context) { return new KidCodeEngine(context); } }kidcode-core/src/main/java/com/kidcode/core/KidCodeEngine.java (1)
39-45: Critical: Duplicate declaration, undefined variable, and isolated ExecutionContext.Three blocking issues remain unresolved from prior review:
- Line 39 & 42:
stopSignaldeclared twice.- Line 45:
environmentis undefined—no local or field declaration exists.- Line 40: Creating a new
ExecutionContextlocally isolates it fromDebuggerController's instance; HTTP debugger controls won't affect this execution.Minimal fix:
- Supplier<Boolean> stopSignal = () -> executionStopped; - ExecutionContext context = new ExecutionContext(); - Set<Integer> breakpoints = new HashSet<>(); // empty set for now, add breakpoints as needed - Supplier<Boolean> stopSignal = () -> executionStopped || context.isTerminated(); + ExecutionContext context = new ExecutionContext(); // TODO: inject shared instance + Set<Integer> breakpoints = new HashSet<>(); + Supplier<Boolean> stopSignal = () -> executionStopped || context.isTerminated(); + Environment environment = new Environment(); Evaluator evaluator = new Evaluator(stopSignal, context, breakpoints); return evaluator.evaluate(program, environment);Essential: Refactor to inject a shared
ExecutionContextvia constructor so HTTP controls work:public class KidCodeEngine { private volatile boolean executionStopped = false; + private final ExecutionContext context; + + public KidCodeEngine(ExecutionContext context) { + this.context = context; + } public List<ExecutionEvent> execute(String sourceCode) { this.executionStopped = false; // ... - ExecutionContext context = new ExecutionContext(); + // Use this.contextProvide the bean in web config:
// kidcode-web/src/main/java/com/kidcode/web/config/DebuggerConfig.java @Configuration public class DebuggerConfig { @Bean public ExecutionContext executionContext() { return new ExecutionContext(); } @Bean public KidCodeEngine engine(ExecutionContext ctx) { return new KidCodeEngine(ctx); } }kidcode-web/src/main/java/com/kidcode/web/DebuggerTest.java (1)
40-45: Race condition: assumes paused without confirmation.Main thread prints "Program paused..." and sleeps without verifying the evaluator reached the breakpoint. The evaluator thread may not have hit line 2 yet, making the test nondeterministic.
Add an
awaitPausedhelper toExecutionContext:+ public synchronized boolean awaitPaused(long timeoutMs) throws InterruptedException { + long end = System.currentTimeMillis() + timeoutMs; + while (state != State.PAUSED && state != State.TERMINATED) { + long waitFor = end - System.currentTimeMillis(); + if (waitFor <= 0) return false; + wait(waitFor); + } + return state == State.PAUSED; + }Then in DebuggerTest:
- System.out.println("Program paused at breakpoint. Waiting 2 seconds..."); - Thread.sleep(2000); + if (context.awaitPaused(5_000)) { + System.out.println("Program paused at breakpoint. Waiting 2 seconds..."); + Thread.sleep(2000); + } else { + System.out.println("Did not pause within timeout."); + }kidcode-core/src/main/java/com/kidcode/core/evaluator/Evaluator.java (2)
24-28: Null-safety and defensive copy needed for constructor parameters.Constructor stores inputs directly without validation. Null
breakpointswill cause NPE at line 59, and external mutation of the set can alter behavior unexpectedly.Apply validation and defensive copy:
+import java.util.Objects; + public class Evaluator { // ... public Evaluator(Supplier<Boolean> stopSignal, ExecutionContext context, Set<Integer> breakpoints) { - this.stopSignal = stopSignal; - this.executionContext = context; - this.breakpoints = breakpoints; + this.stopSignal = Objects.requireNonNull(stopSignal, "stopSignal"); + this.executionContext = Objects.requireNonNull(context, "executionContext"); + this.breakpoints = (breakpoints == null) ? Set.of() : Set.copyOf(breakpoints); }
58-75: Terminate state not honored; evaluation continues after terminate().Currently only
stopSignaland instruction limit gate execution. Ifterminate()is called, evaluation proceeds. AddexecutionContext.isTerminated()checks afterwaitIfPaused(), in the per-statement guard, and in all loops.Apply these checks:
// Wait if paused executionContext.waitIfPaused(); + // Stop immediately if terminated while waiting + if (executionContext.isTerminated()) { + return; + } - if (stopSignal.get() || ++instructionCount > INSTRUCTION_LIMIT) { + if (stopSignal.get() || executionContext.isTerminated() || ++instructionCount > INSTRUCTION_LIMIT) { if (instructionCount > INSTRUCTION_LIMIT) { events.add(new ExecutionEvent.ErrorEvent("Execution timed out! Possible infinite loop.")); } return; }Also update the outer loop (line 38):
for (Statement statement : program) { - if (stopSignal.get()) break; + if (stopSignal.get() || executionContext.isTerminated()) break; evaluateStatement(statement, env); + if (executionContext.isTerminated()) break; }And inner loops (RepeatStatement line 136, function body line 300):
for (int i = 0; i < times; i++) { - if (stopSignal.get()) return; + if (stopSignal.get() || executionContext.isTerminated()) return; for (Statement bodyStatement : repeatStmt.body()) { + if (stopSignal.get() || executionContext.isTerminated()) return; evaluateStatement(bodyStatement, env); + if (executionContext.isTerminated()) return; } }for (Statement bodyStmt : func.body()) { + if (stopSignal.get() || executionContext.isTerminated()) return; evaluateStatement(bodyStmt, localEnv); + if (executionContext.isTerminated()) return; }
🧹 Nitpick comments (3)
kidcode-core/src/main/java/com/kidcode/core/ast/LocatedStatement.java (1)
1-24: Add null validation for inner statement.The constructor accepts
innerwithout validation. A nullinnerwould cause NPE intokenLiteral()(line 22) andgetInner()(line 13).Add validation:
+import java.util.Objects; + public class LocatedStatement implements Statement { private final Statement inner; private final int lineNumber; public LocatedStatement(Statement inner, int lineNumber) { - this.inner = inner; + this.inner = Objects.requireNonNull(inner, "inner statement cannot be null"); this.lineNumber = lineNumber; }kidcode-core/src/main/java/com/kidcode/core/evaluator/ExecutionContext.java (2)
4-4: Unused import: AtomicBoolean.
AtomicBooleanis imported but never used. Remove it.-import java.util.concurrent.atomic.AtomicBoolean; - public class ExecutionContext {
36-44: Add timeout-based awaitPaused helper and isPaused query.
waitIfPaused()can block indefinitely if state never changes. A timeout-based helper would support safer testing and UI polling. Additionally, anisPaused()query method would enable the state endpoint inDebuggerController.+ public synchronized boolean isPaused() { + return state == State.PAUSED; + } + + public synchronized boolean awaitPaused(long timeoutMs) throws InterruptedException { + long end = System.currentTimeMillis() + timeoutMs; + while (state != State.PAUSED && state != State.TERMINATED) { + long waitFor = end - System.currentTimeMillis(); + if (waitFor <= 0) return false; + wait(waitFor); + } + return state == State.PAUSED; + }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
kidcode-core/src/main/java/com/kidcode/core/KidCodeEngine.java(2 hunks)kidcode-core/src/main/java/com/kidcode/core/ast/LocatedStatement.java(1 hunks)kidcode-core/src/main/java/com/kidcode/core/evaluator/Evaluator.java(7 hunks)kidcode-core/src/main/java/com/kidcode/core/evaluator/ExecutionContext.java(1 hunks)kidcode-core/src/main/java/com/kidcode/core/parser/Parser.java(2 hunks)kidcode-web/src/main/java/com/kidcode/web/DebuggerTest.java(1 hunks)kidcode-web/src/main/java/com/kidcode/web/controller/DebuggerController.java(1 hunks)
🔇 Additional comments (3)
kidcode-core/src/main/java/com/kidcode/core/parser/Parser.java (3)
124-127: Good addition: prevents parser stall on missing '='.Adding
advanceToNextStatement()here ensures the parser doesn't loop infinitely when the=token is missing after a variable name.
219-219: Good fix: token advancement in parseIfStatement.Ensures the token stream advances after processing the complete if statement.
223-234: No issues found — design is consistent across all call sites.The review comment's concern about callers double-consuming tokens is unfounded. The code correctly implements the shifted responsibility: all four call sites (
parseFunctionDefinitionStatementline 181,parseRepeatStatementline 194, andparseIfStatementlines 207 & 211) leavecurrentTokenpositioned such thatparseBlock()'snextToken()call at line 225 correctly consumes the next token without duplication. The ELSE case at line 211 is handled correctly—whencurrentTokenis ELSE,parseBlock()consumes it as the "keyword that started the block." No caller performs redundant consumption.
| // Pause if breakpoint | ||
| if (lineNumber != -1 && breakpoints.contains(lineNumber)) { | ||
| executionContext.pause(); | ||
| } | ||
| // Pause if breakpoint | ||
| if (lineNumber != -1 && breakpoints.contains(lineNumber)) { | ||
| executionContext.pause(); | ||
| } |
There was a problem hiding this comment.
Duplicate breakpoint-check code.
Lines 59-61 and 63-65 contain identical breakpoint pause logic. Remove the duplication.
- // Pause if breakpoint
- if (lineNumber != -1 && breakpoints.contains(lineNumber)) {
- executionContext.pause();
- }
// Pause if breakpoint
if (lineNumber != -1 && breakpoints.contains(lineNumber)) {
executionContext.pause();
}📝 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.
| // Pause if breakpoint | |
| if (lineNumber != -1 && breakpoints.contains(lineNumber)) { | |
| executionContext.pause(); | |
| } | |
| // Pause if breakpoint | |
| if (lineNumber != -1 && breakpoints.contains(lineNumber)) { | |
| executionContext.pause(); | |
| } | |
| // Pause if breakpoint | |
| if (lineNumber != -1 && breakpoints.contains(lineNumber)) { | |
| executionContext.pause(); | |
| } |
🤖 Prompt for AI Agents
In kidcode-core/src/main/java/com/kidcode/core/evaluator/Evaluator.java around
lines 58 to 65 there is duplicated breakpoint-check logic; remove the redundant
second block (lines 63–65) so only a single check remains, or consolidate both
into one check that calls executionContext.pause() when lineNumber != -1 and
breakpoints.contains(lineNumber); keep surrounding formatting and comments
intact.
|
Hey, @sakina1303 have you tried running the app in your local environment coz its not able to compile in mine? |
2 participants
This pull request introduces the debugging functionality into the KidCode project by integrating an ExecutionContext for managing program execution state, setting breakpoints, and controlling program flow (pause, resume, step, terminate).
Key Changes:
Added ExecutionContext class in the kidcode-web module to handle debugger state and controls.
Implemented DebuggerTest class to demonstrate breakpoint handling and debugger control flow.
Created DebuggerController REST API endpoints (/pause, /resume, /step, /terminate) to allow external control of program execution through HTTP calls.
Updated kidcode-web/pom.xml to include dependency on kidcode-core and Spring Boot starter web.
Fixed package import and type mismatches between core and web modules to ensure proper interaction.
Verified full build success and ability to run Spring Boot web application serving debugger endpoints.
How to Test:
Run the DebuggerTest main method to verify breakpoint pause and resume functionality.
Start the web application using mvn -pl kidcode-web spring-boot:run.
Use HTTP POST requests to the /debugger endpoints to pause, resume, step, or terminate execution.
Confirm expected behavior via logs and program state changes.
Notes:
This lays the groundwork for adding a full debugger UI or integration with IDEs.
Future work may include enhanced breakpoint management and state inspection APIs.
Screenshot:
![Uploading Screenshot 2025-10-14 at 2.23.12 PM.png…]()
Fixes #16
Summary by CodeRabbit
New Features
Interactive debugging: pause, resume, step through, and terminate program execution.
Breakpoint support by line number to halt execution at specific points.
HTTP endpoints to control debugging actions remotely.
Improvements
Clearer runtime feedback with line-aware statements for easier troubleshooting.
Smoother execution control flow to better support pausing and stepping during runs.
Summary by CodeRabbit
New Features
Bug Fixes / Improvements
Tests