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

I/O safety breaks existing code #89955

Closed
sunfishcode opened this issue Oct 16, 2021 · 4 comments
Closed

I/O safety breaks existing code #89955

sunfishcode opened this issue Oct 16, 2021 · 4 comments
Assignees
Labels
T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@sunfishcode
Copy link
Member

As reported here, the I/O safety feature introduces a breaking change.

Reduced standalone testcase (playground link):

use std::os::unix::io::FromRawFd;
use std::fs::File;

pub fn create_fd<F: FromRawFd>() -> F {
    todo!()
}

fn main() {
    let _f = File::from(create_fd());
}
error[E0283]: type annotations needed
   --> src/main.rs:9:14
    |
9   |     let _f = File::from(create_fd());
    |              ^^^^^^^^^^ cannot infer type for type parameter `T` declared on the trait `From`
    |
    = note: cannot satisfy `File: From<_>`
note: required by `from`
   --> /.../lib/rustlib/src/rust/library/core/src/convert/mod.rs:371:5
    |
371 |     fn from(_: T) -> Self;
    |     ^^^^^^^^^^^^^^^^^^^^^^

It's possible to change the code to avoid the error, for example by changing the let _f = ... line to either of:

    let _f = create_fd::<File>();
    let _f: File = create_fd(); 

or, once I/O safety is stabilized, by changing create_fd to return OwnedFd instead of F: FromRawFd. The reporter also reported that they've already fixed their own code. Nevertheless, it is breakage observed in real-world code.

The problem is caused by I/O safety adding an impl From<OwnedFd> for File. We added these From impls as an alternative to adding a new FromFd trait. However, this means that File now has multiple From impls for types that implement AsRawFd, so one can no longer do File::from(create_fd()) where create_fd is defined like fn create_fd<F: FromRawFd>() -> F, because it's now ambiguous which type it should use for F.

So the questions here are:

  • Is this breakage severe enough to block stabilization for I/O safety?
  • If so, is there a way to fix it other than going back to adding a FromFd trait?
@sunfishcode sunfishcode added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 16, 2021
@sunfishcode sunfishcode self-assigned this Oct 16, 2021
This was referenced Oct 16, 2021
@scottmcm
Copy link
Member

I think this is officially a "minor" change, and thus allowed -- it's like all the breakage that can happen when AsRef implementations are added. Assuming that's true, it's a libs judgment call on how bad the impact is in practice.

@cuviper
Copy link
Member

cuviper commented Oct 17, 2021

This is about to ship in 1.56 -- cc @rust-lang/release

But AFAIK it didn't show problems in crater, so we'll probably let it go as a minor change...

@Mark-Simulacrum
Copy link
Member

Certainly nothing significant, I think. Working with (raw) file descriptors is probably pretty rare, so I'm not expecting much breakage.

@sunfishcode
Copy link
Member Author

1.56 has shipped now, and I haven't seen any other reports of breakage. So following the comments above, I'll close this issue now. If anyone does see any other instances of breakage, please report it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants