Implement String::remove_matches

[jcotton42]

Closes #50206.

I lifted the function help from @frewsxcv's original PR (#50015), hope they don't mind.

I'm also wondering whether it would be useful for remove_matches to collect up the removed substrings into a Vec and return them, right now they're just overwritten by the copy and lost.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[jcotton42]

I can't use copy_nonoverlapping here right? That was my intuition, but I just wanted to check it.

[pickfire]

Weird, I thought it does not overlap? The memory for deletion should not overlap to begin with. So I think it is good to use copy_nonoverlapping.

By the way, I think it may be better to keep a reference to current cursor rather than keeping shrunk_by which we can directly work on. It could probably reduce one add operation here.

[rust-highfive]

Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

========================== Starting Command Output ===========================
[command]/bin/bash --noprofile --norc /home/vsts/work/_temp/bc743f0a-b1be-4350-b56c-236304befd38.sh

##[section]Finishing: Disable git automatic line ending conversion
##[section]Starting: Checkout rust-lang/rust@refs/pull/71780/merge to s
Task         : Get sources
Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
Version      : 1.0.0
Author       : Microsoft
---
##[command]git remote add origin https://github.com/rust-lang/rust
##[command]git config gc.auto 0
##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
##[command]git config --get-all http.proxy
##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/71780/merge:refs/remotes/pull/71780/merge
---
tar: Error is not recoverable: exiting now

##[error]Bash exited with code '2'.
##[section]Finishing: Install awscli
##[section]Starting: Checkout rust-lang/rust@refs/pull/71780/merge to s
Task         : Get sources
Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
Version      : 1.0.0
Author       : Microsoft
Author       : Microsoft
Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
==============================================================================
Cleaning any cached credential from repository: rust-lang/rust (GitHub)
##[section]Finishing: Checkout rust-lang/rust@refs/pull/71780/merge to s
Cleaning up task key
Start cleaning up orphan processes.
Terminate orphan process: pid (3605) (python)
##[section]Finishing: Finalize Job

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

[Mark-Simulacrum]

Let's try r? @kennytm who I believe is fairly familiar with the searcher API and such

[kennytm]

... I don't think using a Vec here counts as "does not use any allocation" (the original promise in #50206).

[jcotton42]

Originally I couldn't think of a way to do this without allocating a Vec to hold the match start/ends, but I think I have an idea now that may work. I'll try it when I get off work today in about 6 hours.

[jcotton42]

I went ahead and tested my idea and unfortunately it doesn't work. The way I can see this you either have to:
a. Store the match start/end pairs somewhere, then start modifying the string, as I've done here
or
b. Modify the string during searching.

The problem with b is that the searcher will end up missing matches as parts it would've matched on are potentially moved behind where it's looking.

Here's the code I tried, for reference:

use core::str::pattern::Searcher;
let vec_ptr = self.vec.as_mut_ptr();

let len = self.len();
let mut shrunk_by = 0;
let mut searcher = pat.into_searcher(self);

// SAFETY: start and end will be on utf8 byte boundaries per
// the Searcher docs
unsafe {
    while let Some((start, end)) = searcher.next_match() {
        ptr::copy(
            vec_ptr.add(end - shrunk_by),
            vec_ptr.add(start - shrunk_by),
            len - end,
        );
        shrunk_by += end - start;
    }
    drop(searcher);
    self.vec.set_len(len - shrunk_by);
}

[frewsxcv]

i see value in this new method, even if it does allocate. i'm not on the libs team so don't weight my opinion too heavily, but my vote would be to merge a working implementation, and then open up a follow up issue for removing the allocation

[pickfire]

I do think this method is better, I don't see why do we need another Vec for this. The only good reason I see doing a two-pass method like here is used in SIMD JSON which the first pass identify the points which makes deserialization faster.

[jcotton42]

By the way since we've gotten this far should I open a tracking issue and update the unstable attribute accordingly?

[dtolnay]

This bound is pretty unusual. Does something block this from being implementable with only a P: Pattern<'a> bound?

[jcotton42]

It wouldn't compile without the higher-rank trait bound. I talked to someone on the community Discord about this and they suggested the HRTB and it made it compile.

I think it had something to do with not having the HRTB meant Rust thought pat was going to live as long as self was, when in fact the lifetime of pat had to be shorter.

[jcotton42]

