Change `AsFd::as_fd` etc. from `&self` to `self`. #93869

sunfishcode · 2022-02-10T18:37:36Z

Change the AsFd trait from:

trait AsFd {
    fn as_fd(&self) -> BorrowedFd<'_>;
}

to:

trait AsFd<'a> {
    fn as_fd(self) -> BorrowedFd<'a>;
}

and similar for AsHandle and AsSocket. Instead of implementing AsFd
for OwnedFd etc., it's now implemented for &'a OwnedFd etc.

This means that implementing AsFd changes from:

impl AsFd for MyType {
   fn as_fd(&self) -> BorrowedFd<'_> {
      ...
   }
}

to:

impl<'a> AsFd<'a> for &'a MyType {
   fn as_fd(self) -> BorrowedFd<'a> {
      ...
   }
}

Defining a function that uses an AsFd changes from:

fn frob<Fd: AsFd>(fd: &Fd)

to:

fn frob<'a, Fd: AsFd<'a>>(fd: Fd)

It also means that users of such functions can pass a BorrowedFd to them
directly, instead of having to write an extra & to conceptually add an
extra borrow around BorrowedFd.

In total, it means somewhat more typing for implementors of the traits and
implementors of functions that take AsFd parameters. But it also
avoids a common confusion, and allows users to have fewer &s. And, it
means fewer "reference to reference"-like situations.

As an example of this in practice, the new unstable fchown function in std
is already taking an AsFd by value instead of by reference. As
written, it can't be passed a &File, and passing it a File consumes
and closes the file. With this PR, it accepts &File and never consumes
anything.

One subtle detail is that the Unix internal File, Socket, and
AnonPipe types need to stop implementing AsFd, so that they don't
require stability attributes which are otherwise apparently required.
Since these are internal types, this doesn't affect the public API.
There are comments in the code for these.

Another subtle detail is that with the convention of types implementing
AsFd for &'a T, they don't implement it for &'a mut T. For example,
with the frob function above, frob(&mut file) gets this error:

error[E0277]: the trait bound `&mut File: AsFd<'_>` is not satisfied

One needs to use frob(&file) instead. If one has a mut T, it's just
a matter of typing & instead of &mut, but if one has a &mut T, one
needs to use &*. This is a little awkward, however the only alternative
I know about is to oblige all types that implement AsFd<'a> for &'a T
to also implement AsFd<'a> for &'a mut T, which seems worse.

