add slice::replace

[izik1]

Hi! This is my first pr here and it's a bit of a drive by "what if libcore had a method I reach for sometimes."

I'm not really sure of all the things I'm supposed to be doing, however, I have tried copying how other contribution have been formatted to the best of my ability.

I haven't added any tests yet because I'm not entirely sure myself if this is something that's wanted in libcore.

[rust-highfive]

r? @Mark-Simulacrum

(rust_highfive has picked a reviewer for you, use r? to override)

[lcnr]

I first thought that the name might be slightly confusing, as replace could also be used on the whole slice at once. But slice::swap already sets a precedent here (https://doc.rust-lang.org/nightly/std/primitive.slice.html#method.swap) so this method seems like a good idea to me 👍

Not a member of t-libs though, it's them who have to decide if we should add this method

[Mark-Simulacrum]

I would prefer to avoid this, as the name is "really nice" and the mem::replace seems relatively straightforward to reach for. Plus, I'd like for us to have https://doc.rust-lang.org/nightly/std/primitive.str.html#method.replace on slices eventually, and this addition would mean that we'd need another name for that...

However, I am not on the libs team, so will defer to r? @KodrAus for final call here.

[scottmcm]

I'm not entirely sure myself if this is something that's wanted in libcore

Thanks for the PR! The other option would be to chat about it on URLO/Zulip, but for something where making an initial PR isn't a huge pain, personally I like just having it concrete to see.

---

But slice::swap already sets a precedent here

I think swap is materially different, here. Replace is trivial to write with just safe code: slice.replace(i, value) is just replace(&mut slice[i], value). But if you try to write swap, it's a royal pain because you need two &muts into the slice. The shortest thing I can come up with is something like this:

fn myswap<T>(x: &mut[T], i: usize, j: usize) {
    if i == j { return; }
    let (i, j) = (i.min(j), i.max(j));
    let (a, b) = x.split_at_mut(j);
    std::mem::swap(&mut a[i], &mut b[0]);
}

But there's no way that will codegen as well as the unsafe-using version in core.

And since mem::replace is no-fail, combining the two APIs doesn't make for a simpler precondition or anything either.

So I think I'm with Mark on this one -- though of course I'm not on libs either.

[scottmcm]

nit: don't bother making this change until we find out whether libs wants to move forward, but note that mem::replace is must_use, so this one probably should be too -- maybe something like

#[must_use = "if you don't need the old value, you can just assign the new value directly with `*slice[index] = value;`"]

[abonander]

slice.replace(i, val) looks much nicer than replace(&mut slice[i], val), especially if the index expression i is non-trivial. Note also that it's considered best practice to not directly import free functions but to invoke them from their module so that would be mem::replace(&mut slice[i], val) which is even more syntactic overhead.

slice.replace(...) is also easier to discover and doesn't require remembering both that mem::replace exists and that you can take a mutable reference to a slice index.

There is a number of methods on slices already that can be written directly without unsafe but are just mildly annoying:

• slice.contains(&val) can be written as slice.iter().any(|x| x == val)
• slice.is_empty() can be written as slice.len() == 0 (which is something people coming from other languages tend to do anyway before they learn .is_empty() exists; sometimes I forget it and I write Rust every day for work)
  • oftentimes used as !slice.is_empty() which is arguably harder to read than slice.len() != 0
• slice.repeat(n) can be written as slice.iter().cycle().take(n * slice.len()).cloned().collect::<Vec<_>>()
• slice.to_vec() can be written as slice.iter().cloned().collect;:<Vec<_>>()

In terms of syntactic and cognitive overhead, I think mem::replace(&mut slice[i], val) is very similar to slice.iter().any(|x| x == val), so if slice.contains(...) exists, why not slice.replace(...)?

I do see the argument that [T]::replace() might want to be used for the analogue of str::replace(), which already has some precedent with [T]::split() et al. Maybe for this method we use something like swap_in or swap_with instead?

[scottmcm]

slice.replace(...) is also easier to discover and doesn't require remembering both that mem::replace exists and that you can take a mutable reference to a slice index.

This, to me, is one of those places where something and its functional opposite can both be true. That statement is true, but it's also true that once you know both mem::replace and that you can take a mutable reference to a slice index, it's more discoverable to just combine them and that doesn't require remembering that replace exists on slices and what its parameter order is.

So I tend to weight it by what I'd rather see in a code review. And between mem::replace(&mut qux[foo], bar) and qux.replace(foo, bar), I think I prefer the former. (It'd be worse with literals, .replace(4, 5), but that's probably pretty uncommon.) This is probably impacted by me considering mem::replace as quintessential rust, as essential as it (or it in its take disguise) is for moving between &mut and owned.

slice.repeat(n) can be written as slice.iter().cycle().take(n * slice.len()).cloned().collect::<Vec<_>>()

That one's interesting in that it's accidentally a great example of the kind of thing I meant by "a simpler precondition or anything either". It's actually equivalent to slice.iter().cycle().take(n.checked_mul(slice.len()).unwrap()).cloned().collect::<Vec<_>>() -- a very easy thing to overlook (CVE-2018-1000810), and thus an example of a place where the dedicated combination can have more value than it otherwise would because of it.

(And this is also an example of a place where unsafe code in the implementation allows it to be more efficient, see #48657.)

[abonander]

That statement is true, but it's also true that once you know both mem::replace and that you can take a mutable reference to a slice index

My point being that these two things are relatively unconnected concepts that a newcomer to the language may have trouble making a mental connection between. In my experience, mem::replace() is not actually that common in production code. Sure, it appears a lot in the compiler if you grep for it since it does a lot of in-place mutations, but we use it exactly once in sqlx and not at all in several sizable work projects. It's just not something that comes to mind very often.

On the other hand, slice.replace(...) being a direct method on slices means it's really easy to come across when casually browsing the documentation, and if you needed something like this and didn't know you could do it with mem::replace, you'd most likely look at the inherent methods on slices first for a possible solution. We are trying to make the language easier to pick up, aren't we?

The point on parameter order is a bit moot given that the opposite really doesn't make as much sense. We already have Vec::insert() which theoretically has the same problem with ordering and literals, and arguably HashMap::insert() and BTreeMap::insert() as well if you're using the same type (or same kinds of types, e.g. integral) for key and value. The paradigm of <operation>(<index/key>, <value>) is pretty common in general, and not really Rust-specific.

You could argue that if you have a Vec<T> where T is integral, someone might assume slice.replace(4, 5) means "replace every instance of 4 in this slice with 5" but that would be alleviated by bikeshedding a different method name which is already on the table.

Using slice.repeat(n) as a counter-example isn't really that compelling since the iterator version is the most unwieldy of the bunch anyway. slice.contains(&val) also uses specialization to optimize [u8]::contains() to memchr().

However, slice.to_vec() is a great example of what I'm talking about. Its implementation is literally just

let mut vec = Vec::with_capacity(s.len());
vec.extend_from_slice(s);
vec

and arguably the with_capacity is redundant since .extend_from_slice() will do an exact resize. The implementation is more syntactic overhead since it requires a let binding but it could use the impl <T> From<&'_ [T]> for Vec<T> where T: Clone and just call .into() (although that ironically invokes to_vec() so the implementations would need to be swapped). The point is, it makes an existing operation much more discoverable by being an inherent method.

[KodrAus]

Thanks for the PR @izik1!

There are definitely reasons to want to combine a few independently simple operations into a single more semantically readable one. Encapsulating unsafe is usually a good one. Making common operations more discoverable is another. In this thread we've seen examples of both cases.

Specifically for slice::replace, my opinion is that the discoverability motivation isn't quite enough on its own, but could definitely be convinced. The naive approach of:

let old = mem::replace(&mut slice[i], new)

is about as good as you can do. You could imagine someone working with Copy values that isn't familiar with mem::replace writing:

let old = slice[i];
slice[i] = new;

which generates equivalent code to mem::replace.

So I think if we do want this then we should do some more exploration of how naive code attempting to replace an element within a slice could be improved by slice::replace, what options for specialization might be opened up by adding it to std, along with a look at other extensions, such as replacing entire subslices.

[dylni]

I do see the argument that [T]::replace() might want to be used for the analogue of str::replace(), which already has some precedent with [T]::split() et al. Maybe for this method we use something like swap_in or swap_with instead?

I agree with this argument, and it's what I thought the PR was adding before I read it. I also agree that the arguments might be unclear when this method is used:

You could argue that if you have a Vec<T> where T is integral, someone might assume slice.replace(4, 5) means "replace every instance of 4 in this slice with 5" but that would be alleviated by bikeshedding a different method name which is already on the table.

Would it have the same effect to mention mem::replace in the documentation for the slice module or primitive?

[crlf0710]

Triage: What's the current status here?

[izik1]

Hi! Sorry for not coming back to this, I keep forgetting about it ^^;

I think it would probably be fine to just close this, as I mentioned in the OP, it was a bit of a drive by and wasn't entirely sure if it was wanted so it wouldn't bother me much to close it, there are a lot of valid points raised about:

• the name replace is desirable
• the ergonomics aren't that much of an improvement over let prev = mem::replace(&mut slice[idx], el) (at least, for how infrequently I'd expect it to be used, if it was every 2-4 lines of code I'd say that most improvements would be worthwhile)
• it doesn't really avoid any footguns or have any special performance impact.

I'm not sure what proper PR protocol is here, so I'm going to just close?