If I change P: for<'x> Pattern<'x> to P: Patten<'a> compiling fails with:

error[E0502]: cannot borrow `self.vec` as mutable because it is also borrowed as immutable
    --> src/liballoc/string.rs:1274:21
     |
1249 |     pub fn remove_matches<'a, P>(&'a mut self, pat: P)
     |                           -- lifetime `'a` defined here
...
1256 |             let mut searcher = pat.into_searcher(self);
     |                                -----------------------
     |                                |                 |
     |                                |                 immutable borrow occurs here
     |                                argument requires that `*self` is borrowed for `'a`
...
1274 |                     self.vec.as_mut_ptr().add(end - shrunk_by),
     |                     ^^^^^^^^ mutable borrow occurs here

error[E0502]: cannot borrow `self.vec` as mutable because it is also borrowed as immutable
    --> src/liballoc/string.rs:1275:21
     |
1249 |     pub fn remove_matches<'a, P>(&'a mut self, pat: P)
     |                           -- lifetime `'a` defined here
...
1256 |             let mut searcher = pat.into_searcher(self);
     |                                -----------------------
     |                                |                 |
     |                                |                 immutable borrow occurs here
     |                                argument requires that `*self` is borrowed for `'a`
...
1275 |                     self.vec.as_mut_ptr().add(start - shrunk_by),
     |                     ^^^^^^^^ mutable borrow occurs here

error[E0502]: cannot borrow `self.vec` as mutable because it is also borrowed as immutable
    --> src/liballoc/string.rs:1280:13
     |
1249 |     pub fn remove_matches<'a, P>(&'a mut self, pat: P)
     |                           -- lifetime `'a` defined here
...
1256 |             let mut searcher = pat.into_searcher(self);
     |                                -----------------------
     |                                |                 |
     |                                |                 immutable borrow occurs here
     |                                argument requires that `*self` is borrowed for `'a`
...
1280 |             self.vec.set_len(len - shrunk_by);
     |             ^^^^^^^^ mutable borrow occurs here

error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0502`.
error: could not compile `alloc`.

[jcotton42]

@dtolnay does this answer your question about why I used this trait bound?

[dtolnay]

It does not -- it just says that the current implementation relies on the too strict bound. In #71780 (review) I wrote that we'd want to fix the implementation so that a less strict bound is used.

[dtolnay]

For example the following code also requires an unnecessarily strict bound, and does not compile if you remove 'a: 'static, but it's easy to fix the implementation to not require that bound.

pub fn first<'a, T>(input: &'a [T]) -> Option<&'a T>
where
    'a: 'static,
{
    let slice: &'static [T] = input;
    slice.get(0)
}

[dtolnay]

I am on board with landing this unstable (with an appropriate tracking issue). But I haven't reviewed the implementation at all other than the signature.

[dtolnay]

Regarding #71780 (review): @jcotton42 it would be good to understand the error better. Can you put together a reproduction in the playground by stubbing out a Pattern trait and copying the method body from this PR?

Pattern<'a> is supposed to mean that it can search inside of data with lifetime 'a. Here we have incoming data with lifetime 'a that gets searched inside of, so any stricter trait bound should be fixable by fixing the implementation.

[jcotton42]

Sorry about the delay on testing the lifetime stuff, I've been having difficulties getting Rust to build on Windows, so I've switched to WSL in the meantime and am currently waiting for it to build so I can reproduce the lifetime error

[joshtriplett]

Looks good to me.

@bors r+

[bors]

📌 Commit a2571cf has been approved by joshtriplett

[bors]

⌛ Testing commit a2571cf with merge b1000219aa1d66a36070a3a334f793692a4c0316...

[Dylan-DPC-zz]

@bors retry yield (included in rollup)

[rust-log-analyzer]

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

[bors]

⌛ Testing commit a2571cf with merge eb95ace...

[bors]

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing eb95ace to master...

[frewsxcv]

I know this comment couldnt come at a worse time, but should this method be on &mut str instead of String?

…

On Thu, Mar 18, 2021, 11:29 PM bors ***@***.***> wrote: Merged #71780 <#71780> into master. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#71780 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADFWP4W3U6LQI3JQ5SZ2KTTELAIJANCNFSM4MXNXYUA> .

[joshtriplett]

It's still unstable, so it's not too late to make changes.

However, this method needs to be on String, because it mutates the string. An analogous method on str would need to make a copy.