Implement exception handling in java-slang - #96
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for compiling try-catch-finally statements, refactors the JVM's ExceptionTable to be iterable and use a dedicated class structure, and adds unit tests for exception routing. It also fixes a buffer offset calculation bug in the disassembler when reading code attributes. The reviewer identified several critical bugs in the TryStatement code generation implementation—including a pass-by-value bug with unresolved label offsets, an early return bug when catches are absent but a finally block is present, and incorrect execution paths for finally blocks—and provided a comprehensive rewrite to address these issues.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success1148 tests passing in 65 suites. Report generated by 🧪jest coverage report action from df4fede |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces support for parsing and compiling Java try-catch-finally and throw statements, including AST extraction, code generation, and JVM exception table handling. Feedback on the changes identifies several critical issues: first, accessing finallyNode.blockStatements directly in the code generator will throw a runtime TypeError because finallyNode is of type Finally and its statements reside under finallyNode.block.blockStatements; second, the catch-all exception handler for the finally block is added before specific catch clauses, which would shadow them in the JVM exception table; and third, raw parser tokens (such as LCurly, RCurly, Throw, Try, Catch, and Finally) are incorrectly assumed to have a .location property or are used directly as Location objects, and should instead have their location properties explicitly mapped.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
This reverts commit 8e933b6.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
| @@ -0,0 +1,202 @@ | |||
| import { runTest, testCase } from "./__utils__/test-utils"; | |||
There was a problem hiding this comment.
Adding this file makes yarn test fail. It's the second suite to use runTest from __utils__/test-utils.ts, and that harness isn't safe to run concurrently — __tests__/index.ts was its only consumer until now, so the problem couldn't surface.
Every test writes Main.class and output.log into the same fixed directory (test-utils.ts line 19 and lines 33–44). Jest runs suites in parallel worker processes, so one worker overwrites Main.class while another is about to execute it.
Measured on this branch, 12-core machine:
| invocation | result |
|---|---|
yarn test |
98 failed, 2 suites red |
yarn test --runInBand |
1148/1148 pass |
both compiler suites, --maxWorkers=1 |
105/105 pass |
both compiler suites, --maxWorkers=2 |
7 failed |
The same full parallel run on main is 1134/1134 green, so this is specific to the branch.
CI passing doesn't reassure me much here: build.yml runs plain yarn test, and Jest sizes its worker pool from the core count, so a small runner ends up with a single worker — the one configuration where this can't fire.
Cleanest fix is probably giving each runTest call its own temp directory (fs.mkdtempSync) and dropping the process.chdir. Scoping maxWorkers: 1 to the compiler tests, or --runInBand in the test script, would also work but are slower and leave the harness fragile.
Entirely reasonable to say this belongs in a separate PR since the harness is pre-existing — but this branch is where it starts breaking.
This review was generated with Claude Code.
| const catchAllLabel = finallyNode ? cg.generateNewLabel() : null | ||
|
|
||
| // For normal path: run finally block if it exists | ||
| if (finallyNode) { |
There was a problem hiding this comment.
The finally block is inlined in three places: the normal fall-through path (here), the end of each catch clause, and the catch-all rethrow handler. Nothing is emitted before an abrupt-completion opcode, so return, break and continue inside the try skip it entirely.
JLS SE 8 §14.20.2 requires finally to run when the try completes abruptly for any reason. Four-case test run on this branch, all four fail:
| exit path | expected | actual |
|---|---|---|
return; |
["7"] |
[] |
return 1; |
["9", "1"] |
["1"] |
break |
["8", "8", "0"] |
["8", "0"] |
continue |
["6", "6", "0"] |
["0"] |
The return 1; row is the one that worries me — the method still returns 1 and nothing crashes, the cleanup just silently doesn't run, so any test asserting only on return values passes.
For reference, javac stashes the return value in a local, runs the inlined finally, reloads it, then returns. jsr/ret aren't an option since they're banned from class file version 51.0 and this compiler emits 52.
This review was generated with Claude Code.
| accessFlags: 0, | ||
| index: cg.maxLocals, | ||
| typeName: varTypeStr, | ||
| typeDescriptor: cg.symbolTable.generateFieldDescriptor(varTypeStr) |
There was a problem hiding this comment.
This generateFieldDescriptor(varTypeStr) call makes the same queryClass lookup as the catch-type resolution above, but unguarded — and it throws. Line 637 wraps its lookup in a try/catch with a fallback; this one doesn't.
So catching a type the symbol table doesn't know throws SymbolNotFoundError out of the compiler as an unhandled internal error rather than a compile diagnostic. From a student's point of view that's a crash.
I tested 15 catch types on this branch — only Exception and Throwable compile. RuntimeException, Error, ArithmeticException, NullPointerException, ArrayIndexOutOfBoundsException, NumberFormatException, IllegalArgumentException, ClassCastException, StackOverflowError and fully-qualified java.lang.NullPointerException all crash. Several of those are what students hit first.
The root cause sits outside this PR — src/types/types/references.ts models the whole hierarchy as Throwable and Exception extends Throwable — but this PR is what makes it reachable, since catch didn't exist before.
There's a second problem stacked on it: even if this call were guarded, the fallback on line 640 builds the class name by replacing dots with slashes, so bare NullPointerException would become the constant-pool entry NullPointerException rather than java/lang/NullPointerException, and the handler would silently never match.
This didn't surface in the tests because the catch types across all three test files are Exception ×10, Throwable ×1 and String ×1 — exactly the two that work.
Could we either populate the common java.lang exception subclasses, or at minimum emit a proper compile error instead of crashing? A test catching a subtype would be worth having either way.
This review was generated with Claude Code.
| let isChecked = true | ||
| if (!(exceptionBase instanceof TypeCheckerError)) { | ||
| // checked if it's an Exception subtype | ||
| isChecked = (exceptionBase as any).canBeAssigned(declaredException) |
There was a problem hiding this comment.
This treats an exception as checked if and only if it's a subtype of Exception. Per JLS §11.1.1 the rule is subtype of Exception excluding subtypes of RuntimeException — so as written, NullPointerException and friends would be classified checked and their callers rejected, even though the code is legal Java.
I couldn't make it actually misfire today: throws RuntimeException fails earlier with CannotFindSymbolError, since RuntimeException isn't a modelled type. So this is latent rather than an active bug — flagging it because it goes live the moment the hierarchy above is filled in, and it'll be harder to spot then.
(Error is handled correctly by accident, being outside the Exception subtree.)
This review was generated with Claude Code.
3 participants
Implement exception handling for the following components (in progress):