Fix SORT_REGULAR with EG flag for transitive comparison

[jmarble]

Problem

array_unique() with SORT_REGULAR was missing duplicates, and sort() was producing inconsistent results depending on input order when comparing mixed types (numeric strings, non-numeric strings, integers, doubles).

The root cause: SORT_REGULAR was using PHP's comparison operators (which are non-transitive by design) directly in sorting algorithms that require transitive comparisons to function correctly.

When comparing these types, the comparison created cycles:

• "5" < "10" (numeric: 5 < 10)
• "10" < "3A" (lexicographic: '1' < '3')
• "5" > "3A" (lexicographic: '5' > '3') Creates a cycle

This violated the fundamental requirement of sorting algorithms: the comparison function must be transitive. This affected not only scalar strings but also nested arrays and objects with string properties.

Solution

Added EG(transitive_compare_mode) flag to zend_executor_globals that enforces transitive comparison during sorting operations, with consistent ordering that matches PHP 8+ semantics: empty-strings < numeric-types < numeric-strings < non-numeric.

Modified three comparison functions to check this flag:

• zendi_smart_strcmp() - handles string-to-string comparisons
• compare_long_to_string() - handles integer-to-string comparisons
• compare_double_to_string() - handles double-to-string comparisons

The flag is properly initialized in init_executor() and uses save/restore pattern in php_array_data_compare_unstable_i() to handle reentrancy correctly.

Important: This fix does not change the behavior of comparison operators like <=>, maintaining backward compatibility. The fix only affects sorting and array operations with SORT_REGULAR.

Tests

Added comprehensive tests:

• gh20262.phpt - Bug reproduction test covering scalars, objects, and nested arrays
• gh20262_bailout.phpt - Tests bailout handling to prevent mode leakage
• gh20262_reentrancy.phpt - Tests reentrancy handling in comparisons
• sort/sort_variation_numeric_strings.phpt - Edge case tests for numeric string handling
• sort/ksort_variation_numeric_strings.phpt - Tests transitive mode in key sorting

All existing array sorting tests pass without modification!

---

Fixes #20262

[jmarble]

@devnexen @nielsdos I reached out to @nikic regarding the four failed tests.

Update: Fixed! The tests were failing because:

1. Empty strings weren't being handled as a special case (they should sort before numbers)
2. The transitive_compare_mode flag wasn't initialized (caused issues on Windows ZTS)

[withinboredom]

Nice! Some suggested additional tests that could help:

$a = ["", "0", "00", "A"];
$b = ["A", "00", "0", ""];
sort($a, SORT_REGULAR);
sort($b, SORT_REGULAR);
var_dump($a === $b); // expect true
// and expected order: ["", "0", "00", "A"]?

$a = [" 5", "+5", "-0", "0", "A"];
sort($a, SORT_REGULAR);
// expect numeric ones before "A", retaining numeric ordering: ["-0","0","+5"," 5","A"] and stable or no (+5 vs [space]5 insertion order)?

$a = ["5e2", "500", "NAN", "INF", "-INF"];
sort($a, SORT_REGULAR);
// So: ["500","5e2", "-INF","INF","NAN"]?

$a = ["0x10", "16", "0b10000"];
sort($a, SORT_REGULAR);
// expect: ["16", "0b10000", "0x10"]?

$a = [10, "3A", 5, "10", ""];
sort($a, SORT_REGULAR);
// expect: ["", 5, 10, "10", "3A"]?

$a = ["9223372036854775807", "9223372036854775808", 9223372036854775807];
sort($a, SORT_REGULAR);
// ensure consistency across LONG_MAX vs double promotions

[iluuu1994]

Hi @jmarble. Sorry for the delayed response. One immediate concern is that this introduces an obvious inconsistency for key and value sorting. These two should remain consistent.

$units = ['5', '3A'];
sort($units);
var_dump($units);
// array(2) {
//   [0]=>
//   string(1) "5"
//   [1]=>
//   string(2) "3A"
// }

$units = ['5' => 1, '3A' => 1];
ksort($units);
var_dump($units);
// array(2) {
//   ["3A"]=>
//   int(1)
//   [5]=>
//   int(1)
// }

There's an obvious performance impact, given how common comparisons are (+0.4% instruction count as suggested by the Symfony Demo benchmark). Whether there's a true real-world impact remains to be seen.