Change the `AsFd` trait from: ```rust trait AsFd { fn as_fd(&self) -> BorrowedFd<'_>; } ``` to: ```rust trait AsFd<'a> { fn as_fd(self) -> BorrowedFd<'a>; } ``` and similar for `AsHandle` and `AsSocket`. Instead of implementing `AsFd` for `OwnedFd` etc., it's now implemented for `&'a OwnedFd` etc. This means that implementing `AsFd` changes from: ```rust impl AsFd for MyType { fn as_fd(&self) -> BorrowedFd<'_> { ... } } ``` to: ```rust impl<'a> AsFd<'a> for &'a MyType { fn as_fd(self) -> BorrowedFd<'a> { ... } } ``` Defining a function that uses an `AsFd` changes from: ```rust fn frob<Fd: AsFd>(fd: &Fd) ``` to: ```rust fn frob<'a, Fd: AsFd<'a>>(fd: Fd) ``` It also means that users of such functions can pass a `BorrowedFd` to them directly, instead of having to write an extra `&` to conceptually add an extra borrow around `BorrowedFd`. In total, it means somewhat more typing for implementors of the traits and implementors of functions that take `AsFd` parameters. But it also avoids a common confusion, and allows users to have fewer `&`s. And, it means fewer "reference to reference"-like situations. As an example of this in practice, the new unstable `fchown` function in std is [already taking an `AsFd` by value instead of by reference]. As written, it can't be passed a `&File`, and passing it a `File` consumes and closes the file. With this PR, it accepts `&File` and never consumes anything. One subtle detail is that the Unix internal `File`, `Socket`, and `AnonPipe` types need to stop implementing `AsFd`, so that they don't require stability attributes which are otherwise apparently required. Since these are internal types, this doesn't affect the public API. There are comments in the code for these. Another subtle detail is that with the convention of types implementing `AsFd` for `&'a T`, they don't implement it for `&'a mut T`. For example, with the `frob` function above, `frob(&mut file)` gets this error: ``` error[E0277]: the trait bound `&mut File: AsFd<'_>` is not satisfied ``` One needs to use `frob(&file)` instead. If one has a `mut T`, it's just a matter of typing `&` instead of `&mut`, but if one has a `&mut T`, one needs to use `&*`. This is a little awkward, however the only alternative I know about is to oblige all types that implement `AsFd<'a>` for `&'a T` to also implement `AsFd<'a>` for `&'a mut T`, which seems worse. [already taking an `AsFd` by value instead of by reference]: https://github.com/rust-lang/rust/blob/734368a200904ef9c21db86c595dc04263c87be0/library/std/src/os/unix/fs.rs#L974

rust-highfive · 2022-02-10T18:37:41Z

r? @yaahc

(rust-highfive has picked a reviewer for you, use r? to override)

joshtriplett · 2022-02-11T00:55:44Z

Hmmm, interesting. And good catch regarding fchown; we shouldn't stabilize that until we've addressed this.

If this were just more typing for people implementing AsFd, the tradeoff would seem much more clear-cut.

But the substantial added complexity for people writing functions that take an AsFd seems painful, and similarly error-prone, especially compared to things like AsRef<Path> or AsRef<str>.

Is there no other way we could fix this to be more usable, and accept borrowed values, without introducing explicit lifetimes everywhere? Is there some way we could make this feel closer to Path/PathBuf or str/String, both for users and for functions accepting values or references, to make it feel familiar?

sunfishcode · 2022-02-11T02:06:01Z

Is there no other way we could fix this to be more usable, and accept borrowed values, without introducing explicit lifetimes everywhere? Is there some way we could make this feel closer to Path/PathBuf or str/String, both for users and for functions accepting values or references, to make it feel familiar?

Ooh, I think I just figured it out. What would you think if std added this, instead of this PR:

impl<T: AsFd> AsFd for &T {
    fn as_fd(&self) -> BorrowedFd<'_> {
        T::as_fd(self)
    }
}
impl<T: AsFd> AsFd for &mut T {
    fn as_fd(&self) -> BorrowedFd<'_> {
        T::as_fd(self)
    }
}

Everything else would stay the same. With just this change, fchown as-is would accept a &File and it would do the right thing. It'd work for &mut File too.

It wouldn't stop users from passing an owned File into fchown, causing it to be consumed and closed, which is a little surprising, but maybe it's ok. They'd get compile errors if they try to use the file afterwards, at least.

Add implementations of `AsFd` for `&T` and `&mut T`, so that users can write code like this: ```rust pub fn fchown<F: AsFd>(fd: F, uid: Option<u32>, gid: Option<u32>) -> io::Result<()> { ``` with `fd: F` rather than `fd: &F`. And similar for `AsHandle` and `AsSocket` on Windows. Also, adjust the `fchown` example to pass the file by reference. The code can work either way now, but passing by reference is more likely to be what users will want to do. This is an alternative to rust-lang#93869, and is a simpler way to achieve the same goals: users don't need to pass borrowed-`BorrowedFd` arguments, and it prevents a pitfall in the case where users write `fd: F` instead of `fd: &F`.

sunfishcode · 2022-02-11T02:36:09Z

I've now submitted #93888 which implements this new approach. It's much simpler; thanks for pushing me in this direction!

sunfishcode · 2022-02-11T16:53:07Z

#93888 is working out well in my own tests, so I'll close this in favor of it.

joshtriplett · 2022-02-11T17:14:03Z

@sunfishcode

It wouldn't stop users from passing an owned File into fchown, causing it to be consumed and closed, which is a little surprising, but maybe it's ok. They'd get compile errors if they try to use the file afterwards, at least.

AsRef<Path> allows that too; you can pass an owned PathBuf to such a function. And if this causes an error, you can always add a & to borrow it.

Glad the alternative worked out!

…or-ref, r=joshtriplett Implement `AsFd` for `&T` and `&mut T`. Add implementations of `AsFd` for `&T` and `&mut T`, so that users can write code like this: ```rust pub fn fchown<F: AsFd>(fd: F, uid: Option<u32>, gid: Option<u32>) -> io::Result<()> { ``` with `fd: F` rather than `fd: &F`. And similar for `AsHandle` and `AsSocket` on Windows. Also, adjust the `fchown` example to pass the file by reference. The code can work either way now, but passing by reference is more likely to be what users will want to do. This is an alternative to rust-lang#93869, and is a simpler way to achieve the same goals: users don't need to pass borrowed-`BorrowedFd` arguments, and it prevents a pitfall in the case where users write `fd: F` instead of `fd: &F`. r? `@joshtriplett`

…or-ref, r=joshtriplett Implement `AsFd` for `&T` and `&mut T`. Add implementations of `AsFd` for `&T` and `&mut T`, so that users can write code like this: ```rust pub fn fchown<F: AsFd>(fd: F, uid: Option<u32>, gid: Option<u32>) -> io::Result<()> { ``` with `fd: F` rather than `fd: &F`. And similar for `AsHandle` and `AsSocket` on Windows. Also, adjust the `fchown` example to pass the file by reference. The code can work either way now, but passing by reference is more likely to be what users will want to do. This is an alternative to rust-lang#93869, and is a simpler way to achieve the same goals: users don't need to pass borrowed-`BorrowedFd` arguments, and it prevents a pitfall in the case where users write `fd: F` instead of `fd: &F`. r? ``@joshtriplett``

…or-ref, r=joshtriplett Implement `AsFd` for `&T` and `&mut T`. Add implementations of `AsFd` for `&T` and `&mut T`, so that users can write code like this: ```rust pub fn fchown<F: AsFd>(fd: F, uid: Option<u32>, gid: Option<u32>) -> io::Result<()> { ``` with `fd: F` rather than `fd: &F`. And similar for `AsHandle` and `AsSocket` on Windows. Also, adjust the `fchown` example to pass the file by reference. The code can work either way now, but passing by reference is more likely to be what users will want to do. This is an alternative to rust-lang#93869, and is a simpler way to achieve the same goals: users don't need to pass borrowed-`BorrowedFd` arguments, and it prevents a pitfall in the case where users write `fd: F` instead of `fd: &F`. r? ```@joshtriplett```

rust-highfive assigned yaahc Feb 10, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 10, 2022

This comment has been minimized.

Sign in to view

This was referenced Feb 10, 2022

Add documentation about BorrowedFd::to_owned. #93354

Merged

Tracking Issue for RFC 3128: I/O Safety #87074

Closed

Change Windows' internal File, similarly to Unix.

d84a7b7

sunfishcode mentioned this pull request Feb 10, 2022

Tracking Issue for #![feature(unix_chown)] #88989

Closed

4 tasks

sunfishcode mentioned this pull request Feb 11, 2022

Implement AsFd for &T and &mut T. #93888

Merged

sunfishcode closed this Feb 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change `AsFd::as_fd` etc. from `&self` to `self`. #93869

Change `AsFd::as_fd` etc. from `&self` to `self`. #93869

sunfishcode commented Feb 10, 2022

rust-highfive commented Feb 10, 2022

This comment has been minimized.

joshtriplett commented Feb 11, 2022

sunfishcode commented Feb 11, 2022 •

edited

Loading

sunfishcode commented Feb 11, 2022

sunfishcode commented Feb 11, 2022

joshtriplett commented Feb 11, 2022 •

edited

Loading

Change AsFd::as_fd etc. from &self to self. #93869

Change AsFd::as_fd etc. from &self to self. #93869

Conversation

sunfishcode commented Feb 10, 2022

rust-highfive commented Feb 10, 2022

This comment has been minimized.

joshtriplett commented Feb 11, 2022

sunfishcode commented Feb 11, 2022 • edited Loading

sunfishcode commented Feb 11, 2022

sunfishcode commented Feb 11, 2022

joshtriplett commented Feb 11, 2022 • edited Loading

Change `AsFd::as_fd` etc. from `&self` to `self`. #93869

Change `AsFd::as_fd` etc. from `&self` to `self`. #93869

sunfishcode commented Feb 11, 2022 •

edited

Loading

joshtriplett commented Feb 11, 2022 •

edited

Loading