Improve context detection for PSR12.Operators.OperatorSpacing / Squiz.WhiteSpace.OperatorSpacing - avoids fixer conflict, bow out on parse error

[fredden]

Description

I have investigated one of the issues identified in #152

• Standard: PSR12
• Test file: src/Standards/Generic/Tests/PHP/RequireStrictTypesUnitTest.13.inc
• Sniffs involved in the conflict:
  • PSR12.Operators.OperatorSpacing
  • PSR12.Files.DeclareStatement

4 | ERROR | [x] Expected at least 1 space before "="; 0 found (PSR12.Operators.OperatorSpacing.NoSpaceBefore)
4 | ERROR | [x] Expected no space between directive and the equals sign in a declare statement (PSR12.Files.DeclareStatement.SpaceFoundAfterDirective)

Verbose output from phpcbf run

	=> Fixing file: 1/1 violations remaining
---START FILE CONTENT---
1|<?php
2|
3|// Safeguard sniff handles parse error/live coding correctly.
4|declare(strict_types=
5|
--- END FILE CONTENT ---
	PSR12.Operators.OperatorSpacing:96 replaced token 6 (T_EQUAL on line 4) "=" => "�[30;1m·�[0m="


	=> Fixing file: 1/1 violations remaining [made 1 pass]...
	* fixed 1 violations, starting loop 2 *
---START FILE CONTENT---
1|<?php
2|
3|// Safeguard sniff handles parse error/live coding correctly.
4|declare(strict_types =
5|
--- END FILE CONTENT ---
	PSR12.Files.DeclareStatement:102 replaced token 6 (T_WHITESPACE on line 4) "�[30;1m·�[0m=" => "="

---

In order to resolve / fix this conflict, I have taken the following steps:

• Rename existing test cases for both sniffs to allow multiple test files.
• Add new test with a parse error to each sniff (code taken from trigger test file listed above) -- see also Tests: parse error tests for sniffs should be in their own file #143
• Change the code in both sniffs to bow out when a parse error has been detected.

Suggested changelog entry

Updated PSR12.Operators.OperatorSpacing and PSR12.Files.DeclareStatement to bow out when a parse error is detected.

Related issues/external references

• QA: standards should not have conflicting rules / fix fixer conflicts #152

Types of changes

• Bug fix (non-breaking change which fixes an issue)

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
• I have verified that the code complies with the projects coding standards.

[jrfnl]

Hi @fredden Thanks for this PR!

I agree with your analysis of the fixer conflict and the solution direction.

I do have some remarks and questions though about the implemented fixes:

1. For both "rename test file" commits: the switch statement is missing the default case returning an empty array.
2. Again both sniffs: if no fixes are expected, there should not be a .fixed file.
3. For the OperatorSpacing sniff - which extends the Squiz OperatorSpacing sniff:
   • Is there a particular reason why you put the fix in the PSR12 sniff ? (instead of in the Squiz sniff)
     I can imagine the same issue would also be troublesome for the Squiz sniff.
   • Is there a particular reason why you do the live coding check after the isOperator() check ?
   • Why are you checking for live coding using isset($tokens[($stackPtr + 2)]) instead of doing a findNext(T_WHITESPACE, ($stackPtr + 1), null, true) ?
     If there would be multiple new lines after the unfinished = your check would pass and the fixer conflict would still come into play.
4. For the DeclareStatement sniff:
   • Aside from the same question about the isset(), I see you've put the "bow out" in the section of the sniff strictly dealing with the assignment operator.
     Have you considered that the sniff should maybe not run at all if the declare statement is unfinished, i.e. does not have a close parenthesis ?

Just FYI: I have a WIP branch locally to do a complete rewrite of the PSR12 DeclareStatement sniff as there is a lot more wrong with that sniff. Hoping to at some point find the time to finish the rewrite. I won't let it block this PR though.

[fredden]

1. For both "rename test file" commits: the switch statement is missing the default case returning an empty array.

switch statements do not require a default case. There is a return statement immediately following the switch. To appease this remark, I have moved this into the switch.

2. Again both sniffs: if no fixes are expected, there should not be a .fixed file.

These files were very useful during debugging / development. I have removed them as per your preference.

3. For the OperatorSpacing sniff - which extends the Squiz OperatorSpacing sniff:
   • Is there a particular reason why you put the fix in the PSR12 sniff ? (instead of in the Squiz sniff)
     I can imagine the same issue would also be troublesome for the Squiz sniff.

I was focusing on the PSR12 standard. I have now moved the fix to the Squiz standard and added a test there to catch the same problem.

• Is there a particular reason why you do the live coding check after the isOperator() check ?

I didn't want to modify anything outside of the scope I was looking at: PSR12. Given the above, the check is now in the isOperator() check.

• Why are you checking for live coding using isset($tokens[($stackPtr + 2)]) instead of doing a findNext(T_WHITESPACE, ($stackPtr + 1), null, true) ?
  If there would be multiple new lines after the unfinished = your check would pass and the fixer conflict would still come into play.

I don't have a good justification for this. I've applied the suggested change.

4. For the DeclareStatement sniff:
   • Aside from the same question about the isset(), I see you've put the "bow out" in the section of the sniff strictly dealing with the assignment operator.
     Have you considered that the sniff should maybe not run at all if the declare statement is unfinished, i.e. does not have a close parenthesis ?

This sounds like a good improvement that could be made to the sniff. I don't think it's in scope here, but I've make a code change to cover this to address this feedback.

[fredden]

Questions for @jrfnl:

• Do you want me to rename src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.inc to src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.1.inc (and ...inc.fixed) in this pull request?
  • See 25883e8. I'm happy to open a separate pull request for this, or cherry-pick that commit into this branch / pull request.
• Would you like me to rebase this to reduce the number of back/forth commits, or would you prefer to squash-merge this pull request when ready?

[jrfnl]

@fredden

switch statements do not require a default case. There is a return statement immediately following the switch.

🤦🏻‍♀️ I'm sorry, I completely missed those return statements after the switch when reviewing. My bad.

These files were very useful during debugging / development. I have removed them as per your preference.

Not so much my preference, as it is a convention in this repo.

Having said that, I didn't realize until your remark that if a .fixed file is missing and fixes are being made for a test case file, that the test suite would not flag this.

I've had a look at this and prepared a fix, but I think this will need to wait until PHPCS 4.0 as there are quite a few external standards which use the PHPCS native test framework and changing the framework to flag a missing fixed file as a test failure will likely break builds for a number of those.

Aside from that, if I run the PHPCS native test suite with that extra failure, it flags up 5 missing .fixed files in PHPCS itself and those will need to be sorted too before the extra failure check could be turned on.

I've opened issue #299 and #300 to address these.

I was focusing on the PSR12 standard. I have now moved the fix to the Squiz standard and added a test there to catch the same problem.

I didn't want to modify anything outside of the scope I was looking at: PSR12. Given the above, the check is now in the isOperator() check.

Thanks for moving the check. Looking at it as it currently is - I still have a question:

• Why did you put the check at the end of the isOperator() method ?
  I would expect it at the very beginning before any of the other checks as determining whether the operator is an operator which should be handled, is irrelevant if the sniff detects a live coding situation.

I don't have a good justification for this. I've applied the suggested change.

👍🏻

Might be good to safeguard this by adding one or two extra new lines at the end of the file in the "live coding" test case files ?

This sounds like a good improvement that could be made to the sniff. I don't think it's in scope here, but I've make a code change to cover this to address this feedback.

Not all questions need to be addressed, sometimes they are just questions. 😉

I've looked at the fix you applied now and am going to ask you to remove that commit. As I said before, there is a lot wrong with that sniff and the extra code change you've applied now do not mitigate this. Not your fault, it's just a really bad sniff.

Case in point: the $stackPtr + 1 may not exist, the findNext() on line 49 could return false which is not being checked etc

I will address this (and a dozen or more other bugs) in that complete rewrite PR.

Do you want me to rename src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.inc to src/Standards/Squiz/Tests/WhiteSpace/OperatorSpacingUnitTest.1.inc (and ...inc.fixed) in this pull request?

• See 25883e8. I'm happy to open a separate pull request for this, or cherry-pick that commit into this branch / pull request.

Yes please, that should be in a commit prior to the fix commit.

Would you like me to rebase this to reduce the number of back/forth commits, or would you prefer to squash-merge this pull request when ready?

I would not squash merge this PR as it contains file renames and those don't always get tracked correctly when squashed in with other changes (and it is much more helpful when looking at the file history if they do get tracked correctly).

I would prefer the actual "fix" commits to be squashed to one commit per fix.

I'm happy to do this myself when the PR is ready for commit though, so you don't need to worry about this if you don't want to.

If you do end up rebasing and reorganizing the commits yourself, I have one extra suggestion: prefix commits with the (short) name of the sniff they relate to, like "PSR12/DeclareStatement: rename test file", that makes the commit history much more easy to scan.

Does that answer your questions ?

[fredden]

• Why did you put the check at the end of the isOperator() method ?
  I would expect it at the very beginning before any of the other checks as determining whether the operator is an operator which should be handled, is irrelevant if the sniff detects a live coding situation.

The other checks in the function are less expensive. Calling a function is more expensive than using a built-in like isset() or hash look-ups and equality operators (===). I can move this to the start of the function if you would prefer. (Putting it at the start makes sense from a logical point of view, so I don't have objections if you would like this at the start instead.)

Might be good to safeguard this by adding one or two extra new lines at the end of the file in the "live coding" test case files ?

There are a lot of variations which could be covered by tests. These are likely to identify more problems with the sniff(s) though. For example, <?php declare(; produces a number of errors which don't make sense (like "Expected no space between opening parenthesis and directive in a declare statement"). I'd prefer to have these in another pull request - perhaps the "rewrite sniff" work that you've alluded to.

Do you want me to rename [...]
Yes please, that should be in a commit prior to the fix commit.

I've added this commit to the pull request. I've put it at the end so that reviewing is kept easy. (GitHub has a nice feature to show what has changed since "my last review," which breaks when history is rewritten.) We can change the order of commits when we're ready to re-base.

[jrfnl]

@fredden

• Why did you put the check at the end of the isOperator() method ?
  I would expect it at the very beginning before any of the other checks as determining whether the operator is an operator which should be handled, is irrelevant if the sniff detects a live coding situation.

The other checks in the function are less expensive. Calling a function is more expensive than using a built-in like isset() or hash look-ups and equality operators (===).

Agreed, but the other checks don't just do hash look-ups, they also include array_*() function calls (more expensive) and a call to File::isReference() (also more expensive as it is a) a function call too and b) contains more logic).

I can move this to the start of the function if you would prefer. (Putting it at the start makes sense from a logical point of view, so I don't have objections if you would like this at the start instead.)

Yes please, I think it makes sense to move it to the start.

Might be good to safeguard this by adding one or two extra new lines at the end of the file in the "live coding" test case files ?

There are a lot of variations which could be covered by tests. These are likely to identify more problems with the sniff(s) though. For example, <?php declare(; produces a number of errors which don't make sense (like "Expected no space between opening parenthesis and directive in a declare statement"). I'd prefer to have these in another pull request - perhaps the "rewrite sniff" work that you've alluded to.

That's totally fair. I will keep it in mind for the refactor PR. (and yes, I need to find time to finish that at some point, the reason I didn't finish it before is largely cause so much needs to change that it becomes very difficult to do this in clean atomic commits and to decide what commits are needed and in which order)

Do you want me to rename [...]
Yes please, that should be in a commit prior to the fix commit.

I've added this commit to the pull request. I've put it at the end so that reviewing is kept easy. (GitHub has a nice feature to show what has changed since "my last review," which breaks when history is rewritten.) We can change the order of commits when we're ready to re-base.

👍🏻

[fredden]

Thanks for all the feedback. I think we're at a position where the final delta in this pull request is acceptable. If that's the case, shall I now rewrite the history so that the commits are cleaner?

[jrfnl]

Thanks for making those updates and reorganizing this branch @fredden, all looks good now.

I'll fix up that alignment issue and will merge it now.

[jrfnl]

Nitpick: subsequent line alignment is off-by-one.

[fredden]

When I pasted this in, my editor prefixed the lines with * , so it ended up with * * [...] called again on the [...]. I removed the *, but it seems I missed the extra space. This was not identified when I ran phpcs over the codebase (which I do before committing). Perhaps there's a bug / oversight in the rule-set?

[jrfnl]

I seem to remember working on a fix for this years ago and that there was some discussion on what the fix should look like... Pff.. I probably still have that branch somewhere, though it may have gotten lost on an old laptop (I have a feeling I worked on this while travelling).

Let's park it for when the docs sniffs get revisited in their entirety.

[jrfnl]

@michalbundyra A search showed me the WebImpress standard extends the Squiz/OperatorSpacing sniff. You may want to have a look at this PR and update the sniff if needed.

[michalbundyra]

Thanks, @jrfnl, will check it out