Refactor I/O Errors (nushell#14927)

# Description  As mentioned in nushell#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 (nushell#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 (nushell#4323). 3. **`path`**: Optional `PathBuf` to give context about the file or directory involved in the error (nushell#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): ![Screenshot 2025-01-25 190426](https://github.com/user-attachments/assets/a41b6aa6-a440-497d-bbcc-3ac0121c9226) ## `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: https://github.com/nushell/nushell/blob/7ea489551315a5ab68fd1e9e5a7ed5a0b3ef494a/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 nushell#14640, as it’s no longer needed. - Improved error messages in `glob_from` (nushell#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  - 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  I updated tests to account for the new I/O error structure and formatting changes. # After Submitting  This PR closes nushell#7695 and closes nushell#14892 and partially addresses nushell#4323 and nushell#10698. --------- Co-authored-by: Darren Schroeder <343840+fdncred@users.noreply.github.com>
WindSoilder · Jan 28, 2025 · 66bc054 · 66bc054
1 parent ec1f7de
commit 66bc054
Show file tree

Hide file tree

Showing 105 changed files with 1,944 additions and 1,052 deletions.
diff --git a/crates/nu-cli/src/commands/history/history_.rs b/crates/nu-cli/src/commands/history/history_.rs
@@ -1,5 +1,5 @@
 use nu_engine::command_prelude::*;
-use nu_protocol::HistoryFileFormat;
+use nu_protocol::{shell_error::io::IoError, HistoryFileFormat};
 use reedline::{
     FileBackedHistory, History as ReedlineHistory, HistoryItem, SearchDirection, SearchQuery,
     SqliteBackedHistory,
@@ -93,10 +93,11 @@ impl Command for History {
                         )
                     })
                 })
-                .ok_or(ShellError::FileNotFound {
-                    file: history_path.display().to_string(),
-                    span: head,
-                })?
+                .ok_or(IoError::new(
+                    std::io::ErrorKind::NotFound,
+                    head,
+                    history_path,
+                ))?
                 .into_pipeline_data(head, signals)),
             HistoryFileFormat::Sqlite => Ok(history_reader
                 .and_then(|h| {
@@ -109,10 +110,11 @@ impl Command for History {
                         .enumerate()
                         .map(move |(idx, entry)| create_history_record(idx, entry, long, head))
                 })
-                .ok_or(ShellError::FileNotFound {
-                    file: history_path.display().to_string(),
-                    span: head,
-                })?
+                .ok_or(IoError::new(
+                    std::io::ErrorKind::NotFound,
+                    head,
+                    history_path,
+                ))?
                 .into_pipeline_data(head, signals)),
         }
     }

diff --git a/crates/nu-cli/src/commands/history/history_import.rs b/crates/nu-cli/src/commands/history/history_import.rs
@@ -1,7 +1,10 @@
 use std::path::{Path, PathBuf};
 
 use nu_engine::command_prelude::*;
