Skip to content
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

Squiz/NonExecutableCode: fix false positives when code goes in and out of PHP/HTML #3770

Merged
merged 6 commits into from
Oct 12, 2023
Merged

Squiz/NonExecutableCode: fix false positives when code goes in and out of PHP/HTML #3770

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
33f38f1
Ignore whitespace between tags in unreachable test
fredden Feb 27, 2023
01410b4
Fix false-negative when missing semicolon
fredden Jun 8, 2023
b055898
Add more tests
fredden Jun 8, 2023
0924523
Combine 'if' statements, update comment
fredden Jun 12, 2023
34ba861
Add test case with multiple statements on one line
fredden Jun 12, 2023
2067e7c
Add test case with one statement on several lines
fredden Jun 12, 2023
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
1 change: 1 addition & 0 deletions package.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1857,6 +1857,7 @@ http://pear.php.net/dtd/package-2.0.xsd">
<file baseinstalldir="PHP/CodeSniffer" name="LowercasePHPFunctionsUnitTest.php" role="test" />
<file baseinstalldir="PHP/CodeSniffer" name="NonExecutableCodeUnitTest.1.inc" role="test" />
<file baseinstalldir="PHP/CodeSniffer" name="NonExecutableCodeUnitTest.2.inc" role="test" />
<file baseinstalldir="PHP/CodeSniffer" name="NonExecutableCodeUnitTest.3.inc" role="test" />
<file baseinstalldir="PHP/CodeSniffer" name="NonExecutableCodeUnitTest.php" role="test" />
</dir>
<dir name="Scope">
Expand Down
10 changes: 10 additions & 0 deletions src/Standards/Squiz/Sniffs/PHP/NonExecutableCodeSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,16 @@ public function process(File $phpcsFile, $stackPtr)
continue;
}

// Skip HTML whitespace.
if ($tokens[$i]['code'] === T_INLINE_HTML && \trim($tokens[$i]['content']) === '') {
continue;
}

// Skip PHP re-open tag (eg, after inline HTML).
if ($tokens[$i]['code'] === T_OPEN_TAG) {
Copy link
Contributor

Choose a reason for hiding this comment

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

What about if the "re-open tag" is a PHP open tag with echo <?= ?

Might be good to add some tests for that as well.

Copy link
Contributor Author

@fredden fredden Jun 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, the sniff already handles this case correctly. I've added a test (in b055898) to confirm.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've just tested this, but as far as I can see, the sniff does not handle this correctly, but the test used for this is flawed.

Try this:

<!-- no problem here -->
<?php if (true): ?>
    <?php foreach ([] as $item): ?>
        <?php continue; ?>
        <?= ?>
    <?php endforeach; ?>
<?php endif; ?>

<!-- should detect an error here -->
<?php if (true): ?>
    <?php foreach ([] as $item): ?>
        <?php continue; ?>
        <?=
        'unreachable'
        ?>
    <?php endforeach; ?>
<?php endif; ?>

While an empty <?= ?> isn't very useful, that's not the concern of the sniff.
If the <?= was handled correctly, the first and only error for the above code would be on the line containing the 'unreachable'. As things are, both lines containing a <?= also throw errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback. I've looked into both of these suggested test cases.

The sniff will complain about any line of code which is unreachable. (See

PHP_CodeSniffer/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.2.inc

Lines 1 to 12 in d148feb

<?php
exit();
// Errors are thrown from here down from the exit() above.
foreach ($vars as $var) {
if ($something === TRUE) {
break;
break;
}
}
and

PHP_CodeSniffer/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.php

Lines 89 to 92 in d148feb

7 => 1,
8 => 1,
9 => 1,
10 => 2,
- one complaint for each of lines 7-10 from the exit on line 3, and an additional complaint for line 10 from the break on line 9.)

I didn't see an existing test showing the expected behaviour when there are multiple unreachable statements on one line. I've added a test showing the existing behaviour in 34ba861.

Looking at these two suggested sniffs:

  • <?= ?> is a parse error, so I don't think it needs any special treatment in this sniff. https://3v4l.org/MXtbr
  • One statement split across multiple lines also isn't covered by existing tests; I've added a test showing the existing behaviour in 2067e7c.
  • We could add some logic to detect something like <?= ''; ?> which isn't a parse error, and will produce no output, but such a line of code seems like something that should be flagged to developers for review, so I'm comfortable with this sniff reporting that as unreachable. I don't think it's feasible for this sniff to detect output of a variable which happens to be empty and alert differently. A human can review the line of code in that case and make a sensible judgement.

@jrfnl do these cover the concerns raised here, or would you like me to add these specific tests as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm - the tests added in the 2 case file just document existing behaviour. The behaviour regarding these is not changed by this PR. Correct ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the changes in src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.2.inc (and the corresponding additions in src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.php) are documenting existing behaviour. I can move these to a separate pull request if you would prefer.

continue;
}

$line = $tokens[$i]['line'];
if ($line > $lastLine) {
$type = substr($tokens[$stackPtr]['type'], 2);
Expand Down
31 changes: 31 additions & 0 deletions src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.3.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<!-- no problem here -->
<?php if (true): ?>
<?php foreach ([] as $item): ?>
<?php continue; ?>
<?php endforeach; ?>
<?php endif; ?>

<!-- no problem here -->
<?php if (true) { ?>
<?php foreach ([] as $item) { ?>
<?php continue; ?>
<?php } ?>
<?php } ?>

<!-- should detect an error here -->
<?php if (true): ?>
<?php foreach ([] as $item): ?>
<?php continue; ?>
<div>non-executable</div>
<?php endforeach; ?>
<?php endif; ?>

<!-- should detect an error here -->
<?php if (true): ?>
<?php foreach ([] as $item): ?>
<?php continue; ?>

<div>non-executable</div>

<?php endforeach; ?>
<?php endif; ?>
5 changes: 5 additions & 0 deletions src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,11 @@ public function getWarningList($testFile='')
54 => 2,
];
break;
case 'NonExecutableCodeUnitTest.3.inc':
return [
19 => 1,
28 => 1,
];
default:
return [];
break;
Expand Down