Repository navigation
Inner expression compilation (for match) can result in wrong line numbers #18985
Description
Description
The following code:
<?php
echo match(15) {
13 => "A",
15 => "B",
default => "C",
};Resulted in this output:
L0002 0000 MATCH int(15) 13: 0001, 15: 0003, default: 0005
L0003 0001 T1 = QM_ASSIGN string("A")
L0003 0002 JMP 0007
L0004 0003 T1 = QM_ASSIGN string("B")
L0004 0004 JMP 0007
L0005 0005 T1 = QM_ASSIGN string("C")
L0005 0006 JMP 0007
L0005 0007 ECHO T1 <<<<<<<<<<<<<<<<<<<<<<<< line 5 instead of 2!
L0007 0008 RETURN int(1)
But I expected this output instead:
L0002 0000 MATCH int(15) 13: 0001, 15: 0003, default: 0005
L0003 0001 T1 = QM_ASSIGN string("A")
L0003 0002 JMP 0007
L0004 0003 T1 = QM_ASSIGN string("B")
L0004 0004 JMP 0007
L0005 0005 T1 = QM_ASSIGN string("C")
L0005 0006 JMP 0007
L0002 0007 ECHO T1
L0007 0008 RETURN int(1)
The echo is on line 2, not line 5. This is because the line no gets overwritten. There's probably many more of these cases.
Discovered via krakjoe/pcov#87
Possible fix idea (may be incomplete):
https://github.com/proxy/gist.github.com/nielsdos/fe1a4b30aee84efff23f32dc4b0e5ffe
cc @iluuu1994
PHP Version
8.3+
Operating System
No response
Activity
iluuu1994 commentedon Jun 30, 2025
Your patch works for match, and it's likely one of the bigger offender because it's very common to format it over multiple lines. Hence, I would be ok with this simple patch. It does seem like this issue is not limited to match. For example, a simple binary op: https://3v4l.org/XmIih/vld#vnull
The settings of
CG(zend_lineno)happens a bit all over the place. Not having thought about it too deeply, it may be possible to always resetCG(zend_lineno)at the end ofzend_compile_expr(), and depend on other expressions to setlinenofor themselves. If it doesn't, it will fallback to thelinenoset inzend_compile_stmt(). This should be more predictable. But I haven't tried it.ndossche commentedon Jun 30, 2025
I briefly explored it and it also occurs for IIFEs.
I briefly tried this and it didn't really work but I didn't spend time on it to figure out why.
iluuu1994 commentedon Jul 10, 2025
I tried this quickly, and while it does solve this issue for
match, it changes behavior and may lead to other regressions.One issue is that our lineno tracking for ASTs is quite inaccurate. To elaborate, we update
CG(zend_lineno)when advancing the lexer, which is only used once the parsing of the full expression is done, when creating the AST node. For example,$a \n + \n $bwill not create theADDnode until we've seen$b, at which point we'll have advancedCG(zend_lineno)accordingly. Intuitively, it would seem best for theADDnode to store the lexical position of the+symbol as its lineno. One way to solve this is to backupCG(zend_lineno)in the parser:While not particularly complicated, this will get very tedious to add for every expression. Not sure if this change is worth it, but I can create a PoC.
ndossche commentedon Jul 10, 2025
I see. Can indeed be dangerous regression-wise. Maybe we should for now fix it for match only?
I also found e528762 which may be related (it was reverted later, see comments).
iluuu1994 commentedon Jul 10, 2025
I'm ok fixing this for match only. One thing that confuses me though: Why does this break coverage in the first place? Should coverage not just highlight these lines as gray if no opcode is produced for the given line?
Edit: Oh, I guess it's the opposite? Because multiple expressions are collapsed to the last line, it's reported as covered even though it's not?
ndossche commentedon Jul 10, 2025
Indeed, lines are marked as covered that should not be, because the lines of different opcodes are the same.
iluuu1994 commentedon Jul 11, 2025
Nevermind, this statement was not accurate. I missed that
zend_ast_create_Nall have the following condition:I.e. when the AST has children, we disregard
CG(zend_lineno)and just use thelinenofrom the first child. Maybe a general approach then would not be so problematic after all. There might still be some issues, which I'll consider a bit more closely. E.g. for something like:We might want
foo()to be on line 1 and the addition on line 2 so that both lines are covered, i.e. we'd set thelinenotochildren[1]->lineno. Obviously the inverse can also happen, but seems a bit less likely.This would report line 2 for both. That also seems to be the current behavior, so at least we're not making things worse.
krakjoe commentedon Sep 18, 2025
@iluuu1994 can you do some words about your preferred approach here ?
We reverted that bison change (which I obviously like) after looking at one rather coarse metric produced by the first attempted (or committed anyway) implementation. It's a significant difference but I miss why it's so significant. If the bison location tracking is at all viable, I would rather we do that. If this difference can be minimized or preferably essentially eliminated, we aren't going to have to revisit this issue again (for as long as we use bison, I would suppose).
I would also note, the problems bad location tracking creates is not limited to coverage collection, it also effects debuggers ability to break at the correct locations. It's typically new language features where location tracking is inaccurate, this has knock on effects that are quite nasty - your ability as an end user to debug new language features is impacted.
jrchamp commentedon Nov 2, 2025
For anyone following along, here is the current gist patch from @ndossche.
RETURNopcode forreturn <multi-line conditional expression>;is attributed to a line inside a never-executed branch, causing false-positive code coverage #22727 as a duplicate of this issueTweak lineno compilation
iluuu1994 commentedon Aug 12, 2026
This caused too many changes for Derick that would have been a huge time-sink to fix, to I reverted GH-22833 for now. We'll need to apply a different fix, so let's reopen.
[skip ci] Restore NEWS entry for GH-18985 in old release notes
sebastianbergmann commentedon Nov 9, 2026
Thank you for working on this, Ilija, and for reopening the issue.
I understand that the changed line numbers would have meant a lot of work for Derick (also see #23397 (comment)), and I don't want to make light of that: going through and updating a large number of test expectations in Xdebug is real effort.
That said, I am a bit uneasy about this being the reason for reverting the fix. As far as I can tell, any fix for this bug will necessarily change line numbers, so test expectations that were written against the current behaviour will have to be updated sooner or later. Unless, of course, some of the new line numbers are wrong or worse than before. If that is the case, it would be good to know which cases are affected so that they can be addressed.
The incorrect line attribution affects more than one tool. In sebastianbergmann/php-code-coverage#1257, for instance, a never-executed
defaultarm of amatchexpression is reported as covered. As Joe pointed out, debuggers are affected as well, and #22727 shows that the line reported for aTypeErrorcan be wrong, too. Tools such as php-code-coverage can work around some of this, but they cannot fix it.Would it be possible to re-apply #22833, or a revised version of it, early in the next release cycle, so that extension maintainers have as much time as possible to adapt?
iluuu1994 commentedon Nov 9, 2026
Hey Sebastian. To add some context: Derick didn't complain about the number of changes. It's more that many things changed that weren't necessarily objectively better, and fixing those cases manually would have taken a long time. If we want to move this forward, it would likely be better to reconsider Nikita's old approach that Nora linked: e528762
If just
matchspecifically causes a lot of issues, we can push a quick-fix, though that seems like a bit of a one-off.sebastianbergmann commentedon Nov 9, 2026
Then I misunderstood #23397 (comment), my bad.
It is not just
match, butmatchcomes up a lot. I trust you to make the right decision with regard to one-off quick fix versus a more thorough fix.