This is related to GH-9882. Enums are also plagued by them not being properly handled in array_unique(), so I proposed to introduce List\unique() and Assoc\unique() functions. Before that, we have also discussed a ARRAY_UNIQUE_IDENTICAL flag much like in your original idea, which was disliked by internals. Sadly, there was a lot of bikeshedding regarding naming and namespaces, so the proposal died down. Regardless, this does not resolve your problem for sort() (which isn't a concern for enums).

I also wonder how this relates to @Girgias' efforts to make comparisons in PHP transitive. Maybe she can comment on that.

[jmarble]

Hi @iluuu1994 thank you for taking the time to review this so carefully. Admittedly, I got a bit too focused on resolving the issue I had run into. I'll push a commit here in a moment to resolve the discrepancy.

There's an obvious performance impact, given how common comparisons are (+0.4% instruction count as suggested by the Symfony Demo benchmark). Whether there's a true real-world impact remains to be seen.

The CI benchmark was actually only +0.04%, so only 0.04ms on a hypothetical 100ms request scenario. I think this is acceptable for correctness. We could always try compensating with a performance improvement somewhere.

I saw your email thread to internals regarding ARRAY_UNIQUE_IDENTICAL. I also emailed internals regarding the need for a "strict" comparison. I ended up creating a quick POC for an ARRAY_UNIQUE_STRICT` flag. I considered a formal RFC but got busy with other things.

[iluuu1994]

The CI benchmark was actually only +0.04%, so only 0.04ms

@jmarble You're right, sorry. I mistyped. It's not completely insignificant (performance improvements are very hard to come by nowadays), but unlikely to be a blocker if we otherwise decide this is the right behavior.

I do think it may be useful to consider consolidating comparison as a whole. Gina has done some work on that, as mentioned. Maybe we can solve this in a more consistent manner. If not, I don't object to your solution.

The array_unique case specifically may be better served with a case that doesn't rely on sorting. Some people have suggested introducing a proper Set class. However, this opens another can of worms because (in my very strong opinion) data types should have value type semantics, which isn't currently possible for classes.

[jmarble]

@iluuu1994 looks like we got a pretty favorable run on this last benchmark? A swing from +0.04% to -0.19% and faster across the board? Could be noise, or just more proof of negligible impact on performance.

I do think it may be useful to consider consolidating comparison as a whole.

I agree. Admittedly, I took the path of least resistance. I felt there was enough precedent for an EG flag.

The array_unique case specifically may be better served with a case that doesn't rely on sorting. Some people have suggested introducing a proper Set class. However, this opens another can of worms because (in my very strong opinion) data types should have value type semantics, which isn't currently possible for classes.

Yes, a proper Set class would most certainly open a can of worms haha. Prior to settling on this EG flag, I made some attempts to resolve the issue directly within array_unique with hash maps, but I found too many edge cases for my appetite, and I eventually realized that SORT_REGULAR was fundamentally flawed without transitive comparison.

[Girgias]

For context, I had started thinking about what comparison semantics PHP should have and some of it is written down in one of my draft RFC: https://github.com/Girgias/php-rfcs/blob/master/comparison-equality-semantics.md

However, not that comparison operators cannot be transitive, even if you just include numeric values due to NaN never being equal to itself.

In general I believe that getting rid of "ordering" for strings with the comparison operators is a better approach and have people use strcmp() instead.

However, this doesn't really address SORT_REGULAR. And I don't really understand why you are changing the implementation of zend_compare() when seemingly you could create a new php_array_sort_regular() function that handles the transitivity issue for the limited cases needed and then defer to zend_compare()?

[jmarble]

@Girgias thank you for sharing your RFC draft on equality semantics -- you make a compelling case!

To address your question: creating a standalone php_array_sort_regular() function would require duplicating not just the top-level comparison, but also zend_compare_arrays(), zend_objects_compare(), and their recursive call chains to handle nested structures (arrays containing arrays/objects with string properties).

Ultimately, I felt this path of least resistance was acceptable given the amount of refactoring that would be necessary to implement a proper alternative. Perhaps this solution might at least inspire someone who might be willing to make such a refactor?

In my recent commit, I found some opportunities to improve comparison performance. I noticed ZEND_THREEWAY_COMPARE() was more efficient than normal comparison, which I assume is due to better CPU optimization? I also implemented some "fast path" optimizations which provided a moderate performance gain over master (~30%) for integers with sort().

In real-world scenarios, I would expect no meaningful performance impact. However, the correctness bug this resolves is a demonstrable win.

[jmarble]

@Girgias Thanks again for the earlier feedback -- it pushed me to try the approach you suggested. I’ve opened a new PR that leaves zend_compare() alone and adds a php_array_sort_regular() fast path + shared helpers so we only enforce transitivity when SORT_REGULAR actually needs it. Would really appreciate your take on the detection logic, optimizations, and helpers when you have a moment.