Declaration utility #440
Declaration utility #440
Conversation
magicant
added
enhancement
WalkthroughThis pull request introduces support for declaration utilities in the Yash shell, a feature defined in the POSIX standard. The changes span multiple packages within the Yash project, focusing on enhancing the parsing and handling of commands with declaration utility behavior. Key modifications include adding a new Changes
Sequence DiagramsequenceDiagram
participant Parser
participant Glossary
participant Command
participant Env
Parser->>Glossary: is_declaration_utility(command_name)
Glossary-->>Parser: Option<bool>
alt is declaration utility
Parser->>Command: Set expansion mode to Single
else not a declaration utility
Parser->>Command: Set expansion mode to Multiple
end
Parser->>Env: Process command with appropriate expansion
Possibly related PRs
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The core part of the // Handle command argument word
match is_declaration_utility {
Some(true) => {
result.words.push(determine_expansion_mode(token.word));
continue;
}
Some(false) => {
result.words.push((token.word, ExpansionMode::Multiple));
continue;
}
None => (),
}
// Tell assignment from word
let assign_or_word = if result.words.is_empty() {
Assign::try_from(token.word)
} else {
Err(token.word)
};
let mut assign = match assign_or_word {
Ok(assign) => assign,
Err(word) => {
is_declaration_utility = word_names_declaration_utility(&word);
result.words.push((word, ExpansionMode::Multiple));
continue;
}
}; |
This commit adds code fragments to the `simple_command` parser to determine whether a word names a declaration utility. Words that follow a declaration utility name are parsed with a single expansion mode.
This commit adds the Config struct to the parser module. Config is a builder for constructing a parser. It allows the user to optionally customize the parser settings before creating a parser while providing default settings in case the user does not want to customize them. This change should improve forward compatibility with settings that may be added in the future.
Now that the glossary is part of the parser configuration, it would not be reasonable to force the user to pass it as a parameter to the `new` function. The function should be as simple as possible, only taking the required lexer parameter.
This commit renames the glossary field in the Parser struct to aliases to avoid confusion with the glossary for declaration utilities which is to be added in the next commit.
This commit makes the typeset built-in a declaration utility as an extension to the POSIX standard. To make it possible, the read-eval-loop now configures the parser with the environment as a glossary for declaration utilities. The glossary implementation looks up the command name in the built-in utilities and returns the value of the `is_declaration_utility` field, which is also added in this commit.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (28)
yash-cli/CHANGELOG-bin.md (1)
15-15: Enhance the changelog entry with more details about declaration utilities.While the entry follows the changelog format, it could be more informative for users by explaining what declaration utilities are and their impact. Consider expanding it to include:
- What declaration utilities are in POSIX
- Examples of common declaration utilities
- Key benefits or changes users might notice
Here's a suggested expansion:
- The shell now supports declaration utilities as defined in POSIX. + The shell now supports declaration utilities as defined in POSIX, allowing commands like `export` and `readonly` to properly handle variable assignments. This improves compatibility with POSIX shells and enables correct parsing of commands like `readonly foo=bar` or `export PATH=/usr/bin:$PATH`.yash-semantics/CHANGELOG.md (2)
12-13: Consider adding more details about the new functions.While the addition is documented, it would be helpful to briefly describe the purpose and differences between
expand_word_multipleandexpand_word_with_modefunctions, especially sinceexpand_word_multiplereplaces the previousexpand_wordsfunction.- Added the `expand_word_multiple` and `expand_word_with_mode` functions to the - `expansion` module. + `expansion` module: + - `expand_word_multiple`: Replaces the previous `expand_words` function for streamlined word expansion + - `expand_word_with_mode`: Provides flexible word expansion based on the specified `ExpansionMode`
17-19: Consider clarifying the simple command execution change.The current description might be unclear to users unfamiliar with the codebase. Consider explaining what this change means in practical terms.
- The execution of a simple command (`impl command::Command for yash_syntax::syntax::SimpleCommand`) - now honors the `ExpansionMode` specified for the words in the command. + now honors the `ExpansionMode` specified for the words in the command, + enabling proper handling of declaration utilities and their argument expansions.yash-semantics/src/expansion.rs (8)
20-24: Clarify how multi-field results are obtainedThe revised documentation around
expand_word_multipleis quite helpful. One suggestion is to explicitly highlight that$@or$*expansions might lead to multiple fields. This extra note will improve user understanding of multi-field expansions.
65-69: Ensure clarity on quote removal orderingThe documentation states that quote removal in
expand_word_multipleoccurs before pathname expansion. Consider emphasizing the ordering again if there are known edge cases or complexities (e.g., unusual quote usage or backslash escapes).
389-391: Brace expansion TODOThe
// TODO brace expansioncomment stands out. This is a known gap that might cause unexpected behavior for users expecting POSIX brace expansions. Ensure you have an issue filed to track this.
400-404: Potential optimization for field splittingWhen dealing with large expansions, field splitting could be done in a streaming fashion or via an iterator to avoid building large intermediate strings. This might help performance in large scripts or multi-line expansions.
430-446: Code readability and debugging clarityThe match on
ExpansionModeis well-structured. If debugging is needed, consider logging expansions if a user sets a shell debug flag. This can help identify subtle expansion issues without requiring a separate debug instrumentation phase.
569-577: Test best-practice: Include negative scenariosThe
expand_word_multiple_performs_initial_expansiontest checks a success path. Some negative test coverage (e.g., invalid arithmetic expansions, nonexistent commands, etc.) could be beneficial.
586-597: Test clarity: default IFS usageGreat job verifying field splitting with default IFS. Consider adding an assertion that ensures additional whitespace forms also split fields correctly (tabs, newlines, etc.).
631-638: Verify complex quoting scenariosTesting quote removal with a single case is good. You might add more quotes or nested expansions to confirm no regressions with advanced quoting patterns.
yash-syntax/src/syntax.rs (1)
584-597: Consistent naming and usage of enumExpansionModeThe new
ExpansionModeis well-defined. One small note: if you plan on adding more modes later (e.g., “NoSplit”), consider marking the enum as non-exhaustive or ensuring it can be extended without breakage.yash-syntax/src/parser/simple_command.rs (6)
35-56:determine_expansion_modecorrectness checks
- Good approach to parse
name=valueforms.- Consider edge cases with escaped
=signs or unusual expansions.- If you plan to handle array assignments differently, ensure you do so consistently.
62-62: Field rename clarityChanging
wordsto store(Word, ExpansionMode)might require consistent naming in docs (e.g., “words_with_mode”). A small doc tweak can help maintainers.
163-171: Handling assignment vs. word withAssign::try_fromThis approach is neat. Just remain cautious if variable names can contain unusual characters that might overlap with expansions or reserved syntax. Additional boundary tests might be warranted.
201-201: Might clarify in doc comment that final check is happeningLine 201 returns the built command. The code is self-explanatory, but if disclaimers or final validations are needed, consider them.
228-234: Test: single expansion forfoo=Verifies correct mode selection for nonempty name. Additional coverage might include underscores or digits in variable names, or Unicode in the future.
Line range hint
284-290: Many array values testParsing multiple array items successfully. Good coverage. Consider adding a test that includes expansions or quotes within
( ... ).yash-syntax/src/parser/core.rs (2)
134-160:declaration_utilitiesmethod: reference doc improvementsLeaving an inline doc referencing POSIX or other custom glossaries is beneficial. Possibly mention examples aligning with real use cases.
430-439:word_names_declaration_utilitymethodImplementation is short and to the point. Consider adding a small debug log for tricky expansions or ambiguous names if needed.
yash-syntax/src/parser/function.rs (1)
57-57: Consider destructuring the tuple for clarity
Using.pop().unwrap().0directly works, but refactoring to a pattern deconstruction (e.g.let (name, _mode) = intro.words.pop().unwrap();) could improve readability.yash-syntax/src/parser/if.rs (1)
193-193: Parser setup check forparser_if_command_many_elifs.
Looks good. Keep an eye out for any calls that might rely on the old constructor’s behavior.yash-semantics/src/command/compound_command/for_loop.rs (1)
221-228: Addedis_declaration_utilityfield toBuiltin.
This change aligns with the PR’s objective to handle declaration utilities. TheSome(false)default is sensible, but verify that any commands which should be recognized as declaration utilities are appropriately set toSome(true). This ensures consistent behavior when the parser checks for declaration usage.Would you like help validating or explaining how to differentiate built-ins that are declaration utilities from regular commands?
yash-syntax/src/parser/lex/tilde.rs (1)
148-190: Avoid potential slice out-of-bounds.
While this doc and logic forparse_tilde_everywhere_afterappear correct, consider a bound check on theindexargument to prevent runtime panics ifindex > self.units.len().Here’s an optional diff:
pub fn parse_tilde_everywhere_after(&mut self, index: usize) { + if index >= self.units.len() { + return; + } let mut i = index; ... }yash-cli/tests/scripted_test/declutil-y.sh (2)
3-3: Add a comment explaining the purpose of tmpfileThe empty file creation should be documented to clarify its role in the tests.
->tmpfile +# Create empty tmpfile for pathname expansion tests +>tmpfile🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 3-3: This redirection doesn't have a command. Move to its command (or use 'true' as no-op).
(SC2188)
1-40: Consider adding cleanup for temporary filesThe script creates a tmpfile but doesn't clean it up after the tests complete.
Add cleanup at the end of the script:
+# Cleanup +cleanup() { + rm -f tmpfile +} +trap cleanup EXIT🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
[warning] 3-3: This redirection doesn't have a command. Move to its command (or use 'true' as no-op).
(SC2188)
yash-syntax/CHANGELOG.md (1)
11-11: Fix unnecessary comma in sentence.Remove the comma before "so that" as it's not required in this context.
-the parser is configured on construction, so that the parser can be constructed with +the parser is configured on construction so that the parser can be constructed with🧰 Tools
🪛 LanguageTool
[typographical] ~11-~11: The conjunction “so that” does not require a comma.
Context: ...the parser is configured on construction, so that the parser can be constructed with more...(SO_THAT_UNNECESSARY_COMMA)
yash-semantics/src/tests.rs (1)
Line range hint
56-240: Well-structured implementation of declaration utility supportThe implementation demonstrates a clean and consistent approach to identifying declaration utilities:
- Each built-in explicitly declares its declaration utility status
- The pattern is uniformly applied across all built-in implementations
- The separation of test utilities makes the code maintainable and testable
This architecture aligns well with the PR's objective of enhancing declaration utility support in the shell.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
-
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (63)
-
yash-builtin/CHANGELOG.md(2 hunks) -
yash-builtin/Cargo.toml(3 hunks) -
yash-builtin/src/command/identify.rs(3 hunks) -
yash-builtin/src/command/invoke.rs(3 hunks) -
yash-builtin/src/command/search.rs(2 hunks) -
yash-builtin/src/lib.rs(8 hunks) -
yash-builtin/src/set.rs(1 hunks) -
yash-cli/CHANGELOG-bin.md(2 hunks) -
yash-cli/CHANGELOG-lib.md(2 hunks) -
yash-cli/Cargo.toml(2 hunks) -
yash-cli/tests/scripted_test.rs(1 hunks) -
yash-cli/tests/scripted_test/declutil-p.sh(1 hunks) -
yash-cli/tests/scripted_test/declutil-y.sh(1 hunks) -
yash-env-test-helper/CHANGELOG.md(2 hunks) -
yash-env-test-helper/Cargo.toml(2 hunks) -
yash-env/CHANGELOG.md(2 hunks) -
yash-env/Cargo.toml(3 hunks) -
yash-env/src/builtin.rs(4 hunks) -
yash-env/src/decl_util.rs(1 hunks) -
yash-env/src/lib.rs(1 hunks) -
yash-prompt/CHANGELOG.md(2 hunks) -
yash-prompt/Cargo.toml(2 hunks) -
yash-semantics/CHANGELOG.md(2 hunks) -
yash-semantics/Cargo.toml(2 hunks) -
yash-semantics/src/command/and_or.rs(1 hunks) -
yash-semantics/src/command/compound_command.rs(1 hunks) -
yash-semantics/src/command/compound_command/for_loop.rs(1 hunks) -
yash-semantics/src/command/compound_command/subshell.rs(2 hunks) -
yash-semantics/src/command/compound_command/while_loop.rs(2 hunks) -
yash-semantics/src/command/pipeline.rs(3 hunks) -
yash-semantics/src/command/simple_command.rs(3 hunks) -
yash-semantics/src/command/simple_command/builtin.rs(3 hunks) -
yash-semantics/src/command_search.rs(12 hunks) -
yash-semantics/src/expansion.rs(12 hunks) -
yash-semantics/src/runner.rs(1 hunks) -
yash-semantics/src/runner_legacy.rs(1 hunks) -
yash-semantics/src/tests.rs(8 hunks) -
yash-semantics/src/trap/exit.rs(1 hunks) -
yash-semantics/src/trap/signal.rs(1 hunks) -
yash-syntax/CHANGELOG.md(2 hunks) -
yash-syntax/Cargo.toml(2 hunks) -
yash-syntax/src/decl_util.rs(1 hunks) -
yash-syntax/src/lib.rs(1 hunks) -
yash-syntax/src/parser.rs(2 hunks) -
yash-syntax/src/parser/and_or.rs(4 hunks) -
yash-syntax/src/parser/case.rs(25 hunks) -
yash-syntax/src/parser/command.rs(4 hunks) -
yash-syntax/src/parser/compound_command.rs(11 hunks) -
yash-syntax/src/parser/core.rs(25 hunks) -
yash-syntax/src/parser/for_loop.rs(18 hunks) -
yash-syntax/src/parser/from_str.rs(10 hunks) -
yash-syntax/src/parser/function.rs(9 hunks) -
yash-syntax/src/parser/grouping.rs(10 hunks) -
yash-syntax/src/parser/if.rs(13 hunks) -
yash-syntax/src/parser/lex/core.rs(1 hunks) -
yash-syntax/src/parser/lex/tilde.rs(1 hunks) -
yash-syntax/src/parser/list.rs(18 hunks) -
yash-syntax/src/parser/pipeline.rs(10 hunks) -
yash-syntax/src/parser/redir.rs(18 hunks) -
yash-syntax/src/parser/simple_command.rs(24 hunks) -
yash-syntax/src/parser/while_loop.rs(11 hunks) -
yash-syntax/src/syntax.rs(2 hunks) -
yash-syntax/src/syntax/impl_display.rs(3 hunks)
✅ Files skipped from review due to trivial changes (6)
- yash-prompt/CHANGELOG.md
- yash-builtin/CHANGELOG.md
- yash-env/src/lib.rs
- yash-env-test-helper/CHANGELOG.md
- yash-builtin/src/set.rs
- yash-semantics/src/command/pipeline.rs
🧰 Additional context used
📓 Learnings (1)
yash-prompt/Cargo.toml (1)
Learnt from: magicant
PR: magicant/yash-rs#418
File: yash-prompt/Cargo.toml:3-3
Timestamp: 2024-11-12T08:51:39.677Z
Learning: The `CHANGELOG.md` for the `yash-prompt` package is located at `yash-prompt/CHANGELOG.md`.
🪛 Shellcheck (0.10.0)
yash-cli/tests/scripted_test/declutil-p.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
[warning] 3-3: posix appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 6-6: This redirection doesn't have a command. Move to its command (or use 'true' as no-op).
(SC2188)
[warning] 26-26: This does not export 'a'. Remove
(SC2163)
yash-cli/tests/scripted_test/declutil-y.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
[warning] 3-3: This redirection doesn't have a command. Move to its command (or use 'true' as no-op).
(SC2188)
🪛 LanguageTool
yash-syntax/CHANGELOG.md
[typographical] ~11-~11: The conjunction “so that” does not require a comma.
Context: ...the parser is configured on construction, so that the parser can be constructed with more...
(SO_THAT_UNNECESSARY_COMMA)
🔇 Additional comments (302)
yash-cli/CHANGELOG-bin.md (1)
11-12: LGTM! Version numbering and linking are correct.
The version bump to 0.3.0 is appropriate given the new POSIX feature addition, and the version link follows the established pattern.
Also applies to: 106-106
yash-semantics/CHANGELOG.md (3)
8-9: LGTM!
The version header follows the Keep a Changelog format and semantic versioning guidelines.
20-22: LGTM!
The dependency updates are properly documented with their version changes.
169-169: LGTM!
The version link follows the established pattern and is properly formatted.
yash-syntax/src/lib.rs (1)
42-42: Ensure test coverage and documentation for the new module.
Consider creating or updating unit tests that cover the functionalities within decl_util. Additionally, verify that the API usage is clearly documented, as this module now extends the public interface.
yash-cli/CHANGELOG-lib.md (4)
12-13: LGTM! Version bump is appropriate.
The minor version bump to 0.3.0 aligns with semantic versioning guidelines, as the PR introduces new functionality (declaration utility support) without breaking changes.
150-150: LGTM! Version link follows the established pattern.
The link to version 0.3.0 is correctly formatted and placed in chronological order.
16-18: Verify compatibility with external dependency updates.
The version bumps for yash-env and yash-syntax appear to support the new declaration utility feature. Let's verify the changes in these dependencies.
✅ Verification successful
Dependencies are compatible with declaration utility feature
The changelog entries confirm compatibility:
-
yash-env 0.6.0:- Added
Glossarytrait implementation forEnvstruct - Added
is_declaration_utilityfield toBuiltinstruct - Depends on
yash-syntax 0.14.0
- Added
-
yash-syntax 0.14.0:- Explicitly adds support for declaration utilities
- Adds
decl_utilmodule withGlossarytrait - Adds
ExpansionModeenum for word expansion
The version bumps are intentional and contain the necessary changes to support the declaration utility feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the changelog entries of the dependent crates to ensure they contain related changes
# Check yash-env changelog
echo "Checking yash-env changelog for version 0.6.0:"
fd -t f "CHANGELOG" . | xargs rg "## \[0\.6\.0\]" -A 10
# Check yash-syntax changelog
echo "Checking yash-syntax changelog for version 0.14.0:"
fd -t f "CHANGELOG" . | xargs rg "## \[0\.14\.0\]" -A 10
Length of output: 3309
19-22: Verify the necessity of internal dependency updates.
While updates to yash-builtin and yash-semantics are clearly needed for declaration utility support, let's verify if the yash-prompt update is necessary.
✅ Verification successful
Internal dependency updates are appropriately justified
The verification results show that:
-
yash-builtinandyash-semanticshave extensive changes related to declaration utility functionality, as evidenced by multiple files containing relevant changes -
yash-promptupdate (0.3.0 → 0.4.0) is a transitive dependency update, as its changelog shows it depends on:- yash-semantics 0.5.0 → 0.6.0
- yash-syntax 0.13.0 → 0.14.0
- yash-env 0.5.0 → 0.6.0
All internal dependency updates are necessary and properly justified.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if all internal dependency updates are justified
# Check for declaration utility related changes in builtin and semantics
echo "Checking yash-builtin for declaration utility changes:"
rg -l "declaration.*utility" --glob "yash-builtin/**"
echo "Checking yash-semantics for declaration utility changes:"
rg -l "declaration.*utility" --glob "yash-semantics/**"
# Check if yash-prompt has any related changes
echo "Checking yash-prompt changelog for version 0.4.0:"
fd -t f "CHANGELOG" . | xargs rg "## \[0\.4\.0\]" -A 10
Length of output: 4674
yash-semantics/src/expansion.rs (8)
104-104: Kudos for introducing ExpansionMode
Importing the ExpansionMode enum here signals a clear contract for how words are expanded. This approach helps keep expansion behaviors consistent.
358-358: Documentation consistency
The doc comment references multiple expansions. It aligns well with the new expand_word_multiple approach. No issues detected here.
369-385: Validate collection type & capacity checks
Inside expand_word_multiple, after the initial expansion, consider verifying that split_fields usage doesn't risk reallocation. Though Vec::with_capacity is used, be mindful of extreme or user-controlled expansions that might exceed initial field counts.
410-414: Consider fallback behavior if glob matches nothing
You may want to ensure consistent behavior if glob fails to match any path. Some shells keep the literal pattern, while others remove the field. Confirm if that’s the intended behavior and document it or handle it here.
416-429: Clear separation of single vs multiple expansions
This chunk of docs clarifies how expand_word_with_mode decides between single or multiple expansions. The design is straightforward. Just ensure the caller is aware of potential side effects of single expansions swallowing $@/$*.
448-457: Consistent method references
These doc lines for expand_words thoroughly reference related methods. Looks good. Ensure all references remain updated if you refactor or rename methods in the future.
Line range hint 606-621: Coverage for non-default IFS usage
This test effectively checks a custom IFS scenario. You might add a scenario with an empty IFS to confirm the no-splitting logic.
645-647: Good retrieval of last command substitution’s exit status
This test verifies correctness. No issues found. Maybe combine multiple edge cases (e.g., multiple expansions with a command substitution that fails) to ensure expected exit statuses are reported.
yash-syntax/src/syntax.rs (2)
604-608: Switch to (Word, ExpansionMode) keeps expansions flexible
Using tuples for words clarifies each token’s intended expansion style. This is more maintainable than a separate parallel structure. Carefully track usage across the codebase for consistency.
627-627: Keyword check reliability
Ensuring the first word is recognized as a possible reserved keyword is crucial. If a word is partially computed or partially literal, confirm that partial expansions do not break your word.to_string_if_literal() logic.
yash-syntax/src/parser/simple_command.rs (35)
28-28: Importing ExpansionMode early
Having ExpansionMode imported near the top clarifies its usage in subsequent code blocks. No issues seen.
150-160: Ensuring no second guess on is_declaration_utility
The debug assertion (line 154) is valuable, but if conditions change, ensure that multiple checks for declaration utilities remain aligned. Relying on a one-time determination could be risky if subsequent code modifies whether a command is recognized as a declaration utility.
175-177: is_declaration_utility logic ensures correct expansions
By setting is_declaration_utility early, subsequent expansions are properly handled. Looks good.
211-211: Import usage from crate::decl_util::EmptyGlossary
The import reveals that the parser can be run with a minimal glossary. Good demonstration of the design’s extensibility.
215-216: TextUnits usage
Line 215–216 references TextUnit and WordUnit. If expansions or quotes become more intricate (like nested expansions), confirm that the parser can gracefully handle them.
218-219: FutureExt import usage
Bringing futures into scope is standard for async. No concerns here.
220-226: Test: empty assignment does not toggle single expansion
This test ensures that an empty name triggers ExpansionMode::Multiple. The coverage is robust.
236-242: Test: special expansions remain multiple
The test with a non-literal name confirms that expansions beyond straightforward name=value remain multiple. Implementation looks good.
244-259: Test: tilde expansions after =
Line 250 onward checks for expansions in both sides of the =. Implementation is correct. Possibly highlight further expansions like $HOME=~/some_dir.
264-264: Array parsing logic
Properly returns None if ( is missing. Straightforward logic.
Line range hint 272-277: Parsing empty “()” as array
Test verifies that an empty array is successfully parsed. The code is consistent.
Line range hint 302-308: Multi-line array input with newlines and comments
Strong test coverage for ignoring newlines and comments. No issues found.
Line range hint 314-323: Unclosed array capturing errors
The error handling for missing ) is well-tested.
Line range hint 332-341: Invalid word in array
Spot checking an invalid token. Good demonstration of how errors are thrown.
Line range hint 350-357: Parser returns None if no valid command is found
Legible. No concerns.
Line range hint 359-366: Ignore keyword if no words have been parsed
Ensures commands can’t start with reserved words in certain contexts. Good for correctness.
Line range hint 368-377: Single assignment for my=assignment
Check if final AST structures are correct for a single assignment. Implementation is consistent.
Line range hint 386-395: Multiple assignments recognized together
a= b=! c=X scenario is well-tested. Good coverage.
416-424: One-word command case
Ensures ExpansionMode::Multiple by default if not recognized as assignment or declaration utility.
430-442: Many words scenario
Verifies the argument words all default to multiple expansions. This aligns with standard shell behavior.
Line range hint 448-466: One or more redirections
Check for <one >two >>three. Redirections parse as expected. No issues found.
Line range hint 498-508: Mixed assignment word and normal word
if=then else scenario ensures we get an assignment plus a normal word. Works as intended.
514-522: Word followed by redirection
word <redirection tested. Seems correct.
Line range hint 533-542: Redirection followed by assignment
<foo a=b scenario is tested. Good coverage.
Line range hint 552-562: Assignment, redirection, then another argument
if=then <foo else scenario ensures correct ordering.
Line range hint 573-582: Array assignment a=()
Confirms empty array assignment is recognized.
Line range hint 592-602: Empty assignment then blank then parenthesis
a= () scenario. The logic is consistent with typical shell behavior.
Line range hint 613-624: Non-empty assignment before parenthesis
a=b() scenario ensures correct parse.
631-647: Declaration utility singled out expansions
export a=b sets it to ExpansionMode::Single. Implementation matches design.
649-665: Non-assignment word remains multiple expansion
export foo scenario is tested. Behavior is correct.
667-683: Non-declaration utility fallback
foo a=b scenario ensures no single expansions.
685-702: Late determination of declaration utility
command command export foo a=b test shows dynamic resolution. This is a good approach to handle POSIX-compatible scenarios.
704-721: Empty glossary scenario
Ensures no built-ins are recognized as declaration utilities. Great fallback test.
723-748: Custom glossary logic
Validates a user-supplied glossary that declares “foo” as a declaration utility. Implementation is flexible and aligned with user needs.
750-770: Ensure assignment is not incorrectly recognized as a command name
Using a custom glossary confirms that a=b cannot be considered “foo.” This clarifies boundaries of your design.
yash-syntax/src/parser/core.rs (11)
32-32: Importing Word to unify type usage
Helps keep the parser core from referencing too many modules. Looks fine.
97-105: Config struct’s doc clarity
Clear explanation about how to build parser configurations. This is user-friendly.
106-111: aliases & decl_utils fields
These fields unify custom expansions and declarations into a single config approach. Great design for pluggability.
113-122: Default constructs
Providing defaults ensures minimal friction for new integrators. This is a good practice.
124-132: aliases method clarity
Allows chaining to set aliases. Straightforward method naming.
161-170: Chained initialization with input
Pattern is consistent with builder patterns in Rust.
173-177: Default trait usage
Ensures simple usage when we don’t want to specify advanced config.
179-185: Parser doc improvements
This block clarifies how the parser sets up token reading. No issues found.
205-211: Introducing aliases field in Parser
Mirrors the config struct, ensuring the parser has consistent reference.
212-214: decl_utils field alignment
Matches design in config. Straightforward bridging from config to parser.
216-222: token field changes
No direct code changes beyond re-checking that alias expansions are used. Straightforward.
yash-env/src/decl_util.rs (1)
1-34: Implementation of Glossary for Env looks good
- Logic defers to
builtins.get(name). - Returning
Some(false)for unknown commands is straightforward. - If future expansions include external commands recognized as declarations, consider an extension mechanism.
yash-syntax/src/parser.rs (2)
48-48: Confirmed the updated parser construction approach.
The removal of the EmptyGlossary parameter simplifies parser instantiation. This snippet looks correct and matches the new Parser signature.
91-91: Public export of Config.
The new configuration struct aligns with the broader refactor, enabling more flexible parser initialization. No further issues identified.
yash-syntax/src/parser/and_or.rs (4)
84-84: Parser instantiation updated.
Using the new signature Parser::new(&mut lexer) is consistent with recent refactors.
93-93: Redundant comment but verifying correctness.
Same rationale as above: matches the updated constructor call.
104-104: Test usage of the updated parser constructor.
The new constructor signature improves clarity. Thank you for the consistent change.
119-119: No issues with consistency.
The updated parser instantiation follows the same approach as other test cases.
yash-builtin/src/command/invoke.rs (3)
117-117: Builtin metadata expanded.
Adding is_declaration_utility: Some(false) is consistent with the new field logic.
156-156: Consistent built-in changes.
No functional issues. Confirm if any built-in might require Some(true) usage. Otherwise looks fine.
186-186: Alignment with the new field introduction.
is_declaration_utility: Some(false) is clear and maintains consistency.
yash-semantics/src/trap/exit.rs (1)
104-111: Trap action built-in update.
Introducing the is_declaration_utility field is consistent with the pattern used elsewhere. The assigned value Some(false) matches standard built-ins that are not declaration utilities.
yash-cli/tests/scripted_test.rs (2)
183-186: Well-structured test addition.
These lines introduce a new test function declaration_utilities. The test name clearly describes its purpose, and it maintains consistency with the existing test pattern of invoking run(...) for a shell script test. No issues found.
188-191: Additional test coverage for declaration utilities.
Similar to the previous addition, declaration_utilities_ex provides extended coverage by invoking a -y.sh script. This ensures that declaration utilities are tested across multiple scenarios, aligning with the existing test naming convention.
yash-builtin/src/command/search.rs (2)
197-197: Ensure default value logic is correct.
is_declaration_utility: Some(false) indicates that this builtin is not a declaration utility by default. Confirm that there's no scenario in which this builtin should be recognized as a declaration utility instead. Otherwise, this is fine.
216-216: Consistent initialization for builtins.
The field is_declaration_utility: Some(false) consistently follows the prior assignment. No functional issues are apparent.
yash-semantics/src/command/compound_command/subshell.rs (2)
129-129: Synchronized setting of declaration utility field.
For the exit builtin, the field is_declaration_utility: Some(false) matches the expectation that exit is not a declaration utility. No immediate issues found.
242-242: Same pattern for the trap builtin.
Similarly, trap is appropriately flagged as a non-declaration utility. This change maintains consistency with other builtins.
yash-syntax/src/parser/pipeline.rs (11)
108-108: Inline test reorganization note.
The added line modifies a test range (#[cfg(test)] region). Ensure that any removed or refactored code is also fully tested. No immediate issues.
116-116: Parser creation update.
Instantiating the parser in a more streamlined way (without passing a glossary) seems consistent with the new approach. Keep verifying that all relevant settings are properly configured.
125-125: Straightforward single-command pipeline test.
No obvious logical concerns here. The test checks for correct pipeline parsing with one command.
137-137: Multiple-command pipeline test.
This line shows the parser handling a pipeline with multiple commands separated by |. The approach is consistent with the existing style.
151-151: Negation test ensures correct handling of !.
This explicitly tests pipeline negation. The logic aligns well with p.negation usage.
163-163: Double negation syntax error test.
This covers syntax error detection when two ! are used without a command in between. Code is consistent with the parser's approach to raise SyntaxError::DoubleNegation.
176-176: Missing command after ! test.
Validates error handling for a missing command, ensuring the parser properly identifies a syntax error.
192-192: Missing command after pipe test.
Similar to the previous case, this checks if the parser raises the correct error for a missing command after |.
208-208: Bang after bar error scenario.
Ensures robust error detection (BangAfterBar) for a pipeline missing a command after | !. This is correct.
230-230: Prevent aliasing of !.
Alias expansions can lead to tricky parsing. Ensuring the parser does not alias the ! token inadvertently is critical to POSIX compliance.
250-250: Alias substitution yielding newline.
Alias expansions that insert newlines can introduce corner cases. This test adds coverage for that scenario, ensuring correct pipeline command handling.
yash-semantics/src/command/simple_command.rs (3)
26-26: No issues found importing expand_word_with_mode.
42-42: Clarity Enhanced by Explicitly Importing ExpansionMode.
Importing ExpansionMode directly is a good practice, improving readability and clarity in how expansions are handled.
197-210: Encapsulated Word Expansion Logic Looks Good.
The new expand_words function modularizes word expansion and consistently updates the last_exit_status. This helps maintain a clear separation of concerns in the command execution flow.
yash-syntax/src/parser/grouping.rs (10)
101-101: No concerns about test modifications at line 101.
All looks fine here.
110-110: Test Logic Reading is Clear.
The parser instance creation here is straightforward, no issues identified.
122-122: Parser instantiation for grouping test is valid.
The approach to testing grouping logic is consistent with the rest of the suite.
134-134: Unclosed brace test coverage.
This test effectively validates error handling for unclosed grouping; no issues.
154-154: Handling empty grouping is well-tested.
Checks for POSIX compliance properly; no changes needed.
189-189: Alias-based grouping test remains robust.
The usage of Parser::config().aliases(...) is appropriate for advanced alias testing.
201-201: Subshell parsing test logic is clear.
Demonstrates correct handling of short subshell syntax.
217-217: Expanded subshell test is consistent.
No issues with verifying behavior under more complex commands.
233-233: Error handling for unclosed subshell.
Test covers error scenario thoroughly.
253-253: Empty subshell test coverage.
Correctly checks for POSIX compliance with an empty subshell.
yash-env/src/builtin.rs (5)
17-17: Consistent Documentation Format.
The updated title comment at line 17 helps unify header styles across modules.
36-36: Enum Type Documentation is Clean and Informative.
No concerns; it clarifies distinctions between built-in types.
105-105: Result Struct Comments Look Good.
The clarifying remarks on exit status and redirection retention are precise.
237-237: Main Type Definition is Well-Documented.
No issues; it clearly states the arguments the built-in function expects.
245-261: New Field is_declaration_utility Observed.
This addition neatly integrates the ability to mark built-ins as declaration utilities. Documentation references and guidelines for usage seem coherent.
yash-semantics/src/command/compound_command.rs (1)
195-195: Built-in Declaration Utility Marker.
Setting is_declaration_utility: Some(false) clarifies the nature of this built-in. Looks consistent with the rest of the codebase.
yash-syntax/src/parser/compound_command.rs (3)
99-103: Imports for AliasSet, HashEntry, and ExpansionMode look consistent.
These imports are used in the test module to support aliasing and expansion logic. The usage appears correct, and it aligns with the overall parser structure.
254-254: Parser instantiation structure is correct.
No functional concerns here; this line simply creates a Parser instance for testing.
257-257: Straightforward usage of ExpansionMode::Multiple.
In test code, assigning ("foo".parse().unwrap(), ExpansionMode::Multiple) is consistent with how words and expansion modes are handled for short function definitions.
yash-semantics/src/trap/signal.rs (1)
214-221: Added is_declaration_utility: Some(false) in Builtin.
This change clarifies that the "check" built-in is not recognized as a declaration utility. The usage in test code ensures consistent metadata for built-ins, aligning well with the new logic for declaration utilities introduced in the codebase.
yash-builtin/src/lib.rs (1)
Line range hint 118-389: Introduction of the is_declaration_utility field across built-ins.
Applying either Some(true), Some(false), or None to define whether each built-in is a declaration utility aligns with the PR’s objective. This consistent approach ensures the parser can accurately determine how to expand command arguments. The choice of None for commands like command (line 167) is correct per POSIX, and Some(true) for export and readonly (lines 209, 274) is also accurate. Overall, these updates meet POSIX expectations and maintain clarity.
yash-syntax/src/decl_util.rs (4)
139-156: Glossary trait design is clear and extensible.
Defining Glossary to return Option<bool> allows for flexible handling of partial decisions (None) vs. definite ones (Some(true) or Some(false)). This approach suits the POSIX specification where certain commands delegate the final decision to subsequent words.
158-170: EmptyGlossary implementation is correct but non-POSIX.
As described in the doc comments, always returning Some(false) means no command is treated as a declaration utility. This is straightforward and functional, though it deviates from POSIX compliance if used alone.
172-192: PosixGlossary handles essential declarations properly.
- Returns
Some(true)forexportandreadonly. - Delegates the decision (
None) forcommand. - Returns
Some(false)for all others.
This is the minimal POSIX support needed. Implementation is succinct and aligns well with the doc comments.
194-214: Reference and RefCell implementations enhance composability.
Allowing either immutable references, mutable references, or RefCell-wrapped structures to implement Glossary adds necessary flexibility for advanced usage. The borrowing logic is coherent, and the code properly panics if borrowed mutably at the same time (common Rust pattern).
yash-syntax/src/parser/while_loop.rs (11)
101-101: Import usage looks correct
This newly introduced import is presumably required for alias-related tests.
110-110: Parser instantiation adjusted
Switching from passing a glossary to a simpler Parser::new(&mut lexer) call is consistent with the new parser configuration pattern.
126-126: Parser instantiation adjusted
Same reasoning as line 110. This complies with the updated parser refactoring.
142-142: Parser instantiation adjusted
The parser no longer needs the EmptyGlossary, aligning with the refactored initialization approach.
162-162: Parser instantiation adjusted
Using Parser::new here maintains consistency with the project-wide parser upgrades.
194-194: Refined parser configuration
Using Parser::config().aliases(&aliases).input(&mut lexer) provides a cleaner, more flexible setup for parsing with aliases.
207-207: Parser instantiation adjusted
Continues the consistent approach to remove the glossary parameter and simplify the instantiation.
223-223: Parser instantiation adjusted
Aligns well with the standardized parser initialization.
239-239: Parser instantiation adjusted
Again, this reflects the updated parser interface without EmptyGlossary.
259-259: Parser instantiation adjusted
Maintains uniformity across all test cases in this file.
291-291: Enhanced parser configuration usage
Constructing the parser with .config().aliases(&aliases).input(...) is a more idiomatic approach in the new design.
yash-syntax/src/parser/function.rs (15)
95-95: Additional imports look valid
Alias and hash entry usage in tests requires these imports.
98-98: ExpansionMode import
Bringing ExpansionMode into scope aligns with the new approach to store per-word expansion settings.
105-105: Parser instantiation
Removing the extra parameter is consistent with the updated parser signature.
125-125: Parser instantiation
Same as above: migrating to the new parser setup is in line with the refactoring across the codebase.
128-128: Updated word structure
Storing tuples (Word, ExpansionMode) is consistent with your new design.
142-142: Parser instantiation
Again, removing the unused glossary is a straightforward improvement.
145-145: Words with ExpansionMode
Continues the unified approach for word storage.
164-164: Parser instantiation
Maintains consistency with other tests in this file.
167-167: Words with ExpansionMode
Reflects the new tuple-based representation.
186-186: Parser instantiation
Same reasoning: leverages the updated parser constructor.
189-189: Words with ExpansionMode
Ensures expansions can be handled more flexibly.
229-229: Parser configuration with aliases
This approach is more expressive and aligns well with the parser refactor.
270-270: Parser configuration with aliases
Matches the pattern established for alias injection.
305-305: Parser configuration with aliases
Indicates the test scenario requires alias handling.
308-308: Words with ExpansionMode
Consistent usage of tuple-based words for function definitions.
yash-semantics/src/command/simple_command/builtin.rs (4)
132-132: New field for declaration utility
The is_declaration_utility assignment to Some(false) clarifies that this builtin is not a declaration utility.
188-188: New field for declaration utility
Consistently marks this built-in as non-declaration.
289-289: New field for declaration utility
Maintains the pattern of explicitly specifying non-declaration.
297-297: New field for declaration utility
Same addition, ensuring consistent built-in metadata.
yash-semantics/src/command/and_or.rs (2)
327-327: Declaration utility field
Adding is_declaration_utility: Some(false) indicates this builtin is not recognized as a declaration utility.
335-335: Declaration utility field
Consistent with the previous built-in, maintaining structured metadata.
yash-semantics/src/runner_legacy.rs (1)
145-148: Ensure consistency with parser configuration usage.
Good to see the parser is now configured with .aliases(&self.env) and .declaration_utilities(&self.env). Verify that any new parser behaviors (e.g., handling of declaration utilities) align with the rest of the codebase and that no calls to the old constructor remain.
yash-syntax/src/parser/if.rs (12)
150-150: Parser setup check for parser_if_command_minimum.
Initialization changed to Parser::new(&mut lexer). Confirm that the new parser construction matches the updated config-based approach across other tests.
171-171: Parser setup check for parser_if_command_one_elif.
Same rationale: ensure consistency with the removal of the old Glossary parameter.
214-214: Parser setup check for parser_if_command_else.
No issues noted. The code remains clean.
233-233: Parser setup check for parser_if_command_elif_and_else.
Code continues to read well with the new Parser::new(&mut lexer).
252-252: Parser setup check for parser_if_command_without_then_after_if.
Maintain consistent error handling logic.
271-271: Parser setup check for parser_if_command_without_then_after_elif.
Ensure no functional differences in the error handling.
291-291: Parser setup check for parser_if_command_without_fi.
All good. Error detection remains properly validated.
311-311: Parser setup check for parser_if_command_empty_condition.
The parser logic remains consistent with the new constructor.
325-325: Parser setup check for parser_if_command_empty_body.
No immediate issues seen.
339-339: Parser setup check for parser_if_command_empty_elif_condition.
The change is straightforward. Implementation remains compatible.
356-356: Parser setup check for parser_if_command_without_fi edge cases.
Check that removing the old glossary has no side effect on error messages.
373-373: Parser setup check for parser_if_command_empty_else.
Well-structured. Looks consistent with the rest of the code.
yash-syntax/src/parser/redir.rs (18)
151-151: Parser setup check for parser_redirection_less.
Use of Parser::new(&mut lexer) is correct.
168-168: Parser setup check for parser_redirection_less_greater.
No issues observed.
182-182: Parser setup check for parser_redirection_greater.
Implementation is consistent.
196-196: Parser setup check for parser_redirection_greater_greater.
All good.
210-210: Parser setup check for parser_redirection_greater_bar.
Again, clean transition to the new constructor.
224-224: Parser setup check for parser_redirection_less_and.
No problems found.
238-238: Parser setup check for parser_redirection_greater_and.
Logic remains cohesive.
252-252: Parser setup check for parser_redirection_greater_greater_bar.
Implementation looks good.
266-266: Parser setup check for parser_redirection_less_less_less.
Successfully uses the updated parser approach.
280-280: Parser setup check for parser_redirection_less_less.
The heredoc logic remains unaffected.
300-300: Parser setup check for parser_redirection_less_less_dash.
No concerns.
320-320: Parser setup check for parser_redirection_with_io_number.
All looks consistent.
340-340: Parser setup check for parser_redirection_fd_out_of_range.
Check if error handling has changed.
356-356: Parser setup check for parser_redirection_not_operator.
Still returning Ok(None) properly.
365-365: Parser setup check for parser_redirection_non_word_operand.
Behavior remains correct after constructor shift.
381-381: Parser setup check for parser_redirection_eof_operand.
No functional regressions noted.
397-397: Parser setup check for parser_redirection_not_heredoc_delimiter.
Implementation remains consistent.
413-413: Parser setup check for parser_redirection_eof_heredoc_delimiter.
All good.
yash-semantics/src/runner.rs (1)
150-155: Ensure consistency of the parser configuration.
The chained configuration is clear and aligns with the PR’s objective to introduce declaration utilities. Consider verifying that each method (aliases, declaration_utilities, etc.) is part of the final configuration contract. Otherwise, looks good.
yash-syntax/src/parser/list.rs (18)
221-221: Confirmed removal of the EmptyGlossary parameter.
This change simplifies parser instantiation. Ensure the pertinent test coverage remains sufficient.
230-230: Same removal of glossary parameter.
Consistent with the refactored parser API.
241-241: No issues in adopting the new Parser::new signature.
Looks straightforward.
252-252: Verify that alias or expansions remain unaffected.
Even though EmptyGlossary was dropped, confirm no test regressions.
278-278: Continued removal of the glossary argument.
No concerns—improves clarity.
287-287: Test scenario for here-doc remains intact.
Ensure we preserve coverage for advanced parsing.
313-313: Initialization remains consistent in test.
The Parser::new(&mut lexer) approach is correct for the test environment.
323-323: Retains valid behavior for empty lines test.
Implementation remains straightforward.
333-333: Here-doc content handling test.
Properly verifying parser error scenario.
349-349: Edge-case test for missing subshell opening.
Still aligns well with new parser signature.
362-362: Missing separator test.
No issues with the updated constructor.
375-375: Invalid command token test.
Nothing problematic in the new code.
391-391: Empty list scenario.
Parser usage remains clear.
401-401: Multiple commands test.
No concerns.
411-411: Newline handling test.
Implementation looks good.
423-423: Brace delimiter scenario.
Retains correct error-check logic.
435-435: Invalid command token error test.
No further remarks.
452-452: Another invalid command token test.
Ensures robust error handling.
yash-semantics/src/command_search.rs (12)
297-297: No issue with initializing is_declaration_utility to false.
This field addition is consistent with the change to support declaration utilities.
313-313: Appropriate default for non-declaration built-in.
No concerns here.
346-346: Setting is_declaration_utility to false for special built-in.
Consistent approach.
370-370: Mandatory built-in, not a declaration utility.
Logic is correct.
388-388: Elective built-in, also false by default.
All good.
406-406: Extension built-in with is_declaration_utility: Some(false).
No problems found.
426-426: Priority test for function over mandatory built-in.
No further suggestions.
450-450: Priority test for function over elective built-in.
Implementation is coherent.
474-474: Priority test over extension built-in.
No differences beyond the new field.
496-496: Substitutive built-in with is_declaration_utility: Some(false).
Integration is correct.
517-517: Check for absent executable.
No concerns with the new field.
531-531: Function priority over substitutive built-in.
The new field is used consistently.
yash-builtin/src/command/identify.rs (3)
346-346: Consistent field addition for is_declaration_utility.
This field is uniformly added across different built-ins and appears aligned with the PR's goal of distinguishing declaration utilities.
496-496: Ensuring consistent usage of is_declaration_utility.
No issues found. It improves maintainability by uniformly applying the property to built-ins.
517-517: Maintaining alignment with earlier changes.
Same rationale as above, helps keep the built-in details consistent throughout the codebase.
yash-syntax/src/parser/for_loop.rs (19)
162-162: Reorganized Parser initialization in tests.
Switching to a straightforward Parser::new call simplifies test setup.
170-170: Good practice in test parser creation.
Maintains consistency and readability in test code.
187-187: Consistent usage of Parser::new.
No functional issue, straightforward approach.
204-204: Readability improvement in test code.
Keeping the parser creation simple.
221-221: Parser instantiation pattern remains consistent.
Tests remain clear and coherent.
238-238: Parser creation approach is uniform across tests.
No concerns; the test logic remains intact.
255-255: Clear test parser creation.
No changes to actual parsing logic, just the constructor.
277-277: Uniform test pattern.
Maintains the same code structure for parser instantiation.
299-299: Consistent approach once again.
Encourages standardized test code.
316-316: Parser creation with the updated signature.
All good.
348-348: Switching to config-based parser creation.
This uses Parser::config().aliases(&aliases).input(&mut lexer), which is more explicit about configuration.
379-379: Repeats the new config-based approach.
No issues.
395-395: Another test updated for simpler parser setup.
Works fine.
409-409: Maintains consistency with previous lines.
No functional regressions.
423-423: Ensures uniform approach to parser creation.
Looks good.
Line range hint 453-460: Added is_declaration_utility field to the check builtin.
This keeps the built-in structure consistent with the rest of the code.
476-476: Parser creation updated in test.
No issues.
512-512: Uses parser config with aliases.
Clear approach for alias expansions.
535-535: Maintains consistent parser usage.
All good.
yash-semantics/src/command/compound_command/while_loop.rs (2)
201-208: Insertion of is_declaration_utility: Some(false).
Matches the PR’s declaration-utility logic. Simplifies extended checks for built-ins in loop contexts.
453-460: Same field usage for the check builtin in the until loop test.
Ensures uniformity and synergy with while-loop code.
yash-syntax/src/parser/case.rs (25)
194-194: Test uses the new parser config approach.
No issues.
219-219: Aliases configured with Parser::config().
Allows alias expansions properly in case item tests.
231-231: Parser creation updated in test.
Consistent with the rest.
247-247: Refined approach to parser initialization.
No further remarks.
263-263: Maintains consistent parser creation.
Helps unify test style.
281-281: Another instance using Parser::new.
Straightforward.
299-299: Double-semicolon test with updated constructor.
All good.
315-315: Check for body before double-semicolon.
Test logic unchanged, parser usage updated.
332-332: Semicolon-and style parse test.
Consistent with new parser instantiation.
348-348: Checks pattern error case.
Uses direct parser creation.
361-361: Open parenthesis pattern test.
No concerns.
374-374: Invalid pattern scenario.
Parser construction approach aligned to the rest.
387-387: Missing pattern test.
No issues found.
400-400: Unclosed pattern list scenario.
Uniform test structure.
416-416: Minimal case usage test.
Configuration consistent.
448-448: Alias-based expansions for case.
Clear usage of config-based parser approach.
483-483: Subject alias expansions.
Parser configured similarly as earlier tests.
519-519: Another alias expansion test.
No issues.
538-538: One case item test.
Construct remains consistent.
558-558: Multiple items test for case without final double-semicolon.
Maintains test clarity.
577-577: Multiple items with final double-semicolon.
Same updated parser usage.
595-595: Missing subject in case.
Good negative test coverage.
609-609: Invalid subject scenario.
No issues with the code.
623-623: Missing in test.
Uses the new parser approach.
643-643: Missing esac scenario.
Maintains consistent style for test setup.
yash-syntax/src/parser/from_str.rs (10)
195-195: Confirm tuple indexing correctness.
Using word.0.location indicates that word is now a tuple, likely (Word, ExpansionMode). Ensure this indexing is always valid and that we indeed need the Word's location and not the ExpansionMode.
251-251: Constructor consistency looks good.
Switching to Parser::new(&mut lexer); without a glossary parameter aligns with the new configuration approach. Make sure any glossary-dependent logic is handled elsewhere.
280-280: Parser instantiation remains consistent.
Reinstating Parser::new(&mut lexer); again here. No issues spotted. Just verify you have the correct configuration for all parser features.
[approve]
306-306: Repeated removal of the EmptyGlossary parameter.
Same pattern: Parser::new(&mut lexer);. Ensure that any logic previously handled by the glossary is addressed, if relevant.
333-333: Ensuring no breakage from removed glossary parameter.
Looks appropriate. Confirm that downstream code doesn’t rely on the old glossary usage.
356-356: Consistent approach to Parser creation.
No functional issues are apparent. This unifies parser instantiation across the code.
379-379: Parser instantiation simplification approved.
No direct concerns. Good to keep object creation consistent.
402-402: Further confirmation of uniform parser initialization.
Same pattern. No immediate issues. Ensure all tests pass to confirm correctness.
[approve]
434-434: Maintaining a consistent pattern in parser usage.
This continuity is beneficial. Just verify no functionalities are lost.
452-452: Streamlined parser creation looks good.
No conflicts found. Ensures uniform approach to instantiation across all references.
yash-syntax/src/syntax/impl_display.rs (4)
240-240: Formatting words with tuple indexing.
let i2 = self.words.iter().map(|x| &x.0 as &dyn fmt::Display); now extracts only the Word portion. Ensure ExpansionMode is intentionally ignored here if you do not need it in display output.
745-747: Appending a word tuple with ExpansionMode::Multiple.
Pushing (Word::from_str("echo").unwrap(), ExpansionMode::Multiple) is correct if “echo” indeed needs multiple expansion. If you intend different expansions, double-check.
750-752: Appending a word tuple with ExpansionMode::Single.
This indicates “foo” is expanded only once. Validate that this choice matches the intended behavior for “foo” in the command flow.
789-789: Keyword-like word uses multiple expansion.
("if".parse().unwrap(), ExpansionMode::Multiple) is valid if “if” is purely treated as a simple word rather than a special shell keyword. Confirm the parser logic if needed.
[approve]
yash-syntax/src/parser/lex/core.rs (1)
753-753: Parser instantiation without a glossary reference.
let mut parser = super::super::Parser::new(self); is consistent with the new approach. Ensure any previous glossary functionality is handled if still needed.
yash-env-test-helper/Cargo.toml (3)
3-3: Version bump to 0.4.0.
Increasing the version signals a non-trivial update. Verify change logs and dependency references are coherent across crates.
15-15: Package marked as non-publishable.
publish = false is fine if this crate is strictly for local testing and not meant for crates.io. No concerns.
21-21: Dependency version upgrade to 0.6.0.
Ensure compatibility with the new yash-env version. Tests should confirm no regressions.
yash-syntax/Cargo.toml (1)
3-3: LGTM! Version bump and publish flag are appropriate.
The version increment to 0.14.0 aligns with the significant changes introduced for declaration utilities support. The publish = false flag is consistent with other packages in the ecosystem.
Also applies to: 15-15
yash-prompt/Cargo.toml (1)
3-3: Verify CHANGELOG updates for version 0.4.0
The version bump and dependency updates look good and are properly coordinated across packages.
Let's verify the CHANGELOG updates:
Also applies to: 15-15, 19-21, 24-24
✅ Verification successful
CHANGELOG.md is properly updated with version 0.4.0 changes
The CHANGELOG.md correctly documents:
- Version 0.4.0 (marked as unreleased)
- Updated dependency on yash-env from 0.5.0 to 0.6.0
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if CHANGELOG.md exists and contains entries for version 0.4.0
# Note: Path known from retrieved learning
if [ -f yash-prompt/CHANGELOG.md ]; then
echo "Checking CHANGELOG.md for version 0.4.0 entries..."
rg -A 5 "## \[0\.4\.0\]" yash-prompt/CHANGELOG.md
else
echo "CHANGELOG.md not found in yash-prompt directory"
fi
Length of output: 299
yash-cli/tests/scripted_test/declutil-y.sh (2)
6-12: LGTM! Good test coverage for typeset declaration utility behavior
The test effectively validates that typeset, as a declaration utility, does not perform pathname expansion or field splitting.
14-22: LGTM! Comprehensive test cases for printf behavior
The test cases thoroughly validate that printf:
- Performs pathname expansion and field splitting
- Doesn't handle tilde expansions as a declaration utility
- Maintains consistent behavior when used with command builtin
This provides good coverage of the non-declaration utility behavior.
Also applies to: 24-30, 32-40
yash-semantics/Cargo.toml (2)
3-3: LGTM: Version updates are consistent across dependencies
The version bumps align well with the scope of changes for declaration utilities support:
- Package version: 0.6.0
- yash-env: 0.6.0
- yash-syntax: 0.14.0
- yash-env-test-helper: 0.4.0
Also applies to: 23-23, 26-26, 31-31
15-15: LGTM: Good practice to prevent accidental publishing
Setting publish = false is appropriate for internal packages.
yash-cli/Cargo.toml (2)
3-3: LGTM: Version updates maintain consistency
The version updates are well-coordinated across the ecosystem:
- CLI version bump to 0.3.0 is appropriate for new features
- All yash-* dependencies are updated to their latest versions
- Dependency versions match those in yash-semantics
Also applies to: 23-24, 26-28
15-15: LGTM: Consistent publishing configuration
Setting publish = false maintains consistency with other packages.
yash-env/Cargo.toml (2)
3-3: LGTM: Version updates and improved formatting
Changes maintain ecosystem consistency:
- Version 0.6.0 aligns with other packages
- yash-syntax dependency updated to 0.14.0
- Features section formatting enhances readability
Also applies to: 31-33
15-15: LGTM: Publishing configuration
Setting publish = false maintains consistency across the ecosystem.
yash-builtin/Cargo.toml (1)
3-3: LGTM! Version updates and publishing configuration look good.
The version updates are consistent across all dependencies, and the publish = false flag is appropriate for an internal crate.
Also applies to: 15-15, 28-32, 38-39
yash-cli/tests/scripted_test/declutil-p.sh (2)
8-73: LGTM! Comprehensive test coverage for declaration utilities.
The test cases effectively cover various aspects of declaration utilities including pathname expansion, field splitting, tilde expansion, and command prefixes. The test structure is clear and maintainable.
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 26-26: This does not export 'a'. Remove
(SC2163)
74-75: LGTM! Good documentation of POSIX compliance.
The comment clearly explains why negative tests for non-declaration utilities are omitted, which is helpful for future maintenance.
yash-syntax/src/parser/command.rs (1)
59-59: LGTM! Parser initialization simplified appropriately.
The removal of EmptyGlossary parameter from Parser::new calls in tests is consistent with the broader changes to support declaration utilities. The core parsing functionality remains unchanged.
Let's verify that this change is consistent across all test files:
Also applies to: 74-74, 89-89, 104-104
✅ Verification successful
Let's verify the implementation of Parser::new to confirm the signature change.
Let me try a different approach to find the Parser implementation.
Let me check the core module for the Parser implementation.
Parser initialization changes are consistent across the codebase
Based on the verification results:
- The
Parser::newimplementation incore.rsshows a single parameter signaturepub fn new(lexer: &'a mut Lexer<'b>), confirming the removal ofEmptyGlossary. - All usages of
Parser::newacross the codebase (100+ occurrences in test files) consistently use the new single-parameter signature. - The change is part of a broader refactoring where default glossaries are now handled through
ParserConfig, as evidenced by the implementation details.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Parser::new usage across test files
# Expect: All test files should use the new signature without EmptyGlossary
# Search for Parser::new usage in test code
rg -p "Parser::new\(" "yash-syntax/src/parser/"
Length of output: 10229
Script:
#!/bin/bash
# Search for Parser::new implementation
ast-grep --pattern 'impl Parser {
$$$
pub fn new($_) -> $_ {
$$$
}
$$$
}'
Length of output: 87
Script:
#!/bin/bash
# Search for Parser::new implementation with more context
rg -p "impl Parser" -A 10 "yash-syntax/src/parser/"
Length of output: 7068
Script:
#!/bin/bash
# Search for Parser::new implementation in core.rs
rg -p "fn new" -B 2 -A 10 "yash-syntax/src/parser/core.rs"
Length of output: 1252
yash-env/CHANGELOG.md (1)
8-19: LGTM! Changelog entries are clear and well-structured.
The changelog properly documents the new features and changes:
- Implementation of
yash_syntax::decl_util::Glossary - Addition of
is_declaration_utilityfield - Updated dependency versions
yash-syntax/CHANGELOG.md (1)
8-33: LGTM! Comprehensive documentation of changes.
The changelog effectively captures the significant changes:
- Addition of
ExpansionModeenum - New
decl_utilmodule withGlossarytrait - Parser configuration improvements
🧰 Tools
🪛 LanguageTool
[typographical] ~11-~11: The conjunction “so that” does not require a comma.
Context: ...the parser is configured on construction, so that the parser can be constructed with more...
(SO_THAT_UNNECESSARY_COMMA)
yash-semantics/src/tests.rs (4)
56-56: LGTM: Correct classification for exit built-in
The is_declaration_utility: Some(false) setting is correct as the exit command is a special built-in but not a declaration utility according to POSIX.
83-83: LGTM: Correct classification for flow control built-ins
The is_declaration_utility: Some(false) setting is correct for all flow control built-ins (return, break, continue, suspend). None of these are declaration utilities according to POSIX specifications.
Also applies to: 104-104, 125-125, 144-144
183-183: LGTM: Correct classification for local built-in with implementation notes
The is_declaration_utility: Some(true) setting is correct as local is a declaration utility that manages variable scope.
However, there are some TODOs and unimplemented error handlers in the implementation:
- Invalid name validation is missing
- Assignment error handling is unimplemented
- Write error handling is unimplemented
Would you like me to help implement these missing validations and error handlers?
207-207: LGTM: Correct classification for I/O built-ins
The is_declaration_utility: Some(false) setting is correct for both echo and cat built-ins. These are standard I/O utilities, not declaration utilities according to POSIX specifications.
Also applies to: 240-240
| # declutil-p.sh: test of declaration utilities for any POSIX-compliant shell | ||
|
|
||
| posix="true" | ||
|
|
||
| # Pathname expansion may match this dummy file in incorrect implementations. | ||
| >tmpfile | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add shebang and improve script initialization.
The script initialization can be improved for better portability and clarity:
+#!/bin/sh
# declutil-p.sh: test of declaration utilities for any POSIX-compliant shell
-posix="true"
+: "${posix:=true}" # Use parameter expansion for better portability
# Pathname expansion may match this dummy file in incorrect implementations.
-">tmpfile"
+: > tmpfile # Use null command for redirection📝 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.
| # declutil-p.sh: test of declaration utilities for any POSIX-compliant shell | |
| posix="true" | |
| # Pathname expansion may match this dummy file in incorrect implementations. | |
| >tmpfile | |
| #!/bin/sh | |
| # declutil-p.sh: test of declaration utilities for any POSIX-compliant shell | |
| : "${posix:=true}" # Use parameter expansion for better portability | |
| # Pathname expansion may match this dummy file in incorrect implementations. | |
| : > tmpfile # Use null command for redirection |
🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
[warning] 3-3: posix appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 6-6: This redirection doesn't have a command. Move to its command (or use 'true' as no-op).
(SC2188)
ExpansionModeSimpleCommand::wordsusingExpansionModeexpand_word_multipleexpand_word_with_modeImplementexpand_words_with_modeDeprecateexpand_wordsyash_semantics::expansionandyash_semantics::command::simple_commandRefactor Word-to-Assign conversion to optimize out the impossibleValue::ArraycaseSummary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
Release Notes for Yash Shell v0.3.0
New Features
ExpansionModefunctionalityChanges
Dependency Updates
Breaking Changes
SimpleCommandstructure modified to include expansion modesEmptyGlossaryin parser instantiation