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

Provide more detailed I/O errors #7269

Open
cptpiepmatz opened this issue Feb 4, 2025 · 5 comments
Open

Provide more detailed I/O errors #7269

cptpiepmatz opened this issue Feb 4, 2025 · 5 comments

Comments

@cptpiepmatz
Copy link

The UIoError only contains an std::io::Error and some context and both aren't really accessible. For people that use uucore as a library that makes it very hard to build good errors from that. It would be really nice if that error type could expose the std::io::Error, a path that failed if it happened at some path and maybe even an index for commands that take multiple paths like cp or mv. I think restructuring UIoErrors in that direction would also harmonize error messages across the different binaries and you maybe don't need to use as many custom contexts anymore.

@tertsdiepraam
Copy link
Member

Hi! I'm just chipping in because I wrote the UError stuff. What you're saying is correct and it is a pain. However, I think the problem isn't really with the error types but with the standard library (and libc). The error itself cannot find the paths and other necessary context. Instead we'd have to wrap standard functions with versions that have more elaborate error types. Kind of similar to this: https://docs.rs/fs-err/latest/fs_err/.

If you have a better idea to be smarter about this, I'd love to hear it though!

@cptpiepmatz
Copy link
Author

I recently refactored the I/O errors in nushell#14927 to provide exactly this necessary information. And yeah, we also had to extend the std::io::Error with our own IoError to store everything needed. I think most often the function that actually produces the std::io::Errors know about the context (e.g. the save command). In our case I added some constructors for our IoError to take this information and went through the code to call this constructor everywhere, which was kinda painful but now we have nicer errors.

Just now with the direction to replace our filesystem commands with the uutils ones, we lose that information again. :/

@tertsdiepraam
Copy link
Member

Sounds good! Feel free to ping me if you want to discuss changes you'd like to make.

@cptpiepmatz
Copy link
Author

I think similar to the Nushell IoError, the UIoError could look like this:

pub struct UIoError {
    // hold the inner io error to extract further data from it later on
    inner: std::io::Error,
    // reference a path that caused the error
    path: Option<std::path::PathBuf>,
    // an index to which path failed, `cp` or `mv` need two paths
    path_index: usize,
    // additional context if that is useful
    additional_context: Option<String>,
}

But I'm not really sure about the path_index here. For Nushell we provide a span, but I don't think that is necessary here. But somehow we should track which of the paths we had caused an issue.

@cptpiepmatz
Copy link
Author

@tertsdiepraam I need some guidance about the path_index field of my proposed UIoError it doesn't feel completely right but I think we need something.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants