Skip to content
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

Various fixes #361

Merged
merged 5 commits into from
Apr 21, 2024
Merged

Various fixes #361

merged 5 commits into from
Apr 21, 2024

Conversation

magicant
Copy link
Owner

@magicant magicant commented Apr 18, 2024

Addresses issues in #336

Summary by CodeRabbit

  • Bug Fixes

    • Enhanced error handling in file operations, ensuring safer and more reliable file interactions.
    • Addressed issues with newline parsing, alias substitution, and character location indexing in script parsing, improving script accuracy and reliability.
  • Refactor

    • Refined command parsing logic in pipelines, enhancing the handling of reserved words and command separators for increased parsing precision.
    • Improved file descriptor manipulation for better error handling and system resource management.
  • Documentation

    • Updated changelogs to reflect recent enhancements and fixes in file handling and script parsing functionalities.

This fixes a bug where a newline after a bar in a pipeline was not
parsed correctly if it appeared as a result of alias substitution.
The next token after a blank-ending alias should be subject to alias
substitution. However, the shell was not substituting the alias if there
was a line continuation before the next token. This was because the
LexerCore::is_after_blank_ending_alias function was only looking for the
last non-blank character before the current position. This commit
changes the function to also check if the character is a line
continuation.

To enable this, a new LexerCore::mark_line_continuation function is
added to mark the characters in a given range as line continuation. The
Lexer::line_continuation function now calls this function to mark the
characters it has consumed as line continuation.
@magicant magicant added the bug Something isn't working label Apr 18, 2024
@magicant magicant self-assigned this Apr 18, 2024
Copy link

coderabbitai bot commented Apr 18, 2024

Walkthrough

The recent updates across various components of the yash project focus on refining error handling, updating the handling of file descriptors, and enhancing the parsing logic for better accuracy in command interpretation and alias substitution. These changes aim to improve the robustness and functionality of the system, addressing previous issues and optimizing the user experience.

Changes

File Path Change Summary
.../alias-p.sh, .../command-p.sh Removed TODO comments, adjusted test cases for alias substitution and command descriptions.
yash-env/CHANGELOG.md, .../real.rs Updated open_tmpfile function to not set the O_CLOEXEC flag, refactored error handling and file descriptor manipulation.
yash-syntax/CHANGELOG.md, .../core.rs, .../pipeline.rs Enhanced lexer and parser for better newline and alias handling, introduced SourceCharEx struct, refined command parsing in pipelines.

Poem

🐰🌟
In the land of code and night,
A rabbit hopped with sheer delight.
Bugs squashed, features fine-tuned bright,
Yash dances now in lighter byte.
Cheers to devs, with every commit tight!


Recent Review Details

Configuration used: CodeRabbit UI

Commits Files that changed from the base of the PR and between 541ec7c and 3662079.
Files selected for processing (7)
  • yash-cli/tests/scripted_test/alias-p.sh (3 hunks)
  • yash-cli/tests/scripted_test/command-p.sh (1 hunks)
  • yash-env/CHANGELOG.md (1 hunks)
  • yash-env/src/system/real.rs (1 hunks)
  • yash-syntax/CHANGELOG.md (1 hunks)
  • yash-syntax/src/parser/lex/core.rs (15 hunks)
  • yash-syntax/src/parser/pipeline.rs (4 hunks)
Additional Context Used
LanguageTool (74)
yash-env/CHANGELOG.md (6)

Near line 3: Possible spelling mistake found.
Context: # Changelog All notable changes to yash-env will be documented in this file. The ...


