Skip to content

Fix Concat operations with variables #953 #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Nov 10, 2022
Merged

Fix Concat operations with variables #953 #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f795a1a
Fix Concat operations with variables #953
Slamdunk Nov 8, 2022
33b6f30
Test against heredocs to ensure first line gets always counted correctly
Slamdunk Nov 10, 2022
9735971
Add coverage for empty methods
Slamdunk Nov 10, 2022
abe0c54
Gather CC of empty functions too
Slamdunk Nov 10, 2022
c4eaf4d
Revert unnecessary cast
Slamdunk Nov 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions src/StaticAnalysis/ExecutableLinesFindingVisitor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
use PhpParser\Node\Stmt\Finally_;
use PhpParser\Node\Stmt\For_;
use PhpParser\Node\Stmt\Foreach_;
use PhpParser\Node\Stmt\Function_;
use PhpParser\Node\Stmt\Goto_;
use PhpParser\Node\Stmt\If_;
use PhpParser\Node\Stmt\Property;
Expand Down Expand Up @@ -165,6 +166,14 @@ private function getLines(NodeAbstract $node, bool $fromReturns): array
}

if ($node instanceof BinaryOp) {
if ($node instanceof BinaryOp\Concat &&
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how is concat special vs. other binary operators line plus?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to revert it, as it is unstable (line is not present in xdebug) when the var expr is known (from constatnt expr).

Copy link
Author

@Slamdunk Slamdunk Nov 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

@mvorisek mvorisek Nov 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

$var1 =
    'right'
    .
    $var1
;

$var1 =
    $var1
    .
    'left'
;

Where exactly are these two cases tested now in sebastianbergmann#949 ?

Copy link
Owner

@mvorisek mvorisek Nov 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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

(
$node->left instanceof Node\Expr\Variable ||
$node->right instanceof Node\Expr\Variable
)) {
return [$this->getNodeStartLine($node->right)];
}

return $fromReturns ? $this->getLines($node->right, $fromReturns) : [];
}

Expand All @@ -180,6 +189,10 @@ private function getLines(NodeAbstract $node, bool $fromReturns): array

if ($node instanceof ClassMethod) {
if ($node->name->name !== '__construct') {
if ($node->stmts === []) {
return [$node->getEndLine()];
}

return [];
}

Expand Down Expand Up @@ -232,6 +245,14 @@ private function getLines(NodeAbstract $node, bool $fromReturns): array
return [$this->getNodeStartLine($node->cond)];
}

if ($node instanceof Function_) {
if ($node->stmts === []) {
return [$node->getEndLine()];
}

return [];
}

return [$this->getNodeStartLine($node)];
}

Expand Down Expand Up @@ -292,6 +313,7 @@ private function isExecutable(Node $node): bool
$node instanceof Finally_ ||
$node instanceof For_ ||
$node instanceof Foreach_ ||
$node instanceof Function_ ||
$node instanceof Goto_ ||
$node instanceof If_ ||
$node instanceof Match_ ||
Expand Down
34 changes: 34 additions & 0 deletions tests/_files/source_with_multiline_constant_return.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,10 +485,44 @@ public function unaryMinusNowdoc(): float
{
return
-
(int)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no cast needed here too, and the type can be changed to int

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uncaught TypeError: Unsupported operand types: string * int

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without the typecast, it doesn't compile

<<<'EOF'
1.
2
EOF
;
}

public function concatWithVar(): string
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

binary plus operator needs same test

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reverted, see #2 (comment)

{
$var1 = 'start';

$var1 =
<<<'EOF'
right
EOF
.
$var1
;

$var1 =
$var1
.
<<<'EOF'
left
EOF
;

return $var1;
}

public
function
emptyMethod
(
)
:
void
{
}
}
4 changes: 4 additions & 0 deletions tests/_files/source_with_oneline_annotations.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,7 @@ function baz()

print '*';
}

function xyz()
{
}
11 changes: 10 additions & 1 deletion tests/tests/Data/RawCodeCoverageDataTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,7 @@ public function testInlineCommentsKeepTheLine(): void

$this->assertEquals(
[
13,
19,
22,
26,
Expand All @@ -320,6 +321,7 @@ public function testInlineCommentsKeepTheLine(): void
32,
33,
35,
40,
],
array_keys(RawCodeCoverageData::fromUncoveredFile($file, new ParsingFileAnalyser(true, true))->lineCoverage()[$file])
);
Expand Down Expand Up @@ -497,7 +499,14 @@ public function testReturnStatementWithConstantExprOnlyReturnTheLineOfLast(): vo
456,
466,
478,
489,
490,
498,
500,
505,
508,
512, // This is correct: not the line with the $var1, but the right operand of the Concat
516,
527,
],
array_keys(RawCodeCoverageData::fromUncoveredFile($file, new ParsingFileAnalyser(true, true))->lineCoverage()[$file])
);
Expand Down