fix: 27 correctness divergences from Zend 8.4 across codegen, folding, and declaration validation - #39
Open
AlessioGiacobbe wants to merge 40 commits into
Open
Conversation
… 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.
Unary minus concatenated '-' directly onto the operand's generated C++.
For a compound operand the minus then bound to the wrong subexpression:
PHP's -($a ? $b : $c) emitted `-cond ? b : c`, which C++ parses as
`(-cond) ? b : c` — the negation lands on the condition and the branch
choice itself can flip (pick(1,2,3) returned 2 instead of -2). The
previous str_starts_with('-') guard only covered operands already
beginning with '-' (the `--` token-pasting case).
Emit '-(' operand ')' unconditionally; this subsumes the pre-decrement
guard. Unary '+' emits no operator text and boolean/bitwise not already
close their operands, so they are unaffected.
PHP evaluates the right-hand side of ??= lazily: `$a = 1; $a ??= sideEffect() + 1;` never calls sideEffect(). When the RHS was a compound expression the compiler materialized its lowered statements (the call result temporary) into the enclosing statement context, so the generated C++ executed the side-effecting call unconditionally before the isset check. Generalize the conditional-lambda lowering that already protected native-object targets: whenever the RHS captured before/after statements, emit an immediately-invoked lambda whose not-set branch contains those statements, the assignment and the cleanup. The simple inline form (`$b ??= f()`) keeps its existing conditional-expression codegen unchanged.
A git worktree shares vendor/ with the primary checkout via a symlink, and Composer's generated autoloader resolves the TypePhp\ prefix relative to the realpath of vendor/. The suite then silently loads and tests the OTHER checkout's src/ tree. Prepend an autoloader anchored to this checkout so the tests always exercise the sources they ship with; in a standalone checkout this is a no-op.
Zend's inheritance check treats return-by-reference as covariant
(zend_do_perform_implementation_check): an error is raised only when the
parent returns by reference and the child does not. The child adding `&`
is a strictly stronger guarantee and is accepted:
class A { public function f(): array {} }
class B extends A { public function &f(): array {} } // OK in Zend
validateMethodOverrideSignature compared returnsByRef with exact equality,
rejecting this valid program. Make the check one-directional; dropping a
parent's by-ref return remains fatal.
Zend's zend_do_perform_implementation_check does not compare variadic-ness
per position. Its rules are:
- a variadic parent requires a variadic child (unbounded contract);
- a trailing child variadic stands in for every remaining parent
position (decorator pattern), with the variadic's type checked for
contravariance against each covered parent parameter and by-ref-ness
matched per position;
- when the parent is variadic, extra child parameters are validated
against the parent's variadic slot.
validateMethodOverrideSignature required an exact per-position variadic
match, rejecting valid programs such as parent f(int $a, int $b)
overridden by f(int ...$args). Rework the position loop per the Zend
rules; the required-argument-count and extra-optional-parameter checks
are unchanged.
The pre-existing testVariadicMismatch expectation (untyped f($x)
overridden by f(...$x) must fail) contradicts Zend 8.4, which accepts
it; the test now asserts the program compiles.
checkParentMethodCanBeOverridden()'s internal-parent branch only checked
the PRIVATE and FINAL modifiers via reflection and then stopped, so an
override of any Zend built-in method was never signature-checked:
narrowed parameters, static/instance mismatches, narrowed visibility and
incompatible real return types were all accepted (all fatal in Zend,
e.g. "Declaration of C::offsetGet(int $key): string must be compatible
with ArrayObject::offsetGet(mixed $key): mixed").
Add validateInternalMethodOverrideSignature(), mirroring Zend's
zend_do_perform_implementation_check on host ReflectionMethod data:
- visibility may widen but not narrow; staticness must match;
- a by-ref return may be added but not dropped;
- the child may not require more arguments; extra parameters must be
optional or variadic;
- parameters are contravariant with invariant by-ref-ness, and a
trailing child variadic absorbs remaining parent positions (a
variadic parent requires a variadic child);
- the return type is covariant, enforced ONLY for real return types:
ReflectionMethod::getReturnType() is null for TENTATIVE return
types, which Zend merely deprecates on mismatch, never fatals.
ReflectionType data (named/nullable/union/intersection, incl. self,
parent and static) is mapped into the existing accepted-types DNF so the
comparison reuses isReturnTypeCoveredBy()/isAcceptedTypeSubset().
checkParentMethodCanBeOverridden() returned immediately for
__construct, so overriding a FINAL parent constructor was accepted
(Zend: "Cannot override final method A::__construct()") and an
ABSTRACT parent constructor's signature was never validated (Zend
checks it exactly like an interface constructor).
Zend's constructor rules (zend_do_inheritance):
- a concrete parent constructor imposes no signature contract: the
child may change parameters and even narrow visibility — this
exemption is kept;
- a private parent constructor may be redeclared freely, but FINAL
still wins: `final private function __construct()` cannot be
overridden (constructors are the one place PHP allows final
private);
- an abstract parent constructor's signature is a real contract.
Keep walking the parent chain for constructors, skipping only the
private-override error and the concrete-signature validation; final
checks (userland and built-in parents) and abstract-constructor
validation now run.
checkPropertyOverride() compared type, visibility, set-visibility, readonly and final between a child property and the parent's, but never Modifiers::STATIC. Redeclaring `public static int $x` as `public int $x` (or the reverse) was accepted, while Zend fatals with "Cannot redeclare static A::$x as non static B::$x" (and "Cannot redeclare non static ... as static ..." in the other direction): static and instance properties are different kinds of storage and can never override one another.
checkConstantOverride() required exact type equality between a child constant and the parent's declared type, rejecting valid PHP 8.3 programs: class constant types are covariant, so a child may narrow (parent `const int|string X` overridden by `const int X`, or `?int` by `int`) but never widen or move to an unrelated type (Zend: "Type of B::X must be compatible with A::X of type int"). Composite declared types (unions, nullables) were also collapsed to a single variant type at parse time, making them unrepresentable in the check. ConstantDef now records the accepted-types DNF of its declared type (built by the existing buildTypeCheckFromNode machinery in a parseClassConstDef override, while the declaration's name-resolution context is still active), and the override check reuses the DNF clause-subtyping used for covariant returns. Untyped parent constants remain unchecked, and a typed parent still requires a typed child.
parseUnaryMinus now always parenthesizes its operand, so the -INF float literal is emitted as -(std::numeric_limits<double>::infinity()). The C++ value is unchanged; only the spelling assertion needed updating.
…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.
…ots to PHP semantics Compound assignment on a native int slot (typed parameters, int-typed native properties) emitted the raw C++ compound operator: `$a += $b` had undefined signed overflow where PHP promotes to float, `$a /= $b` truncated zend_long division (7 /= 2 gave 3, PHP 3.5) and missed the catchable DivisionByZeroError, `%= 0` and out-of-range shift counts were undefined behavior. Raw `++`/`--` on those slots had the same overflow UB at PHP_INT_MAX/PHP_INT_MIN. Lower `$x op= $y` on int slots (+= -= *= /= %= <<= >>=) and on float slots (/= %= <<= >>=) to the equivalent plain assignment `$x = $x op $y`, reusing the existing PHP-semantics binary operator routing and the established typed-slot store coercion, for both local variables and int/float typed properties. `++$x`/`$x--` on int slots lower the same way; post-increment keeps its old-value expression semantics through a native temporary inside one comma expression, so no side effect moves out of its evaluation position. Well-defined raw forms are kept: bitwise &= |= ^= on ints, += -= *= and ++/-- on floats, Variant slots, and everything under `use native_types`.
Interface constants were never validated: checkInterfaceImplementation()
had no constants loop and checkConstantOverride() only walks the class
extends chain. Incompatible retypings, final-constant overrides,
narrowed visibility and ambiguous multi-interface inheritance were all
accepted (all fatal in Zend 8.4).
Model Zend's constants-table merge (zend_do_inheritance +
do_inherit_constant_check): a class-like's effective table is built from
the parent class's table (private constants are not inherited), its own
declarations, then its interfaces, each entry keeping the ORIGINAL
declaring class/interface. When a same-name constant arrives from a
different declaration:
- a FINAL inherited constant cannot be overridden — "C::X cannot
override final constant I::X" — including through an ancestor class
that implemented the interface (the origin travels with the entry);
- two different declarations are ambiguous unless the type declares
the constant itself — "Class C inherits both I1::X and I2::X,
which is ambiguous" (a diamond of one declaration is fine);
- an override of an interface constant must stay public — "Access
level to C::X must be public (as in interface I)";
- a typed interface constant requires a typed, covariant override; an
untyped one may be redefined freely.
The same validation runs for interfaces extending interfaces and for
enums implementing interfaces. Enum cases live in a separate table in
Zend and never conflict with inherited constants.
getCExpr() emitted int class constants, property defaults and parameter defaults with strval(), so `const M = PHP_INT_MIN;` produced `ZVAL_LONG(&const_M_value, -9223372036854775808)`. C parses that as unary minus applied to the literal 9223372036854775808, which exceeds long long and is ill-formed, so the generated extension source does not compile. The expression path already handles this via genIntegerLiteral (ZEND_LONG_MIN); give the stub metadata path the same spelling. The float paths (17-digit round-trip, -0.0 sign, INF/NAN) were already fixed upstream in 2d81626; the new test pins those literals down together with the int boundary values.
Zend decides trait constant/property compatibility by comparing the EVALUATED definition (zend_is_identical on the resolved zvals plus matching flags and declared type), so `const int X = 1 + 1` in one trait and `const int X = 2` in another are the same definition, as are `[1, 2]` and `array(1, 2)` property defaults. composeTraitAst() compared pretty-printed source text and isCompatibleTraitConstant() compared lowered value strings, rejecting these valid compositions. Evaluate both initializers with the existing evaluateClassConstValue() machinery and compare with identity semantics (1 vs 1.0 or 1 vs '1' still conflict, matching Zend), coercing an integer initializer to float first when the member's declared type is float — Zend performs that coercion at declaration time, so `public float $f = 1` and `= 1.0` are identical. When a value cannot be evaluated at compile time the previous source-text comparison remains as the fallback. Flag, declared-type and (for properties) presence-of-default equality checks are unchanged.
…space rules constantNumericValue() matched strtolower($name) with no namespace resolution, so two invalid folds happened: `namespace N; const PHP_INT_MAX = 5; PHP_INT_MAX + 1` folded to 9.22e18 where PHP resolves the namespaced constant and yields 6, and a lowercase `php_int_max` silently folded to the global value where PHP raises an undefined-constant Error. Resolve the fetched name the way parseConstFetch() does: a `use const` alias resolves to its target, a fully qualified name is global, an unqualified name inside a namespace participates in PHP's runtime fallback (Namespace\NAME can be defined before the fetch executes) and therefore never provably names the global, and the match is now case-sensitive. Only a provable global PHP_INT_MAX/PHP_INT_MIN folds.
composeTraitAst() consumed matching traitAliases/traitIgnored entries
but never verified that every adaptation matched anything, silently
ignoring rules Zend rejects while binding traits:
- `use A { missing as g; }` — "An alias (g) was defined for method
missing(), but this method does not exist";
- `use A { A::missing as g; }` — "An alias was defined for
A::missing but this method does not exist";
- an alias or precedence rule naming a trait outside the class's use
list — "Required Trait B wasn't added to C";
- `use A, B { B::f insteadof A; }` with no B::f — "A precedence
rule was defined for B::f but this method does not exist" (the
OVERRIDDEN trait need not declare the method — only the preferred
one, matching Zend).
Composition now records which "trait::method" keys were seen (methods
arriving from nested traits are keyed under the directly-used trait,
matching how adaptations are registered) and validates every adaptation
afterwards. Because the Preprocessor registers an unqualified alias
under EVERY used trait's key, entries now carry the source adaptation's
group id — a group is satisfied when any variant matched — plus the
method name and explicit qualifier for diagnostics; precedence entries
record the rule for winner-existence validation (consumers only isset()
the key, so the value change is compatible).
The >=16-significant-digit float-literal-to-php::Decimal promotion
(docs/en/HIGH_PRECISION_TYPES.md) counted every digit in the raw
literal with preg_replace('/[^0-9]/'), so exponent digits and trailing
zeros counted as significant: 1.23456789012345e300 (15 significant
digits) and 999999999999999.0 became Decimal, making
is_float(2.220446049250313E-16) compile to false. Hex literals whose
digits contain E (0x123456789E1234567) matched the [.eE] test and
became Decimal("0x..."), where Zend folds an overflowing hex literal
to its exact double.
Three fixes, keeping the documented feature:
- Count true mantissa significant digits (strip sign, exponent,
leading and trailing zeros) and additionally require that the double
cannot reproduce the literal exactly - a literal that round-trips
(every var_export/serialize output, PHP_FLOAT_EPSILON) has lost
nothing and stays float, while 3.14159265358979323846 still promotes.
- Exclude hex/octal/binary notation from the reclassification.
- When a Decimal-classified literal meets a float-typed expression in
a binary op or comparison, demote the literal to its exact double
instead of the "Cannot convert float expression to Decimal" fatal:
PHP evaluates every float literal as a double, so
0.1 + 0.2 == 0.30000000000000004 is valid PHP and must be true.
checkParentMethodCanBeOverridden() fataled with "Cannot override
private method" when a child declared a method whose nearest parent
declaration is PRIVATE. Zend inherits no private methods: a child may
redeclare one with any signature, visibility or staticness, and FINAL
is ignored on non-constructor private methods (declaring one only
raises "Private methods cannot be final..."). Only the final private
CONSTRUCTOR remains protected, which the constructor path already
enforces.
Dispatch stays correct after removing the fatal:
- canDevirtualize() (Parser/MethodCallTrait) devirtualizes any call
whose resolved method is private to the DECLARING class's body.
That is exactly PHP's private-scope binding (zend_std_get_method
prefers the calling scope's private copy), verified against the
manual's Bar/Foo::testPrivate example;
- method resolution walks from the receiver's static class, so code
in the child binds the child's redeclaration;
- a call on a receiver statically typed as the declaring class from
OUTSIDE its scope is rejected by getNativeMethod()'s accessibility
check, and dynamically typed receivers go through Zend dispatch;
- Native (C++) classes give private methods no virtual slot
(isNativeVirtualMethod() excludes PRIVATE), so no C++ override can
reroute a parent's internal private call.
The two tests asserting the old fatal encoded rejects-valid programs
(both run fine under Zend 8.4, printing the parent's private result);
they now assert successful compilation.
A class constant holding an enum case (`class K { const CB = E::B; }`)
was registered with the folded scalar: ZVAL_LONG(4) for a backed case,
or the case-name string for a pure case. Dynamic access
(constant('K::CB'), $cls::CB, reflection) then observed an int/string
where PHP has the case object, so K::CB === E::B was false on those
paths. Additionally, the preprocessor read the raw AST ->value property
of the case expression, so an expression-valued backed case
(`case A = 1 + 1;`) emitted an "Undefined property" diagnostic and was
recorded as a pure case, folding referencing constants to the "A"
case-name string.
Two fixes:
- Preprocessor evaluates the case expression with the existing class
constant expression evaluator (ClassConstantValueTrait moves from
Translator to Preprocessor so prepare-time code can use it).
- Enum case objects have request lifetime and cannot live in a MINIT
zval, so the registration mirrors the array-constant mechanism: the
MINIT value stays a scalar placeholder, request init re-binds the
constant via php::updateConstant + php::getEnumCase (following
constant chains, parent classes and interfaces), and request shutdown
resets the slot so no request-bound object dangles in the persistent
class entry. The expression path already emitted php::getEnumCase and
is unchanged.
Zend rejects at compile time, and TypePHP previously accepted silently:
- properties in enums (instance, static, hooked): enum class entries
have no property table ("Enum E cannot include properties")
- magic methods other than __call/__callStatic/__invoke ("Enum E
cannot include magic method __x"); the banned set was probed one by
one against Zend 8.4.13
- a case value on a non-backed enum and a missing value on a backed
enum ("Case A of ... enum E must (not) have a value")
- duplicate case names and case/const name collisions: enum cases are
class constants ("Cannot redefine class constant E::A")
- a backing type other than int|string
- explicitly implementing UnitEnum/BackedEnum, which Zend adds itself
("cannot implement previously implemented interface"), including the
non-backed-enum BackedEnum variant
- abstract methods in enum bodies: an enum can never be abstract
Enum ClassDef flags now carry Modifiers::FINAL, mirroring ZEND_ACC_FINAL
on enum class entries, so `class B extends E` is rejected by the
existing final-class inheritance check without touching the Translator.
…parent chain An abstract method declared by a class was never checked against its parent: turning a concrete inherited method abstract compiled (Zend: "Cannot make non abstract method A::f() abstract in class B"), and an abstract redeclaration of an inherited abstract contract skipped the signature check entirely. Both now run through checkParentMethodCanBeOverridden with a childIsAbstract mode, covering userland and built-in parents. Trait-originated abstract requirements are exempt: Zend lets an inherited concrete method satisfy them, so only abstract methods the class itself declares participate.
The readonly checks previously lived only in the Native-class branch;
ZendVM-backed classes accepted declarations Zend rejects at compile
time. addClassProperty now enforces, for declared and promoted
properties alike (probed against Zend 8.4.13):
- readonly property with a default value ("Readonly property A::$x
cannot have default value") - a readonly property carries runtime
initialization state, so a compile-time default is meaningless
- untyped readonly property, including untyped promoted readonly ctor
params ("Readonly property A::$x must have type")
- static readonly ("Static property A::$x cannot be readonly")
- a `readonly class` applies the same three rules to every property:
the class-level Modifiers::READONLY flag (already recorded on
ClassDef->flags for the Translator-side inheritance check) is OR-ed
into the per-property check
Promoted readonly params keep accepting parameter defaults: the default
belongs to the constructor argument, not the property (Zend-verified).
The inheritance_error_prop_readonly fixture used `readonly int $x = 2`,
which Zend itself rejects with the default-value error before ever
reaching the readonly-mismatch link error; the default is dropped so the
fixture still exercises the inheritance mismatch.
… class methods Two abstract-method rules Zend enforces at compile time were missing: - an abstract method with a body was accepted and the body silently dropped; Zend fatals with "Abstract function A::f() cannot contain body" (applies to classes and traits alike, probed on 8.4.13) - `abstract private function` in a class can never be implemented, since private methods do not participate in overriding; Zend fatals with "Abstract function A::f() cannot be declared private". Traits keep accepting it (allowed since PHP 8.0: the consuming class supplies the private implementation) Zend reports the private-modifier error before the body error when both apply; the checks are ordered to match.
parseInterface accepted several declarations Zend rejects at compile
time (all wordings probed on 8.4.13, which renamed the modifier errors
to "must not be abstract/final"):
- interface method with a body ("Interface function I::f() cannot
contain body")
- private/protected interface method ("Access type for interface
method I::f() must be public")
- explicit `abstract` modifier on an interface method ("Interface
method I::f() must not be abstract")
- `final` interface method ("Interface method I::f() must not be
final")
- private/protected interface constant ("Access type for interface
constant I::X must be public"); `final` interface constants remain
legal per PHP 8.1
- explicit `abstract` on an interface hooked property ("Property in
interface cannot be explicitly abstract...")
- `interface I extends A` where A is a known class, enum, or trait
("I cannot implement A - it is not an interface"); only checked when
A's declaration has already been prepared - a parent declared later
is left to the Translator (deferred to integrator)
- the same interface listed twice in extends ("Interface I cannot
implement previously implemented interface A")
Zend's precedence for combined modifier violations (visibility, then
abstract, then final, then body) is preserved.
…ent kind Zend seals readonly-ness across a hierarchy: a non-readonly class cannot extend a readonly one and vice versa. Both directions compiled silently. Likewise `interface I extends SomeClass` compiled even though an interface can only extend interfaces; it now fails with Zend's message instead of being misreported as a missing symbol.
…operties
The interface path already validated hook placement; class and trait
properties accepted every combination. parseClassPropertyDef now
mirrors Zend's compile-time rules (probed on 8.4.13, including the
precedence order static -> readonly -> abstract rules):
- hooks on a static property ("Cannot declare hooks for static
property")
- hooks on a readonly property, including properties made readonly by
a `readonly class` ("Hooked properties cannot be readonly")
- `abstract` on a hook-less property ("Only hooked properties may be
declared abstract")
- abstract hooked property with a default value ("Cannot specify
default value for virtual hooked property A::$x")
- abstract hooked property whose hooks all have bodies ("Abstract
property A::$x must specify at least one abstract hook")
- abstract hooked property in a non-abstract class; traits stay exempt
(the consuming class satisfies the hook) and enums are already
rejected by the property ban
- bodiless hook on a non-abstract property, in classes and traits
("Non-abstract property hook must have a body"); previously the
lowering fabricated a concrete backing-store accessor for it
…roperty/constant types
Two promotion/type gaps against Zend (probed on 8.4.13):
- `__construct(public int ...$x)` was accepted and even registered
the property before the variadic-position check ran. A variadic
parameter collects its arguments into an array, so there is no single
value to promote; Zend fatals with "Cannot declare variadic promoted
property". The check now precedes the property registration.
- `callable` is a calling-scope-dependent type, so Zend forbids it in
property types (declared, promoted, interface hooked) and class
constant types (class and interface), bare or as a nullable/union
member: "Property A::$x cannot have type ?callable" /
"Class constant A::X cannot have type callable". Intersection
members are left to the compound-type validation, which rejects
every non-class standard type there.
`void`/`never` property and parameter types were already rejected by
parseTypeDecl ("The type `void`/`never` is allowed only for return
type") - verified, no change needed; union members are covered by the
compound-type validation.
Compound assignment on int-typed properties is now lowered to the plain assignment through the Variant operators (raw typephp_static_int_ref compounds have undefined signed overflow where PHP promotes to float and the typed-property store raises TypeError). Update the codegen assertions accordingly; behavior is covered by tests/compiler/operator/typed-compound-assign.phpt.
Lowering a later call argument or concat operand that materializes captured statements (an assignment, a call result) appended them to the enclosing statement, executing the side effect before earlier operands were read: two($j, $j = 5) with $j = 1 produced "5,5" (Zend "1,5") and $m . "," . ($m = 9) produced "9,9" (Zend "1,9"). Call arguments: Zend SENDs strictly left to right, so when a later argument hoists statements, every earlier by-value plain-variable argument is snapshotted into a temporary at its own argument position. By-reference parameters, unpacked arguments, $this and $GLOBALS are left alone. Concat chains: Zend reads a CV operand when its CONCAT opcode executes, so in the left-associated chain the first two items are read together at the first op (after both items' side effects: $s . ($s = 'b') . $s is "bbb") and each later item after the side effects of everything up to itself. The flattened braced-list lowering now snapshots a plain-variable item exactly at that read position, deferring the first item's snapshot until the second item has been lowered. Plain arithmetic is intentionally unchanged: Zend's ADD reads the CV at op time, so $k + ($k = 5) is 10 in both worlds, and the existing codegen already matches.
…e type keywords
resolveTypeDecl now runs a shared well-formedness pass before resolving,
so parameters, returns, properties, class/interface constants, and
closure signatures all obey Zend's compile-time compound-type rules
(each probed on 8.4.13):
- duplicate union members, case-insensitive and after alias/namespace
resolution ("Duplicate type int is redundant", "Duplicate type
App\Sub\Thing is redundant"); iterable is expanded to
array|Traversable first, so iterable|array and iterable|\Traversable
report the overlapping component exactly like Zend, while a
namespace-local Traversable stays legal
- bool with false/true names the literal as the duplicate in either
order; true|false demands bool ("Type contains both true and false,
bool must be used instead")
- mixed/void/never inside a union ("... can only be used as a
standalone type"), ?mixed ("Type mixed cannot be marked as nullable
since mixed already includes null"), ?null, ?void, ?never
- intersection members must be class types ("Type int cannot be part
of an intersection type"); duplicate intersection members are
redundant; self/parent/static keep the established TypeCheckGenerator
diagnostic; redundancy between whole DNF groups is not checked (Zend
uses a distinct "Type X&Y is redundant with type X&Y" pass)
- self/static return types on free functions ("Cannot use \"static\"
when no class scope is active"); closures keep accepting them since
they may be bound to a scope later, matching Zend
- duplicate interfaces in an implements list, for classes and enums
("Class A cannot implement previously implemented interface I");
duplicate trait use stays legal - Zend deduplicates it silently
Adds 59 phpunit cases (7 suites, 59 fixtures) for the new compile-time rejections: enum declaration rules, readonly property rules, abstract method rules, interface member rules, property-hook placement, promotion and callable-type restrictions, and compound-type well-formedness. Every error fixture was cross-checked to fatal under Zend 8.4.13 and every *_valid fixture to run clean, so the suites pin TypePHP to Zend's behavior rather than to its own output.
# Conflicts: # src/Preprocessor.php
Two interfaces declaring the same method were never cross-checked: `interface J extends I1, I2` and a class implementing both compiled even when the declarations were mutually incompatible (Zend: "Declaration of I1::f(): int must be compatible with I2::f(): string"). The first-seen declaration is now validated as an override of every later one, mirroring Zend's merge order; diamond inheritance of one original declaration never conflicts, and a method the class chain defines silences the pairwise check (it is validated against each interface individually instead) — all probed against Zend 8.4.
AlessioGiacobbe
force-pushed
the
fix/correctness-audit
branch
from
August 31, 2026 14:20
1357af6 to
0e1714f
Compare
…mespace fix) into fix/correctness-audit # Conflicts: # src/Translator.php
The new codegen tests hardcoded the macOS zend_long literal suffix
(1LL); Linux emits 1L, so every such assertion now matches L{1,2}.
Unary minus also no longer parenthesizes a bare numeric literal: a
single C++ token cannot change the parse, emitting -7L keeps the code
readable, and the existing LocalVariableInitializerTest snapshots stay
valid. Hazardous operands (ternaries, nested negation) remain
parenthesized.
…semantics Three PHPT failures on Linux: - enum/5.phpt: the eager enum-case evaluation fataled on a value referencing a class declared later in the file (forward references are legal in PHP). Evaluation now runs under a throwing diagnostic reporter (new withThrowingDiagnostics helper) so an unresolvable value degrades to the null placeholder; the stub registration evaluates the expression independently in a later phase, so the registered runtime value is unaffected. - type_hits/native-type.phpt asserted truncating int division (std::int(10) / 4 === int(2)). Division on typed int operands now follows PHP semantics in non-native mode, consistent with the pre-existing + - * routing through php::Var (whose comment states exactly this contract); the expectation becomes float(2.5). Native mode (use native_types) keeps raw division. - object_property/native-int-property-assign-op-var.phpt encoded pre-Zend behavior: Zend coerces += "3" on an int property (no TypeError, value 6) and reports "Unsupported operand types: int + string" for += "abc". Expectations updated to the Zend-verified behavior; the native_types sibling test keeps its raw semantics and still passes.
Member
|
Thank you very much for the substantial effort and contribution in this PR. We really appreciate the systematic investigation, the detailed explanations, and the large amount of accompanying test coverage. However, this PR is exceptionally large and contains many independent behavioral changes and fixes across code generation, constant folding, declaration validation, inheritance, traits, enums, and the test suite. Reviewing these changes safely requires a significant amount of time and careful verification against both Zend PHP semantics and TypePHP's existing design constraints. For that reason, we cannot merge the PR directly as a single change set. We will continue reviewing it incrementally. We may split or extract smaller, well-scoped parts of the implementation and merge them into Thank you again for your patience and for investing so much effort in improving TypePHP. |
Member
|
Thank you again for the exceptionally thorough audit, detailed analysis, and extensive test coverage. After reviewing the scope, we have decided to treat PR #39 as a reference source for verified issues and tests, rather than as a branch to be merged directly. The changes span too many independent compiler subsystems and need to be reviewed and integrated separately to preserve correctness and make regressions easier to isolate. We plan to split the findings into approximately 18 independent issues and address them one by one through small, focused PRs. We will continue using the reproductions, Zend comparisons, tests, and implementation ideas from this PR as important reference material, with appropriate credit retained. Therefore, PR #39 itself will not be merged. We will close it later after the individual work items have been recorded and organized. Thank you for your understanding and for the substantial amount of work you contributed. |
Contributor
Author
i would be happy to split this into smaller, scoped, issues if that's ok with you and if i don't overlap with somebody else's work, let me know if i can contribute that way |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes 27 verified correctness divergences from Zend PHP 8.4 semantics, found by systematically differential-testing the compiler front-end against Zend 8.4.13 and inspecting the generated C++/arginfo. Every fix was probed against Zend first (the exact rule, both directions), and every fix ships with PHPUnit coverage; runtime-visible fixes also add phpt tests with Zend-validated expectations.
The findings fall into three groups.
Miscompilations — valid PHP that compiled but behaved differently at runtime
int/floatdivision emitted raw C++/—divInts(7, 2)returned3.0instead of3.5;/ 0was UB instead ofDivisionByZeroError;float / 0.0producedINFinstead of throwing. Division now routes throughphp::Varlike+ - *already did. Both-int%(the existing guard was inverted) and dynamic<</>>(UB for counts ≥ 64 or negative) are routed the same way; constant folds are untouched.-($a ? $b : $c)compiled to-(toBool(a)) ? b : c, losing the sign and selecting the wrong branch (sibling of fix(parser): parenthesize unary minus operand to avoid C++ pre-decrement pasting #20's--xcase, which the narrow guard could not catch). The operand is now always parenthesized.??=evaluated a side-effecting RHS unconditionally when the RHS materialized statements. The native-object conditional-lambda mechanism is generalized so the RHS executes only in the not-set branch.++/--on typed scalar slots were raw C++ —$a /= $btruncated,+=overflow wrapped (UB) instead of following the compiler's own established assignment semantics. These now lower to the equivalent plain assignment through the safe binary path.two($j, $j = 5)with$j = 1printed"5,5"(Zend:"1,5"); same for string concatenation. Earlier plain-variable operands are now snapshotted at their exact Zend read positions (concat's read positions were probed empirically;$k + ($k = 5)codegen is byte-identical, matching Zend's CV-read-at-op behavior).K::CB === E::Bcompiled tofalse, and expression-valued cases (case A = 1 + 1;) warned during compilation and lost their value. Case expressions are now evaluated with the constant evaluator, and enum-case-valued constants are re-bound each RINIT viaphp::updateConstant(..., php::getEnumCase(...))(enum case objects are request-scoped, so a MINIT zval is impossible), mirroring the array-constant mechanism.PHP_INT_MINclass constants emitted an ill-formed C literal (-9223372036854775808); nowZEND_LONG_MIN. (The precision-14 float emission andcount()folding issues found in the same audit were already fixed on master by fix(optimizer): stop folding count() on unfoldable array literals #24/fix(generator): normalize float literal emission and handle INF/NAN constants #33 — thanks!)is_float(2.220446049250313E-16)compiled tofalse), overflowing hex literals containing the digitEbecameDecimal("0x…"), and validvar_export()round-trip literals like0.1 + 0.2 == 0.30000000000000004failed to compile. Classification now counts real mantissa precision with a round-trip-exactness check, excludes hex/octal/binary, and demotes a Decimal-classified bare literal to its exact double when it meets a float-typed expression.PHP_INT_MAX/PHP_INT_MINfold was case-insensitive and namespace-blind —namespace N; const PHP_INT_MAX = 5; PHP_INT_MAX + 1folded to9.2e18instead of reading the namespaced constant. Resolution now mirrorsparseConstFetch./ 0and% 0were a compile-time fatal even in dead/guarded code, unlike the equivalent1 % (1 - 1)spelling. The literal spelling now warns and produces the same catchable runtimeDivisionByZeroError.Accepted invalid PHP — Zend compile fatals that typephp registered anyway
Compiled classes are installed as internal classes, where ZendVM does not re-run these checks, so these hierarchies previously reached runtime:
class C extends ArrayObject { public function offsetGet(int $key): string }compiled). A reflection-based validator now enforces the full Zend ruleset — visibility, staticness, by-ref return covariance, parameter contravariance with variadic absorption, return covariance — while exempting tentative return types, which Zend only deprecates.__constructoverrides skipped every check, includingfinalparent constructors and abstract-constructor signature contracts. Ordinary concrete parent constructors remain exempt from LSP variance, as in Zend.finaloverride bans, visibility, and two-origin ambiguity are now enforced through an effective-constants table that preserves the original declaring interface, including contracts arriving via parent classes, parent interfaces, and enumimplements.implements UnitEnum, andclass B extends Enumall compiled.interface I extends SomeClasswas misreported instead of rejected.interface J extends I1, I2(and a class implementing both without defining the method) compiled with mutually incompatible declarations. The first-seen declaration is now validated as an override of every later one, with diamond inheritance and class-defined methods exempt, matching Zend's merge order.callableproperty types were accepted.use A { missing as g; }andB::f insteadof Awith noB::fnow fail with Zend's diagnostics (unqualified aliases are grouped so any matching trait satisfies them).?mixed,mixed|T, and related invalid declarations.Rejected valid PHP
const int X = 1 + 1;andconst int X = 2;(or[1, 2]vsarray(1, 2)defaults) were rejected as conflicting. Values are now compared by evaluation, with Zend's declaration-time int→float coercion for float-typed members.int|string→int) is now accepted, widening still rejected.Verification
tests/compilercorpus (1132 phpt files) front-end-compiles with zero new failures.CompilerBaseApiTest::testLibraryImportStubCombines…, turned out to reproduce on pristine master too and is repo-path-dependent, not caused by this PR).Found while working on #19 (this PR is independent of it, though both touch trait composition — whichever merges second may need a trivial rebase). The audit also confirmed #20/#21/#24/#33 fixed their targets. Happy to split this into per-subsystem PRs if that's easier to review.