Skip to content

fix(optimizer): preserve typed builtin argument validation - #18

Open
hafung wants to merge 1 commit into
swoole:masterfrom
hafung:fix/optimized-builtin-typed-arguments
Open

fix(optimizer): preserve typed builtin argument validation#18
hafung wants to merge 1 commit into
swoole:masterfrom
hafung:fix/optimized-builtin-typed-arguments

Conversation

@hafung

@hafung hafung commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • keep reflection-optimized builtin calls on their direct C++ ABI only when every supplied primitive/container argument is statically safe for that ABI
  • fall back the whole call to Zend dispatch when a runtime-backed string, int, float, bool, or array value still needs PHP's parameter validation
  • apply the same rule to typed custom handlers without disabling their proven static and native-object fast paths

Invariant

An optimizer must not erase the runtime zval type before an internal function has applied its parameter contract. A direct ABI call is valid only when the compiler proves that its conversion has the same accepted/rejected behavior as Zend; otherwise all arguments remain on the ordered php::call() path.

This fixes strict calls such as a mixed value passed to in_array()'s boolean parameter being coerced by generated C++ instead of raising the same TypeError as Zend.

Coverage

  • fixed and variadic reflected parameters
  • string, int, float, bool, and array ABI parameters
  • nullable/default handling and the existing literal-null policy
  • nested value-used calls, unpacking, and left-to-right evaluation order
  • selected custom handlers (array_keys, array_key_exists, round, count, define, and function_exists)
  • exact/static scalar paths, Decimal round(), and native Countable paths remain optimized

Verification

  • PHP 8.5.10 Zend baseline: regression fixture passes
  • Windows/MSVC source AOT artifact: regression fixture passes at O0 and O3
  • Linux/GCC generated-code PHPUnit: 1 test, 7 assertions pass
  • adjacent AOT regressions: 7/7 pass (array_keys, lookup calls, func_get_arg, Decimal round, count(Countable), and internal-interface coverage)
  • stdlib AOT suite: 60/62 pass; the two remaining failures are existing expectation mismatches (PHP 8.5 bindec() deprecations and Windows path separators)

The full PHPUnit suite was also attempted, but the current Windows run has unrelated existing failures and terminates on a Linux-only profile fixture, so it is not claimed as passing. Remote CI and non-MSVC native artifacts were not run locally.

@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.

Thank you for the thorough investigation and for preserving the whole-call fallback path. The original mixed argument issue is real, and this PR fixes that case in the right direction.

I found two remaining correctness issues in hasOptimizerSafeTypedArguments() that need to be addressed before merging:

  1. Treating every Native scalar-to-scalar conversion as safe is too broad.
declare(strict_types=1);

function typedInt(): int
{
    return 1;
}

in_array('1', [1], typedInt());

Zend throws a TypeError, but this PR emits php::toBool(...) and returns false. A declared Native return type does not make a cross-type conversion valid for an internal function. The fast path should normally require an exact type, with only explicitly proven Zend-compatible widening such as int -> float; otherwise the whole call should fall back to Zend dispatch.

  1. Literal null cannot be accepted unconditionally.
declare(strict_types=1);

strlen(null);

Zend throws a TypeError, while this PR emits php::toString(php::null) and returns 0. Please use the reflected nullable metadata when deciding whether null is safe. If a particular wrapper intentionally accepts null as a legacy/default policy, that exception should be explicit per function/parameter rather than globally applied.

I reproduced both differences by compiling the PR head and comparing the resulting executable with Zend PHP. These are implementation issues rather than requests for additional test coverage.

The macOS PHP 8.5 CI failure appears unrelated: Composer failed because Packagist DNS resolution timed out. The remaining CI jobs 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