Review use of T_SEMICOLON in sniffs

[jrfnl]

The bug as reported in #537 highlights an area where a full sniff review would not be amiss.

Basically any time a findPrevious()/findNext() call searches for a T_SEMICOLON token to find the end of a statement, or when using "manual" token walking, but again looking for a T_SEMICOLON, there is a chance that the sniff should also check for the T_CLOSE_TAG token (and/or possibly the T_OPEN_TAG/T_OPEN_TAG_WITH_ECHO tokens).

Keep in mind: the ?> in PHP will automatically inject a semi-colon if one is not already there.

Task list

A quick search of the code base yields the following files/sniffs which will need to be reviewed. I only expect a small number of these to need changes.

To Do

• File class
• Generic.CodeAnalysis.AssignmentInCondition
• Generic.CodeAnalysis.EmptyPHPStatement
• Generic.CodeAnalysis.JumbledIncrementer
• Generic.CodeAnalysis.UnusedFunctionParameter
• Generic.ControlStructures.InlineControlStructure
• Generic.Formatting.DisallowMultipleStatements
• Generic.Formatting.MultipleStatementAlignment
• Generic.NamingConventions.UpperCaseConstantName
• Generic.PHP.LowerCaseConstant
• Generic.PHP.LowerCaseType
• PEAR.Commenting.FileComment
• PEAR.Functions.FunctionCallSignature
• PEAR.Functions.FunctionDeclaration
• PSR1.Files.SideEffects
• PSR2.Classes.PropertyDeclaration
• PSR2.ControlStructures.SwitchDeclaration
• PSR2.Namespaces.UseDeclaration
• PSR12.Classes.AnonClassDeclaration
• PSR12.Files.DeclareStatement
• PSR12.Traits.UseDeclaration
• Squiz.Commenting.FunctionComment
• Squiz.Commenting.InlineComment
• Squiz.Commenting.LongConditionClosingComment
• Squiz.Commenting.PostStatementComment
• Squiz.ControlStructures.ControlSignature
• Squiz.ControlStructures.ForLoopDeclaration
• Squiz.ControlStructures.InlineIfDeclaration
• Squiz.Operators.ComparisonOperatorUsage
• Squiz.Operators.IncrementDecrementUsage
• Squiz.PHP.DisallowSizeFunctionsInLoops
• Squiz.PHP.EmbeddedPhp
• Squiz.PHP.NonExecutableCode
• Squiz.WhiteSpace.ControlStructureSpacing
• Squiz.WhiteSpace.FunctionSpacing
• Squiz.WhiteSpace.LanguageConstructSpacing
• Squiz.WhiteSpace.MemberVarSpacing
• Squiz.WhiteSpace.ScopeKeywordSpacing
• Squiz.WhiteSpace.SemicolonSpacing
• Tokenizers\PHP class
• Tokenizers\Tokenizer class

* Note: a few CSS/JS specific files have been excluded from the above task list as this is a PHP specific issue.

Under review

nothing yet

Reviewed and PR submitted with update

• Generic.CodeAnalysis.UselessOverridingMethod - @jrfnl PR Generic/UselessOverridingMethod: improve handling of PHP open/close tags between statements #554
• PSR2.Classes.ClassDeclaration - @jrfnl: no code change needed, but adding a test is warranted. PR PSR2/ClassDeclaration: add tests and remove some redundant code #556
• Squiz.Classes.SelfMemberReference - @jrfnl PR Squiz/SelfMemberReference: handle namespace declaration ending on PHP close tag #555
• Squiz.PHP.DisallowMultipleAssignments - @jrfnl PR Squiz/DisallowMultipleAssignments: fix sniff walking back too far when going in/out of PHP #538

Reviewed and concluded no changes needed

• Generic.CodeAnalysis.ForLoopShouldBeWhileLoop - @jrfnl: sniff only looks within for condition. This is fine.
• Generic.CodeAnalysis.ForLoopWithTestFunctionCall - @jrfnl: sniff only looks within for condition. This is fine.
• Generic.WhiteSpace.LanguageConstructSpacing - @jrfnl: the semicolon check is used to bow out early when no space is needed (between the keyword and the semi-colon). I think it's reasonable for the sniff to expect a space between the keyword and a PHP close tag though, so I don't think any changes are needed.
• MySource.Objects.AssignThis - @jrfnl: JS-only sniff
• MySource.Objects.CreateWidgetTypeCallback - @jrfnl: JS-only sniff
• MySource.PHP.EvalObjectFactory - @jrfnl: well, this sniff should probably also look for T_CLOSE_TAG, but this is a deprecated sniff and just looking at the code, there's a lot more which needs fixing if the sniff wasn't deprecated already. IMO it is not worth spending time on this sniff.
• PSR2.Files.ClosingTag - @jrfnl: this sniff is about the close tag itself, so the close tag is already taken into account.
• Squiz.Strings.EchoedStrings - @jrfnl: already handled correctly
• Zend.Files.ClosingTag - @jrfnl: this sniff is about the close tag itself, so the close tag is already taken into account.

Guidelines

• Some sniffs/code may already handle this correctly and the review would just confirm this.
• There are some statements in PHP which cannot really contain embedded PHP, so in those cases, no check for T_CLOSE_TAG is needed.
• In all other cases, a critical look at the code referencing the T_SEMICOLON token is warranted.

Any changes made should be accompanied by one or more tests demonstrating the false positives/false negatives prevented by the change.

PRs for this ticket should preferably contain the changes for one sniff per PR.

If you want to contribute to this task, please leave a comment "claiming" one or more files you will be reviewing. This way we can prevent multiple people working on the same thing.

If a review concludes that no changes are needed, just note that in a comment as well and I'll update the above task list.