fix(codegen): PHP semantics for division, modulo, shifts and compound assignment on typed scalars - #45
Conversation
matyhtf
left a comment
There was a problem hiding this comment.
Thank you for splitting this work out of #39. The general direction is correct: non-native_types division, modulo, and shifts must not fall through to C++ operations with different semantics or undefined behavior, and a literal zero divisor should remain a catchable runtime error rather than reject dead code at compile time.
However, the compound-assignment and increment/decrement part is not safe to merge yet. It computes through Variant, but then parseAssign() coerces the result back into the existing native php::Int/php::Float slot. A PHP parameter type constrains the value at function entry; it does not prevent that local variable from changing type later.
For example:
function divParam(int $value): mixed
{
$value /= 2;
return $value;
}
function incParam(int $value): mixed
{
$value++;
return $value;
}With this PR, divParam(7) returns int(3) instead of float(3.5), and incParam(PHP_INT_MAX) returns PHP_INT_MIN instead of promoting to float. The same problem affects overflowing +=, -=, and *=.
Float compound operations expose two more cases:
function modFloat(float $value): mixed
{
$value %= 2;
return $value;
}
function shiftFloat(float $value): mixed
{
$value <<= 1;
return $value;
}PHP produces an int result for these operations. This PR coerces %= back to float, while the shift currently generates invalid C++ similar to double << double and fails during native compilation.
The existing divEven(8) test hides the division issue by using an evenly divisible value and converting the result to string. Please add runtime coverage for:
intparameter7 /= 2, returningmixed;PHP_INT_MAX += 1andPHP_INT_MAX++on parameters;- float
%=returning an integer value; - float
<<=/>>=compiling and returning PHP-compatible integer values.
Suggested revision:
- Keep the ordinary binary division/modulo/shift and literal-zero fixes.
- Remove the typed-local compound assignment and
++/--lowering from this PR for now, or first make analysis represent every potentially type-changing local asphp::Varinstead of coercing the result back into a native slot. - Rebase on the latest master before revising the integer-division path, because that area has recently gained an inline PHPX integral-RHS fast path and should not restore unnecessary RHS boxing.
I verified the PR's own targeted PHPUnit and PHPT tests successfully, but the cases above reliably reproduce incorrect output or a C++ compilation failure. Please update the implementation and tests before this is merged.
… operators Typed int/int and float-typed division fell through to a raw C++ '/': 7 / 2 on zend_long operands truncated to 3 where PHP returns 3.5, integer division by zero was undefined behavior and float division by zero produced INF, while PHP raises a catchable DivisionByZeroError in both cases; PHP_INT_MIN / -1 also has UB in C++ but promotes to float in PHP. The '%' guard only routed through php::fn::mod when NOT both operands were int, so both-int modulo kept raw C++ '%' (UB for a zero divisor and for PHP_INT_MIN % -1, which PHP defines as 0). Dynamic int shifts were raw C++ too: PHP defines counts >= the word size as 0 (or -1 for negative right shifts) and raises ArithmeticError for negative counts, both undefined in C++. Route all of these through the encapsulated php::Var operators / php::fn::mod in non-native mode, matching the existing +/-/* pattern. Constant folds are untouched; constant shifts that C++ defines identically to PHP still emit raw operators.
…oError
A literal `/ 0` or `% 0` (including `/=` and `%=`) was a compile-time
fatal, rejecting valid PHP: Zend compiles it and raises a catchable
DivisionByZeroError only when the statement executes, so dead or
guarded code like `if ($cond) { $x = 1 % 0; }` must compile. The
equivalent spellings `1 % (1 - 1)` and `10 / ZERO` were already
accepted and lowered to the catchable runtime error.
Give the literal spelling the same lowering: route the operation
through the encapsulated Variant operators (compound assignments on
Variant slots already defer via operator/= and operator%=), keep a
compile-time warning in normal mode, and keep the fatal in native mode
where the C++ operation would be undefined behavior.
The six OperatorTest cases asserting the old compile-time fatal now
assert the runtime-error lowering instead.
…ped ints The literal-division assertions hardcoded the macOS zend_long suffix (LL); Linux emits L, so they now match either. native-type.phpt asserted the truncating int division this change removes: division on typed int operands follows PHP semantics in non-native mode, consistent with the pre-existing + - * routing (use native_types keeps raw division), so std::int(10) / 4 is now float(2.5).
|
Revised as suggested:
Re-verified on the rebased branch: targeted PHPUnit suites and the operator/type_hits/loop/object_property phpt front-end sweep are green, and |
1bf87fa to
6c34d96
Compare
matyhtf
left a comment
There was a problem hiding this comment.
Thank you for revising the PR. The unsafe compound-assignment and increment/decrement lowering from the previous version has been removed as requested, so the earlier type-changing-local issues are no longer introduced by this branch. The remaining binary division/modulo/shift direction is sound, and the PR's targeted tests pass.
There is still one blocking regression caused by changing every literal-zero compound assignment from a compile-time fatal error to a warning.
A typed native slot continues to use the raw C++ compound operator:
function divInt(int $value): mixed
{
try {
$value /= 0;
} catch (DivisionByZeroError $e) {
return $e->getMessage();
}
return $value;
}The generated code is:
value /= php::toInt(0L);This does not enter the PHPX Variant error path. In an end-to-end PHPT it terminates the process with:
Floating point exception (core dumped)
The same issue affects:
int $value; $value %= 0, which is raw C++ modulo by zero;float $value; $value /= 0.0, which producesINFinstead of throwing;- native typed locals created through paths such as
std::int().
The new PHPT only tests $v = 10; $v /= 0, where $v is a php::Var, so it does not cover this path.
Please ensure literal-zero /= and %= on native scalar slots also raise a catchable DivisionByZeroError without modifying the left-hand side. A focused lowering through a temporary php::Var is possible because a proven zero divisor always throws before assignment. If that cannot be implemented safely in this PR, keep the existing compile-time rejection for native scalar slots until the separate typed compound-assignment work is ready.
Please add runtime coverage for typed int division/modulo assignment, typed float division assignment, and an explicit std::int() local.
Separately, binary shifts with typed float operands still generate invalid C++ such as double << double. That is a pre-existing gap rather than a regression from this revision, so it may be handled in the later typed-operator PR, but the current title/description should avoid implying that every typed scalar shift is covered.
…runtime error Downgrading the literal-zero compile fatal to a warning exposed the raw C++ compound path on typed native slots: `int $value; $value /= 0` compiled to `value /= php::toInt(0L)` and killed the process with SIGFPE instead of the catchable DivisionByZeroError (`%= 0` likewise; float `/= 0.0` produced INF). A proven zero divisor always throws before any assignment happens, so the whole compound lowers to the PHP-semantics binary operation through php::Var and the target is left untouched. Native-types mode keeps the compile-time rejection.
|
Fixed. A literal zero divisor in Added the runtime PHPT you asked for: typed int Also updated the PR description with a scope note: binary shifts with typed float operands ( |
Typed
int/floatdivision fell through to a raw C++/:divInts(7, 2)withintparameters returned3.0instead of3.5,/ 0was undefined behavior instead of a catchableDivisionByZeroError, andfloat / 0.0producedINF. Both-int%kept the raw C++ operator (the existing guard only routed the mixed case throughphp::fn::mod), and dynamic<</>>were UB for counts ≥ 64 or negative. The same divergences applied to compound assignment and++/--on typed local slots (7 /= 2gave3,+=overflow wrapped).This routes dynamic division/modulo/shifts on typed scalars through the encapsulated
php::Varoperators — exactly the pattern the existing+ - *routing already uses, and per its comment, only outsideuse native_types— and lowers compound assignment/incdec on typed local slots to the equivalent plain assignment. Literal/ 0and% 0now warn and produce the catchable runtime error like the equivalent1 % (1 - 1)spelling already did, instead of refusing to compile guarded/dead code. Constant folds are untouched; typed-property compound assignment is handled by af466b0 and untouched here.type_hits/native-type.phptasserted the truncating division this removes (std::int(10) / 4 === int(2)); its expectation becomesfloat(2.5), consistent with the+ - *contract. Verified against Zend 8.4.13; new phpt tests carry Zend-validated expectations.Part of the split of #39.
Scope note: binary shifts with typed float operands still generate invalid C++ (
double << double) — a pre-existing gap that this PR does not cover; it belongs to the follow-up typed compound-assignment work together with float%=/<<=.