Squiz/FunctionDeclarationArgumentSpacing: various bug fixes and prevent fixer conflicts #783
Conversation
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
The fixer for the `SpacingBetween` error code would only handle one whitespace token at a time, which is inefficient and could also lead to more complex fixer conflicts.
This commit changes the fixer to handle all whitespace between the parentheses in one go.
For the code sample added as a test, this means the difference between the fixer needing 7 loops (including two skipped loops due to possible conflicts) versus 2 loops.
<details>
<summary>Old:</summary>
```
=> Fixing file: 1/1 violations remaining
Squiz.Functions.FunctionDeclarationArgumentSpacing:133 replaced token 6 (T_WHITESPACE on line 3) "\n\n" => "\n"
=> Fixing file: 1/1 violations remaining [made 1 pass]...
* fixed 1 violations, starting loop 2 *
Squiz.Functions.FunctionDeclarationArgumentSpacing:133 replaced token 6 (T_WHITESPACE on line 3) "\n\n" => "\n"
=> Fixing file: 1/1 violations remaining [made 2 passes]...
* fixed 1 violations, starting loop 3 *
**** Squiz.Functions.FunctionDeclarationArgumentSpacing:133 has possible conflict with another sniff on loop 1; caused by the following change ****
**** replaced token 6 (T_WHITESPACE on line 3) "\n " => " " ****
**** ignoring all changes until next loop ****
=> Fixing file: 0/1 violations remaining [made 3 passes]...
* fixed 0 violations, starting loop 4 *
Squiz.Functions.FunctionDeclarationArgumentSpacing:133 replaced token 6 (T_WHITESPACE on line 3) "\n " => " "
=> Fixing file: 1/1 violations remaining [made 4 passes]...
* fixed 1 violations, starting loop 5 *
**** Squiz.Functions.FunctionDeclarationArgumentSpacing:133 has possible conflict with another sniff on loop 3; caused by the following change ****
**** replaced token 6 (T_WHITESPACE on line 3) " )" => ")" ****
**** ignoring all changes until next loop ****
=> Fixing file: 0/1 violations remaining [made 5 passes]...
* fixed 0 violations, starting loop 6 *
Squiz.Functions.FunctionDeclarationArgumentSpacing:133 replaced token 6 (T_WHITESPACE on line 3) " )" => ")"
=> Fixing file: 1/1 violations remaining [made 6 passes]...
* fixed 1 violations, starting loop 7 *
=> Fixing file: 0/1 violations remaining [made 7 passes]...
```
</details>
<details>
<summary>New:</summary>
```
=> Fixing file: 1/1 violations remaining
=> Changeset started by Squiz.Functions.FunctionDeclarationArgumentSpacing:133
Q: Squiz.Functions.FunctionDeclarationArgumentSpacing:135 replaced token 6 (T_WHITESPACE on line 3) "\n\n" => "\n"
Q: Squiz.Functions.FunctionDeclarationArgumentSpacing:135 replaced token 7 (T_WHITESPACE on line 4) "\n\n" => "\n"
Q: Squiz.Functions.FunctionDeclarationArgumentSpacing:135 replaced token 8 (T_WHITESPACE on line 5) "\n " => " "
Q: Squiz.Functions.FunctionDeclarationArgumentSpacing:135 replaced token 9 (T_WHITESPACE on line 6) " )" => ")"
A: Squiz.Functions.FunctionDeclarationArgumentSpacing:137 replaced token 6 (T_WHITESPACE on line 3) "\n\n" => "\n"
A: Squiz.Functions.FunctionDeclarationArgumentSpacing:137 replaced token 7 (T_WHITESPACE on line 4) "\n\n" => "\n"
A: Squiz.Functions.FunctionDeclarationArgumentSpacing:137 replaced token 8 (T_WHITESPACE on line 5) "\n " => " "
A: Squiz.Functions.FunctionDeclarationArgumentSpacing:137 replaced token 9 (T_WHITESPACE on line 6) " )" => ")"
=> Changeset ended: 4 changes applied
=> Fixing file: 4/1 violations remaining [made 1 pass]...
* fixed 4 violations, starting loop 2 *
=> Fixing file: 0/1 violations remaining [made 2 passes]...
```
</details>
…rence` does not flag new lines A `T_WHITESPACE` token only containing new line characters will have its token `'length'` set to `0`. This means that if there was a new line after a reference token, the sniff would not report it. Fixed now. Also note that, as the sniff demands no whitespace after a reference operator, we don't need the extra check for `$gap !== 0` as we already know that the next token is a whitespace token and needs to be flagged. Includes tests.
…nce` fixer
The fixer for the `SpacingAfterReference` error code would only handle one whitespace token at a time, which is inefficient and could also lead to more complex fixer conflicts.
This commit changes the fixer to handle all whitespace tokens after a reference token in one go.
For the test code sample added in the previous commit, this means the difference between the fixer needing 5 loops (including one skipped loop due to possible conflicts) versus 2 loops.
<details>
<summary>Old:</summary>
```
=> Fixing file: 1/1 violations remaining
Squiz.Functions.FunctionDeclarationArgumentSpacing:164 replaced token 9 (T_WHITESPACE on line 4) "\n\n" => "\n"
=> Fixing file: 1/1 violations remaining [made 1 pass]...
* fixed 1 violations, starting loop 2 *
Squiz.Functions.FunctionDeclarationArgumentSpacing:164 replaced token 9 (T_WHITESPACE on line 4) "\n " => " "
=> Fixing file: 1/1 violations remaining [made 2 passes]...
* fixed 1 violations, starting loop 3 *
**** Squiz.Functions.FunctionDeclarationArgumentSpacing:164 has possible conflict with another sniff on loop 1; caused by the following change ****
**** replaced token 9 (T_WHITESPACE on line 4) " $param" => "$param" ****
**** ignoring all changes until next loop ****
=> Fixing file: 0/1 violations remaining [made 3 passes]...
* fixed 0 violations, starting loop 4 *
Squiz.Functions.FunctionDeclarationArgumentSpacing:164 replaced token 9 (T_WHITESPACE on line 4) " $param" => "$param"
=> Fixing file: 1/1 violations remaining [made 4 passes]...
* fixed 1 violations, starting loop 5 *
=> Fixing file: 0/1 violations remaining [made 5 passes]...
```
</details>
<details>
<summary>New:</summary>
```
=> Fixing file: 1/1 violations remaining
=> Changeset started by Squiz.Functions.FunctionDeclarationArgumentSpacing:164
Q: Squiz.Functions.FunctionDeclarationArgumentSpacing:166 replaced token 9 (T_WHITESPACE on line 4) "\n \n" => " \n"
Q: Squiz.Functions.FunctionDeclarationArgumentSpacing:166 replaced token 10 (T_WHITESPACE on line 5) " \n " => " "
Q: Squiz.Functions.FunctionDeclarationArgumentSpacing:166 replaced token 11 (T_WHITESPACE on line 6) " $param" => "$param"
A: Squiz.Functions.FunctionDeclarationArgumentSpacing:169 replaced token 9 (T_WHITESPACE on line 4) "\n \n" => " \n"
A: Squiz.Functions.FunctionDeclarationArgumentSpacing:169 replaced token 10 (T_WHITESPACE on line 5) " \n " => " "
A: Squiz.Functions.FunctionDeclarationArgumentSpacing:169 replaced token 11 (T_WHITESPACE on line 6) " $param" => "$param"
=> Changeset ended: 3 changes applied
=> Fixing file: 3/1 violations remaining [made 1 pass]...
* fixed 3 violations, starting loop 2 *
=> Fixing file: 0/1 violations remaining [made 2 passes]...
```
</details>
…adic` does not flag new lines A `T_WHITESPACE` token only containing new line characters will have its token `'length'` set to `0`. This means that if there was a new line after an ellipsis token, the sniff would not report it. Fixed now. Also note that, as the sniff demands no whitespace after a reference operator, we don't need the extra check for `$gap !== 0` as we already know that the next token is a whitespace token and needs to be flagged. Includes test.
…ic` fixer
The fixer for the `SpacingAfterVariadic` error code would only handle one whitespace token at a time, which is inefficient and could also lead to more complex fixer conflicts.
This commit changes the fixer to handle all whitespace tokens after a variadic token in one go.
For the test code sample added in the previous commit, this means the difference between the fixer needing 5 loops (including one skipped loop due to possible conflicts) versus 2 loops.
<details>
<summary>Old:</summary>
```
=> Fixing file: 1/1 violations remaining
Squiz.Functions.FunctionDeclarationArgumentSpacing:190 replaced token 9 (T_WHITESPACE on line 4) "\n\n" => "\n"
=> Fixing file: 1/1 violations remaining [made 1 pass]...
* fixed 1 violations, starting loop 2 *
Squiz.Functions.FunctionDeclarationArgumentSpacing:190 replaced token 9 (T_WHITESPACE on line 4) "\n " => " "
=> Fixing file: 1/1 violations remaining [made 2 passes]...
* fixed 1 violations, starting loop 3 *
**** Squiz.Functions.FunctionDeclarationArgumentSpacing:190 has possible conflict with another sniff on loop 1; caused by the following change ****
**** replaced token 9 (T_WHITESPACE on line 4) " $param" => "$param" ****
**** ignoring all changes until next loop ****
=> Fixing file: 0/1 violations remaining [made 3 passes]...
* fixed 0 violations, starting loop 4 *
Squiz.Functions.FunctionDeclarationArgumentSpacing:190 replaced token 9 (T_WHITESPACE on line 4) " $param" => "$param"
=> Fixing file: 1/1 violations remaining [made 4 passes]...
* fixed 1 violations, starting loop 5 *
=> Fixing file: 0/1 violations remaining [made 5 passes]...
```
</details>
<details>
<summary>New:</summary>
```
=> Fixing file: 1/1 violations remaining
=> Changeset started by Squiz.Functions.FunctionDeclarationArgumentSpacing:190
Q: Squiz.Functions.FunctionDeclarationArgumentSpacing:192 replaced token 9 (T_WHITESPACE on line 4) "\n\n" => "\n"
Q: Squiz.Functions.FunctionDeclarationArgumentSpacing:192 replaced token 10 (T_WHITESPACE on line 5) "\n " => " "
Q: Squiz.Functions.FunctionDeclarationArgumentSpacing:192 replaced token 11 (T_WHITESPACE on line 6) " $param" => "$param"
A: Squiz.Functions.FunctionDeclarationArgumentSpacing:195 replaced token 9 (T_WHITESPACE on line 4) "\n\n" => "\n"
A: Squiz.Functions.FunctionDeclarationArgumentSpacing:195 replaced token 10 (T_WHITESPACE on line 5) "\n " => " "
A: Squiz.Functions.FunctionDeclarationArgumentSpacing:195 replaced token 11 (T_WHITESPACE on line 6) " $param" => "$param"
=> Changeset ended: 3 changes applied
=> Fixing file: 3/1 violations remaining [made 1 pass]...
* fixed 3 violations, starting loop 2 *
=> Fixing file: 0/1 violations remaining [made 2 passes]...
```
</details>
…s` + `SpaceAfterEquals` do not flag new lines for spacing 0 A `T_WHITESPACE` token only containing new line characters will have its token `'length'` set to `0`. This means that if there was a new line before/after the equal sign and the `$equalsSpacing` property was set to the default value of `0`, the sniff would not report it. When the `$equalsSpacing` property was set to `1`, the sniff already flagged this correctly (though there are other problems with that in the fixer - see later commit). Fixed now. Includes tests.
…s` + `SpaceAfterEquals` vs comments
When there is a comment between the parameter variable and the equal sign or the equal sign and the default value, the fixer would previously unintentionally and incorrectly remove most of these comments.
For the tests added in this commit, the fixer would previously result in the following - note that three out of the four comments have been removed..
```php
function newlineBeforeAndAfterEqualsSignFixedRespectsComments(
$param=true
) {}
function commentBeforeAndAfterEqualsSignFixedRespectsComments(
$param=/*comment*/ true
) {}
```
In this commit, I'm changing the sniff to still allow for flagging the spacing issue when there are comments, but to no longer auto-fix these to prevent the sniff removing the comments.
Includes tests.
…e `SpaceBeforeEquals` + `SpaceAfterEquals` fixers
The fixers for the `SpaceBeforeEquals` and `SpaceAfterEquals` error codes would only handle one whitespace token at a time, which is inefficient.
While this worked correctly when `$equalsSpacing` was set to `0`, as covered by the `newlineBeforeAndAfterEqualsSignShouldBeFixedForSpacing0()` test, it would take four loops instead of two to make the fixes.
However, when `$equalsSpacing` was set to `1` AND there were new lines to deal with, the fixer would get into a fixer conflict with itself as it would keep trying to add a space before the equal sign.
```
Squiz.Functions.FunctionDeclarationArgumentSpacing:227 replaced token 31 (T_WHITESPACE on line 13) " =" => " ="
```
This commit changes both fixers to handle all whitespace tokens in one go.
This makes the fixers more efficient, but also fixes the fixer conflict which was identified.
Tested via the pre-existing `newlineBeforeAndAfterEqualsSignShouldBeFixedForSpacing0()` test case, as well as the new `newlineBeforeAndAfterEqualsSignShouldBeFixedForSpacing1()` test case.
…oreComma` message
When there would be a new line between the end of the previous parameter and the comma, the error message was not always descriptive enough.
For example, for the test added in this commit, the messages would be:
```
182 | ERROR | [x] Expected 0 spaces between argument "$paramA" and comma; 0 found
| | (Squiz.Functions.FunctionDeclarationArgumentSpacing.SpaceBeforeComma)
185 | ERROR | [x] Expected 0 spaces between argument "$paramB" and comma; 4 found
| | (Squiz.Functions.FunctionDeclarationArgumentSpacing.SpaceBeforeComma)
```
In particular, take not of the "Expected 0 spaces... 0 found".
This commit improves the messaging to take new lines into account.
Includes tests.
…fixer The fixer for the `SpaceBeforeComma` error code would only handle one whitespace token at a time, which is inefficient and could also lead to more complex fixer conflicts. This commit changes the fixer to handle all whitespace tokens in one go. For the test code sample added in the previous commit, this means the difference between the fixer needing 4 loops versus 2 loops. Additionally, this commit updates the fixer to cleanly handle moving the comma in the case of a trailing comment between the end of the previous parameter and the comma. Tested via the `newlineBeforeCommaShouldBeFixedInOneGo()` test case and the new `newlineBeforeCommaFixerRespectsComments()` test case.
... to allow for adding additional test case files.
…foreHint` error ... which was so far not covered by tests.
No need to recreate the type hint when it's already available in the return value of the `getMethodParameters()` method.
fredden
suggested changes
Jan 17, 2025
Comment on lines
341
to
345
| if ($tokens[$i]['code'] === T_COMMENT | ||
| && substr($tokens[$i]['content'], -1) === $phpcsFile->eolChar | ||
| ) { | ||
| $phpcsFile->fixer->replaceToken($i, substr($tokens[$i]['content'], 0, -1)); | ||
| } |
There was a problem hiding this comment.
This doesn't look safe. A comment which starts with // or # ends with a newline. Removing its newline effectively makes the following line a comment too. I've made a code suggestion for the test case for this change which demonstrates this bug. All the current test cases have the stray comma being the last thing on a line, which makes the newline after the comma end the comment. If there's more content on the line after the comma, it gets swallowed by the comment with this implementation.
There was a problem hiding this comment.
@fredden Good catch! Thanks for finding that. I've just added a new commit (to be squashed into the original) which should fix this.
src/Standards/Squiz/Tests/Functions/FunctionDeclarationArgumentSpacingUnitTest.1.inc
Outdated
Show resolved
Hide resolved
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Squiz/FunctionDeclarationArgumentSpacing: improve SpacingBetween fixer
The fixer for the
SpacingBetweenerror code would only handle one whitespace token at a time, which is inefficient and could also lead to more complex fixer conflicts.This commit changes the fixer to handle all whitespace between the parentheses in one go.
For the code sample added as a test, this means the difference between the fixer needing 7 loops (including two skipped loops due to possible conflicts) versus 2 loops.
Old:
New:
Squiz/FunctionDeclarationArgumentSpacing: bug fix /
SpacingAfterReferencedoes not flag new linesA
T_WHITESPACEtoken only containing new line characters will have its token'length'set to0.This means that if there was a new line after a reference token, the sniff would not report it.
Fixed now.
Also note that, as the sniff demands no whitespace after a reference operator, we don't need the extra check for
$gap !== 0as we already know that the next token is a whitespace token and needs to be flagged.Includes tests.
Squiz/FunctionDeclarationArgumentSpacing: improve
SpacingAfterReferencefixerThe fixer for the
SpacingAfterReferenceerror code would only handle one whitespace token at a time, which is inefficient and could also lead to more complex fixer conflicts.This commit changes the fixer to handle all whitespace tokens after a reference token in one go.
For the test code sample added in the previous commit, this means the difference between the fixer needing 5 loops (including one skipped loop due to possible conflicts) versus 2 loops.
Old:
New:
Squiz/FunctionDeclarationArgumentSpacing: bug fix /
SpacingAfterVariadicdoes not flag new linesA
T_WHITESPACEtoken only containing new line characters will have its token'length'set to0.This means that if there was a new line after an ellipsis token, the sniff would not report it.
Fixed now.
Also note that, as the sniff demands no whitespace after a reference operator, we don't need the extra check for
$gap !== 0as we already know that the next token is a whitespace token and needs to be flagged.Includes test.
Squiz/FunctionDeclarationArgumentSpacing: improve
SpacingAfterVariadicfixerThe fixer for the
SpacingAfterVariadicerror code would only handle one whitespace token at a time, which is inefficient and could also lead to more complex fixer conflicts.This commit changes the fixer to handle all whitespace tokens after a variadic token in one go.
For the test code sample added in the previous commit, this means the difference between the fixer needing 5 loops (including one skipped loop due to possible conflicts) versus 2 loops.
Old:
New:
Squiz/FunctionDeclarationArgumentSpacing: bug fix /
SpaceBeforeEquals+SpaceAfterEqualsdo not flag new lines for spacing 0A
T_WHITESPACEtoken only containing new line characters will have its token'length'set to0.This means that if there was a new line before/after the equal sign and the
$equalsSpacingproperty was set to the default value of0, the sniff would not report it.When the
$equalsSpacingproperty was set to1, the sniff already flagged this correctly (though there are other problems with that in the fixer - see later commit).Fixed now.
Includes tests.
Squiz/FunctionDeclarationArgumentSpacing: bug fix /
SpaceBeforeEquals+SpaceAfterEqualsvs commentsWhen there is a comment between the parameter variable and the equal sign or the equal sign and the default value, the fixer would previously unintentionally and incorrectly remove most of these comments.
For the tests added in this commit, the fixer would previously result in the following - note that three out of the four comments have been removed..
In this commit, I'm changing the sniff to still allow for flagging the spacing issue when there are comments, but to no longer auto-fix these to prevent the sniff removing the comments.
Includes tests.
Squiz/FunctionDeclarationArgumentSpacing: fix fixer conflict / improve
SpaceBeforeEquals+SpaceAfterEqualsfixersThe fixers for the
SpaceBeforeEqualsandSpaceAfterEqualserror codes would only handle one whitespace token at a time, which is inefficient.While this worked correctly when
$equalsSpacingwas set to0, as covered by thenewlineBeforeAndAfterEqualsSignShouldBeFixedForSpacing0()test, it would take four loops instead of two to make the fixes.However, when
$equalsSpacingwas set to1AND there were new lines to deal with, the fixer would get into a fixer conflict with itself as it would keep trying to add a space before the equal sign.This commit changes both fixers to handle all whitespace tokens in one go.
This makes the fixers more efficient, but also fixes the fixer conflict which was identified.
Tested via the pre-existing
newlineBeforeAndAfterEqualsSignShouldBeFixedForSpacing0()test case, as well as the newnewlineBeforeAndAfterEqualsSignShouldBeFixedForSpacing1()test case.Squiz/FunctionDeclarationArgumentSpacing: bug fix / unclear
SpaceBeforeCommamessageWhen there would be a new line between the end of the previous parameter and the comma, the error message was not always descriptive enough.
For example, for the test added in this commit, the messages would be:
In particular, take note of the "Expected 0 spaces... 0 found".
This commit improves the messaging to take new lines into account.
Includes tests.
Squiz/FunctionDeclarationArgumentSpacing: improve SpaceBeforeComma fixer
The fixer for the
SpaceBeforeCommaerror code would only handle one whitespace token at a time, which is inefficient and could also lead to more complex fixer conflicts.This commit changes the fixer to handle all whitespace tokens in one go.
For the test code sample added in the previous commit, this means the difference between the fixer needing 4 loops versus 2 loops.
Additionally, this commit updates the fixer to cleanly handle moving the comma in the case of a trailing comment between the end of the previous parameter and the comma.
Tested via the
newlineBeforeCommaShouldBeFixedInOneGo()test case and the newnewlineBeforeCommaFixerRespectsComments()test case.Squiz/FunctionDeclarationArgumentSpacing: rename test case file
... to allow for adding additional test case files.
Squiz/FunctionDeclarationArgumentSpacing: add parse error/live coding tests
Squiz/FunctionDeclarationArgumentSpacing: add test for the
NoSpaceBeforeHinterror... which was so far not covered by tests.
Squiz/FunctionDeclarationArgumentSpacing: minor simplification
No need to recreate the type hint when it's already available in the return value of the
getMethodParameters()method.Suggested changelog entry
Changed:
SpaceBeforeCommaerror codeFixed:
equalsSpacingwas set to0.equalsSpacingwas set to1.Related issues/external references
Follow up on "future scope" as identified in #620 (review)
Types of changes