Add the portable option and its parser-level restrictions#812
Merged
Conversation
Introduce the new `Portable` shell option, intended to disable non-portable features of the shell. The option is recognized by the option parser and listed by the set built-in, but has no effect yet; behavior will be added incrementally per #807. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
List the new `portable` option in the shell-option reference, the `set -o`/`set +o` examples, and the topic index. Add a "Writing portable scripts" section to the POSIX compliance page explaining the option's purpose and how it differs from `posixlycorrect`. The option has no effect yet; the affected behaviors will be listed as they are added. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Introduce the infrastructure that lets the parser see option states that affect which syntax is accepted, so that per-item rejections for the portable option can be added later (#807). - yash-env: add `parser::Mode` (currently a single `portable` flag, created from an `OptionSet` via `From`) and a `mode` field on `parser::Config`. - yash-syntax: store the mode in the lexer and expose `Lexer::mode` and `Lexer::set_mode`. Both lexer-level and parser-level code can read it through the lexer. - yash-semantics: refresh the lexer's mode from the current options before parsing each command line, so changes via the `set` built-in take effect on subsequent input. The mode is plumbed end to end but not yet consulted, so parsing behavior is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
When the lexer's parsing mode has `portable` enabled, the case command parser now rejects the `;;&` and `;|` terminators (both parsed as `CaseContinuation::Continue`), which are extensions not portable across POSIX-conforming shells. The portable `;;` and `;&` terminators remain accepted. This is the first behavior driven by the `portable` shell option (#807), exercising the parsing-mode plumbing end to end: `set -o portable` on one command line takes effect when the following command line is parsed. - yash-syntax: add `SyntaxError::NonPortableCaseTerminator` and the rejection in the case parser, reading the mode via a new internal `Parser::mode` accessor. - yash-cli: document the option's first effect (bumped to 3.3.0) and add scripted tests. - docs: list the rejection under "Writing portable scripts" and note it on the case command and option pages. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
When the lexer's parsing mode has `portable` enabled, the redirection parser now rejects two non-portable constructs (#807): - The `>>|` and `<<<` operators, which are extensions not portable across POSIX-conforming shells (`SyntaxError::NonPortableRedirOperator`). - An `IO_NUMBER` or `IO_LOCATION` token used as a redirection operand, i.e. a number or `{...}` immediately followed by a redirection operator (`SyntaxError::NonPortableRedirOperand`). The error message advises adding a space so the token is parsed as a plain word. The standard operators and blank-separated operands remain accepted. The `{n}` redirection prefix is already rejected unconditionally because the I/O location feature is unimplemented, so it needs no change here. Also renames the lexer test `lexer_token_digit_not_followed_by_less_or_greater` to `lexer_token_digit_followed_by_semicolon` and adds `lexer_token_digit_followed_by_blank`, covering the (portable-independent) rule that a blank prevents IO_NUMBER recognition. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Under the portable option, the compound command parser now rejects a
clause-delimiting reserved word (`}`, `done`, `fi`, `then`, `elif`,
`else`, `esac`, `do`) that immediately follows a subshell or a
redirection without a separator, as in `{ ( : ) }` or `{ { :; } >foo }`
(#807).
POSIX recognizes a reserved word only when it is the first word of a
command or follows another reserved word. The lexer tags every keyword
token with which reserved word it would be, but whether the token is
actually treated as a reserved word depends on its syntactic position,
which only the parser knows. The check therefore lives in
`full_compound_command`, right after the compound command and its
redirections are parsed: a subshell ends with `)` and a redirection ends
with a word, so a reserved word that follows one (without a separator) is
not portably recognized. A reserved word after another reserved word
(`{ { :; } }`, `{ if ...; fi }`) and constructs with a separator remain
accepted, as does everything without the portable option.
The error uses a new `SyntaxError::MissingSeparatorBeforeReservedWord`
rather than reusing `MissingSeparator`: this is essentially a special
case of a missing separator (were the shell always in portable mode it
would simply be one), but a dedicated variant lets the message state that
the restriction comes from the portable option while the label still
points out that a separator is missing.
- yash-syntax: add the variant and the rejection (read via `Parser::mode`).
- yash-cli: document the option's new effect (3.3.0) and add scripted
tests (grouping-y.sh), including portable-off counterparts.
- docs: explain reserved-word recognition and the extension centrally in
the keywords page, linked from the grouping and POSIX-compliance pages.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The escapes \E, \?, \u, \U, and \c@ are yash extensions that POSIX does not define for $'...', and more than two hexadecimal digits after \x is left unspecified by POSIX. With the portable option enabled, reject these with SyntaxError::NonPortableEscape and SyntaxError::TooLongHexEscape respectively. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Other shells parse `((...))` as an arithmetic command and `!(...)` as an extended glob, neither of which yash-rs implements. yash-rs parses them as nested subshells and a negated subshell, but the form is non-portable. With the portable option enabled, reject `((` and `!(` at the beginning of a command with SyntaxError::UnsupportedArithmeticCommand and SyntaxError::UnsupportedExtendedGlob respectively. A separating space (`( (` or `! (`) remains portable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The previous name was easily confused with the sibling `NonPortableRedirOperator` and implied the operand itself was non-portable. The error actually fires when a token that should be a redirection operand is lexed as an `IO_NUMBER` or `IO_LOCATION` token, so the new name names that condition directly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The previous message ("a redirection operand immediately followed by a
redirection operator is not portable") implied that any operand abutting
a redirection operator is non-portable, which is not the case. The new
message instead explains the situation to the user: the operand is
effectively missing because the token binds to the next redirection.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Errors raised because the `portable` option is enabled now carry a `note:` footnote explaining that the construct is rejected only because the option is on. Several such errors (e.g. `UnsupportedArithmeticCommand` rendering as "unsupported syntax", `IoTokenAsRedirOperand`, and `NonPortableEscape`) previously gave no hint that disabling the option would accept the input. Add `SyntaxError::footnotes` (and a delegating `ErrorCause::footnotes`), which returns the supplementary footnotes for an error as a slice of `(FootnoteType, &str)` pairs. `Error::to_report` now appends these to the report, so the portable note and the pre-existing `BangAfterBar` suggestion both flow through one data-driven path instead of ad-hoc special cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Now that portable-option errors carry a `note:` footnote explaining the `portable` option caused them, the messages and labels no longer need to repeat "in portable mode". Reword them to describe the offending syntax instead: - Labels state why the construct is non-portable (e.g. "`;;&` is not a POSIX case terminator") rather than "cannot be used in portable mode". - `UnsupportedArithmeticCommand` and `UnsupportedExtendedGlob` get their own messages explaining the `((`/`!(` ambiguity instead of the generic "unsupported syntax" shared with truly unsupported constructs, and their labels point out how other shells read the token. - `MissingSeparatorBeforeReservedWord` drops the "the portable option does not allow ..." phrasing for "a separator is missing before the reserved word". Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughAdds the Changesportable Shell Option
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
yash-cli/tests/scripted_test/redir-y.sh (1)
18-26: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the non-portable acceptance counterpart for
IO_LOCATIONoperands.Lines 18-20 only prove the portable-mode failure. Without a matching success case, an unconditional rejection of
< {n}>/dev/nullwould still pass this file. Please mirror theIO_NUMBERpair with a plain-mode test that creates'{n}'first and then runs: < {n}>/dev/null.As per coding guidelines,
yash-cli/tests/scripted_test/**: For shell-observable behavior changes, add or update scripted tests underyash-cli/tests/scripted_test/.🤖 Prompt for 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. In `@yash-cli/tests/scripted_test/redir-y.sh` around lines 18 - 26, Add the plain-mode success counterpart for IO_LOCATION handling in the scripted redirection tests. The current `test_O` case in `redir-y.sh` only covers portable rejection of `< {n}>/dev/null`, so add a matching non-portable `test_OE` scenario that first creates `'{n}'` and then runs `: < {n}>/dev/null` to confirm it is accepted outside portable mode. Mirror the existing `IO_NUMBER` test pair structure so the behavior is covered by both failure and success cases.Source: Coding guidelines
yash-cli/tests/scripted_test/quote-y.sh (1)
23-34: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
\uportable-mode case here.
portablenow rejects both\uand\U, but this fixture only exercises\U. A regression that leaves\uaccepted would still pass, so please add a siblingtest_O -d -e 2 ... $'\u0041'case next to these escape checks.As per coding guidelines,
yash-cli/tests/scripted_test/**: For shell-observable behavior changes, add or update scripted tests underyash-cli/tests/scripted_test/.🤖 Prompt for 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. In `@yash-cli/tests/scripted_test/quote-y.sh` around lines 23 - 34, Add a portable-mode scripted test case for the \u escape alongside the existing escape checks in quote-y.sh. The current fixture covers \c@, \E, and \U via test_O, but it does not exercise \u, so portable acceptance of \u could regress unnoticed. Insert a sibling test_O case using the same pattern and expected failure behavior as the other portable rejection cases, near the existing escape assertions in the scripted test.Source: Coding guidelines
🤖 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 `@docs/src/posix.md`:
- Around line 28-30: The redirection example in the POSIX docs is misleading:
the current “< 1>file” wording does not match how the parser tokenizes
redirections. Update the example in the posix.md content to a case that actually
demonstrates the rejected pattern, such as an IO_NUMBER or {…} token immediately
followed by a redirection operator, and make sure the wording matches the parser
behavior used by the redirection and case-command docs referenced in this
section.
In `@yash-env/Cargo.toml`:
- Line 3: The version for yash-env needs to be bumped as a breaking pre-1.0
release because the public API changes in Option::Portable and
parser::Config.mode can break downstream compilation. Update the yash-env
version in Cargo.toml to 0.16.0, and make sure the corresponding changelog entry
matches that breaking-release version. Also check any workspace dependency
version references that need to stay in sync with yash-env.
---
Nitpick comments:
In `@yash-cli/tests/scripted_test/quote-y.sh`:
- Around line 23-34: Add a portable-mode scripted test case for the \u escape
alongside the existing escape checks in quote-y.sh. The current fixture covers
\c@, \E, and \U via test_O, but it does not exercise \u, so portable acceptance
of \u could regress unnoticed. Insert a sibling test_O case using the same
pattern and expected failure behavior as the other portable rejection cases,
near the existing escape assertions in the scripted test.
In `@yash-cli/tests/scripted_test/redir-y.sh`:
- Around line 18-26: Add the plain-mode success counterpart for IO_LOCATION
handling in the scripted redirection tests. The current `test_O` case in
`redir-y.sh` only covers portable rejection of `< {n}>/dev/null`, so add a
matching non-portable `test_OE` scenario that first creates `'{n}'` and then
runs `: < {n}>/dev/null` to confirm it is accepted outside portable mode. Mirror
the existing `IO_NUMBER` test pair structure so the behavior is covered by both
failure and success cases.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a01115fe-f9a5-4cb2-ab98-a61fb76b6f4c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (36)
Cargo.tomldocs/src/environment/options.mddocs/src/language/commands/case.mddocs/src/language/commands/grouping.mddocs/src/language/commands/pipelines.mddocs/src/language/words/keywords.mddocs/src/language/words/quoting.mddocs/src/posix.mddocs/src/topic_index.mdyash-builtin/src/set.rsyash-cli/CHANGELOG.mdyash-cli/Cargo.tomlyash-cli/tests/scripted_test.rsyash-cli/tests/scripted_test/case-y.shyash-cli/tests/scripted_test/grouping-y.shyash-cli/tests/scripted_test/quote-y.shyash-cli/tests/scripted_test/redir-y.shyash-env/CHANGELOG.mdyash-env/Cargo.tomlyash-env/src/option.rsyash-env/src/parser.rsyash-semantics/CHANGELOG.mdyash-semantics/Cargo.tomlyash-semantics/src/runner.rsyash-syntax/CHANGELOG.mdyash-syntax/Cargo.tomlyash-syntax/src/parser/case.rsyash-syntax/src/parser/compound_command.rsyash-syntax/src/parser/core.rsyash-syntax/src/parser/error.rsyash-syntax/src/parser/grouping.rsyash-syntax/src/parser/lex/core.rsyash-syntax/src/parser/lex/escape.rsyash-syntax/src/parser/lex/token.rsyash-syntax/src/parser/pipeline.rsyash-syntax/src/parser/redir.rs
Mirror the existing IO_NUMBER coverage so the non-portable acceptance of an IO_LOCATION token used as a redirection operand is guarded too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
magicant
changed the title
21 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Implements the
portableshell option. This PR is part of #807.;;∧|terminators in the case command{ { :; } >foo }and{ ( : ) }IO_NUMBERorIO_LOCATIONtokens appearing as a redirection operand!(and((as the beginning of a commandItems not addressed in this PR
exportandread){n}prefix in redirection:PWD,OLDPWD,OPTIND,OPTARG, andLINENOvariables readonlysourceas an alias for the.built-inunsetbuilt-in is invoked without an operandChecklist
yash-cli/tests/scripted_test.rs)Summary by CodeRabbit
portableshell option to restrict scripts to portable syntax.portableacceptance/rejection behavior across affected constructs.