Fix Concat operations with variables #953 #2
Conversation
| @@ -485,10 +485,30 @@ public function unaryMinusNowdoc(): bool | |||
| { | |||
| return | |||
| - | |||
| (int) | |||
There was a problem hiding this comment.
no cast needed here too, and the type can be changed to int
There was a problem hiding this comment.
Uncaught TypeError: Unsupported operand types: string * int
There was a problem hiding this comment.
Without the typecast, it doesn't compile
| @@ -165,6 +165,14 @@ private function getLines(NodeAbstract $node, bool $fromReturns): array | |||
| } | |||
|
|
|||
| if ($node instanceof BinaryOp) { | |||
| if ($node instanceof BinaryOp\Concat && | |||
There was a problem hiding this comment.
how is concat special vs. other binary operators line plus?
There was a problem hiding this comment.
I have no idea, but it seems it is special.
I guess it is because it can trigger typecasting on object by a __toString() call, wether other binary ops don't
There was a problem hiding this comment.
Your reasoning might make sense, but I still belive the var can be optimized to constant with opcache and thus var. itself should not generate a coverage line. I need to test.
There was a problem hiding this comment.
I had to revert it, as it is unstable (line is not present in xdebug) when the var expr is known (from constatnt expr).
There was a problem hiding this comment.
I have to say, I'm kind of disappointed on how you are handling this contribution.
I created a valid test for a valid use case, I got the test passing, you found another edge case and istead of adding them to the tests, you throw my contribution away?
I'm losing my willingness to help, if you discard my help
There was a problem hiding this comment.
I am sorry to hear that, I already spent about 2 - 3 MDs on static analyser to be really a minimal subset of xdebug and implementing sebastianbergmann#953 (comment) might take another several MDs. So what I was supposed to do?
instead of adding them to the tests
test is added in https://github.com/sebastianbergmann/php-code-coverage/blob/c4630fdec1def41d63ce7844d6e91ced5ccfa478/tests/_files/source_with_heavy_indentation.php#L95-L110 ($xa is non-const assign, $xb is const, not an uncommon case)
There was a problem hiding this comment.
$var1 =
'right'
.
$var1
;
$var1 =
$var1
.
'left'
;Where exactly are these two cases tested now in sebastianbergmann#949 ?
There was a problem hiding this comment.
(as of PHP 8.0) the variable assignments are covered
the concat const expr does not produce a coverage line if the variable is const expr - this is tested a for ex. in l105 in the example I posted - I also tested locally concat is not different than any other binary operator (like plus) in sense of coverage
| <<<'EOF' | ||
| foo | ||
| foo | ||
| EOF | ||
| ; | ||
| } | ||
|
|
||
| public function concatWithVar(): string |
There was a problem hiding this comment.
binary plus operator needs same test
Fix NOT sebastianbergmann#953