Fix executable lines analysis

[mvorisek]

add more tests for #948 and fix found issues to make executable lines really a maximal subset (edge cases can be non-present, but not the other way around, otherwise #942) of xdebug

fix https://github.com/sebastianbergmann/php-code-coverage/pull/909/files#r820517185

fix #942 (nested array test added and tested againt about 50k real LoC)

fix #954 (merged via mvorisek#2)

fix #938 (test added)

[codecov]

Codecov Report

Merging #949 (ff6b391) into 9.2 (74b7413) will increase coverage by 0.05%.
The diff coverage is 97.43%.

@@             Coverage Diff              @@
##                9.2     #949      +/-   ##
============================================
+ Coverage     83.65%   83.71%   +0.05%     
- Complexity     1169     1191      +22     
============================================
  Files            59       59              
  Lines          3421     3427       +6     
============================================
+ Hits           2862     2869       +7     
+ Misses          559      558       -1     

Impacted Files	Coverage Δ
...c/StaticAnalysis/ExecutableLinesFindingVisitor.php	97.00% <97.43%> (-0.57%)	⬇️
src/Driver/Xdebug3Driver.php	47.05% <0.00%> (-1.52%)	⬇️
src/Report/Html/Renderer/Dashboard.php	96.29% <0.00%> (-0.17%)	⬇️
src/Report/Text.php	79.33% <0.00%> (-0.14%)	⬇️
src/Filter.php	97.50% <0.00%> (-0.07%)	⬇️
src/Report/Html/Renderer.php	99.23% <0.00%> (-0.02%)	⬇️
src/Driver/PhpdbgDriver.php	0.00% <0.00%> (ø)
src/Report/Html/Renderer/File.php	93.40% <0.00%> (ø)
src/StaticAnalysis/CachingFileAnalyser.php	0.00% <0.00%> (ø)
src/StaticAnalysis/ParsingFileAnalyser.php	98.82% <0.00%> (ø)
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[mvorisek]

Hi @Slamdunk, I added more tests, can you please adjust the static analyser for them?

data generated by xdebug

l53 is not critical to be added, but l125 vs. l127 means whole method will be uncovered (issue #946)

I will then conduct more testing - what is the definition of executable line, what are the cases when it should differ from xdebug coverage output?

[Slamdunk]

Sorry for late reply: I'll answer your questions and review the PR as soon as I can

[Slamdunk]

what is the definition of executable line, what are the cases when it should differ from xdebug coverage output?

Due to the runtime nature of the language, there is no definition of executable line.
Let's take two examples:

 1 function a() {
 2    return
 3        ''
 4        .
 5        ''
 6    ;
 7 }
 8 
 9 function b() {
10     return
11         a()
12         .
13         ''
14     ;
15 }

• In function a() the only executable line reported by a driver will be 5
• but in the function b() the only executable line reported by a driver will be 11, so without 13

So how did I shape the ExecutableLinesFindingVisitor?
Any obvious executable line like a function call or variable fetch or constant fetch is counted.
When no executable line is found, I try my best to match the driver's behavoir aiming at the smallest subset possible.

As far as I can tell, there's no bulletproof algorithm.
In mvorisek#1 I fixed most of the sensible cases, but I turned down the L125 vs L127 dispute.

Who is going to write in real life the example below?

return
    true
    or
    false
;

Almost any similar case involves a function call or a variable fetch or constant fetch: all these three cases are already correctly handled.

[sebastianbergmann]

Can this be closed then?

[Slamdunk]

Can this be closed then?

The opposite: as soon as mvorisek#1 gets merged, this PR will be updated as well and the CI should pass, with a fix for other edge cases that @mvorisek found

[sebastianbergmann]

Ah, so you sent him a PR that will update this PR when merged. Got it, was confused / not fully awake yet. Thanks!

[Slamdunk]

@mvorisek awesome: you can now to this PR into "Ready for review" I guess

[kukulich]

Will this PR fix this?

[mvorisek]

@kukulich what lines do you expect covered and why?

[Slamdunk]

@kukulich as far as I can tell, your code coverage sample works as expected

[kukulich]

Hmm, it looks I sent bad screenshot because I thought both are caused by the change. So now the right screenshots:

phpunit/php-code-coverage 9.2.17

phpunit/php-code-coverage 9.2.18

The problem is that infection/infection now think that the line is not covered and reports mutants.

With phpunit/php-code-coverage 9.2.17: 10 mutants were not covered by tests
With phpunit/php-code-coverage 9.2.18: 68 mutants were not covered by tests

29) /home/runner/work/BetterReflection/BetterReflection/src/Reflection/StringCast/ReflectionAttributeStringCast.php:27    [M] Concat

