std.sort.pdq use correct recursion depth

[oittaa]

Fixes #24399

[oittaa]

Well, I tried to fix the bug but the checks just keep failing. I don't understand how this minor change can break the builds.

[mlugg]

Take a look at the start of the stack trace:

thread 3949973 panic: reached unreachable code
/home/ci/actions-runner9/_work/zig/zig/lib/std/debug.zig:559:14: 0x55dc66b in assert (zig)
    if (!ok) unreachable; // assertion failure
             ^
/home/ci/actions-runner9/_work/zig/zig/lib/std/math.zig:1264:11: 0x5644157 in log2_int__anon_26847 (zig)
    assert(x != 0);
          ^
/home/ci/actions-runner9/_work/zig/zig/lib/std/sort/pdq.zig:47:36: 0x6d8694f in pdqContext__anon_612807 (zig)
    const max_limit = math.log2_int(usize, b - a) + 1;

We're failing an assert(x != 0) in the call to log2_int. It's easy to guess that x will be the argument, but of course, you can always just check that in the code if you want:

zig/lib/std/math.zig

Lines 1261 to 1264 in 1c73b25

	pub fn log2_int(comptime T: type, x: T) Log2Int(T) {
	if (@typeInfo(T) != .int or @typeInfo(T).int.signedness != .unsigned)
	@compileError("log2_int requires an unsigned integer, found " ++ @typeName(T));
	assert(x != 0);

Yep -- so, we're calling log2_int with an argument of 0, which isn't allowed. Well, b - a == 0 is the same as b == a! So, pdqContext is being called with b == a; in other words, it's being asked to sort zero elements, which causes this problem becuase log2(0) isn't allowed.

Since there are no steps involved in sorting zero elements, you should be able to solve this simply by short-circuiting that case at the very top of the function like this:

if (a == b) return;

[alichraghi]

I think it's better to assert len > 0in all sorting functions and check the length at call site

[oittaa]

I think it's better to assert len > 0in all sorting functions and check the length at call site

Isn't that a bit dangerous? All the popular programming languages just do nothing without errors. That might also break some existing Zig programs.

On the other hand selection algorithms are a bit more complicated.

• C++ to me seems to have undefined behavior https://en.cppreference.com/w/cpp/algorithm/nth_element.html
  GCC: return without errors https://github.com/gcc-mirror/gcc/blob/master/libstdc%2B%2B-v3/include/bits/stl_algo.h#L4806-L4807
  LLVM: return without errors https://github.com/llvm/llvm-project/blob/main/libcxx/include/__algorithm/nth_element.h#L58-L62
• Rust panics https://doc.rust-lang.org/std/primitive.slice.html#method.select_nth_unstable

IMHO sorting algorithms should work with zero length slices, but with selection algorithms we should behave more like Rust. At least check with assert(len > 0) like we do now with #24414

[mlugg]

I don't see any reason sorting functions should require a non-empty input. Sorting a zero-length slice is well-defined with obvious behavior, and it requires either no special handling or in the worst case one trivial check.