Implement shell parser and AST for /bin/sh#887
Conversation
Add a recursive-descent parser that consumes the lexer's Token stream and produces an abstract syntax tree. AST types: SimpleCommand (words, assignments, redirections), Pipeline (commands connected by |), List (pipelines connected by ;, &&, ||), and Subshell (parenthesized lists). Operator precedence is structural: | binds inside Pipeline, while &&/||/; are connectors between pipelines in the flat List. The parser handles all seven redirection operators including fd-number prefixes (e.g. 2>&1), variable assignments preceding commands, and nested subshells. 46 host-side unit tests cover simple commands, pipelines, compound lists, subshells, all redirection forms, fd-number redirects, syntax errors, and realistic command lines. Closes #875 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughA new ChangesShell Parser Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@base/sh/src/parser.rs`:
- Around line 354-372: The code consumes a digit word via self.bump() as soon as
try_parse_fd(w) succeeds, then bails out if the following token isn't a
redirect, which drops input like "(echo) 2"; instead, only consume the digit
when a redirect operator actually follows. Change the flow in the parse loop so
you check for a redirect operator before calling self.bump() — e.g., use a
lookahead/peek of self.current (or a peek method) with
is_redirect_token(&self.current) or similar, and only call self.bump() then
parse_redirect_op() and expect_redirect_target() to push into redirects; if no
redirect operator is present, do not bump/consume the digit and simply break.
Ensure you update the branch that currently calls self.bump() and then checks
is_redirect_token so it peeks first and preserves the digit token when no
redirect follows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4287abc-7a24-44e9-b436-6e63710fafdc
📒 Files selected for processing (2)
base/sh/src/main.rsbase/sh/src/parser.rs
When parse_redirect_list consumed a digit-only word speculatively and the following token was not a redirect operator, the word was silently dropped. Now return UnexpectedToken instead, preventing inputs like "(echo) 2" from parsing as if the "2" never existed. Adds two tests: one for the error case and one confirming that fd-number redirects after subshells (e.g. "(echo) 2> err") still work. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Closes #875
Summary
base/sh/src/parser.rs) that consumes the lexer'sTokenstream and produces an ASTSimpleCommand(words + assignments + redirections),Pipeline(commands connected by|),List(pipelines connected by;/&&/||),Subshell(parenthesized lists with optional redirections)|binds insidePipeline, while&&/||/;are connectors between pipelines in the flatList— matching POSIX.1-2024 section 2.10 left-to-right evaluation semantics<,>,>>,<<,>&,<&,<>), fd-number prefixes (e.g.2>&1), and variable assignments preceding commandsTest plan
cargo test --manifest-path base/sh/Cargo.toml— 95 tests pass (49 lexer + 46 parser), zero warningscargo xtask build— cleancargo xtask smoke— all 42 markers presentcargo xtask test—blocking_sync::rwlock_concurrent_readersflake is pre-existing on main (confirmed by running same test on main branch)