Parser menhir to handmade#68
Conversation
- Remove Parser.mly (Menhir grammar) and menhirLib dependency - Add Parser.ml: handmade recursive descent / Pratt parser - Update Core.ml to call Parser.program directly - Update dune build to remove menhir stanza - Update dune-project to remove menhir dependency - All 701 unit tests pass, CLI cram tests pass - Minor improvement: error pointers now span full expressions Co-authored-by: David Sancho <davesnx@users.noreply.github.com>
- Add benchmarks/bench_parser.ml: measures parse-only performance across 22 queries of varying complexity (100k iterations each) - Add bench-parser Makefile target - Clean up inlined error handling in Parser.ml zero-arg function case - Average parse time: ~2.7 microseconds per query Co-authored-by: David Sancho <davesnx@users.noreply.github.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 4 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| match (peek stream : Lexer.token) with | ||
| | FN -> parse_fn_def stream | ||
| | TRY -> parse_try stream | ||
| | _ -> parse_pipe_expr stream |
There was a problem hiding this comment.
Try-catch result not composable with pipe operators
High Severity
parse_fn_or_expr returns the result of parse_try directly without passing it through parse_pipe_expr. In the old Menhir grammar, try-catch was a production inside sequence_expr alongside PIPE and COMMA, so try a catch b | c correctly produced Pipe(Try(a, Some b, None), c). The new parser fails to parse this because the | c is left unconsumed. This affects all contexts using parse_sequence_expr: top-level programs, array constructions like [try .foo catch null | .bar], function arguments, if-then-else branches, etc.
Additional Locations (1)
|
|
||
| and parse_pipe_expr stream = | ||
| let left = parse_comma_expr stream in | ||
| parse_pipe_right stream left |
There was a problem hiding this comment.
Pipe-level operators not composable with trailing comma
Medium Severity
parse_pipe_expr calls parse_comma_expr then parse_pipe_right, but the result of parse_pipe_right is never fed back through comma handling. In the old Menhir grammar, COMMA had higher precedence than PIPE/ALTERNATIVE/UPDATE_ASSIGN, so a ?? b, c correctly produced Comma(Alternative(a, b), c). In the new parser, after parse_pipe_right produces Alternative(a, b), the trailing , is unconsumed, causing a parse error. Affects patterns like .foo |= . + 1, .bar |= . + 2 and a ?? "default", b.
Additional Locations (1)
| match (peek stream : Lexer.token) with | ||
| | FN -> parse_fn_def stream | ||
| | TRY -> parse_try stream | ||
| | _ -> parse_pipe_expr stream |
There was a problem hiding this comment.
Try-catch result not composable with pipe operators
High Severity
parse_fn_or_expr returns the result of parse_try directly without passing it through parse_pipe_expr. In the old Menhir grammar, try-catch was a production inside sequence_expr alongside PIPE and COMMA, so try a catch b | c correctly produced Pipe(Try(a, Some b, None), c). The new parser fails to parse this because the | c is left unconsumed. This affects all contexts using parse_sequence_expr: top-level programs, array constructions like [try .foo catch null | .bar], function arguments, if-then-else branches, etc.
Additional Locations (1)
|
|
||
| and parse_pipe_expr stream = | ||
| let left = parse_comma_expr stream in | ||
| parse_pipe_right stream left |
There was a problem hiding this comment.
Pipe-level operators not composable with trailing comma
Medium Severity
parse_pipe_expr calls parse_comma_expr then parse_pipe_right, but the result of parse_pipe_right is never fed back through comma handling. In the old Menhir grammar, COMMA had higher precedence than PIPE/ALTERNATIVE/UPDATE_ASSIGN, so a ?? b, c correctly produced Comma(Alternative(a, b), c). In the new parser, after parse_pipe_right produces Alternative(a, b), the trailing , is unconsumed, causing a parse error. Affects patterns like .foo |= . + 1, .bar |= . + 2 and a ?? "default", b.
Rewrite the parser from Menhir to a handmade recursive descent parser to improve performance, error messages, and simplify the code.