Skip to content

Fix dynamic strict flag in optimized array_keys calls - #11

Merged
matyhtf merged 2 commits into
swoole:masterfrom
hafung:fix/array-keys-dynamic-strict
Aug 28, 2026
Merged

Fix dynamic strict flag in optimized array_keys calls#11
matyhtf merged 2 commits into
swoole:masterfrom
hafung:fix/array-keys-dynamic-strict

Conversation

@hafung

@hafung hafung commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix the optimized three-argument array_keys() path when the $strict flag is a runtime-backed expression.

genArrayKeys() passed the parsed third argument directly to php::fn::array_keys_filter(). Typed property reads and typed user-function returns are represented as php::Var at that boundary, while the PHPX helper requires a C++ bool, so otherwise valid PHP translated successfully but failed during native compilation.

The fix routes only the strict flag through the optimizer's shared boolean argument resolver. The one- and two-argument fast paths and the existing array/filter ABI are unchanged.

Semantic invariant

Every valid array_keys($array, $filter, $strict) call must:

  • accept runtime-backed boolean expressions using PHP's internal-function parameter conversion;
  • evaluate each argument exactly once and in source order;
  • pass a C++ bool to php::fn::array_keys_filter();
  • preserve strict versus loose key matching.

Relevance matrix

Dimension Coverage
Arity One, two, and three arguments
Strict expression Literal, fixed local, typed property, typed function return
Other arguments Fixed array plus runtime-backed array/filter returns
Ordering Three side-effecting calls record array,filter,strict
Result semantics Integer 1 versus string '1' distinguishes loose and strict matching
Optimization Real native artifacts at O0 and O3
Fast/fallback Optimized direct helper is fixed; named/unpacked fallback is unchanged
Value context Returned arrays are consumed and compared; discarded-result lowering has no separate call path

Verification

  • New PHPT: PASS (1/1), including native translation, MSVC compilation/link, execution, and EXPECT comparison.
  • phpunit/src/FunctionTest.php: OK (23 tests, 35 assertions).
  • Modified optimizer file: PHP syntax check passed.
  • git diff --check: passed.
  • Original installed compiler reproduces the php::Var to bool C2664 failure at O0 and O3.
  • Patched local source builds and runs the focused reproducer at O0 and O3; both print:
literal=string
local=string
property=string
dynamic=string
order=array,filter,strict

Generated O0/O3 C++ materializes the three dynamic operands in source order and calls php::toBool() only for the third operand.

The full PHPUnit suite is not claimed: this Windows environment has unrelated existing failures and stops at a Linux-only profiling configuration. PHPStan is also not claimed because the repository's Composer script references a phpstan.neon file absent from this checkout. Non-MSVC backends were not run locally.

@hafung

hafung commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

CI update: both Linux PHPT jobs (PHP 8.4 and 8.5), both Linux PHPUnit jobs, and all other platform builds passed. The sole red macOS ARM64 / PHP 8.4 job failed before checkout build steps because setup-php could not download Composer from any mirror. macOS ARM64 / PHP 8.5 passed. I attempted to rerun only the failed job, but fork PR permissions do not allow reruns.

@matyhtf matyhtf left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the focused fix and the thorough positive coverage. The ABI issue for runtime-backed boolean expressions is real, but this implementation also changes PHP's strict parameter-validation semantics.

resolveArg($e, 2, self::ARG_TYPE_BOOL) eventually applies php::toBool() whenever the expression does not already have a fixed C++ bool representation. That is correct for a typed bool property or typed bool function return represented as php::Var, but it also accepts mixed, int, string, and array expressions.

For example, Zend PHP must throw a TypeError here:

declare(strict_types=1);

function strictFlag(): mixed
{
    return 1;
}

array_keys(['integer' => 1], 1, strictFlag());

With this patch, the optimized path converts 1 to true and returns normally, bypassing the internal-function argument check. TypePHP only supports strict_types=1, so this coercion is not compatible.

Please restrict the optimized path to expressions whose semantic type is definitely bool. Typed properties and typed function returns should then continue through resolveArg() to normalize their C++ representation. For mixed, unions, or any non-boolean semantic type, genArrayKeys() should return false and use the existing Zend dynamic-call path for runtime validation.

Please also add a negative PHPT proving that a runtime-backed non-boolean third argument still raises TypeError. The current test only covers runtime values that are actually boolean.

The macOS PHP 8.4 failure appears unrelated: setup-php failed while downloading Composer before the project was built.

@hafung

hafung commented Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Addressed the strict-type review in d480c5e.

  • The specialized three-argument array_keys lowering now runs only when the third expression semantic type is exactly bool.
  • mixed, union, int, string, array, and other non-exact types return false from the optimizer and use the Zend dynamic-call path, preserving runtime TypeError.
  • genArrayKeys now declares string|false; without this, weak return coercion turned false into an empty string and emitted invalid empty-expression C++.
  • The PHPT covers mixed(true) success plus mixed(int), mixed(array), and bool|int(int) TypeError cases, in addition to typed-property/function ABI and evaluation-order coverage.

Local verification: focused PHPT PASS through translation/MSVC/link/run/EXPECT; O0 and O3 torture artifacts both exit 0 with all three TypeErrors observed; FunctionTest.php 23 tests/35 assertions; typephp-native-core Zend suite PASS assertions=38.

@matyhtf
matyhtf merged commit e0c1298 into swoole:master Aug 28, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants