Fix SortArray crashing with bad comparison functions

[poke1024]

Fixes #3327
Fixes #10968

In master, SortArray crashes if the comparison function is not good (i.e. not totally ordered). Note that a comparator is already bad if it implements <= instead of <.

This PR prevents crashing with bad comparators and outputs nagging error messages instead (quite a lot, you won't miss them).

I ran an obligatory stress test against the sorter with good comparators for about 30 minutes, indicating that neither did the usual functionality got broken, nor does any of the three conditions this PR asserts as errors occur in normal operation:

extends Node2D

func _process(delta):
    test_sort()

func test_sort():
	randomize()
	var a = []
	for i in range(2 + (randi() % 1000)):
		a.append(randf())
	a.sort()
	for i in range(len(a) - 1):
		assert(a[i] <= a[i + 1])

DETAILS

The sorting code is complex and relies on many implicit assertions, so here is some lengthy rationale why this is correct.

There seem to be three areas where problems with a bad comparison function could happen inside SortArray (note that I'm using Python slice notation, i.e. x[a:b] excludes x[b]):

(1) In unguarded_linear_insert.

With a totally ordered comparison function, compare(p_value, p_array[next]) will never a return true here for next == 0.

For the sake of shortness, let's just state that if it did, the code would be erroneous, as next-- would set next to -1 and would next access the out-of-bounds p_array[-1] in the while head.

We can safely assume that if compare returns true for next == 0, the comparator is bad here.

Longer explanation: for good comparators, the condition just described (and checked for in this PR) will never happen:

• if called from linear_insert this is achieved via checking if (compare(val, p_array[p_first])) before the call, thus p_value will never be smaller than some lower index element at p_first >= 0.
• if called from unguarded_insertion_sort there will have been a clever (bit obscure) partitioning via introsort beforehand (see sort_range) that always ensures some smaller item in the lower partition of the array (i.e. the call to unguarded_insertion_sort(p_first + INTROSORT_THRESHOLD, p_last, p_array); can never insert an item that will be smaller than all items inp_array[p_first, p_first + INTROSORT_THRESHOLD]).

(2) In partitioner.

In simple terms: p_first, going forward, must hit p_pivot (with a false compare) before going beyond the end of the partition range, and p_last, going backward, must hit p_pivot (with a false compare) before going before its beginning. If all compares, including the compares on the boundaries, return true, the comparator is bad.

Longer explanation: first note that for all calls of partitioner, p_pivot is always an element from p_array[p_first:p_last] (computed via median_of_3).

That means there must an x, p_first <= x < p_last, such that p_array[x] == p_pivot and thus compare(p_array[x], p_pivot) == false.

The two ERR_BAD_COMPARE checks added by this PR check if the condition just stated fails, thus indicating that the comparator must be bad (e.g. for p_first this happens when p_first gets incremented until it reaches the original unmodified p_last - 1 without hitting upon p_pivot).

Note that the p_first++; at the end of the while loop is always safe, as we know from if (!(p_first < p_last)) before that p_first < p_last < unmodified_last, i.e. p_first < unmodified_last - 1, i.e. p_first + 1 < unmodified_last.

(3) The heap functions there should be no problem. push_heap's loop is guarded by parent, and adjust_heap's loop is guarded by second_child.

[reduz]

I don't really think it's a problem that it crashes, though the fix does not really seem to affect performance much. I prefer we merge this after 3.0 as it's not urgent

[vnen]

This should be a debug-only thing, right? Just to inform naive developers that the sort function must be consistent.

[poke1024]

I'm not sure. I'm not against having it in production as well, as the overhead is one additional register compare per iteration without memory access, which will be very hard to measure. While active, it can catch subtle errors in a comparator that might not crash otherwise but just produces wrong sort results (it can make it fail earlier than the crash). Godot has a lot of ERR_FAIL in production code, which is a similar approach.

[Zireael07]

I think it should go in production to avoid people's exported games crashing.

[poke1024]

@Zireael07 That was my idea as well.

[reduz]

I think I would do the following with this PR. Truth is that in some places this code is very performance critical (renderer), so the extra check is undesired. But it is true that for the case you mention (GDScript) it can be useful:

1. Add an extra template argument, by default false bool validate = false , so you can do if (validate)
2. When used from user code (Array in this case), create it with this set as true, otherwise leave as false.

[reduz]

And of course, enabling it always if DEBUG_ENABLED is true

[poke1024]

Changed accordingly via a new template parameter Validate.

I would actually be surprised if this had a performance impact on desktop CPUs, as it's just one unlikely compare of two values that are already in registers, and they should happen for free while waiting for the memory read of the next loop iteration's compare values. Then again, I'm not familiar with mobile CPUs and cross compilers to webasm, and who knows what happens there.