-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_char
aconst fn
is appropriate and improves performance.
42-49
: LGTM!The change to make
is_single_char_name
aconst fn
is appropriate and improves performance.
106-107
: LGTM!The test for
lexer_raw_param_special_parameter
is updated correctly.
124-125
: LGTM!The test for
lexer_raw_param_digit
is updated correctly.
142-143
: LGTM!The test for
lexer_raw_param_posix_name
is updated correctly.
160-161
: LGTM!The test for
lexer_raw_param_posix_name_line_continuations
is updated correctly.
65-86
: Ensure proper error handling.The changes to the
raw_param
function look good, but ensure that errors are properly handled whenconsume_char_if
fails.Verification successful
Proper error handling is present.
The
raw_param
function 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_unit
function is appropriate and improves functionality.
200-201
: LGTM!The change to the
dollar_unit
function is appropriate and improves functionality.
201-201
: LGTM!The change to the
dollar_unit
function 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
resolve
match 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
resolve
function match the new signature.
yash-semantics/src/expansion/initial/param.rs
yash-semantics/src/expansion/initial/param/resolve.rs
Scripts 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
expand
match the new signature.Verification successful
LGTM! But verify the function usage in the codebase.
The code changes are approved.
All instances of the
expand
function 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
ParamRef
struct definition are approved.
43-48
: LGTM!The implementation of
From<&'a BracedParam> for ParamRef<'a>
is approved.
169-179
: LGTM!The new functions
braced_param
andbraced_variable
are 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
expand
match the new signature.yash-semantics/src/expansion/initial/arith.rs (3)
36-36
: Import statement forParam
looks good.The import is necessary for the changes made in the file.
139-139
: Refactor to useparam
inUnsetVariable
looks good.The change aligns with the refactoring to use
Param
instead ofname
.
196-197
: Refactor to useparam
in error handling looks good.The change aligns with the refactoring to use
Param
instead ofname
.yash-syntax/src/parser/error.rs (2)
48-48
: Addition ofInvalidParam
variant toSyntaxError
looks good.The new variant is necessary for handling invalid parameter names.
178-178
: Addition of error message forInvalidParam
looks good.The new error message is necessary for handling invalid parameter names.
yash-semantics/src/expansion.rs (3)
101-101
: Import statement forParam
looks good.The import is necessary for the changes made in the file.
143-144
: Refactor to useparam
inUnsetParameter
looks good.The change aligns with the refactoring to use
Param
instead ofname
.
177-189
: Refactor to useparam
in error handling looks good.The change aligns with the refactoring to use
Param
instead ofname
.yash-semantics/src/expansion/initial/param/switch.rs (6)
32-32
: Import statement forParam
looks good.The import is necessary for the changes made in the file.
98-103
: Refactor to useparam
inVacantError
looks good.The change aligns with the refactoring to use
Param
instead ofname
.
137-138
: Refactor to useparam
inNonassignableErrorCause
looks good.The change aligns with the refactoring to use
Param
instead ofname
.
192-213
: Refactor to useparam
inassign
function looks good.The change aligns with the refactoring to use
Param
instead ofname
.
Line range hint
242-252
: Refactor to useparam
invacant_expansion_error
function looks good.The change aligns with the refactoring to use
Param
instead ofname
.
Line range hint
267-287
: Refactor to useparam
inapply
function looks good.The change aligns with the refactoring to use
Param
instead ofname
.yash-syntax/src/parser/from_str.rs (1)
71-73
: LGTM!The implementation of the
FromStr
trait forBracedParam
correctly handles the conversion and error cases.yash-syntax/src/parser/lex/braced_param.rs (12)
24-24
: LGTM!The import statement for
BracedParam
is necessary for the new functionality.
27-27
: LGTM!The import statement for
ParamType
is necessary for the new functionality.
28-28
: LGTM!The import statement for
SpecialParam
is necessary for the new functionality.
40-67
: LGTM!The
type_of_id
function correctly determines the type of the parameter based on its ID.
Line range hint
126-185
:
LGTM!The
braced_param
function in theWordLexer
implementation 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_minimum
correctly verifies the parsing of a minimal braced parameter.
256-256
: LGTM!The test
lexer_braced_param_alphanumeric_name
correctly verifies the parsing of a braced parameter with an alphanumeric name.
Line range hint
268-279
:
LGTM!The test
lexer_braced_param_positional
correctly verifies the parsing of a braced parameter with a positional parameter.
290-314
: LGTM!The test
lexer_braced_param_positional_zero
correctly verifies the parsing of a braced parameter with a positional parameter of index 0.
316-334
: LGTM!The test
lexer_braced_param_positional_overflow
correctly verifies the parsing of a braced parameter with a large positional index.
336-352
: LGTM!The test
lexer_braced_param_invalid_param
correctly verifies the error handling for an invalid braced parameter.
354-378
: LGTM!The test
lexer_braced_param_special_zero
correctly 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
Unquote
andMaybeLiteral
are necessary for handling unquoting and literal checks.
173-236
:SpecialParam
enum and its methods are correctly implemented.The
SpecialParam
enum and its methods handle special parameters in the shell language.
238-271
:NotSpecialParam
error type and its implementations are correct.The
NotSpecialParam
error type and its implementations handle errors related to special parameters.
273-305
:ParamType
enum and its methods are correctly implemented.The
ParamType
enum and its methods handle different types of parameters in the shell language.
307-345
:Param
struct and its methods are correctly implemented.The
Param
struct and its methods handle parameter representations in the shell language.
347-371
: Conversions toParam
are correctly implemented.The implementations for converting
SpecialParam
andusize
toParam
handle different types of parameters.
373-373
: Reminder: ImplementFromStr
forParam
.The TODO comment indicates a future task for implementing
FromStr
forParam
.
536-584
:BracedParam
struct and its methods are correctly implemented.The
BracedParam
struct and its methods handle parameter expansions enclosed in braces.
Line range hint
626-703
:TextUnit
enum and its methods are correctly implemented.The
TextUnit
enum and its methods handle different types of text units in the shell language.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
name
fields withparam
fields in various error structures.Chores