Make split_inclusive() on an empty slice yield an empty output

[martinvonz]

[].split_inclusive() currently yields a single, empty slice. That's
different from "".split_inslusive(), which yields no output at
all. I think that makes the slice version harder to use.

The case where I ran into this bug was when writing code for
generating a diff between two slices of bytes. I wanted to prefix
removed lines with "-" and a added lines with "+". Due to
split_inclusive()'s current behavior, that means that my code prints
just a "-" or "+" for empty files. I suspect most existing callers
have similar "bugs" (which would be fixed by this patch).

Closes #89716.

[rust-highfive]

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

Please see the contribution instructions for more information.

[cuviper]

Hmm, slice's behavior is consistent with split, so it seems to me that str is the odd one.

That is, str::split, [_]::split, and [_]::split_inclusive all return an empty item, but str::split_inclusive does not. The difference might be related to str::split_terminator, which does not yield a final unterminated empty string.

[martinvonz]

The difference might be related to str::split_terminator, which does not yield a final unterminated empty string.

Yes, I think so. There was some discussion in #67330 about that, and they decided to match str::split_terminator.

I spent some time going through occurrences of split_inclusive on GitHub but it was hard to find examples where it would make a difference because most were already checking their inputs in unrelated ways (not just for emptiness). But see how I worked around it in my repo if you want an example: martinvonz/jj@c0a26f7

[joshtriplett]

I think it makes sense to match the behavior of not emitting a final empty item when the thing to split ends with a terminator.

That's separate from the question of what splitting an empty string/slice should yield. There's a consistent interpretation either way that could justify either behavior. We should figure out which one makes more sense, and also make sure we can still make a change here without breakage.

[joshtriplett]

FWIW, my first instinct would be for split on an empty string/slice to yield no items. I would prefer to go with whatever is most likely to be both unsurprising and minimize special-cases in callers. Based on the reports thus far, I think yielding no items would eliminate more special cases.

[martinvonz]

We should figure out which one makes more sense, and also make sure we can still make a change here without breakage.

Agreed. I think the worst outcome would be if the str and the slice versions end up behaving differently forever. I suspect you'll find more more failures from Crater if you instead change the str version to yield a single empty string.

[joshtriplett]

I'd be in favor of making this change while we can, to not yield an empty slice.

What are the chances we can make the same change to []::splitand []::split_mut?

[m-ou-se]

An overview of the current behaviour:

"..".split('.') = ["", "", ""]
 ".".split('.') = ["", ""]
  "".split('.') = [""]

[0, 0].split(|_| true) = [[], [], []]
   [0].split(|_| true) = [[], []]
    [].split(|_| true) = [[]]

"..".split_inclusive('.') = [".", "."]
 ".".split_inclusive('.') = ["."]
  "".split_inclusive('.') = []

[0, 0].split_inclusive(|_| true) = [[0], [0]]
   [0].split_inclusive(|_| true) = [[0]]
    [].split_inclusive(|_| true) = [[]]

"x.x.x".split_inclusive('.') = ["x.", "x.", "x"]
  "x.x".split_inclusive('.') = ["x.", "x"]
    "x".split_inclusive('.') = ["x"]

[1, 0, 1, 0, 1].split_inclusive(|x| *x == 0) = [[1, 0], [1, 0], [1]]
      [1, 0, 1].split_inclusive(|x| *x == 0) = [[1, 0], [1]]
            [1].split_inclusive(|x| *x == 0) = [[1]]

Not sure which of these would be the odd one out:

• "".split_inclusive('.') = []
• [].split_inclusive(|_| true) = [[]]

[martinvonz]

An overview of the current behaviour:

Thanks for sharing that.

Not sure which of these would be the odd one out:

• "".split_inclusive('.') = []
• [].split_inclusive(|_| true) = [[]]

I think it's clearly [].split_inclusive() that doesn't fit the pattern in the examples you shared. In all the other cases, the number of elements in the outer list drop by 1.

[m-ou-se]

@rfcbot merge

[rfcbot]

Team member @m-ou-se has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @yaahc

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[martinvonz]

@Amanieu, @BurntSushi, @yaahc: Can I get a review from either of you?

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

[martinvonz]

@m-ou-se: Does this need anything else from a human or does it just take this long for automation to get to it?

[m-ou-se]

Thanks for the ping!

@bors r+

[bors]

📌 Commit f6e4c74 has been approved by m-ou-se