fix(compiler): support first-class callable of internal functions - #93
fix(compiler): support first-class callable of internal functions#93Tinywan wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
The first-class callable fix is valid and can be kept. The two new callable PHPT tests pass, and the existing AssignTest suite remains green when only that commit is applied.
The inferred-object reassignment change cannot be merged in its current form:
reassignInferredObjectVar()is not limited to mutually exclusive branches. It permits any inferred local to change between unrelated classes, including ordinary sequential assignments.- It breaks the existing negative contract in
AssignTest::testAssignClass. This code is currently required to fail, but the PR makes it compile:
$obj1 = new stdClass();
$obj2 = new ArrayObject();
$obj1 = $obj2;- Clearing
objects/stableObjectswhile walking the second assignment is source-order mutation, not a control-flow type merge. It implicitly weakens an established object type to genericobject, contrary to TypePHP's fixed inferred-type design. - The new PHPT uses conditions whose results are already statically known (
is_string($a)where$ais a string, and the same check where$bis an array), so it does not demonstrate a sound runtime branch merge.
Please remove the second commit from this PR and keep this PR focused on first-class callable handling. Code that intentionally stores unrelated object classes should opt into dynamic storage with std::any().
If branch-aware object type merging is added later, it needs to be implemented in the SSA/control-flow merge stage, with dedicated tests for both runtime branches, rather than by widening the variable during assignment code generation.
PHP 8.1 `foo(...)` builds a Closure from a single VariadicPlaceholder argument instead of calling foo. Two paths mishandled it: - checkInternalFunctionArgCount() counted the placeholder as a real argument, so first-class callables of internal functions requiring 2+ parameters (e.g. `posix_kill(...)`) failed with a bogus "expects at least N arguments, 1 given". - The math-function return-type optimization read `$expr->args[0]->value` on callables such as `round(...)`, crashing on the undefined VariadicPlaceholder::$value property. Guard both with isFirstClassCallable() so the expression falls through to Closure (Type::OBJECT) resolution.
ac8b102 to
a871e6e
Compare
|
The inferred-object reassignment change has been removed from this PR and preserved on the WIP branch: |
matyhtf
left a comment
There was a problem hiding this comment.
The first-class callable check needs to run before all function return-type optimizations. Currently detectTypeOfExpr() checks STREAM_FUNCTIONS before the generic callable-to-object branch, so stream-returning builtins such as fopen(...) are still inferred as Type::STREAM instead of Type::OBJECT.
A valid example fails to compile:
function useOpener(Closure $open): void {}
useOpener(fopen(...));The compiler reports: Cannot re-assign variable from php::Stream to php::Object.
Please move an isFirstClassCallable() / Type::OBJECT short-circuit ahead of both the math and stream return-type handling, and add a regression test using a stream builtin (for example, passing fopen(...) to a Closure-typed parameter).
Validation performed on the exact PR head: the compiler built successfully; the 34-test PHPUnit slice, both new PHPTs, and all existing placeholder PHPTs passed. The stream-callable regression above fails as described.
Scope: first-class callable handling for internal functions only.
This change skips internal-function argument-count validation when PHP 8.1 first-class callable syntax is used, because
foo(...)creates a Closure rather than invokingfoo.It also skips mathematical-function return-type optimization for that syntax, preventing access to
VariadicPlaceholder::$value, which is absent for the placeholder argument.Validation: callable-only commit replayed on current master;
git range-diffandgit diff --checkpass;php vendor/bin/phpunit phpunit/src/AssignTest.phppasses (34 tests). Regression coverage is inbuiltin-multi-arg-callable.phptandmath-function-callable.phpt.