-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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.
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
jrfnl
merged 6 commits into
squizlabs:master
from
fredden:NonExecutableCode.Unreachable/false-positive
Oct 12, 2023
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
33f38f1
Ignore whitespace between tags in unreachable test
fredden 01410b4
Fix false-negative when missing semicolon
fredden b055898
Add more tests
fredden 0924523
Combine 'if' statements, update comment
fredden 34ba861
Add test case with multiple statements on one line
fredden 2067e7c
Add test case with one statement on several lines
fredden File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
64 changes: 64 additions & 0 deletions
64
src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.3.inc
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
<!-- 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 } ?> | ||
|
||
<!-- no problem here --> | ||
<?php if (true) { ?> | ||
<?php foreach ([] as $item) { ?> | ||
<!-- note missing semicolon on next line --> | ||
<?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): ?> | ||
<!-- note missing semicolon on next line --> | ||
<?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; ?> | ||
|
||
<!-- should detect an error here --> | ||
<?php if (true): ?> | ||
<?php foreach ([] as $item): ?> | ||
<?php continue; ?> | ||
<?= 'unreachable - no semicolon' ?> | ||
<?php endforeach; ?> | ||
<?php endif; ?> | ||
|
||
<!-- should detect an error here --> | ||
<?php if (true): ?> | ||
<?php foreach ([] as $item): ?> | ||
<?php continue; ?> | ||
<?= 'unreachable - with semicolon'; ?> | ||
<?php endforeach; ?> | ||
<?php endif; ?> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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.There was a problem hiding this comment.
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_CodeSniffer/src/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.php
Lines 89 to 92 in d148feb
exit
on line 3, and an additional complaint for line 10 from thebreak
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<?= ''; ?>
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?
There was a problem hiding this comment.
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 ?There was a problem hiding this comment.
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 insrc/Standards/Squiz/Tests/PHP/NonExecutableCodeUnitTest.php
) are documenting existing behaviour. I can move these to a separate pull request if you would prefer.