Skip to content

Fixed GH-22727: Attribute return opcodes to return statement - #23397

Open
joostwaaijer wants to merge 2 commits into
php:PHP-8.4from
joostwaaijer:feature/fix-return-expression-lineno
Open

joostwaaijer wants to merge 2 commits into
php:PHP-8.4from
joostwaaijer:feature/fix-return-expression-lineno

Conversation

@joostwaaijer

Copy link
Copy Markdown

Note

This pull request description was prepared with assistance from OpenAI Codex and reviewed by the contributor.

zend_compile_return() currently leaves CG(zend_lineno) at the last line compiled inside a multiline return expression. As a result, VERIFY_RETURN_TYPE and RETURN are attributed to a match arm or ternary branch instead of the return statement. Coverage drivers then omit the actual return line and may report a different branch line as executed. This is the specific return-expression case described in GH-22727 and is part of the broader GH-18985 issue.

This patch captures the statement line before compiling the expression and restores it before emitting return-related opcodes. It deliberately avoids the broader expression compiler changes from GH-22833 that were reverted.

A PHPT regression test verifies the user-visible TypeError line. The test fails on PHP 8.4.24 with line 6 instead of line 5, and passes on the patched PHP 8.4.26-dev build.

Validation: debug NTS build with all Zend/tests (4,837 passed, 134 skipped, 1 expected failure, 0 failures); debug ZTS build with the focused regression test; and PCOV 1.0.12 verification showing the direct return match line changes from absent to hit count 1.

@joostwaaijer
joostwaaijer marked this pull request as ready for review August 21, 2026 06:35
Comment thread Zend/tests/gh22727.phpt Resolved
Co-authored-by: NickSdot <32384907+NickSdot@users.noreply.github.com>
@LamentXU123
LamentXU123 requested a review from ndossche August 23, 2026 07:13
@ndossche

Copy link
Copy Markdown
Member

So the original fix was rejected by @derickr because it influenced too many things in xdebug. I assume he meant tests.
So I guess this depends on @derickr and @iluuu1994

@ndossche
ndossche removed their request for review August 23, 2026 13:47
@anun333

anun333 commented Oct 30, 2026

Copy link
Copy Markdown

Quoted below: written with Claude, an LLM, as CONTRIBUTING.md asks us to mark. The builds and test runs behind it are on my machine; the commands to repeat them are under the fold.

Tested on today's PHP-8.4 (8d63e87a496) with this PR cherry-picked, debug builds. The line PHP reports for something that happens at a multi-line return, in one test file:

Return on line What is reported PHP-8.4 this PR
12: return match (true) {, the arm on 13 returns a string from an int function TypeError 13 12
17: return $b, the : 'no' branch on 19 TypeError 19 17
28: return implode(, an argument on 30 TypeError 30 28
34: return 1 / + 2; in function &f() "Only variable references should be returned by reference" 35 34
22: return [, the first element on 23 TypeError 23 23
37: fn(): int =>, the body 'x' on 38 TypeError 38 38
46: return intdiv(, the 0 on 48 (control: thrown inside the call) DivisionByZeroError 46 46

So the errors raised by the return itself now point at the statement. Two shapes keep the later line: a multi-line array literal, because the return statement's line comes from the array node, whose line is its first element's (23 rather than the [ on 22); and an arrow function whose body starts on the next line, where there is no return to point at. Zend/tests passes on both (5,023; 5,024 with the new test), so no existing expectation moves, and the new test fails without the fix.

For the merge-up: on today's master Zend/zend_compile.c conflicts, right next to the "Returning a value from a constructor is deprecated" block in zend_compile_return(), so it needs a small hand merge. #23305 also changes zend_compile_return() on master.

How this was tested

Linux x86_64, gcc 13.3, debug builds of origin/PHP-8.4 and of it with this PR's commits cherry-picked:

./buildconf --force
./configure --enable-debug --disable-cgi --disable-phpdbg --disable-fileinfo --enable-pcntl --enable-zend-test --enable-fpm --with-curl --enable-mbstring --disable-mbregex --with-mysqli --with-pdo-mysql --with-openssl --with-bz2
make -j4

One PHP file holds the seven functions; each is called inside try/catch with an error handler, and the getLine() of the throwable (or the handler's line) is printed. Suites: TEST_PHP_EXECUTABLE=$PWD/sapi/cli/php sapi/cli/php run-tests.php -n -q -j4 Zend/tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants