Safer sort partition

[kornelski]

This reduces amount of unsafe code in partition(), while generating essentially the same code.

partition() and functions calling it can only run into out-of-bounds issues if the is_less function is buggy, so I've used cold panic_on_ord_violation() to replace various other panics/aborts. This slightly reduces code size, and gives a more relevant error message.

Related to #144327

[rustbot]

r? @scottmcm

rustbot has assigned @scottmcm.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[workingjubilee]

cc @orlp @Voultapher you may find this interesting (but you don't have to)

[Voultapher]

partition() and functions calling it can only run into out-of-bounds issues if the is_less function is buggy,

I'm not sure that's true. Looking at for example this condition if len == 0 -> abort it doesn't happen because is_less is buggy. It can only happen if future versions of the code calling the function break assumptions. IIRC the only place that can be caused by a buggy is_less is the one currently calling panic_on_ord_violation.

I'm all for reducing unsafe and a smaller code-size, with this PR as is, it might lead to confusing error messages and incorrect understanding of the code in the future. Maybe we can achieve a similar goal using the Option approach but still call abort?

[kornelski]

I mean only is_less can trigger panic_on_ord_violation under assumption that the implementation itself isn't buggy. is_less is used indirectly via partition_lomuto_branchless_cyclic, which is non-trivial and could be buggy itself, so there's never going to be a case that is 100% certain. But it could be helpful in practice.

The additional checks piggyback on returning None, because that's the simplest implementation. Distinguishing between checks in partition would require a Result with a custom Error type and/or moving checks around. That's more code, potentially more branches, and I doubt it's worth it. I could add more debug_asserts?

I still think it's an improvement over the current version of partition, where if len == 0 case happened, it could make partition_at_index_loop just return without reporting any issue, while failures most likely caused by is_less just abort() without explaining why.

[Voultapher]

The arguments you present don't make sense to me.

• It's not just guess work what happens with buggy is_less implementations. The Rust CI an local development has been fuzzing the code for over a year now [1] and I've yet to hear of any aborts.

• While the code is certainly complex it's important to not overgeneralize. len == 0 is a bug caused by the caller of partition not partition itself. So the complexity of the underlying partition implementation isn't relevant. The other part where it could theoretically return a partition index >= len is related to the partition implementation. But in practice for each slice element processed the partition index can be incremented by zero or one. So it is guaranteed < len. What elements are processed does not depend on the outcome of is_less so whether or not the implementation is buggy doesn't play any role here [2]. The simplified variant shows this more clearly, but it's true for both. Nothing other than bidirectional_merge and merge have loop bounds depending on the outcome of is_less. insert_tail has an early exit, but other than that everything else has a loop bound independent of is_less and can be verified accordingly.

• If some future change of the code introduces bugs that would trigger one of the current aborts I don't see how telling our users that "hey your comparison function is buggy" when it's not would help. It's more likely to conceal and suppress reports of it.

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[kornelski]

ok, if reporting is_less is not a concern, then I've simplified it significantly to just aborting.

[Voultapher]

LGTM

View changes since this review

[scottmcm]

In a narrow review this seems reasonable to me, and I'll lean on #147378 (review) for it being ok more broadly.

@bors r+ rollup=iffy (don't think it'll be a concern in the CI builds, but might be nice to have in an uncomplicated commit for bisecting)

---

As an aside, I might steal the or-abort trick for some other places as a less-mono-needed version of some places I have asserts that can't actually fail but do help the optimizer.

[bors]

📌 Commit ac2b421 has been approved by scottmcm

It is now in the queue for this repository.

[Zalathar]

If the goal is to hopefully get a standalone merge, without an increased chance of CI failure, I think rollup=never is more appropriate. Iffy won't help if this ends up being the one iffy PR in a big rollup.

@bors rollup=never