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

Tracking Issue for io::Error::other #91946

Closed
3 tasks done
ibraheemdev opened this issue Dec 15, 2021 · 13 comments
Closed
3 tasks done

Tracking Issue for io::Error::other #91946

ibraheemdev opened this issue Dec 15, 2021 · 13 comments
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@ibraheemdev
Copy link
Member

ibraheemdev commented Dec 15, 2021

Feature gate: #![feature(io_error_other)]

This is a tracking issue for io::Error::other(err), a shortcut for io::Error::new(io::ErrorKind::Other, err)

Public API

// std::io

impl Error {
    pub fn other<E>(error: E) -> Error
    where
        E: Into<Box<dyn error::Error + Send + Sync>>;
}

Steps / History

Unresolved Questions

  • None yet.
@ibraheemdev ibraheemdev added C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Dec 15, 2021
@joshtriplett
Copy link
Member

I'm not aware of any blockers on this one, and I know it'd be useful in real code.

@rust-lang/libs-api, shall we stabilize io::Error::other?

@rfcbot merge

@rfcbot
Copy link

rfcbot commented Jun 18, 2022

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jun 18, 2022
@joshtriplett
Copy link
Member

Unchecking my box, because I just realized this takes an Into, rather than just an Error, which precludes us from storing an error more efficiently (e.g. alternate boxes / thin boxes). I think this method should just take E: Error + ....

@joshtriplett
Copy link
Member

@rfcbot concern into-box

@ibraheemdev
Copy link
Member Author

Hm, one of the most common use cases for this is io::Error::other("something .... went wrong"), and strings don't implement Error.

@ibraheemdev
Copy link
Member Author

@joshtriplett io::Error::new already takes E: Into<Box<dyn error::Error + Send + Sync>>

@yaahc
Copy link
Member

yaahc commented Jul 6, 2022

@joshtriplett io::Error::new already takes E: Into<Box<dyn error::Error + Send + Sync>>

I don't think that necessarily means we should copy the existing API, we have a history of breaking patterns that we consider to have been mistakes, such as Arc<dyn Error> implementing Error whereas Box<dyn Error> instead impls From<E: Error> which precludes the possibility of it also implementing Error.

While the ship has certainly sailed for new and we will have to deal with explicit dependencies upon Box regardless, we don't need to add additional dependencies on Box if we think they were a mistake.

@joshtriplett do you have any specific ideas in mind for how we could store errors more efficiently here? cc @thomcc since I think you're the person who has most recently worked on the implementation. For my part I don't really see what we can gain here. I know that replacing E: Error + ... with E: Into<Box<dyn Error + ...>> will be backwards compatible, so I'm happy to vote to approve the more restrictive bound immediately and work on answering the Box dependency and optimization questions as a followup, but I suspect we will probably want to just go with the less restrictive bound in the end.

@thomcc
Copy link
Member

thomcc commented Jul 6, 2022

So, yeah we could do some clever stuff in the impl to make it faster for E: Error + Send + Sync + 'static. Rather than Box<(ErrorKind, Box<dyn Error + ...>)> we could do ThinBox<(ErrorKind, dyn Error + ...)> (tuples for exposition only, currently we use a struct and would use one for this too).

I would be happy to implement this if we decided to go this route, but I don't think this is worth the hassle, honestly. We'd still have std::io::Error::into_inner, FWIW. So getting your data out on the other side requires boxing anyway (although we'd be able to use it for get_ref and such).

Overall, I do not feel strongly, but my vote would be to stick with Into<Box<dyn Error ...>> because of improved ergonomics.

@m-ou-se
Copy link
Member

m-ou-se commented Jul 6, 2022

Hm, one of the most common use cases for this is io::Error::other("something .... went wrong"), and strings don't implement Error.

The problem is that such an api taking a string literal will be relatively inefficient, because we cannot specialize on the 'static lifetime, meaning that this will have to copy the entire string into a new allocation.

I believe this is exactly what @joshtriplett wanted to avoid with his proposal: to make sure it's only used for other(some_underlying_error) and not for other("some literal message").

There's no space inside an io::Error to store a full wide pointer (&'static str), so our internal hack for that is currently a macro (const_io_error) that puts a (ErrorKind, &'static str) into a static and then stores a reference to that in the io::Error. We can't do that through an api like io::Error::new_const_str(&'static str), because then there's no static ErrorKind+&str to refer to. io::Error::new_const_str<const S: &str>() could potentially work in the near future, though.

@joshtriplett
Copy link
Member

I believe this is exactly what @joshtriplett wanted to avoid with his proposal: to make sure it's only used for other(some_underlying_error) and not for other("some literal message").

I do think it's perfectly fine to support a string or similar; that's not my concern here at all. I wanted to raise the question, before we commit to this API, of whether this API makes it harder for us to optimize io::Error further in the future.

I don't feel strongly about this. If folks feel either that this doesn't preclude such future optimizations or that other aspects of io::Error already preclude those optimizations, I don't want to block this on that basis.

@rfcbot resolved into-box

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Aug 22, 2023
@rfcbot
Copy link

rfcbot commented Aug 22, 2023

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Aug 22, 2023
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Sep 1, 2023
@rfcbot
Copy link

rfcbot commented Sep 1, 2023

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Sep 1, 2023
bors added a commit to rust-lang-ci/rust that referenced this issue Sep 6, 2023
Stabilize `io_error_other` feature

Per the FCP for rust-lang#91946.
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Sep 7, 2023
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Sep 12, 2023
Stabilize `io_error_other` feature

Per the FCP for rust-lang/rust#91946.
waynr added a commit to waynr/bigarchiver that referenced this issue Dec 25, 2023
This PR replaces the use of unsable feature
rust-lang/rust#91946 with its corresponding
stable branch equivalent that is slightly more verbose.
garikello3d pushed a commit to garikello3d/bigarchiver that referenced this issue Dec 25, 2023
This PR replaces the use of unsable feature
rust-lang/rust#91946 with its corresponding
stable branch equivalent that is slightly more verbose.
garikello3d pushed a commit to garikello3d/bigarchiver that referenced this issue Dec 26, 2023
This PR replaces the use of unsable feature
rust-lang/rust#91946 with its corresponding
stable branch equivalent that is slightly more verbose.
@Mr-Leshiy
Copy link

@ibraheemdev should this issue be closed as #115453 was merged

r-bk added a commit to r-bk/ch4 that referenced this issue Jul 12, 2024
This commit auto-upgrades all dependencies except for `built` which
fails in v0.7.4 with error [1].

[1]

error[E0658]: use of unstable library feature 'io_error_other'
  --> /Users/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/built-0.7.4/src/dependencies.rs:24:16
   |
24 |         .ok_or(io::Error::other("Cargo.lock not found"))
   |                ^^^^^^^^^^^^^^^^
   |
   = note: see issue #91946 <rust-lang/rust#91946> for more information
r-bk added a commit to r-bk/ch4 that referenced this issue Jul 12, 2024
Note that `built` is kept on `0.7.2` and not latest `0.7.4` because of
error [1].

[1]

error[E0658]: use of unstable library feature 'io_error_other'
  --> /Users/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/built-0.7.3/src/dependencies.rs:24:16
   |
24 |         .ok_or(io::Error::other("Cargo.lock not found"))
   |                ^^^^^^^^^^^^^^^^
   |
   = note: see issue #91946 <rust-lang/rust#91946> for more information
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

8 participants