--- Original
+++ New
@@ @@
     {
         $arguments = $attributeReflection->getArguments();
         $argumentsFormat = $arguments !== [] ? " {\n  - Arguments [%d] {%s\n  }\n}" : '';
-        return sprintf('Attribute [ %s ]' . $argumentsFormat . "\n", $attributeReflection->getName(), count($arguments), self::argumentsToString($arguments));
+        return sprintf($argumentsFormat . 'Attribute [ %s ]' . "\n", $attributeReflection->getName(), count($arguments), self::argumentsToString($arguments));
     }

[mvorisek]

some non-executable (compile time) lines are excluded probably by #892

this PR does not try to add more lines, this PR tries to exclude lines that are no present in xdebug output when formatted with random newlines between php tokens

@Slamdunk can you please review?

@kukulich please describe the problem in separate PR with a test case and description what should be chaged and why

[kukulich]

@mvorisek I'm not sure if I understand now.

There's #946 mentioned in the release notes of 9.2.18

This issue was fixed by PR #948

And this PR draft has in description "Add more tests for #948 and fix found issues" so it looks to me it is still the same problem and my screenshots are relevant.

However I can open new issue with the problem if it's better for you 🤷‍♂️

[mvorisek]

PR is RTM, I squashed the commits and for easier review I separated "else statement fix" into a separate commit as it affected many test reports

858b0f2...05f0673 comparison before squash can help you to understand/follow the static analyser changes made

[Slamdunk]

tests/_files/source_with_heavy_indentation.php has been changed too much, it's a bit exausting to review now, but I trust @mvorisek work on this.

mvorisek#3 instead need to be merged

[mvorisek]

mvorisek#3 instead need to be merged

I have added a test for concat. Here is a list of lines output by xdebug:

95
96
97
99 (variable assign, present as of PHP 8.0)
101 (variable assign, present as of PHP 8.0)
104
113 (variable assign, present as of PHP 8.0)
114 (variable assign, present as of PHP 8.0)
117
118
122
124
127

132
133
134
136 (variable assign, present as of PHP 8.0)
138 (variable assign, present as of PHP 8.0)
141
150 (variable assign, present as of PHP 8.0)
151 (variable assign, present as of PHP 8.0)
154
155
159
161
164

based on this data and my other experiments, I have not found any difference between binary plus and binary concat expression in sense of coverage.

In mvorisek#3 you impose a different handling of binary concat which is wrong. With the test I added, your PR is breaking it and I do not see an easy way to fix it. Even if you will adjust the PR from binary concat to any binary expr (to handle them the same), it will unfix #942 and #889 again as const/enum variable assignments are const expressions too.

Given this explanation I ask you to approve this PR as the #953 is not even related with this PR and close mvorisek#3.

[Slamdunk]

@mvorisek thank you for your patience and explanations.

I approve this PR.

Nevertheless, I now think we are fixing the wrong problem. What ExecutableLinesFindingVisitor does is creating an AST-based code coverage analysis, which will always be wrong since xDebug/PCOV doesn't produce an AST-base CC.
I'll open a discussion tomorrow with a feasible solution. In the meantime this can be merged.

[sebastianbergmann]

I approve this PR.

Good to know. However, the type checker result scares me:

ERROR: PossiblyUndefinedMethod - src/StaticAnalysis/CodeUnitFindingVisitor.php:317:36 - Method PhpParser\Node\IntersectionType::toString does not exist (see https://psalm.dev/108)
                $types[] = $_type->toString();

[mvorisek]

ERROR: PossiblyUndefinedMethod - src/StaticAnalysis/CodeUnitFindingVisitor.php:317:36 - Method PhpParser\Node\IntersectionType::toString does not exist (see https://psalm.dev/108)
                $types[] = $_type->toString();

@sebastianbergmann I did not change the CodeUnitFindingVisitor class, the error is probably due some fix in external CS/CI dep

[sebastianbergmann]

Merged manually, thanks.

[Ocramius]

Just cross-referencing from Roave/BackwardCompatibilityCheck#696 (comment)

I somehow think this worsened the issue reported in #953 :D

[Slamdunk]

I'm pouring my energies into #959 I hope to give a valuable feedback in few weeks