Near line 5: Only proper nouns start with an uppercase character (there are exceptions for headlines).
Context: ... in this file. The format is based on [Keep a Changelog](https://keepachangelog.com...


Near line 12: This sentence does not start with an uppercase letter.
Context: ...### Fixed - RealSystem::open_tmpfile no longer returns a file descriptor with t...


Near line 12: Possible spelling mistake found.
Context: ...nger returns a file descriptor with the O_CLOEXEC flag set. ## [0.1.0] - 2024-04-13 ##...


Near line 15: Unpaired symbol: ‘[’ seems to be missing
Context: ...h the O_CLOEXEC flag set. ## [0.1.0] - 2024-04-13 ### Added - Initial impl...


Near line 19: Possible spelling mistake found.
Context: ... Added - Initial implementation of the yash-env crate [0.1.0]: https://github.com/mag...

yash-syntax/CHANGELOG.md (68)

Near line 3: Possible spelling mistake found.
Context: # Changelog All notable changes to yash-syntax will be documented in this file. The ...


Near line 5: Only proper nouns start with an uppercase character (there are exceptions for headlines).
Context: ... in this file. The format is based on [Keep a Changelog](https://keepachangelog.com...


Near line 12: This sentence does not start with an uppercase letter.
Context: ... ### Changed - Lexer::source_string is no longer generic. The argument type is...


Near line 13: Possible spelling mistake found.
Context: ...e>instead of a generic implementor of SliceIndex<[SourceChar], Output = [SourceChar]>`. ...


Near line 26: Unpaired symbol: ‘[’ seems to be missing
Context: ...he length of the source code. ## [0.8.0] - 2024-04-09 Starting from this versio...


Near line 28: Possible spelling mistake found.
Context: ...-04-09 Starting from this version, the yash-syntax crate can be compiled on non-Unix plat...


Near line 29: Possible spelling mistake found.
Context: ...e compiled on non-Unix platforms, where RawFd falls back to i32. ### Added - `so...


Near line 47: Possible spelling mistake found.
Context: ...:lex::Keyword::is_clause_delimiternowconstand#[must_use]-parser::lex::Oper...


Near line 48: Possible spelling mistake found.
Context: ...lex::Operator::is_clause_delimiternowconstand#[must_use]-<parser::lex::Ope...


Near line 59: Unpaired symbol: ‘[’ seems to be missing
Context: ...eywordin favor ofFromStr ## [0.7.0] - 2023-11-12 ### Added -impl Defaul...


Near line 67: Possible spelling mistake found.
Context: ...fault for Text ### Changed - Type ofHereDoc::contentfromRefCelltoOnce...


Near line 67: Possible spelling mistake found.
Context: ...nged - Type of HereDoc::content from RefCell<Text> to OnceCell<Text> - External d...


Near line 71: Unpaired symbol: ‘[’ seems to be missing
Context: ...ns - Rust 1.67.0 → 1.70.0 ## [0.6.1] - 2023-05-01 ### Added - Variants of ...


Near line 75: Possible spelling mistake found.
Context: ... - 2023-05-01 ### Added - Variants of parser::SyntaxError: MissingSeparator, `UnopenedGrouping...


Near line 75: Possible spelling mistake found.
Context: ...d - Variants of parser::SyntaxError: MissingSeparator, UnopenedGrouping, `UnopenedSubshe...


Near line 75: Possible spelling mistake found.
Context: ...rser::SyntaxError: MissingSeparator, UnopenedGrouping, UnopenedSubshell, UnopenedLoop`,...


Near line 75: Possible spelling mistake found.
Context: ...MissingSeparator, UnopenedGrouping, UnopenedSubshell, UnopenedLoop, UnopenedDoClause, `...


Near line 76: Possible spelling mistake found.
Context: ...nopenedGrouping, UnopenedSubshell, UnopenedLoop, UnopenedDoClause, UnopenedIf, ...


Near line 76: Possible spelling mistake found.
Context: ..., UnopenedSubshell, UnopenedLoop, UnopenedDoClause, UnopenedIf, UnopenedCase, `InAs...


Near line 76: Possible spelling mistake found.
Context: ...l, UnopenedLoop, UnopenedDoClause, UnopenedIf, UnopenedCase, InAsCommandName` ...


Near line 76: Possible spelling mistake found.
Context: ...oop, UnopenedDoClause, UnopenedIf, UnopenedCase, InAsCommandName ### Changed -pa...


Near line 81: This sentence does not start with an uppercase letter.
Context: ...anged - parser::Parser::command_line to return the newly added variants of `S...


Near line 81: Possible spelling mistake found.
Context: ...to return the newly added variants of SyntaxErrorinstead ofInvalidCommandToken` depen...


Near line 82: Possible spelling mistake found.
Context: ... variants of SyntaxError instead of InvalidCommandToken depending on the type of erroneous t...


Near line 82: In this context, ‘type’ should agree in number with the noun after ‘of’.
Context: ... InvalidCommandToken depending on the type of erroneous tokens. - External dependency versions - R...


Near line 87: Possible spelling mistake found.
Context: ....0 - Internal dependency versions - async-trait 0.1.56 → 0.1.73 - futures-util 0.3....


Near line 89: Possible spelling mistake found.
Context: ... - futures-util 0.3.23 → 0.3.28 - itertools 0.10.3 → 0.11.0 - thiserror 1.0.43 ...


Near line 90: Possible spelling mistake found.
Context: ...8 - itertools 0.10.3 → 0.11.0 - thiserror 1.0.43 → 1.0.47 ## [0.6.0] - 2022-10-0...


Near line 92: Unpaired symbol: ‘[’ seems to be missing
Context: ... - thiserror 1.0.43 → 1.0.47 ## [0.6.0] - 2022-10-01 ### Added - `source::Sou...


Near line 100: This sentence does not start with an uppercase letter.
Context: ... - syntax::CompoundCommand::Subshell from a tuple variant Subshell(List) to a...


Near line 100: It appears that a white space is missing.
Context: ...Command::Subshellfrom a tuple variantSubshell(List) to a struct variantSubshell {...


Near line 101: Don’t put a space before the closing parenthesis.
Context: ... Subshell(List) to a struct variant Subshell { body: Rc<List>, location: Location }. - Internal dependency versions - ...


Near line 105: Unpaired symbol: ‘[’ seems to be missing
Context: ... futures-util 0.3.21 → 0.3.23 ## [0.5.0] - 2022-07-02 This version contains var...


Near line 107: Possible missing article found.
Context: ....0] - 2022-07-02 This version contains variety of fixes. ### Added - `parser::lex::K...


Near line 121: This sentence does not start with an uppercase letter.
Context: ...nged - syntax::CommandSubst::content from String to Rc<str> - parser::Error...


Near line 124: Possible spelling mistake found.
Context: ...::maybe_compound_listnow returning anInvalidCommandToken` error if the list is delimited by a to...


Near line 133: Unpaired symbol: ‘[’ seems to be missing
Context: ... source::pretty::Message<'a>` ## [0.4.0] - 2022-02-27 This version modifies the...


Near line 145: Possible spelling mistake found.
Context: ... The following functions now taking the start_index: usize parameter instead of `opening_location...


Near line 150: This verb may not be in the correct form. Consider using a different form for this context.
Context: ...ed_param- The following functions now returningResult<Option>instead of...


Near line 150: Only proper nouns start with an uppercase character (there are exceptions for headlines).
Context: ...- The following functions now returning Result<Option<TextUnit>> instead of `Result<Result<T...


Near line 150: Possible spelling mistake found.
Context: ...g Result<Option<TextUnit>> instead of Result<Result<TextUnit, Location>>: - `parser::lex::Lexer...


Near line 160: Unpaired symbol: ‘[’ seems to be missing
Context: ...- source::Location::advance ## [0.3.0] - 2022-02-06 This version simplifies t...


Near line 162: Possible spelling mistake found.
Context: ...ons for the abstract syntax tree (AST); syntax::HereDoc::content is now wrapped in RefCell t...


Near line 163: Possible spelling mistake found.
Context: ...ax::HereDoc::contentis now wrapped inRefCell` to remove generic type parameters from...


Near line 164: Possible spelling mistake found.
Context: ... to remove generic type parameters from RedirBody and other AST types. ### Changed - `...


Near line 169: Possible spelling mistake found.
Context: ...syntax::HereDoc::content redefined as RefCell<Text> (previously Text) - `impl From...


Near line 171: Possible spelling mistake found.
Context: ...::Lexer::here_doc_contentnow taking a&HereDocparameter and returningResult<()>` -...


Near line 172: Possible spelling mistake found.
Context: ...memorize_unread_here_docnow taking anRcparameter ### Removed -pa...


Near line 179: Possible spelling mistake found.
Context: ... - Generic type parameters of AST types RedirBody, Redir, SimpleCommand, ElifThen,...


Near line 179: Possible spelling mistake found.
Context: ...pe parameters of AST types RedirBody, Redir, SimpleCommand, ElifThen, `CaseIte...


Near line 179: Possible spelling mistake found.
Context: ...ters of AST types RedirBody, Redir, SimpleCommand, ElifThen, CaseItem, `CompoundComm...


Near line 179: Possible spelling mistake found.
Context: ... RedirBody, Redir, SimpleCommand, ElifThen, CaseItem, CompoundCommand, `FullC...


Near line 179: Possible spelling mistake found.
Context: ..., Redir, SimpleCommand, ElifThen, CaseItem, CompoundCommand, `FullCompoundComma...


Near line 179: Possible spelling mistake found.
Context: ...SimpleCommand, ElifThen, CaseItem, CompoundCommand, FullCompoundCommand, FunctionDefin...


Near line 179: Possible spelling mistake found.
Context: ...ifThen, CaseItem, CompoundCommand, FullCompoundCommand, FunctionDefinition, Command, Pip...


Near line 179: Possible spelling mistake found.
Context: ...ompoundCommand, FullCompoundCommand, FunctionDefinition, Command, Pipeline, AndOrList, ...


Near line 179: Possible spelling mistake found.
Context: ...tionDefinition, Command, Pipeline, AndOrList, Item, Listin thesyntax` module...


Near line 181: Unpaired symbol: ‘[’ seems to be missing
Context: ...List in the syntax module ## [0.2.0] - 2022-02-03 Previously, source code a...


Near line 183: Possible spelling mistake found.
Context: ...ly, source code attribution attached to ASTs was line-oriented. The attribution now ...


Near line 197: There probably shouldn’t be a space before the apostrophe.
Context: ...e source module: - Line renamed to Code - Location's field line renamed to code - `L...


Near line 198: There probably shouldn’t be a space before the apostrophe.
Context: ... - Location's field line renamed to code - Location's field column replaced with index ...


Near line 199: There probably shouldn’t be a space before the apostrophe.
Context: ... - Location's field column replaced with index - Code's field value wrapped in the RefCell ...


Near line 200: There probably shouldn’t be a space before the apostrophe.
Context: ... - Code's field value wrapped in the RefCell - Annotation's field location changed to a reference...


Near line 201: There probably shouldn’t be a space before the apostrophe.
Context: ...tation's field locationchanged to a reference -Annotation's field codeadded -Annotation`'s...


Near line 202: There probably shouldn’t be a space before the apostrophe.
Context: ...rence - Annotation's field code added - Annotation's method new added - Items in the `inpu...


Near line 206: Possible spelling mistake found.
Context: ...de, Error>) - Errorredefined asstd::io::Error(previously(Location, std::io...


Near line 206: Possible spelling mistake found.
Context: ...defined as std::io::Error (previously (Location, std::io::Error)) - Context now `non_exha...


Near line 221: Unpaired symbol: ‘[’ seems to be missing
Context: ...numerate-Lines-lines` ## [0.1.0] - 2021-12-11 ### Added - Functionalit...

AST-based Instructions (17)
yash-env/src/system/real.rs (17)

[warning] 295-295: Found use of unsafe code. Unsafe code should be avoided whenever possible. Instead, prefer safe code and use unsafe code only when necessary.
Context: unsafe { nix::libc::fdopendir(fd.0) }
Note: [CWE-242]: Use of Inherently Dangerous Function [REFERENCES]
- https://doc.rust-lang.org/std/keyword.unsafe.html


[warning] 301-301: Found use of unsafe code. Unsafe code should be avoided whenever possible. Instead, prefer safe code and use unsafe code only when necessary.
Context: unsafe { nix::libc::opendir(path.as_ptr()) }
Note: [CWE-242]: Use of Inherently Dangerous Function [REFERENCES]
- https://doc.rust-lang.org/std/keyword.unsafe.html


[warning] 316-316: Found use of unsafe code. Unsafe code should be avoided whenever possible. Instead, prefer safe code and use unsafe code only when necessary.
Context: unsafe { nix::libc::times(tms.as_mut_ptr()) }
Note: [CWE-242]: Use of Inherently Dangerous Function [REFERENCES]
- https://doc.rust-lang.org/std/keyword.unsafe.html


[warning] 320-320: Found use of unsafe code. Unsafe code should be avoided whenever possible. Instead, prefer safe code and use unsafe code only when necessary.
Context: unsafe { tms.assume_init() }
Note: [CWE-242]: Use of Inherently Dangerous Function [REFERENCES]
- https://doc.rust-lang.org/std/keyword.unsafe.html


[warning] 322-322: Found use of unsafe code. Unsafe code should be avoided whenever possible. Instead, prefer safe code and use unsafe code only when necessary.
Context: unsafe { nix::libc::sysconf(nix::libc::_SC_CLK_TCK) }
Note: [CWE-242]: Use of Inherently Dangerous Function [REFERENCES]
- https://doc.rust-lang.org/std/keyword.unsafe.html


[warning] 353-353: Found use of unsafe code. Unsafe code should be avoided whenever possible. Instead, prefer safe code and use unsafe code only when necessary.
Context: unsafe { nix::sys::signal::sigaction(signal, &new_action) }
Note: [CWE-242]: Use of Inherently Dangerous Function [REFERENCES]
- https://doc.rust-lang.org/std/keyword.unsafe.html


[warning] 411-411: Found use of unsafe code. Unsafe code should be avoided whenever possible. Instead, prefer safe code and use unsafe code only when necessary.
Context: unsafe { nix::libc::pselect(nfds, readers, writers, errors, timeout, signal_mask) }
Note: [CWE-242]: Use of Inherently Dangerous Function [REFERENCES]
- https://doc.rust-lang.org/std/keyword.unsafe.html


[warning] 453-453: Found use of unsafe code. Unsafe code should be avoided whenever possible. Instead, prefer safe code and use unsafe code only when necessary.
Context: unsafe { nix::unistd::fork()? }
Note: [CWE-242]: Use of Inherently Dangerous Function [REFERENCES]
- https://doc.rust-lang.org/std/keyword.unsafe.html


[warning] 505-522: Found use of unsafe code. Unsafe code should be avoided whenever possible. Instead, prefer safe code and use unsafe code only when necessary.
Context: unsafe {
use std::os::unix::ffi::OsStringExt as _;
let size = nix::libc::confstr(nix::libc::_CS_PATH, std::ptr::null_mut(), 0);
if size == 0 {
return Err(Errno::last());
}
let mut buffer = Vec::::with_capacity(size);
let final_size =
nix::libc::confstr(nix::libc::_CS_PATH, buffer.as_mut_ptr() as *mut _, size);
if final_size == 0 {
return Err(Errno::last());
}
if final_size > size {
return Err(Errno::ERANGE);
}
buffer.set_len(final_size - 1); // The last byte is a null terminator.
return Ok(OsString::from_vec(buffer));
}
Note: [CWE-242]: Use of Inherently Dangerous Function [REFERENCES]
- https://doc.rust-lang.org/std/keyword.unsafe.html


[warning] 534-534: Found use of unsafe code. Unsafe code should be avoided whenever possible. Instead, prefer safe code and use unsafe code only when necessary.
Context: unsafe { nix::libc::getrlimit(raw_resource as _, limits.as_mut_ptr()) }
Note: [CWE-242]: Use of Inherently Dangerous Function [REFERENCES]
- https://doc.rust-lang.org/std/keyword.unsafe.html


[warning] 535-535: Found use of unsafe code. Unsafe code should be avoided whenever possible. Instead, prefer safe code and use unsafe code only when necessary.
Context: unsafe { limits.assume_init() }
Note: [CWE-242]: Use of Inherently Dangerous Function [REFERENCES]
- https://doc.rust-lang.org/std/keyword.unsafe.html


[warning] 545-545: Found use of unsafe code. Unsafe code should be avoided whenever possible. Instead, prefer safe code and use unsafe code only when necessary.
Context: unsafe { nix::libc::setrlimit(raw_resource as _, &limits) }
Note: [CWE-242]: Use of Inherently Dangerous Function [REFERENCES]
- https://doc.rust-lang.org/std/keyword.unsafe.html


[warning] 556-558: Found use of unsafe code. Unsafe code should be avoided whenever possible. Instead, prefer safe code and use unsafe code only when necessary.
Context: unsafe {
nix::libc::closedir(self.0.as_ptr());
}
Note: [CWE-242]: Use of Inherently Dangerous Function [REFERENCES]
- https://doc.rust-lang.org/std/keyword.unsafe.html


[warning] 565-565: Found use of unsafe code. Unsafe code should be avoided whenever possible. Instead, prefer safe code and use unsafe code only when necessary.
Context: unsafe { nix::libc::readdir(self.0.as_ptr()) }
Note: [CWE-242]: Use of Inherently Dangerous Function [REFERENCES]
- https://doc.rust-lang.org/std/keyword.unsafe.html


[warning] 570-575: Found use of unsafe code. Unsafe code should be avoided whenever possible. Instead, prefer safe code and use unsafe code only when necessary.
Context: unsafe {
let entry = entry.as_mut();
let name = CStr::from_ptr(entry.d_name.as_ptr());
let name = OsStr::from_bytes(name.to_bytes());
Ok(Some(DirEntry { name }))
}
Note: [CWE-242]: Use of Inherently Dangerous Function [REFERENCES]
- https://doc.rust-lang.org/std/keyword.unsafe.html


[warning] 586-586: Found use of unsafe code. Unsafe code should be avoided whenever possible. Instead, prefer safe code and use unsafe code only when necessary.
Context: unsafe { RealSystem::new() }
Note: [CWE-242]: Use of Inherently Dangerous Function [REFERENCES]
- https://doc.rust-lang.org/std/keyword.unsafe.html


[warning] 599-613: Found use of unsafe code. Unsafe code should be avoided whenever possible. Instead, prefer safe code and use unsafe code only when necessary.
Context: unsafe {
let mut system = RealSystem::new();
let result = system.caught_signals();
assert_eq!(result, []);

        catch_signal(Signal::SIGINT as c_int);
        catch_signal(Signal::SIGTERM as c_int);
        catch_signal(Signal::SIGTERM as c_int);
        catch_signal(Signal::SIGCHLD as c_int);

        let result = system.caught_signals();
        assert_eq!(result, [Signal::SIGINT, Signal::SIGTERM, Signal::SIGCHLD]);
        let result = system.caught_signals();
        assert_eq!(result, []);
    }

Note: [CWE-242]: Use of Inherently Dangerous Function [REFERENCES]
- https://doc.rust-lang.org/std/keyword.unsafe.html

ShellCheck (11)
yash-cli/tests/scripted_test/alias-p.sh (8)

[info] 10-10: The mentioned syntax error was in this explicit subshell.


[warning] 84-84: This { is literal. Check expression (missing ;/\n?) or quote it.


[info] 84-84: Want to escape a single quote? echo 'This is how it'''s done'.


[warning] 84-84: This } is literal. Check expression (missing ;/\n?) or quote it.


[warning] 167-167: This { is literal. Check expression (missing ;/\n?) or quote it.


[warning] 167-167: This } is literal. Check expression (missing ;/\n?) or quote it.


[error] 182-182: Couldn't parse this explicit subshell. Fix to allow more checks.


[error] 186-186: Unexpected keyword/token. Fix any mentioned problems and try again.

yash-cli/tests/scripted_test/command-p.sh (3)

[error] 79-79: Couldn't parse this explicit subshell. Fix to allow more checks.


[error] 85-85: You are missing a required space after the !.


[error] 85-85: Expected a command. Fix any mentioned problems and try again.

Learnings (1)
yash-cli/tests/scripted_test/alias-p.sh (1)
User: magicant"
PR: magicant/yash-rs#329
File: yash/tests/scripted_test/alias-p.sh:59-66
Timestamp: 2023-12-17T02:11:08.076Z
Learning: The `test_OE` specifier in the test harness for the Yash shell automatically checks that the command prints nothing to the standard output or standard error, indicating that no aliases remain after using `unalias -a`.
Additional comments not posted (11)
yash-env/CHANGELOG.md (1)

12-13: Ensure the changelog entry for RealSystem::open_tmpfile is clear about the implications of not setting the O_CLOEXEC flag.

Consider adding a brief explanation of why the O_CLOEXEC flag is no longer set by default, as this could impact the security or behavior of file descriptors in a multi-threaded environment.

yash-syntax/CHANGELOG.md (2)

12-14: Clarify the changelog entry for Lexer::source_string.

Consider expanding on why the argument type was changed to Range<usize>. This could help users understand the benefits or reasons behind this change, such as performance improvements or API simplifications.


18-24: Validate the fixes related to newline parsing and alias substitution.

Ensure that the fixes for newline parsing after a bar in a pipeline and alias substitution behavior are covered by unit tests and function as expected.

yash-syntax/src/parser/pipeline.rs (2)

42-54: The changes to handle the ! reserved word and subsequent command parsing are correctly implemented.


75-91: The refactoring for parsing multiple commands separated by | and the associated error handling are well implemented.

yash-env/src/system/real.rs (1)

219-226: The refactoring of the open_tmpfile function using map_err for streamlined error handling and the manipulation of the file descriptor are correctly implemented. Consider adding documentation about the implications of clearing the CLOEXEC flag, especially in multi-threaded environments.

yash-syntax/src/parser/lex/core.rs (5)

139-144: Introduction of SourceCharEx struct is well-implemented and serves to enhance handling of line continuations.


146-151: The ex function is correctly implemented to facilitate the conversion from SourceChar to SourceCharEx.


158-158: Modification of the source vector to use SourceCharEx integrates well with the lexical analysis logic.


345-360: Consider implementing more graceful error handling for the mark_line_continuation method in production builds, as assertions may not be suitable.


342-369: > 📝 NOTE

This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [363-383]

Updates to the substitute_alias method correctly integrate the SourceCharEx struct, enhancing alias handling.


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger a review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

The previous implementation of the LexerCore::peek_char method naively
used the current index to calculate the start index of the location at
the current position. This approach was incorrect because the index
could be changed by an alias substitution that changed the length of the
source code. This commit fixes the issue by introducing a new method,
LexerCore::next_index, that computes the index from the last character
of the source code.

Fixes #280
@magicant magicant added this to the POSIX 2018 scripting milestone Apr 19, 2024
All `System::open_tmpfile` implementations should consistently return a
file descriptor without the `O_CLOEXEC` flag set. Otherwise, it would
break callers that expect the file descriptor to be visible to the user.

Previously, the 'command exec retains redirection' test was failing
because the file descriptor to the temporary file containing the here-
document content was not visible to the command.
@magicant magicant marked this pull request as ready for review April 20, 2024 16:15
Copy link

@coderabbitai coderabbitai bot left a 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

Actionable comments outside the diff hunks (4)
yash-cli/tests/scripted_test/command-p.sh (2)

Line range hint 79-79: Check the syntax in the subshell.

There seems to be a syntax error in the explicit subshell. Please review the syntax to ensure it is correct and does not prevent further checks.


Line range hint 85-85: Correct the syntax error with the ! command.

- command -v !
+ command -v \!

Ensure there is a space after the ! to avoid syntax errors and to ensure the command is interpreted correctly.

yash-cli/tests/scripted_test/alias-p.sh (2)

Line range hint 10-10: Check the syntax in the explicit subshell.

There appears to be a syntax error in the explicit subshell. Please review the syntax to ensure it is correct and does not prevent further checks.


Line range hint 84-84: Correct the syntax error with the { and } characters.

- alias b=' cat | c - ; a ' c=' cat '
+ alias b=' cat | c - ; a ' c=' cat; '

Ensure that the { and } characters are used correctly in the context of shell scripting to avoid syntax errors and to ensure the command is interpreted correctly.

@magicant magicant merged commit 3662079 into master Apr 21, 2024
6 checks passed
@magicant magicant deleted the fixes branch April 21, 2024 03:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

1 participant