Skip to content

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

Merged
matyhtf merged 1 commit into
swoole:masterfrom
hafung:fix/optimized-builtin-typed-arguments
Aug 31, 2026
Merged

fix(optimizer): preserve typed builtin argument validation#18
matyhtf merged 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 proven safe under strict PHP semantics
  • allow exact types and the Zend-compatible int -> float widening; fall back the whole call for all other runtime scalar conversions
  • accept literal null only for parameters marked nullable by Reflection, including selected custom handlers

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 or declared int result passed to in_array()'s boolean parameter being coerced by generated C++ instead of raising the same TypeError as Zend. It also prevents non-nullable calls such as strlen(null) from being lowered to a permissive C++ conversion.

Coverage

  • fixed and variadic reflected parameters
  • string, int, float, bool, and array ABI parameters
  • exact types and int -> float widening remain direct; int/float/bool cross-conversions otherwise fall back
  • Reflection-nullable literal null remains direct; non-nullable and optional-but-non-nullable null falls back
  • 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)
  • Decimal round() and native Countable branches remain ahead of the shared guard

The existing null_optional_arg.phpt now declares strict types and distinguishes nullable parameters such as substr() length from optional but non-nullable parameters such as offsets and boolean flags.

Verification

  • PHP 8.5.10 Zend baseline: both strict typed-argument and nullability fixtures pass
  • Windows/MSVC source AOT artifacts: both fixtures pass at O0 and O3
  • Linux/GCC generated-code PHPUnit: target test passes with 15 assertions
  • adjacent latest-upstream count-fold verification: 2 PHPUnit tests and 1 AOT regression pass
  • other locally runnable adjacent AOT regressions: 5/5 pass (array_keys, lookup calls, func_get_arg, and two Decimal round() paths)
  • full stdlib AOT pass: 57/62; two remaining failures are existing PHP 8.5/Windows expectation differences, while three compilation failures require PHPX 2.6.8 APIs newly required by current master but absent from the locally installed 2.6.7 preview package

The previous CI run passed 11/12 jobs; the sole macOS PHP 8.5 failure was a Packagist DNS timeout, as noted in review. A fresh CI run is in progress for this revision. 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.

@hafung
hafung force-pushed the fix/optimized-builtin-typed-arguments branch from 1dae4c2 to 5d38a73 Compare August 31, 2026 02:55
@hafung

hafung commented Aug 31, 2026

Copy link
Copy Markdown
Contributor Author

Addressed both blocking points in 5d38a73, rebased onto current master (a182a2c).

  1. The direct ABI path now accepts an exact static type or the strict-compatible int -> float widening only. Other scalar combinations fall back as a whole. The regression now includes the reported typedInt() -> in_array(..., bool) case, plus float -> int and bool -> int custom-handler cases; generated-code assertions verify those use php::call() while exact bool and int -> float stay direct.

  2. Literal null now consults the reflected nullables entry for that parameter. strlen(null) and optional-but-non-nullable arguments fall back and throw TypeError; json_decode(..., null) and nullable substr() length remain direct. I also changed null_optional_arg.phpt from its previous weak-coercion expectations to explicit strict semantics.

I removed in_array('1', [1], 1) as evidence for a generally safe scalar conversion. Zend accepts that particular all-constant call, but the same literal strict flag throws once needle/haystack are runtime expressions, so it does not justify an ABI-level int -> bool rule.

Verification on the revised head:

  • Zend PHP 8.5.10: both focused fixtures pass
  • Windows/MSVC AOT O0 and O3: both focused fixtures pass
  • target generated-code PHPUnit: 15 assertions pass
  • latest-upstream adjacent count-fold tests: 2 PHPUnit tests and 1 AOT test pass
  • five other runnable adjacent AOT tests pass

Fresh CI is running. The local full stdlib run is documented in the PR body, including the installed PHPX 2.6.7 vs current-master 2.6.8 boundary.

@hafung
hafung force-pushed the fix/optimized-builtin-typed-arguments branch from 5d38a73 to 9f6e31c Compare August 31, 2026 03:18
@matyhtf
matyhtf merged commit 223a106 into swoole:master Aug 31, 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