Skip to content

fix(optimizer): intval() with a base must not drop the base - #28

Open
Giandonn wants to merge 2 commits into
swoole:masterfrom
Giandonn:fix/intval-base-argument
Open

fix(optimizer): intval() with a base must not drop the base#28
Giandonn wants to merge 2 commits into
swoole:masterfrom
Giandonn:fix/intval-base-argument

Conversation

@Giandonn

Copy link
Copy Markdown

Fixes #27

The four scalar conversions are lowered to a single-argument Native cast by
dispatchConversion, which reads args[0] and ignores the rest. intval()
takes a $base as its second argument, so it was silently discarded.

Measured on a compiled binary (PHP 8.5.4 ZTS + embed, GCC 15, Linux x64):

call PHP binary before binary after
intval("ff", 16) 255 0 255
intval("0x1A", 16) 26 0 26
intval("101", 2) 5 101 5

The change

A conversion call with any arity other than one now returns false from
dispatchConversion, so it falls through to the dynamic path and both arguments
reach the runtime function. Single-argument intval(), strval(), floatval()
and boolval() still lower to php::toInt() and friends, so the common case is
untouched.

Tests

  • tests/compiler/stdlib/type_conv.phpt gains the base cases, with a literal
    and a variable base. It already covered six single-argument intval() calls;
    no test in the suite passed a base.
  • phpunit/src/ConversionArityTest.php pins both lowering decisions in the
    generated C++ and needs no native toolchain.

The PHPUnit test fails on master and passes with the change. type_conv.phpt
passes against a real build. The full PHPUnit suite reports the same results as
master, and PHPStan reports no new findings for the touched file.

The four scalar conversions are lowered to a single-argument Native cast
by dispatchConversion, which reads args[0] and ignores the rest. intval()
takes a $base as its second argument, so it was silently discarded:

    intval("ff", 16);    // php::toInt("ff")   -> 0,   PHP gives 255
    intval("0x1A", 16);  // php::toInt("0x1A") -> 0,   PHP gives 26
    intval("101", 2);    // php::toInt("101")  -> 101, PHP gives 5

Nothing reports the loss: the program compiles clean and the number is
simply wrong, which is easy to miss in the code that most often uses a
base - parsing hex colors, permission masks and binary flags.

A conversion call with any arity other than one now falls through to the
dynamic path, where both arguments are passed to the runtime function.
Single-argument intval(), strval(), floatval() and boolval() keep their
Native cast, so the common case is unchanged.

type_conv.phpt gains the base cases, with a literal and a variable base;
none of them were covered anywhere in the test suite. ConversionArityTest
pins the lowering decision in the generated C++.

@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 clear report and for covering both literal and variable bases. The ordinary two-argument intval() cases are fixed correctly.

There is still one conversion path that silently drops the call semantics: argument unpacking. An unpacked call has one Node\Arg in the AST regardless of its runtime arity, so the new count($expr->args) === 1 check still accepts it as a single-argument Native conversion.

For example:

$withBase = ['ff', 16];
var_dump(intval(...$withBase)); // PHP: 255

$single = ['42'];
var_dump(intval(...$single));   // PHP: 42

With this PR, both calls are currently lowered as conversions of the array itself:

php::toInt(withBase);
php::toInt(single);

This produces the wrong result and bypasses unpacking entirely. The same structural issue applies to unpacked strval(), floatval(), and boolval() calls.

Please also reject an unpacked argument in dispatchConversion(), for example:

if (
    count($expr->args) !== 1
    || !($expr->args[0] instanceof Node\Arg)
    || $expr->args[0]->unpack
) {
    return false;
}

That form must stay on the dynamic call path so the runtime can determine the expanded arity. Please add PHPT coverage for both intval(...['ff', 16]) and a single expanded argument, plus a code-generation assertion that the unpacked array is not passed to php::toInt().

The PR's current targeted PHPUnit and PHPT tests pass locally, but no GitHub checks are reported yet. Once the unpack case is covered, the core fix looks good.

An unpacked call carries a single Node\Arg regardless of its runtime
arity, so the arity check alone still accepted intval(...$args) as a
single-argument Native conversion and lowered the array itself:

    intval(...['ff', 16])   // php::toInt(withBase) -> int(1)
    strval(...['42'])       // php::toString(single) -> "Array" + warning

A named argument has the same shape and need not be the value being
converted: intval(bogus: 1) must raise "Unknown named parameter", not
fold to a cast of 1.

Reject both in dispatchConversion() so the runtime determines the
expanded arity and the parameter names, matching what
dispatchFuncCall() already does for every other builtin.
@Giandonn

Copy link
Copy Markdown
Author

Thank you — you are right, and the failure is worse than a wrong result. I have pushed the guard.

Reproducing it. Adding your two examples to type_conv.phpt and running them against the previous version of this PR:

017- int(255)          017+ int(1)
018- int(42)           018+ int(1)
019- string(2) "42"    019+ Warning: Array to string conversion ... string(5) "Array"
020- float(42)         020+ float(1)

So intval(...['ff', 16]) was counting the array rather than converting it, and strval(...) stringified the array outright.

The guard. dispatchConversion() now rejects the unpacked form as you suggested. I also added $expr->args[0]->name !== null, because a named argument has exactly the same shape and need not be the value being converted:

intval(bogus: 1);   // PHP: Error: Unknown named parameter $bogus
                    // before: php::toInt(1) -> int(1)

That makes the four conversions agree with dispatchFuncCall(), which already rejects both $arg->name !== null and $arg->unpack for every other builtin. If you would rather keep this PR to the unpack case alone, say the word and I will split the named-argument part out.

Coverage. phpunit/code/intval-unpacked-argument.php holds the six unpacked forms plus the named one; the code-generation test asserts that none of php::toInt( / php::toString( / php::toFloat( / php::toBool( appears and that all six unpacks lower to appendUnpacked(. type_conv.phpt covers the same calls at runtime, including intval(...['ff', 16]) === 255, the single expanded argument, and the partial intval('ff', ...[16]).

Verified locally on PHP 8.5.4 ZTS with embed: the PHPT compiles and runs green, ConversionArityTest passes, PHPStan is clean on the touched file, and the full PHPUnit run matches master.

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.

intval() with an explicit base compiles to a cast that drops the base

2 participants