Conversation
WalkthroughThe changes introduce advanced pattern matching capabilities in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant WhenAst
participant SymbolTable
participant Context
participant Case
User->>WhenAst: execute()
WhenAst->>Context: get current scope
loop for each case
WhenAst->>SymbolTable: create temporary symbol table
WhenAst->>WhenAst: isMatching(patterns, cond_value, temp)
alt pattern matches
WhenAst->>Context: push temp bindings to local scope
WhenAst->>Case: execute matched case
WhenAst->>Context: drop stack frame
WhenAst-->>User: return result
end
end
WhenAst-->>User: throw inexhaustive error
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@MlaumeCasa @Matuku45 Awaiting code approval. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (12)
src/main/java/org/piccode/rt/PiccodeArray.java (1)
12-12: Consider encapsulation implications of public fieldWhile making
nodespublic enables direct access needed for pattern matching in the "when" expression implementation, it breaks encapsulation. Consider alternatives like making the fieldfinalto prevent reassignment while maintaining the required accessibility.- public List<PiccodeValue> nodes; + public final List<PiccodeValue> nodes;src/main/java/org/piccode/rt/PiccodeTuple.java (1)
12-12: Consider encapsulation implications of public fieldSimilar to the change in PiccodeArray, making
nodespublic breaks encapsulation principles. Consider making the fieldfinalto prevent reassignment while maintaining the accessibility needed for pattern matching in the "when" expression.- public List<PiccodeValue> nodes; + public final List<PiccodeValue> nodes;src/main/java/org/piccode/rt/PiccodeObject.java (1)
13-13: Consider encapsulation implications of public fieldSimilar to other runtime classes, making
objpublic breaks encapsulation principles. Consider making the fieldfinalto at least prevent reassignment of the HashMap reference while maintaining the accessibility needed for pattern matching operations.- public HashMap<String, PiccodeValue> obj; + public final HashMap<String, PiccodeValue> obj;src/test/java/org/piccode/ast/TopLevel.java (5)
26-26: Simplify assertion for better readabilityThe negated assertion can be simplified using a more direct positive assertion method.
- assertFalse(!(func instanceof FunctionAst)); + assertTrue(func instanceof FunctionAst);
41-41: Simplify assertion for better readabilityThe negated assertion can be simplified using a more direct positive assertion method.
- assertFalse(!(let instanceof VarDecl)); + assertTrue(let instanceof VarDecl);
60-60: Simplify assertion for better readabilityThe negated assertion can be simplified using a more direct positive assertion method.
- assertFalse(!(mod instanceof ModuleAst)); + assertTrue(mod instanceof ModuleAst);
75-75: Simplify assertion for better readabilityThe negated assertion can be simplified using a more direct positive assertion method.
- assertFalse(!(import_ instanceof ImportAst)); + assertTrue(import_ instanceof ImportAst);
44-44: Enhance pattern match test clarityThe pattern matching assertion uses a compact form that's harder to read. Consider breaking it into multiple assertions for better clarity and failure diagnostics.
- assertTrue(node.value instanceof NumberAst num && num.text.equals("1")); + assertTrue(node.value instanceof NumberAst); + NumberAst num = (NumberAst) node.value; + assertEquals("1", num.text);src/test/java/org/piccode/rt/Runtime.java (3)
5-8: Remove unused / redundant imports
AccessFrameis not referenced, andAssertionsis redundant with the static import of the assertion methods. Keeping them adds noise and may trigger “unused-import” compiler warnings.-import org.editor.AccessFrame; -import org.junit.jupiter.api.Assertions; +// (no changes needed – simply delete the two lines above)
28-29: Replace double negatives with directassertTrue
assertFalse(!(expr))is harder to read thanassertTrue(expr).-assertFalse(!(func instanceof FunctionAst)); +assertTrue(func instanceof FunctionAst);Make the analogous change for the
VarDeclandImportAstassertions.Also applies to: 41-42, 55-56
19-19: Avoid shadowingjava.lang.RuntimeNaming the test class
Runtimecan confuse tooling and readers because it shadowsjava.lang.Runtime. Consider renaming toRuntimeTest.src/main/java/org/piccode/ast/WhenAst.java (1)
79-87: Avoid re-executing literal ASTs
lit.execute()allocates a newPiccodeValueeach time, which is unnecessary and may break reference-based equality semantics.
Consider caching the literal’s value once during AST construction or comparing the literal’s raw token text.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📒 Files selected for processing (6)
src/main/java/org/piccode/ast/WhenAst.java(3 hunks)src/main/java/org/piccode/rt/PiccodeArray.java(1 hunks)src/main/java/org/piccode/rt/PiccodeObject.java(1 hunks)src/main/java/org/piccode/rt/PiccodeTuple.java(1 hunks)src/test/java/org/piccode/ast/TopLevel.java(1 hunks)src/test/java/org/piccode/rt/Runtime.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/main/java/org/piccode/ast/WhenAst.java (1)
src/main/java/org/piccode/rt/Context.java (1)
Context(10-49)
src/test/java/org/piccode/rt/Runtime.java (2)
src/main/java/org/editor/errors/IDEErrorListener.java (1)
IDEErrorListener(12-39)src/main/java/org/piccode/rt/Context.java (1)
Context(10-49)
🔇 Additional comments (1)
src/test/java/org/piccode/ast/TopLevel.java (1)
18-80: Consider testing the new "when" expression featureSince this PR is implementing the "when" expression feature, it would be beneficial to include test cases specifically for this new functionality to ensure it works correctly with different patterns and conditions.
Could you add test cases that verify the parsing and AST generation for the "when" expression? This would ensure the feature is properly implemented and avoid potential regressions in the future.
| private static StatementList compile(String code) { | ||
| var lexer = new PiccodeScriptLexer(CharStreams.fromString(code)); | ||
| var parser = new PiccodeScriptParser(new CommonTokenStream(lexer)); | ||
| lexer.removeErrorListeners(); | ||
| parser.removeErrorListeners(); | ||
|
|
||
| IDEErrorListener errorListener = new IDEErrorListener(); | ||
| lexer.addErrorListener(errorListener); | ||
| parser.addErrorListener(errorListener); | ||
|
|
||
| var visitor = new PiccodeVisitor(); | ||
|
|
||
| return (StatementList) visitor.visit(parser.stmts()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
The compile method should handle exceptions gracefully
The compile method doesn't catch or handle potential exceptions that might occur during parsing. This could cause tests to fail with unhelpful stack traces rather than meaningful assertion failures.
Consider adding exception handling to provide more informative test failures:
private static StatementList compile(String code) {
+ try {
var lexer = new PiccodeScriptLexer(CharStreams.fromString(code));
var parser = new PiccodeScriptParser(new CommonTokenStream(lexer));
lexer.removeErrorListeners();
parser.removeErrorListeners();
IDEErrorListener errorListener = new IDEErrorListener();
lexer.addErrorListener(errorListener);
parser.addErrorListener(errorListener);
var visitor = new PiccodeVisitor();
return (StatementList) visitor.visit(parser.stmts());
+ } catch (Exception e) {
+ fail("Failed to compile code: " + code + " - " + e.getMessage());
+ return null; // This line won't be reached due to fail() above
+ }
}🤖 Prompt for AI Agents
In src/test/java/org/piccode/ast/TopLevel.java around lines 81 to 94, the
compile method lacks exception handling for parsing errors, which can lead to
uninformative test failures. Wrap the parsing and visiting logic in a try-catch
block that catches relevant exceptions, then throw a runtime exception or
assertion error with a clear, descriptive message to improve test failure
diagnostics.
| var code = "function add(x, y) = x + y"; | ||
| var ast = compile(code); | ||
|
|
||
| assertEquals(ast.nodes.size(), 1); |
There was a problem hiding this comment.
Swap expected/actual arguments in assertEquals
JUnit’s signature is assertEquals(expected, actual). Inverting the order can produce misleading failure messages.
-assertEquals(ast.nodes.size(), 1);
+assertEquals(1, ast.nodes.size());Apply the same change in the other two occurrences.
Also applies to: 38-38, 53-53
🤖 Prompt for AI Agents
In src/test/java/org/piccode/rt/Runtime.java at lines 25, 38, and 53, the
arguments to assertEquals are reversed; JUnit expects the first argument to be
the expected value and the second to be the actual value. Swap the arguments in
these three lines so that the expected value comes first and the actual value
second to ensure accurate failure messages.
| IDEErrorListener errorListener = new IDEErrorListener(); | ||
| lexer.addErrorListener(errorListener); | ||
| parser.addErrorListener(errorListener); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check and assert on syntax errors during compilation
An IDEErrorListener is attached but its collected errors are ignored.
Add a post-parse assertion to fail fast when the input code contains syntax errors.
var visitor = new PiccodeVisitor();
var stmtList = (StatementList) visitor.visit(parser.stmts());
+assertTrue(errorListener.getSyntaxErrorInfos().isEmpty(),
+ () -> "Unexpected syntax errors: " + errorListener.getSyntaxErrorInfos());
+
return stmtList;📝 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.
| IDEErrorListener errorListener = new IDEErrorListener(); | |
| lexer.addErrorListener(errorListener); | |
| parser.addErrorListener(errorListener); | |
| var visitor = new PiccodeVisitor(); | |
| var stmtList = (StatementList) visitor.visit(parser.stmts()); | |
| assertTrue(errorListener.getSyntaxErrorInfos().isEmpty(), | |
| () -> "Unexpected syntax errors: " + errorListener.getSyntaxErrorInfos()); | |
| return stmtList; |
🤖 Prompt for AI Agents
In src/test/java/org/piccode/rt/Runtime.java around lines 66 to 69, the
IDEErrorListener is added to the lexer and parser but its collected syntax
errors are not checked. After parsing, add an assertion to check if the
errorListener has recorded any syntax errors and fail the test immediately if
errors exist, ensuring the compilation process fails fast on invalid input.
| return true; | ||
| } |
There was a problem hiding this comment.
Array pattern matches even when the pattern is non-empty
The catch-all branch accepts any ArrayAst pattern whenever the value is an empty list, regardless of whether the pattern itself has elements, resulting in incorrect matches.
Add a check that the pattern contains zero elements:
-if (pattern instanceof ArrayAst && value instanceof PiccodeArray vList && vList.nodes.isEmpty()) {
- return true;
+if (pattern instanceof ArrayAst arrPat
+ && value instanceof PiccodeArray vList
+ && arrPat.nodes.isEmpty()
+ && vList.nodes.isEmpty()) {
+ return true;
}📝 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.
| return true; | |
| } | |
| if (pattern instanceof ArrayAst arrPat | |
| && value instanceof PiccodeArray vList | |
| && arrPat.nodes.isEmpty() | |
| && vList.nodes.isEmpty()) { | |
| return true; | |
| } |
🤖 Prompt for AI Agents
In src/main/java/org/piccode/ast/WhenAst.java at lines 138-139, the code
currently returns true for any ArrayAst pattern when the value is an empty list,
even if the pattern is non-empty. To fix this, add a condition to check that the
pattern's element count is zero before returning true, ensuring only empty
patterns match empty lists.
| var tempSymtable = new HashMap<String, PiccodeValue>(); | ||
| if (isMatching(match_case.match, cond_value, tempSymtable)) { | ||
| Context.top.pushStack(); | ||
| for (var entry : tempSymtable.entrySet()) { | ||
| Context.top.putLocal(entry.getKey(), entry.getValue()); | ||
| } | ||
| var result = match_case.value.execute(); | ||
| Context.top.dropStackFrame(); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
Ensure stack-frame cleanup with try/finally
If match_case.value.execute() throws, dropStackFrame() is skipped, leaking the pushed scope and corrupting subsequent evaluation.
-Context.top.pushStack();
-for (var entry : tempSymtable.entrySet()) {
- Context.top.putLocal(entry.getKey(), entry.getValue());
-}
-var result = match_case.value.execute();
-Context.top.dropStackFrame();
-return result;
+Context.top.pushStack();
+try {
+ for (var entry : tempSymtable.entrySet()) {
+ Context.top.putLocal(entry.getKey(), entry.getValue());
+ }
+ return match_case.value.execute();
+} finally {
+ Context.top.dropStackFrame();
+}📝 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.
| var tempSymtable = new HashMap<String, PiccodeValue>(); | |
| if (isMatching(match_case.match, cond_value, tempSymtable)) { | |
| Context.top.pushStack(); | |
| for (var entry : tempSymtable.entrySet()) { | |
| Context.top.putLocal(entry.getKey(), entry.getValue()); | |
| } | |
| var result = match_case.value.execute(); | |
| Context.top.dropStackFrame(); | |
| return result; | |
| } | |
| var tempSymtable = new HashMap<String, PiccodeValue>(); | |
| if (isMatching(match_case.match, cond_value, tempSymtable)) { | |
| Context.top.pushStack(); | |
| try { | |
| for (var entry : tempSymtable.entrySet()) { | |
| Context.top.putLocal(entry.getKey(), entry.getValue()); | |
| } | |
| return match_case.value.execute(); | |
| } finally { | |
| Context.top.dropStackFrame(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/main/java/org/piccode/ast/WhenAst.java around lines 44 to 53, the call to
Context.top.dropStackFrame() is skipped if match_case.value.execute() throws an
exception, causing a stack-frame leak. To fix this, wrap the code between
Context.top.pushStack() and Context.top.dropStackFrame() in a try/finally block,
ensuring dropStackFrame() is always called regardless of exceptions.
2 participants
I have implemented the when expression (#20) and added the first tests using junit5.
Summary by CodeRabbit