PHPCSStandards/PHP_CodeSniffer

Review use of `T_SEMICOLON` in sniffs

Opened this issue · 0 comments

jrfnl commented

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 #554
  • PSR2.Classes.ClassDeclaration - @jrfnl: no code change needed, but adding a test is warranted. PR #556
  • Squiz.Classes.SelfMemberReference - @jrfnl PR #555
  • Squiz.PHP.DisallowMultipleAssignments - @jrfnl PR #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.