-
Notifications
You must be signed in to change notification settings - Fork 166
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
Type safe path file descriptors? #478
Comments
I appreciate the cleverness of this, but my first impression here is it's too much complexity for too little gain. Accidentally passing |
Also, the name |
Good point, renamed to
While true, I think Alan's point still stands about it being better to capture this at compile time rather than runtime.
That's fair.
I was going to deprecate In general, using BorrowedFd feels kind of icky since it's not a real fd and stuff like |
It does make some things better. But also, we already have a surprising number of types and traits that Rust programmers must know about to work with file descriptors at this abstraction level, and this would add one more. And then there's
I can't store an
In The restriction that Another perspective on this is that the vast majority of Rust code in the world that uses filesystem APIs uses std or things build on top of std, and that's how it should be. Std doesn't yet have rustix/etc. still have their uses, but they tend to be much more focused on "I want to drop down an abstraction level because std can't do what I need". Safety remains essential, but preventing hypothetical non-safety bugs at compile time is nice but less important. At the same time, allowing unusual-but-technically-valid code patterns is more important. Because who knows; maybe someone will find a situation where they do want to pass |
True, but I'm not in favor of dumbing things down for the masses. And the same argument could apply to the existing traits, though I think your point is that the value-add of AsPathFd is significantly less than OwnedFd which is totally fair.
Again, I agree with this, but I think it's downplaying the annoyance of having to wait for a test cycle to know that the call "immediately" failed (it could be deep in the weeds and take a while for people to even notice). I also feel like this argument veers dangerously close to the "programmers know what they're doing, duh" argument that says we should just be using C. We have a phenomenal type system at our disposal, let's use it!
False: struct OldTest<'a> {
fd: BorrowedFd<'a>
}
struct NewTest<Fd: AsPathFd> {
fd: Fd,
}
fn foo() {
// let fd: impl AsPathFd = return_trait(); // Soon hopefully: https://rust-lang.github.io/impl-trait-initiative/explainer/lbit.html
let fd = return_trait();
let t = NewTest { fd };
// Do stuff
}
fn return_trait() -> impl AsPathFd {
Cwd
}
True, but also true of all other generics over traits, so not sure why this case deserves a special callout.
To be clear, I wasn't saying that this case should work, but rather that it's odd for that method to exist and be available given the fact that it is known to always fail. That's what I meant by feeling icky. In general, we're letting people use a type that can be used in a bunch of places where we know statically that the operation will always fail (which is odd/a poor use of the type system). Hence the desire for a new type that doesn't muddle these concepts.
Gotya, seems reasonable. The idea of a
I generally agree with your sentiment and the use cases you presented, but I think this goes back to muddling what a BorrowedFd is for. Using a plain
Since this is a repeated theme, I just want to be clear that the AsPathFd proposal doesn't impose any restrictions as far as I can tell since people can always get their hands dirty and implement that trait without unsafe or implement |
I'm going to take a little more time to think about this, but I did want to respond to one thing quickly:
Thanks. I also suggest changing the "Path" to "Dir", because "Path" in Rust sounds like a string, and a "path file descriptor" in Linux suggests something opened with |
Still not a full reply, just a table I made while thinking about this:
|
Sounds good! I also think I was too quick to dismiss the
Great point again, fixed. :) |
cc @asomers I had a bit of time to look at this today. The posix standard specifies It could look something like this: trait AsDirFd {
fn as_dir_fd(&self) -> RawFd;
}
pub struct Cwd;
impl AsDirFd for Cwd
struct OwnedDirFd(OwnedFd);
impl OwnedDirFd {
fn into_inner(self) -> OwnedFd
}
impl AsDirFd for OwnedDirFd
impl AsDirFd for &OwnedDirFd
fn open(_fd: impl AsDirFd, path, ...) -> Result<OwnedFd> {}
fn open_dir(_fd: impl AsDirFd, path, ...) -> Result<OwnedDirFd> {}
fn write(_fd: impl AsFd, ...) {}
fn foo() {
let dir = open_dir(Cwd);
open(dir);
open(&dir);
open(Cwd);
open(borrowed_fd OR owned_fd); // Fails to compile
write(dir); // Fails to compile
write(&dir); // Fails to compile
write(Cwd); // Fails to compile
} If we do go this route, the key question is how do we handle someone showing up with a BorrowedFd/OwnedFd and wanting to use it for |
Did a bit more research and it does seem like we'd need something like For rustix, I think this proposal is a bit much since it's a breaking change and we don't know how it'll land with people. For nix, we're already breaking everything so this should just be part of the migration process. I'd propose implementing this in nix and then bringing it over to rustix as a breaking change if we like it. |
My instinct here is still that "Dir" and file-vs-dir type safety makes sense to introduce at the abstraction level of std, which is the abstraction level most Rust code is written at. At the abstraction level of rustix, it feels like it will introduce a lot of complexity, more than it's worth, and risk making "interesting" use cases that we can't anticipate more complex or even less safe. That said, it sounds like you're really motivated to do this experiment, and I don't wish to stand in the way :-). I'll be interested to hear how it goes. |
Haha, yeah. :) Hopefully I'll be able to report back in a month or two once things pick back up after the holidays. |
See this discussion over in nix: nix-rust/nix#1863 (comment). I believe rustix can get the same treatment and it should be backwards compatible which is awesome. I'd like to push this through, but just wanted to double check that it'll be accepted before starting.
The text was updated successfully, but these errors were encountered: