More precise ErrorKind::NotFound errors (#15149)

In this PR, the two new variants for `ErrorKind`, `FileNotFound`
and `DirectoryNotFound` with a nice `not_found_as` method for the
`ErrorKind` to easily specify the `NotFound` errors. I also updated some
places where I could of think of with these new variants and the message
for `NotFound` is no longer "Entity not found" but "Not found" to be
less strange.

closes #15142
closes #15055
This commit is contained in:
Piepmatz 2025-02-22 17:42:44 +01:00 committed by GitHub
parent 1d44843970
commit bda3245725
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
13 changed files with 139 additions and 36 deletions

View File

@ -8,7 +8,7 @@ use nu_protocol::{
debugger::WithoutDebug, debugger::WithoutDebug,
engine::{EngineState, Stack, StateWorkingSet}, engine::{EngineState, Stack, StateWorkingSet},
report_parse_error, report_parse_warning, report_parse_error, report_parse_warning,
shell_error::io::IoError, shell_error::io::*,
PipelineData, ShellError, Span, Value, PipelineData, ShellError, Span, Value,
}; };
use std::{path::PathBuf, sync::Arc}; use std::{path::PathBuf, sync::Arc};
@ -28,7 +28,7 @@ pub fn evaluate_file(
let file_path = canonicalize_with(&path, cwd).map_err(|err| { let file_path = canonicalize_with(&path, cwd).map_err(|err| {
IoError::new_internal_with_path( IoError::new_internal_with_path(
err.kind(), err.kind().not_found_as(NotFound::File),
"Could not access file", "Could not access file",
nu_protocol::location!(), nu_protocol::location!(),
PathBuf::from(&path), PathBuf::from(&path),
@ -47,7 +47,7 @@ pub fn evaluate_file(
let file = std::fs::read(&file_path).map_err(|err| { let file = std::fs::read(&file_path).map_err(|err| {
IoError::new_internal_with_path( IoError::new_internal_with_path(
err.kind(), err.kind().not_found_as(NotFound::File),
"Could not read file", "Could not read file",
nu_protocol::location!(), nu_protocol::location!(),
file_path.clone(), file_path.clone(),
@ -57,7 +57,7 @@ pub fn evaluate_file(
let parent = file_path.parent().ok_or_else(|| { let parent = file_path.parent().ok_or_else(|| {
IoError::new_internal_with_path( IoError::new_internal_with_path(
std::io::ErrorKind::NotFound, ErrorKind::DirectoryNotFound,
"The file path does not have a parent", "The file path does not have a parent",
nu_protocol::location!(), nu_protocol::location!(),
file_path.clone(), file_path.clone(),

View File

@ -1,7 +1,5 @@
use chrono::Local; use chrono::Local;
use nu_engine::command_prelude::*; use nu_engine::command_prelude::*;
use nu_protocol::shell_error::io::IoError;
use nu_utils::{get_scaffold_config, get_scaffold_env}; use nu_utils::{get_scaffold_config, get_scaffold_env};
use std::{io::Write, path::PathBuf}; use std::{io::Write, path::PathBuf};
@ -61,7 +59,7 @@ impl Command for ConfigReset {
)); ));
if let Err(err) = std::fs::rename(nu_config.clone(), &backup_path) { if let Err(err) = std::fs::rename(nu_config.clone(), &backup_path) {
return Err(ShellError::Io(IoError::new_with_additional_context( return Err(ShellError::Io(IoError::new_with_additional_context(
err.kind(), err.kind().not_found_as(NotFound::Directory),
span, span,
PathBuf::from(backup_path), PathBuf::from(backup_path),
"config.nu could not be backed up", "config.nu could not be backed up",
@ -71,7 +69,7 @@ impl Command for ConfigReset {
if let Ok(mut file) = std::fs::File::create(&nu_config) { if let Ok(mut file) = std::fs::File::create(&nu_config) {
if let Err(err) = writeln!(&mut file, "{config_file}") { if let Err(err) = writeln!(&mut file, "{config_file}") {
return Err(ShellError::Io(IoError::new_with_additional_context( return Err(ShellError::Io(IoError::new_with_additional_context(
err.kind(), err.kind().not_found_as(NotFound::File),
span, span,
PathBuf::from(nu_config), PathBuf::from(nu_config),
"config.nu could not be written to", "config.nu could not be written to",
@ -88,7 +86,7 @@ impl Command for ConfigReset {
backup_path.push(format!("oldenv-{}.nu", Local::now().format("%F-%H-%M-%S"),)); backup_path.push(format!("oldenv-{}.nu", Local::now().format("%F-%H-%M-%S"),));
if let Err(err) = std::fs::rename(env_config.clone(), &backup_path) { if let Err(err) = std::fs::rename(env_config.clone(), &backup_path) {
return Err(ShellError::Io(IoError::new_with_additional_context( return Err(ShellError::Io(IoError::new_with_additional_context(
err.kind(), err.kind().not_found_as(NotFound::Directory),
span, span,
PathBuf::from(backup_path), PathBuf::from(backup_path),
"env.nu could not be backed up", "env.nu could not be backed up",
@ -98,7 +96,7 @@ impl Command for ConfigReset {
if let Ok(mut file) = std::fs::File::create(&env_config) { if let Ok(mut file) = std::fs::File::create(&env_config) {
if let Err(err) = writeln!(&mut file, "{config_file}") { if let Err(err) = writeln!(&mut file, "{config_file}") {
return Err(ShellError::Io(IoError::new_with_additional_context( return Err(ShellError::Io(IoError::new_with_additional_context(
err.kind(), err.kind().not_found_as(NotFound::File),
span, span,
PathBuf::from(env_config), PathBuf::from(env_config),
"env.nu could not be written to", "env.nu could not be written to",

View File

@ -88,7 +88,7 @@ impl Command for Cd {
path path
} else { } else {
return Err(shell_error::io::IoError::new( return Err(shell_error::io::IoError::new(
std::io::ErrorKind::NotFound, ErrorKind::DirectoryNotFound,
v.span, v.span,
PathBuf::from(path_no_whitespace), PathBuf::from(path_no_whitespace),
) )
@ -98,7 +98,7 @@ impl Command for Cd {
let path = nu_path::expand_path_with(path_no_whitespace, &cwd, true); let path = nu_path::expand_path_with(path_no_whitespace, &cwd, true);
if !path.exists() { if !path.exists() {
return Err(shell_error::io::IoError::new( return Err(shell_error::io::IoError::new(
std::io::ErrorKind::NotFound, ErrorKind::DirectoryNotFound,
v.span, v.span,
PathBuf::from(path_no_whitespace), PathBuf::from(path_no_whitespace),
) )

View File

@ -98,6 +98,7 @@ impl Command for Open {
for path in nu_engine::glob_from(&path, &cwd, call_span, None) for path in nu_engine::glob_from(&path, &cwd, call_span, None)
.map_err(|err| match err { .map_err(|err| match err {
ShellError::Io(mut err) => { ShellError::Io(mut err) => {
err.kind = err.kind.not_found_as(NotFound::File);
err.span = arg_span; err.span = arg_span;
err.into() err.into()
} }

View File

@ -55,8 +55,13 @@ impl Command for Source {
let cwd = engine_state.cwd_as_string(Some(stack))?; let cwd = engine_state.cwd_as_string(Some(stack))?;
let pb = std::path::PathBuf::from(block_id_name); let pb = std::path::PathBuf::from(block_id_name);
let parent = pb.parent().unwrap_or(std::path::Path::new("")); let parent = pb.parent().unwrap_or(std::path::Path::new(""));
let file_path = canonicalize_with(pb.as_path(), cwd) let file_path = canonicalize_with(pb.as_path(), cwd).map_err(|err| {
.map_err(|err| IoError::new(err.kind(), call.head, pb.clone()))?; IoError::new(
err.kind().not_found_as(NotFound::File),
call.head,
pb.clone(),
)
})?;
// Note: We intentionally left out PROCESS_PATH since it's supposed to // Note: We intentionally left out PROCESS_PATH since it's supposed to
// to work like argv[0] in C, which is the name of the program being executed. // to work like argv[0] in C, which is the name of the program being executed.

View File

@ -98,7 +98,7 @@ impl Command for NuCheck {
Ok(Some(path)) => path, Ok(Some(path)) => path,
Ok(None) => { Ok(None) => {
return Err(ShellError::Io(IoError::new( return Err(ShellError::Io(IoError::new(
std::io::ErrorKind::NotFound, ErrorKind::FileNotFound,
path_span, path_span,
PathBuf::from(path_str.item), PathBuf::from(path_str.item),
))) )))
@ -255,7 +255,7 @@ fn parse_file_script(
match std::fs::read(path) { match std::fs::read(path) {
Ok(contents) => parse_script(working_set, Some(&filename), &contents, is_debug, call_head), Ok(contents) => parse_script(working_set, Some(&filename), &contents, is_debug, call_head),
Err(err) => Err(ShellError::Io(IoError::new( Err(err) => Err(ShellError::Io(IoError::new(
err.kind(), err.kind().not_found_as(NotFound::File),
path_span, path_span,
PathBuf::from(path), PathBuf::from(path),
))), ))),

View File

@ -210,7 +210,7 @@ fn filesystem_directory_not_found() {
actual.err actual.err
); );
assert!( assert!(
actual.err.contains("nu::shell::io::not_found"), actual.err.contains("nu::shell::io::directory_not_found"),
"actual={:?}", "actual={:?}",
actual.err actual.err
); );

View File

@ -172,7 +172,7 @@ fn file_not_exist() {
" "
)); ));
assert!(actual.err.contains("nu::shell::io::not_found")); assert!(actual.err.contains("nu::shell::io::file_not_found"));
}) })
} }

View File

@ -251,7 +251,7 @@ fn errors_if_file_not_found() {
// This seems to be not directly affected by localization compared to the OS // This seems to be not directly affected by localization compared to the OS
// provided error message // provided error message
assert!(actual.err.contains("nu::shell::io::not_found")); assert!(actual.err.contains("nu::shell::io::file_not_found"));
assert!(actual.err.contains( assert!(actual.err.contains(
&PathBuf::from_iter(["tests", "fixtures", "formats", "i_dont_exist.txt"]) &PathBuf::from_iter(["tests", "fixtures", "formats", "i_dont_exist.txt"])
.display() .display()

View File

@ -3,7 +3,7 @@ pub use nu_protocol::{
ast::CellPath, ast::CellPath,
engine::{Call, Command, EngineState, Stack, StateWorkingSet}, engine::{Call, Command, EngineState, Stack, StateWorkingSet},
record, record,
shell_error::io::IoError, shell_error::io::*,
ByteStream, ByteStreamType, Category, ErrSpan, Example, IntoInterruptiblePipelineData, ByteStream, ByteStreamType, Category, ErrSpan, Example, IntoInterruptiblePipelineData,
IntoPipelineData, IntoSpanned, IntoValue, PipelineData, Record, ShellError, Signature, Span, IntoPipelineData, IntoSpanned, IntoValue, PipelineData, Record, ShellError, Signature, Span,
Spanned, SyntaxShape, Type, Value, Spanned, SyntaxShape, Type, Value,

View File

@ -3,7 +3,7 @@ use nu_path::canonicalize_with;
use nu_protocol::{ use nu_protocol::{
ast::Expr, ast::Expr,
engine::{Call, EngineState, Stack, StateWorkingSet}, engine::{Call, EngineState, Stack, StateWorkingSet},
shell_error::io::IoError, shell_error::io::{ErrorKindExt, IoError, NotFound},
ShellError, Span, Type, Value, VarId, ShellError, Span, Type, Value, VarId,
}; };
use std::{ use std::{
@ -221,7 +221,7 @@ pub fn current_dir(engine_state: &EngineState, stack: &Stack) -> Result<PathBuf,
// be an absolute path already. // be an absolute path already.
canonicalize_with(&cwd, ".").map_err(|err| { canonicalize_with(&cwd, ".").map_err(|err| {
ShellError::Io(IoError::new_internal_with_path( ShellError::Io(IoError::new_internal_with_path(
err.kind(), err.kind().not_found_as(NotFound::Directory),
"Could not canonicalize current dir", "Could not canonicalize current dir",
nu_protocol::location!(), nu_protocol::location!(),
PathBuf::from(cwd), PathBuf::from(cwd),
@ -241,7 +241,7 @@ pub fn current_dir_const(working_set: &StateWorkingSet) -> Result<PathBuf, Shell
// be an absolute path already. // be an absolute path already.
canonicalize_with(&cwd, ".").map_err(|err| { canonicalize_with(&cwd, ".").map_err(|err| {
ShellError::Io(IoError::new_internal_with_path( ShellError::Io(IoError::new_internal_with_path(
err.kind(), err.kind().not_found_as(NotFound::Directory),
"Could not canonicalize current dir", "Could not canonicalize current dir",
nu_protocol::location!(), nu_protocol::location!(),
PathBuf::from(cwd), PathBuf::from(cwd),

View File

@ -1,6 +1,7 @@
use miette::{Diagnostic, LabeledSpan, SourceSpan}; use miette::{Diagnostic, LabeledSpan, SourceSpan};
use std::{ use std::{
fmt::Display, error::Error as StdError,
fmt::{self, Display, Formatter},
path::{Path, PathBuf}, path::{Path, PathBuf},
}; };
use thiserror::Error; use thiserror::Error;
@ -37,11 +38,7 @@ use super::{location::Location, ShellError};
/// ///
/// # let span = Span::test_data(); /// # let span = Span::test_data();
/// let path = PathBuf::from("/some/missing/file"); /// let path = PathBuf::from("/some/missing/file");
/// let error = IoError::new( /// let error = IoError::new(std::io::ErrorKind::NotFound, span, path);
/// std::io::ErrorKind::NotFound,
/// span,
/// path
/// );
/// println!("Error: {:?}", error); /// println!("Error: {:?}", error);
/// ``` /// ```
/// ///
@ -87,9 +84,8 @@ use super::{location::Location, ShellError};
/// This approach ensures clarity about where such container transfers occur. /// This approach ensures clarity about where such container transfers occur.
/// All other I/O errors should be handled using the provided constructors for `IoError`. /// All other I/O errors should be handled using the provided constructors for `IoError`.
/// This way, the code explicitly indicates when and where a `ShellError` transfer might happen. /// This way, the code explicitly indicates when and where a `ShellError` transfer might happen.
#[derive(Debug, Clone, Error, PartialEq)] #[derive(Debug, Clone, PartialEq)]
#[non_exhaustive] #[non_exhaustive]
#[error("I/O error")]
pub struct IoError { pub struct IoError {
/// The type of the underlying I/O error. /// The type of the underlying I/O error.
/// ///
@ -133,6 +129,9 @@ pub struct IoError {
pub enum ErrorKind { pub enum ErrorKind {
Std(std::io::ErrorKind), Std(std::io::ErrorKind),
NotAFile, NotAFile,
// use these variants in cases where we know precisely whether a file or directory was expected
FileNotFound,
DirectoryNotFound,
} }
#[derive(Debug, Clone, PartialEq, Eq, Error, Diagnostic)] #[derive(Debug, Clone, PartialEq, Eq, Error, Diagnostic)]
@ -269,8 +268,8 @@ impl IoError {
/// ///
/// # Examples /// # Examples
/// ```rust /// ```rust
/// use std::path::PathBuf;
/// use nu_protocol::shell_error::io::IoError; /// use nu_protocol::shell_error::io::IoError;
/// use std::path::PathBuf;
/// ///
/// let error = IoError::new_internal_with_path( /// let error = IoError::new_internal_with_path(
/// std::io::ErrorKind::NotFound, /// std::io::ErrorKind::NotFound,
@ -309,15 +308,30 @@ impl IoError {
} }
} }
impl StdError for IoError {}
impl Display for IoError {
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
match self.kind {
ErrorKind::Std(std::io::ErrorKind::NotFound) => write!(f, "Not found"),
ErrorKind::FileNotFound => write!(f, "File not found"),
ErrorKind::DirectoryNotFound => write!(f, "Directory not found"),
_ => write!(f, "I/O error"),
}
}
}
impl Display for ErrorKind { impl Display for ErrorKind {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
match self { match self {
ErrorKind::Std(std::io::ErrorKind::NotFound) => write!(f, "Not found"),
ErrorKind::Std(error_kind) => { ErrorKind::Std(error_kind) => {
let msg = error_kind.to_string(); let msg = error_kind.to_string();
let (first, rest) = msg.split_at(1); let (first, rest) = msg.split_at(1);
write!(f, "{}{}", first.to_uppercase(), rest) write!(f, "{}{}", first.to_uppercase(), rest)
} }
ErrorKind::NotAFile => write!(f, "Not a file"), ErrorKind::NotAFile => write!(f, "Not a file"),
ErrorKind::FileNotFound => write!(f, "File not found"),
ErrorKind::DirectoryNotFound => write!(f, "Directory not found"),
} }
} }
} }
@ -352,15 +366,28 @@ impl Diagnostic for IoError {
kind => code.push_str(&kind.to_string().to_lowercase().replace(" ", "_")), kind => code.push_str(&kind.to_string().to_lowercase().replace(" ", "_")),
}, },
ErrorKind::NotAFile => code.push_str("not_a_file"), ErrorKind::NotAFile => code.push_str("not_a_file"),
ErrorKind::FileNotFound => code.push_str("file_not_found"),
ErrorKind::DirectoryNotFound => code.push_str("directory_not_found"),
} }
Some(Box::new(code)) Some(Box::new(code))
} }
fn help<'a>(&'a self) -> Option<Box<dyn std::fmt::Display + 'a>> { fn help<'a>(&'a self) -> Option<Box<dyn std::fmt::Display + 'a>> {
let make_msg = |path: &Path| {
let path = format!("'{}'", path.display());
match self.kind {
ErrorKind::NotAFile => format!("{path} is not a file"),
ErrorKind::Std(std::io::ErrorKind::NotFound)
| ErrorKind::FileNotFound
| ErrorKind::DirectoryNotFound => format!("{path} does not exist"),
_ => format!("The error occurred at {path}"),
}
};
self.path self.path
.as_ref() .as_ref()
.map(|path| format!("The error occurred at '{}'", path.display())) .map(|path| make_msg(path))
.map(|s| Box::new(s) as Box<dyn std::fmt::Display>) .map(|s| Box::new(s) as Box<dyn std::fmt::Display>)
} }
@ -417,3 +444,75 @@ impl From<ErrorKind> for std::io::ErrorKind {
} }
} }
} }
/// More specific variants of [`NotFound`](std::io::ErrorKind).
///
/// Use these to define how a `NotFound` error maps to our custom [`ErrorKind`].
pub enum NotFound {
/// Map into [`FileNotFound`](ErrorKind::FileNotFound).
File,
/// Map into [`DirectoryNotFound`](ErrorKind::DirectoryNotFound).
Directory,
}
/// Extension trait for working with [`std::io::ErrorKind`].
pub trait ErrorKindExt {
/// Map [`NotFound`](std::io::ErrorKind) variants into more precise variants.
///
/// The OS doesn't know when an entity was not found whether it was meant to be a file or a
/// directory or something else.
/// But sometimes we, the application, know what we expect and with this method, we can further
/// specify it.
///
/// # Examples
/// Reading a file.
/// If the file isn't found, return [`FileNotFound`](ErrorKind::FileNotFound).
/// ```rust
/// # use nu_protocol::{
/// # shell_error::io::{ErrorKind, ErrorKindExt, IoError, NotFound},
/// # ShellError, Span,
/// # };
/// # use std::{fs, path::PathBuf};
/// #
/// # fn example() -> Result<(), ShellError> {
/// # let span = Span::test_data();
/// let a_file = PathBuf::from("scripts/ellie.nu");
/// let ellie = fs::read_to_string(&a_file).map_err(|err| {
/// ShellError::Io(IoError::new(
/// err.kind().not_found_as(NotFound::File),
/// span,
/// a_file,
/// ))
/// })?;
/// # Ok(())
/// # }
/// #
/// # assert!(matches!(
/// # example(),
/// # Err(ShellError::Io(IoError {
/// # kind: ErrorKind::FileNotFound,
/// # ..
/// # }))
/// # ));
/// ```
fn not_found_as(self, kind: NotFound) -> ErrorKind;
}
impl ErrorKindExt for std::io::ErrorKind {
fn not_found_as(self, kind: NotFound) -> ErrorKind {
match (kind, self) {
(NotFound::File, Self::NotFound) => ErrorKind::FileNotFound,
(NotFound::Directory, Self::NotFound) => ErrorKind::DirectoryNotFound,
_ => ErrorKind::Std(self),
}
}
}
impl ErrorKindExt for ErrorKind {
fn not_found_as(self, kind: NotFound) -> ErrorKind {
match self {
Self::Std(std_kind) => std_kind.not_found_as(kind),
_ => self,
}
}
}

View File

@ -4,7 +4,7 @@ use nu_parser::{flatten_block, parse, FlatShape};
use nu_protocol::{ use nu_protocol::{
engine::{EngineState, Stack, StateWorkingSet}, engine::{EngineState, Stack, StateWorkingSet},
report_shell_error, report_shell_error,
shell_error::io::IoError, shell_error::io::{ErrorKindExt, IoError, NotFound},
DeclId, ShellError, Span, Value, VarId, DeclId, ShellError, Span, Value, VarId,
}; };
use reedline::Completer; use reedline::Completer;
@ -58,7 +58,7 @@ fn read_in_file<'a>(
let file = std::fs::read(file_path) let file = std::fs::read(file_path)
.map_err(|err| { .map_err(|err| {
ShellError::Io(IoError::new_with_additional_context( ShellError::Io(IoError::new_with_additional_context(
err.kind(), err.kind().not_found_as(NotFound::File),
Span::unknown(), Span::unknown(),
PathBuf::from(file_path), PathBuf::from(file_path),
"Could not read file", "Could not read file",