;&, ;|, ;;& in case command
#430
Conversation
WalkthroughThe updates in this pull request primarily enhance the functionality and compliance of the Yash shell with POSIX standards, particularly regarding the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
Previously, we parsed the trailing double semicolon in a case item outside of the `case_item` method. This commit changes the method to consume the token itself, and returns whether it consumed a terminator token. This will allow us to parse the case items more naturally and also to parse different terminators in the `case_item` method in the future.
magicant
force-pushed
the
case-fall-thru
branch
from
94f8ac1 to
c381f93
Compare
Previously, the parser only recognized the `;;` terminator for a case item. This commit changes the parser to recognize all types of CaseContinuation, including `;&` and `;;&`.
magicant
force-pushed
the
case-fall-thru
branch
from
c381f93 to
ca82229
Compare
Added test cases for the newly supported ;& terminator to "case-p.sh" and added a new test script "case-y.sh" for yash-specific behavior of the case command. "case-y.sh" is based on: https://github.com/magicant/yash/blob/31a012761f5f5cd56851e9805312a0a2ec903992/tests/case-y.tst
magicant
force-pushed
the
case-fall-thru
branch
from
ca82229 to
88fee9d
Compare
![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
🧹 Outside diff range and nitpick comments (12)
yash-cli/tests/scripted_test/case-y.sh (3)
3-15: Consider adding more pattern matching test cases.The current test effectively verifies basic pattern expansion order, but consider adding test cases for:
- Empty patterns (
||)- Complex patterns with wildcards
- Patterns with quoted characters
75-166: Consider adding modern shell error cases.The error handling tests are comprehensive for basic syntax, but consider adding test cases for:
- Unicode characters in patterns
- Very long patterns (buffer overflow checks)
- Patterns with ANSI escape sequences
16-18: Enhance documentation of shell-specific behaviors.Consider structuring the compatibility notes more formally:
- Group related compatibility notes into a dedicated section
- Add references to relevant POSIX specifications
- Document the expected behavior differences between shells in a table format
Also applies to: 26-28, 61-62, 70-71
yash-semantics/CHANGELOG.md (1)
12-13: Consider adding more details about case command terminators.The changelog could be more detailed about the specific case command terminators and POSIX mode behavior. Consider expanding it to:
- `<yash_syntax::syntax::CompoundCommand as command::Command>::execute` now - honors the `CaseContinuation` specified for the executed case item. + `<yash_syntax::syntax::CompoundCommand as command::Command>::execute` now + honors the `CaseContinuation` specified for the executed case item: + - Added support for case branch terminators: ;&, ;|, and ;;& + - The ;| operator is disabled in POSIX mode for complianceyash-cli/tests/scripted_test/case-p.sh (1)
214-222: Consider using a variable in the case statement.While the test effectively verifies exit status preservation across empty case items with ;&, consider addressing the static analysis warning by using a variable instead of the constant 'i'.
Here's a suggested improvement:
-case i in +word=i +case $word in🧰 Tools
🪛 Shellcheck
[warning] 216-216: This word is constant. Did you forget the $ on a variable?
(SC2194)
yash-syntax/CHANGELOG.md (1)
8-17: Consider documenting POSIX mode behavior for;|The changelog clearly documents the new case item terminators, but it would be helpful to explicitly mention the POSIX mode behavior mentioned in the PR description, specifically about disabling the
;|operator in POSIX mode.Add a note about POSIX mode behavior:
- Extended the case item syntax to allow `;&`, `;|`, and `;;&` as terminators. - The `SemicolonAnd`, `SemicolonSemicolonAnd`, and `SemicolonBar` variants are added to the `parser::lex::Operator` enum. - The `parser::Parser::case_item` and `syntax::CaseItem::from_str` methods now consume a trailing terminator token, if any. The terminator can be not only `;;`, but also `;&`, `;|`, or `;;&`. + - The `;|` terminator is disabled in POSIX mode for standards compliance.yash-syntax/src/parser/list.rs (1)
54-56: Add test cases for new case operators.Please add test cases to verify error handling for the new operators (
SemicolonAnd,SemicolonSemicolon,SemicolonSemicolonAnd,SemicolonBar) when they appear as trailing tokens in command lines.Here's a suggested test case:
#[test] fn parser_command_line_case_operators_without_case() { let test_cases = [ ("foo;&", ";&"), ("foo;;&", ";;&"), ("foo;;& ", ";;&"), ("foo;|", ";|"), ]; for (input, operator) in test_cases { let mut lexer = Lexer::from_memory(input, Source::Unknown); let mut parser = Parser::new(&mut lexer, &EmptyGlossary); let e = parser.command_line().now_or_never().unwrap().unwrap_err(); assert_eq!(e.cause, ErrorCause::Syntax(SyntaxError::UnopenedCase)); assert_eq!(*e.location.code.value.borrow(), input); assert_eq!(e.location.code.start_line_number.get(), 1); assert_eq!(*e.location.code.source, Source::Unknown); assert_eq!(e.location.range, input.len() - operator.len()..input.len()); } }yash-syntax/src/parser/case.rs (4)
105-115: Address the TODO: Implement POSIX mode restrictions for continuation tokensThere's a
TODOcomment indicating thatContinuetokens should be rejected in strict POSIX mode. Implementing this check will ensure that the parser adheres to POSIX compliance when required.Would you like assistance in implementing this POSIX mode restriction, or should we open a new GitHub issue to track this task?
105-110: Avoid variable shadowing ofcontinuationfor clarityThe variable
continuationis shadowed, which may cause confusion:
- Initially declared as
Option<CaseContinuation>:let continuation = match self.peek_token().await?.id { Operator(op) => op.try_into().ok(), _ => None, };- Then shadowed as
CaseContinuation:let continuation = continuation.unwrap_or_default();Consider renaming one of the variables to improve code readability.
Apply this diff to rename the initial
continuationvariable:let continuation_option = match self.peek_token().await?.id { Operator(op) => op.try_into().ok(), _ => None, }; let continued = continuation_option.is_some(); let continuation = continuation_option.unwrap_or_default();
329-342: Add tests for additional continuation operatorsCurrently, the test
parser_case_item_with_semicolon_andcovers the;&operator. To ensure comprehensive test coverage of the new syntax features, consider adding tests for the;;∧|continuation operators.Would you like assistance in writing these additional test cases?
249-254: Consistency in test assertions forcontinuationandcontinuedIn
parser_case_item_with_open_paren, similar assertions are made:assert_eq!(item.continuation, CaseContinuation::Break); assert!(!continued);Ensure consistency across all tests regarding the expected values of
item.continuationandcontinued, especially when different continuation tokens are involved.yash-syntax/src/syntax.rs (1)
1295-1298: Ensure proper spacing infmt::DisplayforCaseItemThe current formatting may lead to missed spaces between the
bodyandcontinuation, especially ifbodydoes not end with a space.Consider adjusting the format string to include deliberate spacing:
impl fmt::Display for CaseItem { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!( - f, - "({}) {}{}", + write!( + f, + "({}) {} {}", self.patterns.iter().format(" | "), self.body, self.continuation, ) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
yash-cli/CHANGELOG-bin.md(1 hunks)yash-cli/tests/scripted_test.rs(1 hunks)yash-cli/tests/scripted_test/case-p.sh(2 hunks)yash-cli/tests/scripted_test/case-y.sh(1 hunks)yash-semantics/CHANGELOG.md(1 hunks)yash-semantics/src/command/compound_command.rs(1 hunks)yash-semantics/src/command/compound_command/case.rs(2 hunks)yash-syntax/CHANGELOG.md(2 hunks)yash-syntax/src/parser/case.rs(10 hunks)yash-syntax/src/parser/from_str.rs(1 hunks)yash-syntax/src/parser/lex/op.rs(4 hunks)yash-syntax/src/parser/list.rs(1 hunks)yash-syntax/src/syntax.rs(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- yash-semantics/src/command/compound_command.rs
🧰 Additional context used
🪛 Shellcheck
yash-cli/tests/scripted_test/case-p.sh
[warning] 204-204: This word is constant. Did you forget the $ on a variable?
(SC2194)
[warning] 216-216: This word is constant. Did you forget the $ on a variable?
(SC2194)
yash-cli/tests/scripted_test/case-y.sh
[error] 38-38: Couldn't parse this case item. Fix to allow more checks.
(SC1073)
[error] 38-38: Expected a command. Fix any mentioned problems and try again.
(SC1072)
🔇 Additional comments (25)
yash-cli/CHANGELOG-bin.md (1)
15-18: LGTM! Clear documentation of the new case branch terminators.
The documentation clearly explains:
- The behavior of each terminator (
;&for fall-through,;|for pattern matching resumption) - The distinction between POSIX.1-2024 features and extensions
- The compatibility alias
;;&
Let's verify the POSIX.1-2024 compliance claim for the ;& terminator:
yash-semantics/CHANGELOG.md (1)
12-13: LGTM! The changelog entry follows the proper format.
The entry correctly documents the behavioral change in the execute method and follows the Keep a Changelog format.
yash-cli/tests/scripted_test/case-p.sh (3)
203-212: LGTM! Well-structured test for ;& operator.
The test case thoroughly verifies the fall-through behavior and exit status preservation of the ;& operator.
🧰 Tools
🪛 Shellcheck
[warning] 204-204: This word is constant. Did you forget the $ on a variable?
(SC2194)
192-192: LGTM! Good separation of POSIX and extended features.
The comment clearly explains why this test case is disabled here and references where to find it, maintaining a clear separation between POSIX-compliant and extended features.
203-222: Verify comprehensive implementation of the ;& operator.
Let's verify the complete implementation of the ;& operator across the codebase.
✅ Verification successful
Comprehensive implementation of the ;& operator verified.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for comprehensive implementation of ;& operator
# Check for documentation
echo "Checking for documentation..."
rg -l ";&" . --type md
# Check for implementation in parser and related files
echo "Checking for implementation..."
rg -l ";&" . --type rust
# Check for additional test cases
echo "Checking for test coverage..."
rg -l ";&" . --type sh
Length of output: 596
🧰 Tools
🪛 Shellcheck
[warning] 204-204: This word is constant. Did you forget the $ on a variable?
(SC2194)
[warning] 216-216: This word is constant. Did you forget the $ on a variable?
(SC2194)
yash-cli/tests/scripted_test.rs (1)
153-156: LGTM! The test function follows established patterns.
The new test function case_command_ex is well-structured and follows the project's conventions:
- Uses the
-y.shsuffix indicating Yash-specific extensions - Maintains alphabetical ordering with other test functions
- Follows the same implementation pattern as other test functions
Let's verify the test script exists and contains the necessary test cases:
✅ Verification successful
LGTM! The case_command_ex test function is properly implemented and includes test cases for the new case command features.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the test script exists and contains test cases for new case command features
# Check if the test script exists
test -f tests/scripted_test/case-y.sh || echo "Error: case-y.sh not found"
# Look for test cases covering new case command features
rg -A 5 ';&|;&&|;\|' tests/scripted_test/case-y.sh
Length of output: 225
Script:
#!/bin/bash
# Description: Search for 'case-y.sh' in the entire codebase
fd 'case-y.sh'
Length of output: 53
Script:
#!/bin/bash
# Description: Verify that 'case-y.sh' contains test cases for new case command features
# Define the correct path to the test script
TEST_SCRIPT="yash-cli/tests/scripted_test/case-y.sh"
# Check if the test script exists
test -f "$TEST_SCRIPT" || echo "Error: $TEST_SCRIPT not found"
# Look for test cases covering new case command features
rg ';&|;&&|;\|' "$TEST_SCRIPT"
Length of output: 483
yash-syntax/src/parser/lex/op.rs (4)
46-53: LGTM: New operator variants are well-defined and documented.
The new variants for case command terminators (;&, ;;&, ;|) are correctly implemented with clear documentation.
98-101: LGTM: String representations are accurate.
The string representations for the new operators are correctly implemented in the as_str method.
122-138: LGTM: Clause delimiter logic is correctly updated.
The is_clause_delimiter method is properly updated to handle the new case terminators, with comprehensive documentation and correct implementation.
239-260: Verify trie structure remains sorted.
The trie implementation for new operators looks correct, but we should verify that it maintains the sorted property required by the tests.
✅ Verification successful
Trie structure is correctly sorted.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the new operators maintain the sorted order in the trie.
# The operators should be in ASCII order: & < ; | for the SEMICOLON trie.
# Test: Check the order of operators in the SEMICOLON trie
rg -A 15 'const SEMICOLON: Trie = Trie\(&\[' yash-syntax/src/parser/lex/op.rs
Length of output: 435
yash-syntax/src/parser/list.rs (1)
54-56: LGTM! Error handling for case operators is correctly implemented.
The implementation properly handles all case-related operators (;;&, ;&, ;|) by returning UnopenedCase error when they appear outside of a case statement, which aligns with POSIX shell behavior.
yash-semantics/src/command/compound_command/case.rs (5)
62-86: Well-structured control flow in execute function
The integration of the falling_through flag and the updated control flow within the execute function effectively manages the execution of case items based on their continuation type. This enhancement improves readability and aligns the implementation with the intended shell behavior for case commands.
90-116: Efficient encapsulation of pattern matching in matches function
The new matches function neatly encapsulates the logic for pattern matching, including pattern expansion, escape handling, and error propagation. This modular approach enhances code maintainability and adheres to POSIX specifications regarding unquoted backslashes in patterns.
360-373: Comprehensive test for standard ;; terminator behavior
The breaking_terminator test accurately verifies that the execution stops after the first matching pattern when using the standard ;; terminator. This test ensures the correctness of the case command's breaking behavior, aligning with expected shell functionality.
375-393: Effective validation of fall-through behavior with ;& terminator
The falling_through_terminator test successfully checks the fall-through functionality introduced with the ;& terminator. It confirms that subsequent case items are executed sequentially after a match, which is critical for the new syntax and behavior enhancements.
394-413: Proper testing of continuous execution using ;| terminator
The continuing_terminator test effectively verifies the continuous execution flow implemented with the ;| terminator. This test ensures that the execution continues to subsequent case items regardless of matching, which is an essential aspect of the updated control flow logic.
yash-syntax/src/parser/case.rs (3)
233-238: Confirm test correctness and update assertions if necessary
In the test parser_case_item_minimum, the assertions are:
assert_eq!(item.continuation, CaseContinuation::Break);
assert!(!continued);Verify that the CaseContinuation::Break is the expected default and that continued correctly reflects whether there may be additional case items.
Ensure that this test accurately represents the minimal case item and that the assertions align with the intended behavior.
363-368: Verify handling of special tokens in patterns
In the test parser_case_item_esac_after_paren, the pattern is (esac). Since esac is a reserved word, confirm that the parser correctly treats it as a pattern and not as a terminator.
Would you like assistance in verifying the correct parsing of reserved words within patterns?
172-174: Ensure proper loop termination in case_command
In the case_command method:
while let Some((item, continued)) = self.case_item().await? {
items.push(item);
if !continued {
break;
}
}The loop breaks when !continued. Ensure that this logic correctly handles all possible continuation cases, especially with the introduction of new continuation tokens.
Run the following script to verify all cases where continued might affect loop termination:
✅ Verification successful
Loop termination in case_command properly handles all continuation cases.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze the handling of the 'continued' flag in 'case_command'
# Search for all usages of 'continued' in 'case.rs' and display relevant code
rg 'continued' -A 5 yash-syntax/src/parser/case.rs
Length of output: 4443
yash-syntax/src/syntax.rs (6)
1236-1254: TryFrom<Operator> implementation appears correct
The conversion logic from Operator to CaseContinuation correctly handles the mapping, including the intentional merging of SemicolonBar and SemicolonSemicolonAnd into Continue.
1256-1269: From<CaseContinuation> implementation is appropriate
The From trait implementation correctly converts CaseContinuation variants back to their respective Operator values.
1271-1275: fmt::Display implementation leverages existing formatting
Utilizing Operator::from(*self).fmt(f) efficiently reuses the Display logic from Operator, ensuring consistent formatting.
2252-2264: Test case_continuation_conversions covers all variants
The unit test effectively checks the bidirectional conversions between CaseContinuation and Operator, ensuring correctness.
2267-2293: Test case_item_display validates the Display implementation
The test cases cover different scenarios, confirming that CaseItem displays correctly with various continuation variants.
1287-1288: Verify that the new continuation field is properly handled throughout
Adding pub continuation: CaseContinuation to CaseItem is a significant change. Ensure that all usages of CaseItem are updated accordingly, and that serialization or pattern-matching logic accounts for this new field.
Run the following script to search for any patterns that may need updating:
✅ Verification successful
All instances of CaseItem correctly handle the new continuation field.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find instances where `CaseItem` is constructed without the `continuation` field.
# Search for struct literals of `CaseItem` missing the `continuation` field.
rg -A 3 'CaseItem \{[^\}]*\}' --glob '!**/tests/*' | grep -B 1 -A 2 'CaseItem.*\{'
# Search for pattern matches on `CaseItem` missing `continuation`.
rg 'CaseItem\(([^,]*,){2}\s*\)' --glob '!**/tests/*'
Length of output: 158
Script:
#!/bin/bash
# Description: Find instances where `CaseItem` is constructed without the `continuation` field.
# Search for struct literals of `CaseItem` missing the `continuation` field.
rg -A 3 'CaseItem\s*\{[^}]*\}' --glob '!**/tests/*' | grep -B 1 -A 2 'CaseItem\s*{'
# Search for pattern matches on `CaseItem` missing `continuation`.
rg 'CaseItem\([^,]*,[^,]*\)' --glob '!**/tests/*'
Length of output: 137
| test_oE -e 42 'pattern matching after ;|' | ||
| case 1 in | ||
| 0) echo not reached 0;; | ||
| 1) echo matched 1; (exit 12);| |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix shellcheck parsing warnings and enhance terminator tests.
- The shellcheck tool reports parsing issues. Consider adding a space before
;|to improve parsing:
- 1) echo matched 1; (exit 12);|
+ 1) echo matched 1; (exit 12) ;|- Consider adding test cases for:
- Nested case statements with different terminators
- Multiple
;;∧|combinations in the same case statement
Also applies to: 50-54
🧰 Tools
🪛 Shellcheck
[error] 38-38: Couldn't parse this case item. Fix to allow more checks.
(SC1073)
[error] 38-38: Expected a command. Fix any mentioned problems and try again.
(SC1072)
| @@ -353,6 +364,7 @@ command. | |||
| - Functionalities to parse POSIX shell scripts | |||
| - Alias substitution support | |||
|
|
|||
| [0.13.0]: https://github.com/magicant/yash-rs/releases/tag/yash-syntax-0.13.0 | |||
There was a problem hiding this comment.
Remove premature version link
The version link for 0.13.0 should not be added until the version is actually released, as the URL doesn't exist yet and would result in a broken link.
-[0.13.0]: https://github.com/magicant/yash-rs/releases/tag/yash-syntax-0.13.0📝 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.
| [0.13.0]: https://github.com/magicant/yash-rs/releases/tag/yash-syntax-0.13.0 |
| @@ -269,7 +269,7 @@ impl FromStr for CaseItem { | |||
| let mut lexer = Lexer::from_memory(s, Source::Unknown); | |||
| let mut parser = Parser::new(&mut lexer, &EmptyGlossary); | |||
| unwrap_ready(async { | |||
| let item = parser.case_item().await?; | |||
| let item = parser.case_item().await?.map(|(item, _)| item); | |||
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Preserve continuation information for case items
The current implementation discards the continuation information from the case_item() parser. This may lead to improper handling of case item terminators like ;&, ;|, and ;;&.
🔗 Analysis chain
Consider preserving continuation information for case items
The current implementation discards the continuation information from the case_item() parser. This might affect the handling of different case item terminators (;;, ;&, ;|, ;;&).
Consider preserving the continuation information:
- let item = parser.case_item().await?.map(|(item, _)| item);
+ let item = parser.case_item().await?.map(|(item, cont)| {
+ // Handle continuation based on the terminator type
+ match cont {
+ true => item, // Handle ;& ;| or ;;& terminator
+ false => item // Handle ;; terminator
+ }
+ });🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the handling of case item terminators in the codebase
# Test: Search for case item terminator handling in tests and implementation
# Check test coverage for different terminators
rg -A 5 "(;[;&|]|;;)" "yash-cli/tests/scripted_test/"
# Check implementation of terminator handling
ast-grep --pattern 'match $_ {
$$$
Operator::SemicolonSemicolon | Operator::SemicolonAmpersand | Operator::SemicolonPipe | Operator::SemicolonSemicolonAmpersand => $$$
$$$
}'
Length of output: 26681
Script:
#!/bin/bash
# Description: Inspect the implementation of case item terminators handling
ast-grep --pattern 'match $_ {
$$$
Operator::SemicolonSemicolon | Operator::SemicolonAmpersand | Operator::SemicolonPipe | Operator::SemicolonSemicolonAmpersand => $$$
$$$
}' yash-syntax/src/parser/from_str.rs
Length of output: 217
Script:
#!/bin/bash
# Description: Broadly search for handling of case item terminators across the codebase
# Search for any usage of the specific operators in from_str.rs
ast-grep --pattern 'Operator::SemicolonSemicolon|Operator::SemicolonAmpersand|Operator::SemicolonPipe|Operator::SemicolonSemicolonAmpersand' yash-syntax/src/parser/from_str.rs
# Search for definitions or implementations of these operators in the entire codebase
rg 'Operator::SemicolonSemicolon|Operator::SemicolonAmpersand|Operator::SemicolonPipe|Operator::SemicolonSemicolonAmpersand' yash-syntax/src/
Length of output: 722
| let continuation = match self.peek_token().await?.id { | ||
| Operator(op) => op.try_into().ok(), | ||
| _ => None, | ||
| }; | ||
| let continued = continuation.is_some(); | ||
| let continuation = continuation.unwrap_or_default(); | ||
| if continued { | ||
| self.take_token_raw().await?; | ||
| // TODO Reject Continue in strict POSIX mode | ||
| } | ||
|
|
There was a problem hiding this comment.
Handle possible errors when consuming continuation tokens
In the block where continuation tokens are consumed:
if continued {
self.take_token_raw().await?;
// TODO Reject Continue in strict POSIX mode
}Ensure that take_token_raw().await? correctly handles any potential errors that may occur when consuming the token, such as unexpected tokens or end-of-input scenarios.
Consider adding error handling to manage unexpected cases gracefully.
;|in the POSIX modeSummary by CodeRabbit
Release Notes
New Features
;&,;|, and;;&for enhanced control flow.CaseContinuationenum to define termination behavior of case branches.casecommand and improved exit status reporting.Bug Fixes
Tests
Documentation