There is one source of nasty bugs in Rust code that i see introduced over and over, and it's using write where write_all should have been used.
Clippy is trying to bandaid it, but it's just not enough. I just fixed an intstance, where clippy didn't help, because the number of bytes written was being collect for completely different purposes, and not handling of partial writes. Both the author and reviewers did not notice of course. Had this been triggered ever in production it could have been catastrophic for our project.
I believe if this method was named write_partial or just anything else that reminds the reader it's not write_all, these bugs would never happen.
Could we ever rename this method? E.g. as an edition change? New method? A way to alias trait methods? Anything. I believe it is simply worth it.
For me, this would need to be extremely strongly motivated. I would want a sense of how often it is misused and why a Clippy lint doesn't help. The bar for this kind of churn should be set very high.
I don't know how such information could be collected and presented for you to find it convincing.
I've lamented about this footgun before:
I've found this bug in a lot of places. In noobs' code. In code by experienced devs who work professionally on network services. In cargo. In cranelift. In rustdoc. I the png and gif crates. In xml-rs. In firecracker. In rav1e. In openssl and boring. In serde. In sccache. In flate2. In hyper. Even an early version of docs for std::io had this bug in the examples.
I suspect that if Rust won't complain about implementingfn write(…) for the io::Write trait or uses of .write() inside this method, then there won't be any churn. Intentional uses of partial .write() seem to be incredibly rare.
One possible approach would be to have a warn-by-default lint on any write that appears to 1) not be in a loop and 2) not be part of the implementation of another write-like construct (which implies using or returning the length written).
And both of those require looking at the value returned by write: the loop for (1) needs to know how much was written, and the implementation in (2) needs to either use or return the length written.
We have a mechanism for just such a lint, #[must_use], but it doesn't quite suffice because write returns a Result and we don't currently have a way to say that the usize inside the Result is also#[must_use].
There have been some attempts over the years to support #[must_use] of values inside the outermost return value like this, but we still don't have support for that. I think that would be the ideal way to fix this (and several other issues as well).
The main thing such a mechanism needs is a design for specifying the bits that are #[must_use].
It gets worse. Files can error when you close them (especially with buffered writers and NFS) and there's really no way to observe this in Rust. You can call File::sync_all, but does the OS guarantee that if you call that and no intervening operations occur that close will succeed?
It would be nice to have it, and it could help in a lot of cases, but to me that's still not an ideal solution. It highlights the mistake instead of preventing it. The method with a deceptively simple name will remain there forever to tempt everyone who hasn't hit that footgun yet. A general-purpose concept of a "used" value may not be sufficient to catch this mistake: users may treat the return value as equivalent of fwrite's, which doesn't require a retry loop.
Clippy's detection could be made smarter (and integrated into rustc and enabled by default), but that's still not fixing the root cause, only patching consequences of having an unclear function name (a standard name inherited from a standard that also has gems like creat, dup2, and swab[1]).
OTOH if this was called write_partial (or some other name that conveys the unreliability) then none of that would be necessary! Users wouldn't accidentally use a wrong function just by typing the most obvious word. Users and others reading the code would know from just the method name that it's not a simple complete write.
POSIX had to accommodate ANSI C that allowed linkers to truncate names to only 6 characters! ↩︎
Anecdotically, just a week ago I encountered an incorrect use of Read::read/Write::write while reviewing code from a middle-level programmer. It was a very unpleasant surprise since I consider it a junior-level mistake. I completely agree that the name is quite unfortunate (albeit logical, if you are familiar with syscalls/libc), but I think the best way forward in Rust 1.x will be to somehow add #[must_use] annotations to inner types as discussed above. Such feature would useful for some other cases as well.
If it's a regular file then calling write and assuming it'll write everything is a common misconception even if you know that short writes are a thing in principle because - unlike sockets, pipes, etc. - disk-backed filesystems usually make an effort to accept the whole write so one can come to the wrong impression that it's only the other types that need write_all. But that's not guaranteed and there are edge-cases, so it's still incorrect.
I imagine a solution where Write::write would be an alias for Write::write_partial, implemented with compiler magic, so that both names are equivalent and interoperable. This way it would be backwards compatible, and codebases could migrate gradually.
Rust managed to make [].into_iter() map to a different method based on edition, so I assume a similar not-too-invasive hack could be created for write.
Does deprecation/rust lint only in an edition work? No idea how that works out on the implementor side, but the caller getting a bunch of warnings they can autofix only when they bump editions sounds not bad.
The ability to rename without breaking back compatibility sounds like a pretty great feature if someone figures out how it works though!
Or know what Ok(0) might mean, or why one of those meanings isn't an error type?
I don't really see the point in renaming a method that, quite possibly, should simply have an unconditional warning against its use. It seems rare enough to justify a couple allows.
That's throwing baby with a bathwater. Just because there are other issues, doesn't mean it's not worth to improve what can be improved.
I'm puzzled how can anyone be under impression that's it's rare, but it could be just the domain and types of code people are working with. From my PoV, io::Write is a backbone of all things IO in Rust, and there's no replacement. I see and write std::io all the time. I have spotted a lot of instances of this mistake. Some, I wrote myself, despite knowing perfectly well about this pitfall. That's what is getting me so worked up about it - it's soo not Rust to not attempt to steer the user towards correct code via API (like naming). We've just took the name that Unix/C developers used and ran with it.
Anyway, IMO, being able to rename + alias would probably be a very useful general purpose functionality enabling evolving stable code without breaking users, and seamless way to rotate io::Write::write to a new name
It’s not just the domain, but also the level of abstraction. You only have to touch io::Write if you are writing code that needs to work at the level of explicitly choosing what sequence of bytes go into a file/socket/stdio. Many crates will never directly interact with any Write implementor, but only higher-level components. Even when files are involved, one might not write any write() calls; for example,
For almost any domain/topic X, there are a lot of ways to write code that does a lot of X without necessarily interacting with the details of how X per se is defined.
However, in the concrete case of when people are writing write() calls, this mistake is very common in my experience, and so, I entirely agree that write() is a hazard — possibly it should not have had the POSIX-style numeric return value at all, and return something else, like a different must_use enum than Result, but that's not feasible today — and it would be good for Rust to offer a way to rename a trait method (among other things, like having trait aliases that are implementable), so that API design problems like this can be compatibly fixed.
I agree with this. A warning could be added today. In the future if renames/aliases gets implemented, that warning could be revisited, and people making extensive use of explicit calls to write can simply disable the warning crate-wide.
You need to distinguish between using the io::Write trait as a generic interface, implementing the trait via the fn write() trait method, and using the .write() method in a retry loop specifically for its incomplete write behavior.
I use the first two all the time, but I never needed to implement the retry loop myself. I have implemented fn write many times, since that's the basic block this trait wants, despite never needing to use it.
io::Write is a glue between lots of crates, but it can be used productively without ever calling this one lowest-level method directly yourself.