This commit swapped the parameter order or src and dest to rust's memcpy and memmove equivalents: ptr::copy and ptr::copy_nonoverlapping. This was a subtle change to a #[stable] API, without an RFC. Can it be reverted?
For reference, code like this is now subtly broken:
let dst: *mut u8 = ...;
let src: *mut u8 = ...;
ptr::copy_nonoverlapping(dst, src, 16);
It definitely seems a little scary to allow that behaviour to change completely silently. If itâs marked unstable, then users would at least get a warning when they donât get an error from the const/mut swap.
Thanks for posting this!
A lot of libraries are silently broken because of this.
Also I think there should be unit tests to ensure this doesnât happen again. Iâm happy to contribute tests as Iâm soon to put Rust code in production and this completely broke our system.
Yes, this hurt and was surprising. But unit tests caught it (for me). Make sure you crates have tests, and you shouldnât be bitten badly.
Tests canât really be reasonably expected to cover every possibility for âmy libraryâs behavior changed silently under meâ, and even so, itâs way harder to backtrace failing tests to silently changing behavior than it is to read a warning.
I got bitten by this as well. More so: I think the new argument order is just wrong. This code lives in unsafe blocks where you call lots of C APIs which all follow the dst, src argument order.
Ah. I see it wasnât clear that Iâm against this new memcpy argument order. I am. Yâall wouldâve known that if there was an RFC.
I agree that an RFC was warranted here. Recall that âcommunity members get angry and lose faith in the devs when we pull the rug out from under them with surprising changesâ was literally the impetus for the RFC process in the first place.
Ouch. This hurts. Yeah, I think a revert (until an RFC is approved) might be warranted as a sign of good faith. Though perhaps thatâd just break things even further at this pointâŚ
@bstrie, please donât try to turn this into a core-team-versus-community thing. Iâm on the core team and had no idea this was happening, and I think this was the wrong call.
Not trying to cast stones or rally a mob here, just discomfited at this sense of deja vu. Iâm sure that everyoneâs intentions here were noble but people need assurance that contentious changes will be done transparently.
One problem with reverting is that, the change is already out in the beta (not a ânormalâ nighty), if we revert, there can be more confusion. (Which revision expects which order?)
One solution may be, instead of reverting, we rename the low level functions to somehow clearly indicate the input -> output order, so people will not expect the C order.
Iâll prepare an RFC for this and include both candidates.
For calls like this which can easily cause issues if gotten wrong, perhaps we should newtype the parametes? memcpy(From(x), To(y)) or whatever.
Also, filed
Itâs happed now, the argument order is a bikeshed issue, the only problem is the breakage.
If we want to do the best job possible, most realistic mitigation in my mind would maybe be to add a lint that warns when *mut is coerced to *const, possibly even targeting the lint at specific functions.
@Manishearth, this seems a bit heavy weight. A lint as suggested by @blake may be better.
Renaming is not as good a solution as I initially thought. If we add the ordering information to those functionsâ names, then why donât other copy functions like std::io::copy have ordering information too? Is the lack of such information indicating that arguments are reversed for std::io::copy? (The answer is no, but this is an understandable interpretation.)
Also, the functionsâ current names are not that like memcpy and memmove after all. (YMMV of course.) As long as people are aware (or made aware) that Rust consistently follow the src, dst order, it can be okay.
I wonder why was this change required in the first place.
Hmm. My two cents:
Clearly, this change ought to at minimum have been advertised more widely, whether or not an RFC was required. I wasnât involved in the decision, but I can easily imagine though that it seemed harmless enough (âharmonize the ordering of arguments across functions!â). Mistakes happen. The good part is that this is a one-time sort of problem, I think. We wonât even consider silent breaking changes like this after 1.0, clearly.
Whatâs not entirely clear to me is what is the best thing to do now. After all, the new ordering is more consistent, and beta has shippedâintroducing yet another breaking change doesnât necessarily feel like the best solution. A targeted lint might be a good compromise, to help people get their code fixed. Or maybe we just ride this outâby the time a lint is ready, perhaps most crates will already be working again.
I guess the key question is whether people disagree with the substance of the decision (i.e., internal consistency is less important than consistency with memcpy, or perhaps that I/O APIs ought to have been changed instead) or just the silent, breaking nature of it. I personally tend to think the change was right on the meritsâconsistency is better than not, and I donât care as to which ordering we useâbut it ought to have been (at minimum) advertised more widely. Still, Iâm somewhat on the fence.
UPDATE: tweaked wording, formatting