[php-src] PR #23505: Fix inconsistent min()/max() tie-breaking on signed zero via the frameless 2-arg fast path
| From: | 2akouwu | Date: | Sat, 29 Aug 2026 13:07:00 +0000 |
| Subject: | [php-src] PR #23505: Fix inconsistent min()/max() tie-breaking on signed zero via the frameless 2-arg fast path | ||
| Groups: | php.git-pulls | ||
| Request: | Send a blank email to git-pulls+get-38199@lists.php.net to get a copy of this message | ||
Pull Request: https://github.com/php/php-src/pull/23505
Author: 2akouwu
## Root cause
min($a, $b) and max($a, $b) calls with exactly two positional arguments
are compiled by the engine to the ZEND_FRAMELESS_FUNCTION(min, 2) /
ZEND_FRAMELESS_FUNCTION(max, 2) fast paths in ext/standard/array.c (wired
up in Zend/zend_compile.c for any call site whose argument count matches a registered
frameless variant), bypassing the normal argument-array based implementation.
ZEND_FRAMELESS_FUNCTION(max, 2) already resolves ties with >=, so when
the two operands compare equal it keeps lhs (the first-seen operand). That matches
php_array_data_minmax() (used for min(array)/max(array)) and
the variadic PHP_FUNCTION(min)/PHP_FUNCTION(max) implementations, both of
which only replace the running result on a *strict* improvement and therefore also keep the
first-seen operand on a tie.
ZEND_FRAMELESS_FUNCTION(min, 2) was the one path that didn't follow this
convention: every comparison used strict <, so on a tie it returned rhs
(the second operand) instead of lhs. This is invisible for most values, but
-0.0 and 0 compare equal under ordinary IEEE-754 double comparison while
still being distinguishable via var_dump() (float(-0) vs
int(0)). That's exactly what round(-0.01 / 2, 0) produces
(-0.0), so:
```php
var_dump(min(round(-0.01 / 2, 0), 0)); // returned int(0) -- wrong operand picked on tie
var_dump(max(round(-0.01 / 2, 0), 0)); // returned float(-0) -- correct, first operand kept
```
min() and max() disagreeing on which operand "wins" a tie, given the exact same two
operands in the exact same order, is the actual bug -- not merely a cosmetic -0
formatting issue.
## Why this fix
I considered special-casing signed zero directly (e.g. using signbit() so min() always
prefers the negative-zero representation and max() always prefers positive zero, regardless of
argument order, similar to Math.min/Math.max in some other languages). I
rejected that: PHP's comparison operators and zend_compare() have no such
total-ordering concept for -0.0 anywhere else in the engine (array
min/max, the variadic min/max,
<, >, sort callbacks, etc. all treat -0.0 == 0.0), so
introducing it only for the 2-arg frameless path would create a *new*, narrower inconsistency
instead of fixing the existing one.
Instead, the minimal, targeted fix is to make the frameless min() tie-break the same
way as every other min/max code path already does: keep the first-seen operand on a tie. Concretely,
this changes the three < comparisons (long/long, double/double, double/long) and the
zend_compare(...) < 0 generic fallback in ZEND_FRAMELESS_FUNCTION(min,
2) to <=, mirroring the >= already used in
ZEND_FRAMELESS_FUNCTION(max, 2). No behavior changes for non-tied values; only the
tie-break operand selection changes, and only for the frameless 2-arg path.
## Testing
Added ext/standard/tests/array/gh20221.phpt, following this project's
gh<issue>.phpt naming convention (see e.g. gh18480.phpt,
gh17977.phpt in the same directory). It reproduces the issue's exact scenario
(-0.0 vs 0) through the 2-argument frameless calls in both operand orders
for both min() and max(), and cross-checks the same values through the
3-argument variadic path and the array-argument path
(min([...])/max([...])), asserting that the frameless results now agree
with those already-correct paths.
I could not build the PHP interpreter in this sandboxed source-only bundle (no toolchain/build
artifacts are present), so the .phpt was not executed through
run-tests.php. I traced the expected output by hand against the modified control flow
in ext/standard/array.c for every branch the test exercises (long/long, double/double,
double/long, and both operand orders), which is why the test includes the redundant array/variadic
cross-checks -- so a reviewer running it in CI has an internally-consistent oracle, not just my
hand-derived expectations.