9 Commits

Author	SHA1	Message	Date
Piepmatz	66bc0542e0	Refactor I/O Errors (#14927) ... <!-- if this PR closes one or more issues, you can automatically link the PR with them by using one of the [*linking keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword), e.g. - this PR should close #xxxx - fixes #xxxx you can also mention related issues, PRs or discussions! --> # Description <!-- Thank you for improving Nushell. Please, check our [contributing guide](../CONTRIBUTING.md) and talk to the core team before making major changes. Description of your pull request goes here. **Provide examples and/or screenshots** if your changes affect the user experience. --> As mentioned in #10698, we have too many `ShellError` variants, with some even overlapping in meaning. This PR simplifies and improves I/O error handling by restructuring `ShellError` related to I/O issues. Previously, `ShellError::IOError` only contained a message string, making it convenient but overly generic. It was widely used without providing spans (#4323). This PR introduces a new `ShellError::Io` variant that consolidates multiple I/O-related errors (except for `ShellError::NetworkFailure`, which remains distinct for now). The new `ShellError::Io` variant replaces the following: - `FileNotFound` - `FileNotFoundCustom` - `IOInterrupted` - `IOError` - `IOErrorSpanned` - `NotADirectory` - `DirectoryNotFound` - `MoveNotPossible` - `CreateNotPossible` - `ChangeAccessTimeNotPossible` - `ChangeModifiedTimeNotPossible` - `RemoveNotPossible` - `ReadingFile` ## The `IoError` `IoError` includes the following fields: 1. **`kind`**: Extends `std::io::ErrorKind` to specify the type of I/O error without needing new `ShellError` variants. This aligns with the approach used in `std::io::Error`. This adds a second dimension to error reporting by combining the `kind` field with `ShellError` variants, making it easier to describe errors in more detail. As proposed by @kubouch in [#design-discussion on Discord](https://discord.com/channels/601130461678272522/615329862395101194/1323699197165178930), this helps reduce the number of `ShellError` variants. In the error report, the `kind` field is displayed as the "source" of the error, e.g., "I/O error," followed by the specific kind of I/O error. 2. **`span`**: A non-optional field to encourage providing spans for better error reporting (#4323). 3. **`path`**: Optional `PathBuf` to give context about the file or directory involved in the error (#7695). If provided, it’s shown as a help entry in error reports. 4. **`additional_context`**: Allows adding custom messages when the span, kind, and path are insufficient. This is rendered in the error report at the labeled span. 5. **`location`**: Sometimes, I/O errors occur in the engine itself and are not caused directly by user input. In such cases, if we don’t have a span and must set it to `Span::unknown()`, we need another way to reference the error. For this, the `location` field uses the new `Location` struct, which records the Rust file and line number where the error occurred. This ensures that we at least know the Rust code location that failed, helping with debugging. To make this work, a new `location!` macro was added, which retrieves `file!`, `line!`, and `column!` values accurately. If `Location::new` is used directly, it issues a warning to remind developers to use the macro instead, ensuring consistent and correct usage. ### Constructor Behavior `IoError` provides five constructor methods: - `new` and `new_with_additional_context`: Used for errors caused by user input and require a valid (non-unknown) span to ensure precise error reporting. - `new_internal` and `new_internal_with_path`: Used for internal errors where a span is not available. These methods require additional context and the `Location` struct to pinpoint the source of the error in the engine code. - `factory`: Returns a closure that maps an `std::io::Error` to an `IoError`. This is useful for handling multiple I/O errors that share the same span and path, streamlining error handling in such cases. ## New Report Look This is simulation how the I/O errors look like (the `open crates` is simulated to show how internal errors are referenced now):  ## `Span::test_data()` To enable better testing, `Span::test_data()` now returns a value distinct from `Span::unknown()`. Both `Span::test_data()` and `Span::unknown()` refer to invalid source code, but having a separate value for test data helps identify issues during testing while keeping spans unique. ## Cursed Sneaky Error Transfers I removed the conversions between `std::io::Error` and `ShellError` as they often removed important information and were used too broadly to handle I/O errors. This also removed the problematic implementation found here: 7ea4895513/crates/nu-protocol/src/errors/shell_error.rs (L1534-L1583) which hid some downcasting from I/O errors and made it hard to trace where `ShellError` was converted into `std::io::Error`. To address this, I introduced a new struct called `ShellErrorBridge`, which explicitly defines this transfer behavior. With `ShellErrorBridge`, we can now easily grep the codebase to locate and manage such conversions. ## Miscellaneous - Removed the OS error added in #14640, as it’s no longer needed. - Improved error messages in `glob_from` (#14679). - Trying to open a directory with `open` caused a permissions denied error (it's just what the OS provides). I added a `is_dir` check to provide a better error in that case. # User-Facing Changes <!-- List of all changes that impact the user experience here. This helps us keep track of breaking changes. --> - Error outputs now include more detailed information and are formatted differently, including updated error codes. - The structure of `ShellError` has changed, requiring plugin authors and embedders to update their implementations. # Tests + Formatting <!-- Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass (on Windows make sure to [enable developer mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging)) - `cargo run -- -c "use toolkit.nu; toolkit test stdlib"` to run the tests for the standard library > **Note** > from `nushell` you can also use the `toolkit` as follows > ```bash > use toolkit.nu # or use an `env_change` hook to activate it automatically > toolkit check pr > ``` --> I updated tests to account for the new I/O error structure and formatting changes. # After Submitting <!-- If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. --> This PR closes #7695 and closes #14892 and partially addresses #4323 and #10698. --------- Co-authored-by: Darren Schroeder <343840+fdncred@users.noreply.github.com>	2025-01-28 16:03:31 -06:00
Stefan Holderbach	95b78eee25	Change the usage misnomer to "description" (#13598) ... # Description The meaning of the word usage is specific to describing how a command function is *used* and not a synonym for general description. Usage can be used to describe the SYNOPSIS or EXAMPLES sections of a man page where the permitted argument combinations are shown or example *uses* are given. Let's not confuse people and call it what it is a description. Our `help` command already creates its own *Usage* section based on the available arguments and doesn't refer to the description with usage. # User-Facing Changes `help commands` and `scope commands` will now use `description` or `extra_description` `usage`-> `description` `extra_usage` -> `extra_description` Breaking change in the plugin protocol: In the signature record communicated with the engine. `usage`-> `description` `extra_usage` -> `extra_description` The same rename also takes place for the methods on `SimplePluginCommand` and `PluginCommand` # Tests + Formatting - Updated plugin protocol specific changes # After Submitting - [ ] update plugin protocol doc	2024-08-22 12:02:08 +02:00
YizhePKU	bdb6daa4b5	Migrate to a new PWD API (#12603) ... This is the first PR towards migrating to a new `$env.PWD` API that returns potentially un-canonicalized paths. Refer to PR #12515 for motivations. ## New API: `EngineState::cwd()` The goal of the new API is to cover both parse-time and runtime use case, and avoid unintentional misuse. It takes an `Option<Stack>` as argument, which if supplied, will search for `$env.PWD` on the stack in additional to the engine state. I think with this design, there's less confusion over parse-time and runtime environments. If you have access to a stack, just supply it; otherwise supply `None`. ## Deprecation of other PWD-related APIs Other APIs are re-implemented using `EngineState::cwd()` and properly documented. They're marked deprecated, but their behavior is unchanged. Unused APIs are deleted, and code that accesses `$env.PWD` directly without using an API is rewritten. Deprecated APIs: * `EngineState::current_work_dir()` * `StateWorkingSet::get_cwd()` * `env::current_dir()` * `env::current_dir_str()` * `env::current_dir_const()` * `env::current_dir_str_const()` Other changes: * `EngineState::get_cwd()` (deleted) * `StateWorkingSet::list_env()` (deleted) * `repl::do_run_cmd()` (rewritten with `env::current_dir_str()`) ## `cd` and `pwd` now use logical paths by default This pulls the changes from PR #12515. It's currently somewhat broken because using non-canonicalized paths exposed a bug in our path normalization logic (Issue #12602). Once that is fixed, this should work. ## Future plans This PR needs some tests. Which test helpers should I use, and where should I put those tests? I noticed that unquoted paths are expanded within `eval_filepath()` and `eval_directory()` before they even reach the `cd` command. This means every paths is expanded twice. Is this intended? Once this PR lands, the plan is to review all usages of the deprecated APIs and migrate them to `EngineState::cwd()`. In the meantime, these usages are annotated with `#[allow(deprecated)]` to avoid breaking CI. --------- Co-authored-by: Jakub Žádník <kubouch@gmail.com>	2024-05-03 14:33:09 +03:00
Ian Manske	c747ec75c9	Add command_prelude module (#12291) ... # Description When implementing a `Command`, one must also import all the types present in the function signatures for `Command`. This makes it so that we often import the same set of types in each command implementation file. E.g., something like this: ```rust use nu_protocol::ast::Call; use nu_protocol::engine::{Command, EngineState, Stack}; use nu_protocol::{ record, Category, Example, IntoInterruptiblePipelineData, IntoPipelineData, PipelineData, ShellError, Signature, Span, Type, Value, }; ``` This PR adds the `nu_engine::command_prelude` module which contains the necessary and commonly used types to implement a `Command`: ```rust // command_prelude.rs pub use crate::CallExt; pub use nu_protocol::{ ast::{Call, CellPath}, engine::{Command, EngineState, Stack}, record, Category, Example, IntoInterruptiblePipelineData, IntoPipelineData, IntoSpanned, PipelineData, Record, ShellError, Signature, Span, Spanned, SyntaxShape, Type, Value, }; ``` This should reduce the boilerplate needed to implement a command and also gives us a place to track the breadth of the `Command` API. I tried to be conservative with what went into the prelude modules, since it might be hard/annoying to remove items from the prelude in the future. Let me know if something should be included or excluded.	2024-03-26 21:17:30 +00:00
Darren Schroeder	2ad68e3702	cleanup coreutils tagging (#12286) ... # Description Cleanup search terms and help usage to be consistent and include coreutils so people can easily find out which commands are coreutils.  or ```nushell help commands | where usage =~ coreutils | reject params input_output ``` # User-Facing Changes <!-- List of all changes that impact the user experience here. This helps us keep track of breaking changes. --> # Tests + Formatting <!-- Don't forget to add tests that cover your changes. Make sure you've run and fixed any issues with these commands: - `cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to check that you're using the standard code style - `cargo test --workspace` to check that all tests pass (on Windows make sure to [enable developer mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging)) - `cargo run -- -c "use std testing; testing run-tests --path crates/nu-std"` to run the tests for the standard library > **Note** > from `nushell` you can also use the `toolkit` as follows > ```bash > use toolkit.nu # or use an `env_change` hook to activate it automatically > toolkit check pr > ``` --> # After Submitting <!-- If your PR had any user-facing changes, update [the documentation](https://github.com/nushell/nushell.github.io) after the PR is merged, if necessary. This will help us keep the docs up to date. -->	2024-03-25 17:13:46 -05:00
Artemiy	1867bb1a88	Fix incorrect handling of boolean flags for builtin commands (#11492) ... # Description Possible fix of #11456 This PR fixes a bug where builtin commands did not respect the logic of dynamically passed boolean flags. The reason is [has_flag](6f59abaf43/crates/nu-protocol/src/ast/call.rs (L204C5-L212C6)) method did not evaluate and take into consideration expression used with flag. To address this issue a solution is proposed: 1. `has_flag` method is moved to `CallExt` and new logic to evaluate expression and check if it is a boolean value is added 2. `has_flag_const` method is added to `CallExt` which is a constant version of `has_flag` 3. `has_named` method is added to `Call` which is basically the old logic of `has_flag` 4. All usages of `has_flag` in code are updated, mostly to pass `engine_state` and `stack` to new `has_flag`. In `run_const` commands it is replaced with `has_flag_const`. And in a few select places: parser, `to nuon` and `into string` old logic via `has_named` is used. # User-Facing Changes Explicit values of boolean flags are now respected in builtin commands. Before:  After:  Another example: Before:  After:  # Tests + Formatting Added test reproducing some variants of original issue.	2024-01-11 17:19:48 +02:00
Eric Hodel	a95a4505ef	Convert Shellerror::GenericError to named fields (#11230) ... # Description Replace `.to_string()` used in `GenericError` with `.into()` as `.into()` seems more popular Replace `Vec::new()` used in `GenericError` with `vec![]` as `vec![]` seems more popular (There are so, so many)	2023-12-07 00:40:03 +01:00
Eric Hodel	8386bc0919	Convert more ShellError variants to named fields (#11173) ... # Description Convert these ShellError variants to named fields: * CreateNotPossible * MoveNotPossibleSingle * DirectoryNotFoundCustom * DirectoryNotFound * NotADirectory * OutOfMemoryError * PermissionDeniedError * IOErrorSpanned * IOError * IOInterrupted Also place the `span` field of `DirectoryNotFound` last to match other errors. Part of #10700 (almost half done!) # User-Facing Changes None # Tests + Formatting - 🟢 `toolkit fmt` - 🟢 `toolkit clippy` - 🟢 `toolkit test` - 🟢 `toolkit test stdlib` # After Submitting N/A	2023-11-28 06:43:51 -06:00
Taylor	494a5a5286	Add mktemp command (#11005) ... closes #10845 I've opened this a little prematurely to get some questions answered before I cleanup the code. As I started trying to better understand GNUs `mktemp` I've realized its kind of peculiar and we might want to change its behavior to introduce it to nushell. #### quiet and dry run Does it make sense to keep the `quiet` and `dry_run` flags? I don't think so. The GNU documentation says this about the dry run flag "Using the output of this command to create a new file is inherently unsafe, as there is a window of time between generating the name and using it where another process can create an object by the same name." So yeah why keep it? As far as quiet goes, does it make sense to silence the errors in nushell? #### other confusing flags According to the [gnu docs](https://www.gnu.org/software/coreutils/manual/html_node/mktemp-invocation.html), the `-t` flag is deprecated and the `-p`/ `--tempdir` are the same flag with the only difference being `--tempdir` takes an optional path, Given that, I've broken the `-p` away from `--tempdir`. Now there is one switch `--tmpdir`/`-t` and one named param `--tmpdir-path`/`-p`. GNU mktemp ``` -p DIR, --tmpdir[=DIR] interpret TEMPLATE relative to DIR; if DIR is not specified, use $TMPDIR if set, else /tmp. With this option, TEMPLATE must not be an absolute name; unlike with -t, TEMPLATE may contain slashes, but mktemp creates only the final component -t interpret TEMPLATE as a single file name component, relative to a directory: $TMPDIR, if set; else the directory specified via -p; else /tmp [deprecated] ``` to nushell mktemp ``` -p, --tmpdir-path <Filepath> # named param, must provide a path -t, --tmpdir # a switch ``` Is this a terrible idea? What should I do? --------- Co-authored-by: Darren Schroeder <343840+fdncred@users.noreply.github.com>	2023-11-17 19:30:53 -06:00