-use nu_protocol::HistoryFileFormat;
+use nu_protocol::{
+    shell_error::{self, io::IoError},
+    HistoryFileFormat,
+};
 
 use reedline::{
     FileBackedHistory, History, HistoryItem, ReedlineError, SearchQuery, SqliteBackedHistory,
@@ -69,17 +72,16 @@ Note that history item IDs are ignored when importing from file."#
         call: &Call,
         input: PipelineData,
     ) -> Result<PipelineData, ShellError> {
+        let span = call.head;
         let ok = Ok(Value::nothing(call.head).into_pipeline_data());
 
         let Some(history) = engine_state.history_config() else {
             return ok;
         };
         let Some(current_history_path) = history.file_path() else {
-            return Err(ShellError::ConfigDirNotFound {
-                span: Some(call.head),
-            });
+            return Err(ShellError::ConfigDirNotFound { span: span.into() });
         };
-        if let Some(bak_path) = backup(&current_history_path)? {
+        if let Some(bak_path) = backup(&current_history_path, span)? {
             println!("Backed history to {}", bak_path.display());
         }
         match input {
@@ -216,7 +218,7 @@ fn item_from_record(mut rec: Record, span: Span) -> Result<HistoryItem, ShellErr
         hostname: get(rec, fields::HOSTNAME, |v| Ok(v.as_str()?.to_owned()))?,
         cwd: get(rec, fields::CWD, |v| Ok(v.as_str()?.to_owned()))?,
         exit_status: get(rec, fields::EXIT_STATUS, |v| v.as_int())?,
-        duration: get(rec, fields::DURATION, duration_from_value)?,
+        duration: get(rec, fields::DURATION, |v| duration_from_value(v, span))?,
         more_info: None,
         // TODO: Currently reedline doesn't let you create session IDs.
         session_id: None,
@@ -232,19 +234,21 @@ fn item_from_record(mut rec: Record, span: Span) -> Result<HistoryItem, ShellErr
     Ok(item)
 }
 
-fn duration_from_value(v: Value) -> Result<std::time::Duration, ShellError> {
+fn duration_from_value(v: Value, span: Span) -> Result<std::time::Duration, ShellError> {
     chrono::Duration::nanoseconds(v.as_duration()?)
         .to_std()
-        .map_err(|_| ShellError::IOError {
-            msg: "negative duration not supported".to_string(),
-        })
+        .map_err(|_| ShellError::NeedsPositiveValue { span })
 }
 
-fn find_backup_path(path: &Path) -> Result<PathBuf, ShellError> {
+fn find_backup_path(path: &Path, span: Span) -> Result<PathBuf, ShellError> {
     let Ok(mut bak_path) = path.to_path_buf().into_os_string().into_string() else {
         // This isn't fundamentally problem, but trying to work with OsString is a nightmare.
-        return Err(ShellError::IOError {
-            msg: "History path mush be representable as UTF-8".to_string(),
+        return Err(ShellError::GenericError {
+            error: "History path not UTF-8".to_string(),
+            msg: "History path must be representable as UTF-8".to_string(),
+            span: Some(span),
+            help: None,
+            inner: vec![],
         });
     };
     bak_path.push_str(".bak");
@@ -260,24 +264,45 @@ fn find_backup_path(path: &Path) -> Result<PathBuf, ShellError> {
             return Ok(PathBuf::from(bak_path));
         }
     }
-    Err(ShellError::IOError {
-        msg: "Too many existing backup files".to_string(),
+    Err(ShellError::GenericError {
+        error: "Too many backup files".to_string(),
+        msg: "Found too many existing backup files".to_string(),
+        span: Some(span),
+        help: None,
+        inner: vec![],
     })
 }
 
-fn backup(path: &Path) -> Result<Option<PathBuf>, ShellError> {
+fn backup(path: &Path, span: Span) -> Result<Option<PathBuf>, ShellError> {
     match path.metadata() {
         Ok(md) if md.is_file() => (),
         Ok(_) => {
-            return Err(ShellError::IOError {
-                msg: "history path exists but is not a file".to_string(),
-            })
+            return Err(IoError::new_with_additional_context(
+                shell_error::io::ErrorKind::NotAFile,
+                span,
+                PathBuf::from(path),
+                "history path exists but is not a file",
+            )
+            .into())
         }
         Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(None),
-        Err(e) => return Err(e.into()),
+        Err(e) => {
+            return Err(IoError::new_internal(
+                e.kind(),
+                "Could not get metadata",
+                nu_protocol::location!(),
+            )
+            .into())
+        }
     }
-    let bak_path = find_backup_path(path)?;
-    std::fs::copy(path, &bak_path)?;
+    let bak_path = find_backup_path(path, span)?;
+    std::fs::copy(path, &bak_path).map_err(|err| {
+        IoError::new_internal(
+            err.kind(),
+            "Could not copy backup",
+            nu_protocol::location!(),
+        )
+    })?;
     Ok(Some(bak_path))
 }
 
@@ -388,7 +413,7 @@ mod tests {
         for name in existing {
             std::fs::File::create_new(dir.path().join(name)).unwrap();
         }
-        let got = find_backup_path(&dir.path().join("history.dat")).unwrap();
+        let got = find_backup_path(&dir.path().join("history.dat"), Span::test_data()).unwrap();
         assert_eq!(got, dir.path().join(want))
     }
 
@@ -400,7 +425,7 @@ mod tests {
         write!(&mut history, "123").unwrap();
         let want_bak_path = dir.path().join("history.dat.bak");
         assert_eq!(
-            backup(&dir.path().join("history.dat")),
+            backup(&dir.path().join("history.dat"), Span::test_data()),
             Ok(Some(want_bak_path.clone()))
         );
         let got_data = String::from_utf8(std::fs::read(want_bak_path).unwrap()).unwrap();
@@ -410,7 +435,7 @@ mod tests {
     #[test]
     fn test_backup_no_file() {
         let dir = tempfile::tempdir().unwrap();
-        let bak_path = backup(&dir.path().join("history.dat")).unwrap();
+        let bak_path = backup(&dir.path().join("history.dat"), Span::test_data()).unwrap();
         assert!(bak_path.is_none());
     }
 }
diff --git a/crates/nu-cli/src/commands/keybindings_listen.rs b/crates/nu-cli/src/commands/keybindings_listen.rs
@@ -2,6 +2,7 @@ use crossterm::{
     event::Event, event::KeyCode, event::KeyEvent, execute, terminal, QueueableCommand,
 };
 use nu_engine::command_prelude::*;
+use nu_protocol::shell_error::io::IoError;
 use std::io::{stdout, Write};
 
 #[derive(Clone)]
@@ -39,7 +40,13 @@ impl Command for KeybindingsListen {
         match print_events(engine_state) {
             Ok(v) => Ok(v.into_pipeline_data()),
             Err(e) => {
-                terminal::disable_raw_mode()?;
+                terminal::disable_raw_mode().map_err(|err| {
+                    IoError::new_internal(
+                        err.kind(),
+                        "Could not disable raw mode",
+                        nu_protocol::location!(),
+                    )
+                })?;
                 Err(ShellError::GenericError {
                     error: "Error with input".into(),
                     msg: "".into(),
@@ -63,8 +70,20 @@ impl Command for KeybindingsListen {
 pub fn print_events(engine_state: &EngineState) -> Result<Value, ShellError> {
     let config = engine_state.get_config();
 
-    stdout().flush()?;
-    terminal::enable_raw_mode()?;
+    stdout().flush().map_err(|err| {
+        IoError::new_internal(
+            err.kind(),
+            "Could not flush stdout",
+            nu_protocol::location!(),
+        )
+    })?;
+    terminal::enable_raw_mode().map_err(|err| {
+        IoError::new_internal(
+            err.kind(),
+            "Could not enable raw mode",
+            nu_protocol::location!(),
+        )
+    })?;
 
     if config.use_kitty_protocol {
         if let Ok(false) = crossterm::terminal::supports_keyboard_enhancement() {
@@ -94,7 +113,9 @@ pub fn print_events(engine_state: &EngineState) -> Result<Value, ShellError> {
     let mut stdout = std::io::BufWriter::new(std::io::stderr());
 
     loop {
-        let event = crossterm::event::read()?;
+        let event = crossterm::event::read().map_err(|err| {
+            IoError::new_internal(err.kind(), "Could not read event", nu_protocol::location!())
+        })?;
         if event == Event::Key(KeyCode::Esc.into()) {
             break;
         }
@@ -113,9 +134,25 @@ pub fn print_events(engine_state: &EngineState) -> Result<Value, ShellError> {
 
             _ => "".to_string(),
         };
-        stdout.queue(crossterm::style::Print(o))?;
-        stdout.queue(crossterm::style::Print("\r\n"))?;
-        stdout.flush()?;
+        stdout.queue(crossterm::style::Print(o)).map_err(|err| {
+            IoError::new_internal(
+                err.kind(),
+                "Could not print output record",
+                nu_protocol::location!(),
+            )
+        })?;
+        stdout
+            .queue(crossterm::style::Print("\r\n"))
+            .map_err(|err| {
+                IoError::new_internal(
+                    err.kind(),
+                    "Could not print linebreak",
+                    nu_protocol::location!(),
+                )
+            })?;
+        stdout.flush().map_err(|err| {
+            IoError::new_internal(err.kind(), "Could not flush", nu_protocol::location!())
+        })?;
     }
 
     if config.use_kitty_protocol {
@@ -125,7 +162,13 @@ pub fn print_events(engine_state: &EngineState) -> Result<Value, ShellError> {
         );
     }
 
-    terminal::disable_raw_mode()?;
+    terminal::disable_raw_mode().map_err(|err| {
+        IoError::new_internal(
+            err.kind(),
+            "Could not disable raw mode",
+            nu_protocol::location!(),
+        )
+    })?;
 
     Ok(Value::nothing(Span::unknown()))
 }

diff --git a/crates/nu-cli/src/config_files.rs b/crates/nu-cli/src/config_files.rs
@@ -18,7 +18,7 @@ const OLD_PLUGIN_FILE: &str = "plugin.nu";
 
 #[cfg(feature = "plugin")]
 pub fn read_plugin_file(engine_state: &mut EngineState, plugin_file: Option<Spanned<String>>) {
-    use nu_protocol::ShellError;
+    use nu_protocol::{shell_error::io::IoError, ShellError};
     use std::path::Path;
 
     let span = plugin_file.as_ref().map(|s| s.span);
@@ -78,16 +78,12 @@ pub fn read_plugin_file(engine_state: &mut EngineState, plugin_file: Option<Span
                 } else {
                     report_shell_error(
                         engine_state,
-                        &ShellError::GenericError {
-                            error: format!(
-                                "Error while opening plugin registry file: {}",
-                                plugin_path.display()
-                            ),
-                            msg: "plugin path defined here".into(),
-                            span,
-                            help: None,
-                            inner: vec![err.into()],
-                        },
+                        &ShellError::Io(IoError::new_internal_with_path(
+                            err.kind(),
+                            "Could not open plugin registry file",
+                            nu_protocol::location!(),
+                            plugin_path,
+                        )),
                     );
                     return;
                 }
@@ -234,8 +230,8 @@ pub fn eval_config_contents(
 #[cfg(feature = "plugin")]
 pub fn migrate_old_plugin_file(engine_state: &EngineState) -> bool {
     use nu_protocol::{
-        PluginExample, PluginIdentity, PluginRegistryItem, PluginRegistryItemData, PluginSignature,
-        ShellError,
+        shell_error::io::IoError, PluginExample, PluginIdentity, PluginRegistryItem,
+        PluginRegistryItemData, PluginSignature, ShellError,
     };
     use std::collections::BTreeMap;
 
@@ -324,7 +320,15 @@ pub fn migrate_old_plugin_file(engine_state: &EngineState) -> bool {
     // Write the new file
     let new_plugin_file_path = config_dir.join(PLUGIN_FILE);
     if let Err(err) = std::fs::File::create(&new_plugin_file_path)
-        .map_err(|e| e.into())
+        .map_err(|err| {
+            IoError::new_internal_with_path(
+                err.kind(),
+                "Could not create new plugin file",
+                nu_protocol::location!(),
+                new_plugin_file_path.clone(),
+            )
+        })
+        .map_err(ShellError::from)
         .and_then(|file| contents.write_to(file, None))
     {
         report_shell_error(