Memcpy is backwards

[cgaebel]

std: Standardize (input, output) param orderings

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);

[bfops]

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.

[tbu]

Thanks for posting this!

[lambdaburrito]

A lot of libraries are silently broken because of this.

[lambdaburrito]

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.

[seanmonstar]

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.

[bfops]

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.

[mitsuhiko]

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.

[cgaebel]

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.

[bstrie]

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.

[tikue]

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…

[pcwalton]

@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.

[bstrie]

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.

[CloudiDust]

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.

[Manishearth]

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.

[Manishearth]

Also, filed

Issue: Decide what needs an RfC

[bluss]

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.

[CloudiDust]

@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.

[mkpankov]

I wonder why was this change required in the first place.

[nikomatsakis]

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