bugfixes for rsplit (for unicode and empty strings)

[stevengj]

Fixes #26225 (rsplit failure for strings with multi-byte Unicode characters).

Also, I noticed that rsplit was broken in both 0.6 and 0.7 for empty splitter strings: split("abc", "") gives ["a","b","c"] (correct, I think — and there was a test for this so it was clearly the intended behavior), but rsplit("abc", "") on 0.6 incorrectly gives ["ab","c"]. In this PR, they both give ["a","b","c"], and the case with limit=2 is also tested.

The new rsplit implementation is both more correct (I think) and shorter.

I also noticed that both rsplit and split were incorrectly(?) using start(string) to get firstindex. Or is this always guaranteed to return 1?

Finally, I simplified the split/rsplit tests by replacing isequal with ==, since the two should be equivalent for strings. == is more readable, allowed me to use chained comparisons, and gives better diagnostic output for @test failures.

[stevengj]

@StefanKarpinski, is firstindex(string) guaranteed to always be 1 for all AbstractString types? If so, the code could be simplified further (and the docs should be clarified). If not, the _split implementation (and possibly a lot of other Base string code) is wrong.

Update: From this comment, "We want indices to always be 1 through ncodeunits(s)." I will update the PR accordingly.

[stevengj]

CI failures seem unrelated. Timeouts on FreeBSD and Win64, and rmdir: Permission denied on Win32.

Okay to merge?

[nalimilan]

Do we really need to assert this? Why do it here and not elsewhere? I think implementations which get this wrong will realize it quite soon even without assertions given that may things will fail.

[stevengj]

I'm asserting it because the subsequent code implicitly relies on it. Basically, I think of this as a comment with some added value. Assertions are cheap so we shouldn't hesitate to add them, no?

[vtjnash]

(i = firstindex(str)) though goes against the recently added documention that use of @assert might not work in the future (#25610)

[stevengj]

Okay, will fix.

[StefanKarpinski]

Not really since the argument here is to delete it which is exactly what the compiler would do. On the contrary, this is exactly the way that @assert should be used.

[stevengj]

@StefanKarpinski, the point is that you can't completely delete this code because it contains an assignment. Hence this is not correct usage of @assert.

[StefanKarpinski]

Ah, yes. That's true. I missed that bit.