Refactor parameter syntax and expansion implementation #386
Conversation
In POSIX terminology, a parameter refers to an abstract entity that
can be expanded to a value. The syntactic construct `${...}` is called
a parameter expansion, and should not simply be called a parameter.
Since there are two forms of parameter expansions, the one enclosed
in braces is now called `BracedParam` to avoid confusion.
This commit introduces a new Param struct to represent a parameter in parameter expansions. Two new enum types SpecialParam and ParamType are added to represent the details of the parameter.
This commit replaces the `name: String` field in the TextUnit::RawParam variant with a `param: Param` field. The Param struct contains the precomputed parameter type in addition to the identifier.
This commit replaces the `name: String` field of the BracedParam struct with a `param: Param` field. The Param struct contains the precomputed parameter type in addition to the identifier. To compute the parameter type correctly, the lexer is now more strict about acceptable parameters. Specifically, a parameter that starts with a digit but contains non-digit characters is now considered invalid. The new variant `SyntaxError::InvalidParam` is returned in this case.
The use of Param eliminates the need for Name, which is removed in this commit. Passing the Param reference to resolve and apply functions simplifies the code by reducing the number of arguments.
WalkthroughThe recent updates introduce structural changes to parameter handling across multiple modules in the Changes
Sequence Diagram(s)No sequence diagrams are provided as the changes are primarily structural and involve refactoring and improving existing components without introducing new features or altering control flow significantly. Possibly related issues
PoemThank 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://github.com/avatars/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (4)
yash-cli/CHANGELOG-bin.md (1)
15-18: Clarify the changelog entry.The changelog entry is clear and provides examples, but it could be more concise. Consider rephrasing for brevity.
- The shell now rejects an invalid parameter as a syntax error. Specifically, - if a parameter starts with a digit but is not a valid number, the shell now - reports a syntax error instead of treating it as a variable. For example, - `${1abc}` and `${0_1}` are now syntax errors. + Parameters starting with a digit but not valid numbers are now syntax errors. For example, `${1abc}` and `${0_1}`.yash-semantics/CHANGELOG.md (3)
19-22: Clarify the changelog entry.The changelog entry is clear, but consider rephrasing for better readability.
- The `initial::VacantError` struct now has a `param: Param` field. - The `initial::NonassignableErrorCause` enum is a successor to the previous - `NonassignableError` enum. The new `NotVariable` variant has a `param: - Param` field. + The `initial::VacantError` struct now includes a `param: Param` field. + The `initial::NonassignableErrorCause` enum replaces the previous `NonassignableError` enum. The new `NotVariable` variant includes a `param: Param` field.
28-36: Clarify the changelog entry.The changelog entry is clear, but consider rephrasing for better readability.
- The `ErrorCause::UnsetParameter` variant now has a `param: Param` field. - The `message` and `label` methods of `ErrorCause` return more informative - messages for the `UnsetParameter` and `VacantExpansion` variants. - The `expansion::initial::NonassignableError` enum has been replaced with a - struct of the same name so that it can have a `Vacancy` field. - The `MessageBase::additional_annotations` method implementation for the - `Error` struct has been extended to produce more annotations for errors - with `Vacancy` information. + The `ErrorCause::UnsetParameter` variant now includes a `param: Param` field. + The `message` and `label` methods of `ErrorCause` now return more informative messages for the `UnsetParameter` and `VacantExpansion` variants. + The `expansion::initial::NonassignableError` enum has been replaced with a struct of the same name to include a `Vacancy` field. + The `MessageBase::additional_annotations` method implementation for the `Error` struct now produces more annotations for errors with `Vacancy` information.
36-37: Clarify the changelog entry.The changelog entry is clear, but consider rephrasing for better readability.
- yash-syntax 0.10.0 → 0.11.0 + Updated yash-syntax from 0.10.0 to 0.11.0.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (17)
- yash-cli/CHANGELOG-bin.md (1 hunks)
- yash-semantics/CHANGELOG.md (1 hunks)
- yash-semantics/src/expansion.rs (3 hunks)
- yash-semantics/src/expansion/initial/arith.rs (5 hunks)
- yash-semantics/src/expansion/initial/param.rs (19 hunks)
- yash-semantics/src/expansion/initial/param/resolve.rs (6 hunks)
- yash-semantics/src/expansion/initial/param/switch.rs (23 hunks)
- yash-semantics/src/expansion/initial/text.rs (4 hunks)
- yash-semantics/src/expansion/initial/word.rs (2 hunks)
- yash-syntax/CHANGELOG.md (2 hunks)
- yash-syntax/src/parser/error.rs (3 hunks)
- yash-syntax/src/parser/from_str.rs (2 hunks)
- yash-syntax/src/parser/lex.rs (1 hunks)
- yash-syntax/src/parser/lex/braced_param.rs (26 hunks)
- yash-syntax/src/parser/lex/dollar.rs (3 hunks)
- yash-syntax/src/parser/lex/raw_param.rs (6 hunks)
- yash-syntax/src/syntax.rs (48 hunks)
Files skipped from review due to trivial changes (2)
- yash-semantics/src/expansion/initial/word.rs
- yash-syntax/src/parser/lex.rs
Additional comments not posted (57)
yash-syntax/src/parser/lex/raw_param.rs (8)
21-23: Imports look good.The new imports are necessary for the changes in this file.
38-39: LGTM!The change to make
is_special_parameter_characonst fnis appropriate and improves performance.
42-49: LGTM!The change to make
is_single_char_nameaconst fnis appropriate and improves performance.
106-107: LGTM!The test for
lexer_raw_param_special_parameteris updated correctly.
124-125: LGTM!The test for
lexer_raw_param_digitis updated correctly.
142-143: LGTM!The test for
lexer_raw_param_posix_nameis updated correctly.
160-161: LGTM!The test for
lexer_raw_param_posix_name_line_continuationsis updated correctly.
65-86: Ensure proper error handling.The changes to the
raw_paramfunction look good, but ensure that errors are properly handled whenconsume_char_iffails.Verification successful
Proper error handling is present.
The
raw_paramfunction usesawait?to propagate errors fromconsume_char_if, ensuring that any encountered errors are properly handled and returned.
yash-syntax/src/parser/lex/raw_param.rs:
- Lines 65-86: Error handling through
await?is appropriate.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify proper error handling in the `raw_param` function. # Test: Search for error handling in the `raw_param` function. Expect: Proper error handling. rg --type rust -A 5 'fn raw_param' | rg 'Err'Length of output: 45
Script:
#!/bin/bash # Description: Retrieve the entire implementation of the `raw_param` function. # Search for the `raw_param` function implementation in the Rust codebase. ast-grep --lang rust --pattern 'fn raw_param($_) { $$$ }'Length of output: 63
Script:
#!/bin/bash # Description: Retrieve the entire implementation of the `raw_param` function. # Search for the `raw_param` function implementation in the Rust codebase and include sufficient context lines. rg --type rust -A 50 'fn raw_param'Length of output: 8012
yash-syntax/src/parser/lex/dollar.rs (4)
64-65: Imports look good.The new imports are necessary for the changes in this file.
127-128: LGTM!The change to the
dollar_unitfunction is appropriate and improves functionality.
200-201: LGTM!The change to the
dollar_unitfunction is appropriate and improves functionality.
201-201: LGTM!The change to the
dollar_unitfunction is appropriate and improves functionality.yash-semantics/src/expansion/initial/param/resolve.rs (1)
Line range hint
27-58:
LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
resolvematch the new signature.Verification successful
Let's correct the regex pattern and re-run the search to verify the function usage.
Function usage verified
All instances of the
resolvefunction match the new signature.
yash-semantics/src/expansion/initial/param.rsyash-semantics/src/expansion/initial/param/resolve.rsScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `resolve` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type rust -A 5 $'resolve('Length of output: 99
Script:
#!/bin/bash # Description: Verify all function calls to `resolve` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type rust -A 5 'resolve\('Length of output: 17201
yash-semantics/src/expansion/initial/text.rs (1)
104-110: LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
expandmatch the new signature.Verification successful
LGTM! But verify the function usage in the codebase.
The code changes are approved.
All instances of the
expandfunction in the codebase match the new signature.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `expand` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type rust -A 5 $'expand('Length of output: 96
Script:
#!/bin/bash # Description: Verify all function calls to `expand` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type rust -A 5 'expand\('Length of output: 36191
yash-syntax/CHANGELOG.md (1)
8-35: Changelog entry looks good.The changelog entry for version 0.11.0 is clear and comprehensive.
yash-semantics/src/expansion/initial/param.rs (4)
38-48: LGTM!The changes to the
ParamRefstruct definition are approved.
43-48: LGTM!The implementation of
From<&'a BracedParam> for ParamRef<'a>is approved.
169-179: LGTM!The new functions
braced_paramandbraced_variableare approved.
Line range hint
68-116:
LGTM! But verify the function usage in the codebase.The code changes are approved.
However, ensure that all function calls to
expandmatch the new signature.yash-semantics/src/expansion/initial/arith.rs (3)
36-36: Import statement forParamlooks good.The import is necessary for the changes made in the file.
139-139: Refactor to useparaminUnsetVariablelooks good.The change aligns with the refactoring to use
Paraminstead ofname.
196-197: Refactor to useparamin error handling looks good.The change aligns with the refactoring to use
Paraminstead ofname.yash-syntax/src/parser/error.rs (2)
48-48: Addition ofInvalidParamvariant toSyntaxErrorlooks good.The new variant is necessary for handling invalid parameter names.
178-178: Addition of error message forInvalidParamlooks good.The new error message is necessary for handling invalid parameter names.
yash-semantics/src/expansion.rs (3)
101-101: Import statement forParamlooks good.The import is necessary for the changes made in the file.
143-144: Refactor to useparaminUnsetParameterlooks good.The change aligns with the refactoring to use
Paraminstead ofname.
177-189: Refactor to useparamin error handling looks good.The change aligns with the refactoring to use
Paraminstead ofname.yash-semantics/src/expansion/initial/param/switch.rs (6)
32-32: Import statement forParamlooks good.The import is necessary for the changes made in the file.
98-103: Refactor to useparaminVacantErrorlooks good.The change aligns with the refactoring to use
Paraminstead ofname.
137-138: Refactor to useparaminNonassignableErrorCauselooks good.The change aligns with the refactoring to use
Paraminstead ofname.
192-213: Refactor to useparaminassignfunction looks good.The change aligns with the refactoring to use
Paraminstead ofname.
Line range hint
242-252: Refactor to useparaminvacant_expansion_errorfunction looks good.The change aligns with the refactoring to use
Paraminstead ofname.
Line range hint
267-287: Refactor to useparaminapplyfunction looks good.The change aligns with the refactoring to use
Paraminstead ofname.yash-syntax/src/parser/from_str.rs (1)
71-73: LGTM!The implementation of the
FromStrtrait forBracedParamcorrectly handles the conversion and error cases.yash-syntax/src/parser/lex/braced_param.rs (12)
24-24: LGTM!The import statement for
BracedParamis necessary for the new functionality.
27-27: LGTM!The import statement for
ParamTypeis necessary for the new functionality.
28-28: LGTM!The import statement for
SpecialParamis necessary for the new functionality.
40-67: LGTM!The
type_of_idfunction correctly determines the type of the parameter based on its ID.
Line range hint
126-185:
LGTM!The
braced_paramfunction in theWordLexerimplementation correctly parses a parameter expansion enclosed in braces and handles various cases, including length prefixes and suffix modifiers.
231-231: LGTM!The test
lexer_braced_param_minimumcorrectly verifies the parsing of a minimal braced parameter.
256-256: LGTM!The test
lexer_braced_param_alphanumeric_namecorrectly verifies the parsing of a braced parameter with an alphanumeric name.
Line range hint
268-279:
LGTM!The test
lexer_braced_param_positionalcorrectly verifies the parsing of a braced parameter with a positional parameter.
290-314: LGTM!The test
lexer_braced_param_positional_zerocorrectly verifies the parsing of a braced parameter with a positional parameter of index 0.
316-334: LGTM!The test
lexer_braced_param_positional_overflowcorrectly verifies the parsing of a braced parameter with a large positional index.
336-352: LGTM!The test
lexer_braced_param_invalid_paramcorrectly verifies the error handling for an invalid braced parameter.
354-378: LGTM!The test
lexer_braced_param_special_zerocorrectly verifies the parsing of a braced parameter with a special parameter0.yash-syntax/src/syntax.rs (11)
Line range hint
17-49: Documentation comments are clear and detailed.The comments provide a comprehensive explanation of the module's purpose, syntactic elements, lexical elements, parsing, and displaying.
83-88: Import statements and type alias definitions are correct.The import statements and type alias definitions are necessary for the module's functionality.
Line range hint
93-173: Type definitions and trait implementations are correct.The type definitions and trait implementations for
UnquoteandMaybeLiteralare necessary for handling unquoting and literal checks.
173-236:SpecialParamenum and its methods are correctly implemented.The
SpecialParamenum and its methods handle special parameters in the shell language.
238-271:NotSpecialParamerror type and its implementations are correct.The
NotSpecialParamerror type and its implementations handle errors related to special parameters.
273-305:ParamTypeenum and its methods are correctly implemented.The
ParamTypeenum and its methods handle different types of parameters in the shell language.
307-345:Paramstruct and its methods are correctly implemented.The
Paramstruct and its methods handle parameter representations in the shell language.
347-371: Conversions toParamare correctly implemented.The implementations for converting
SpecialParamandusizetoParamhandle different types of parameters.
373-373: Reminder: ImplementFromStrforParam.The TODO comment indicates a future task for implementing
FromStrforParam.
536-584:BracedParamstruct and its methods are correctly implemented.The
BracedParamstruct and its methods handle parameter expansions enclosed in braces.
Line range hint
626-703:TextUnitenum and its methods are correctly implemented.The
TextUnitenum and its methods handle different types of text units in the shell language.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
namefields withparamfields in various error structures.Chores