PSR12/OperatorSpacing: add perCompatible property to not check union type operator spacing in catch() statements #1356
Conversation
…union type operator spacing in catch() statements
jrfnl
left a comment
jrfnl
left a comment
There was a problem hiding this comment.
@klausi Thanks for setting up this PR.
First off, the change to treat multi-catch similar to union types for operator spacing was only made in PER 3.0, not 2.0. Please use the correct PER version.
Looking the PR over, one fundamental problem comes to mind: Operator spacing for multi-catch is now no longer checked at all for PER >= 3.0.
I don't think that's correct or desirable.
Now, to fix this, there are a number of options, all of which would impact the implementation of what you are doing in this PR.
- Flag and fix spacing around the
|operator for multi-catch in this sniff, but flag and fix to 0 spaces for PER >= 3.0.
This would not break existing usage as this change in handling of multi-catch would be opt-in via theperCompatibleproperty. - Create a separate sniff to handle spacing around the
|operator for multi-catch.
Such a separate sniff could either live in an external standard - in which case there will likely be a flurry of slightly different versions of such a sniff in multiple external standards.
Or if the sniff would go into PHPCS itself, that sniff would also need to have theperCompatibleproperty, so it would make more sense for this sniff to always bow out and defer to the separare sniff for multi-catch, but that would break existing behaviour.
There could even be more/other options. Happy to hear them.
Either way, I think we need to have a discussion about this first, before I comment on the code of the PR itself.
jrfnl
changed the title
|
Oh and just noticed the LLM prompt section - please read the CONTRIBUTING GUIDE carefully. LLM generated PRs are NOT welcome at all in any form: https://github.com/PHPCSStandards/PHP_CodeSniffer?tab=contributing-ov-file#do-not-submit-ai-generated-prs (also explains why my draft note of code changes to request is already way larger than it ever should be) |
|
Sorry, totally missed the AI section in the contributing guidelines. I'll only submit pull requests I fully created myself in the future. Thanks for pointing out the 2.0 vs. 3.0 version - I'll will update this PR accordingly (the summary description and the code). As far as I understand it your proposed option 1 is exactly what I want to do here, the test case is demonstrating that. I will add one with setting the version to 4.0 as well to demonstrate code is flagged and fixed for higher versions as well. |
|
Updated the code and the description above, ready for review! |
jrfnl
left a comment
jrfnl
left a comment
There was a problem hiding this comment.
@klausi Thanks for making those changes and sorry for making such a mess of the code view with all my remarks. Having worked with you before, I suspect this is largely due to the original setup having been created by AI... 🤷♀️
I hope the code review remarks help and are clear.
Also note that this PR needs a sister-PR in the documentation repo to document the new sniff property in the Customisable Sniff Properties wiki page.
| </code_comparison> | ||
| <standard> | ||
| <![CDATA[ | ||
| When the "perCompatible" property is set to "3.0" or higher, the requirement for spaces around the pipe operator in a catch block (union types) is removed. |
There was a problem hiding this comment.
| When the "perCompatible" property is set to "3.0" or higher, the requirement for spaces around the pipe operator in a catch block (union types) is removed. | |
| When the "perCompatible" property is set to "3.0" or higher, the requirement for spaces around the single pipe operator in a multi-catch block is changed to a requirement of "no spaces". |
(or something along those lines... - I added the "single" to link this back to the standard description in the first block)
| ]]> | ||
| </standard> | ||
| <code_comparison> | ||
| <code title="Valid: perCompatible set to 3.0 or higher."> |
There was a problem hiding this comment.
| <code title="Valid: perCompatible set to 3.0 or higher."> | |
| <code title="Valid: no spaces around the '|' operator in a multi-catch with 'perCompatible=3.0' (or higher)."> |
| } | ||
| ]]> | ||
| </code> | ||
| <code title="Valid: perCompatible set to a version lower than 3.0 (default 1.0)."> |
There was a problem hiding this comment.
| <code title="Valid: perCompatible set to a version lower than 3.0 (default 1.0)."> | |
| <code title="Invalid: spaces around the '|' operator in a multi-catch with 'perCompatible=3.0' (or higher)."> |
| try { | ||
| // nothing | ||
| } catch (Exception | RuntimeException $e) { | ||
| } | ||
|
|
||
| // Testing that it works with future versions as well. | ||
| // phpcs:set PSR12.Operators.OperatorSpacing perCompatible 4.0 | ||
|
|
||
| try { | ||
| // nothing | ||
| } catch (Exception | RuntimeException $e) { | ||
| } |
There was a problem hiding this comment.
I'm missing tests:
- An "okay" test with
perCompatible 3.0(or higher), i.e. no spaces around the operator, fixer does not kick in. - More than 1 space on either side.
- New line(s) before/after operator. Especially, multiple new lines and/or new line + indentation whitespace are the interesting cases as the fixer as-is would not handle this in a single loop, while it should.
Also, the parent Squiz sniff has an ignoreNewlines property, which this sniff inherits and which IMO should be respected, even when people set the perCompatible property.
This is currently not tested, nor handled correctly by the sniff code.
Scratch that last remark, looks like the PSR12 sniff only extends the Squiz sniff to allow for using the isOperator() method. Looks like it doesn't respect the Squiz ignoreNewlines or ignoreSpacingBeforeAssignments properties at all.
This finding also means the documentation in the wiki should be updated, which I've just done: PHPCSStandards/PHP_CodeSniffer-documentation#85
| try { | ||
| // nothing | ||
| } catch (Exception|RuntimeException $e) { | ||
| } |
There was a problem hiding this comment.
Note: line 47 in the OperatorSpacingUnitTest.1.inc file already tests the "no spaces" case for multi-catch, so this is a 100% duplicate test.
What's missing are tests specifically for multi-catch with:
- 1 space (= okay, fixer does not kick in).
- More than 1 space on either side of the operator (should be auto-fixed and is currently not tested for
catchstatements specifically). - New line(s) before/after operator. Especially, multiple new lines and/or new line + indentation whitespace are the interesting cases.
And yes, the pre-existing fixer code does not handle that correctly in one loop either. This will need to be fixed, but is outside of the scope of this PR, so it's fine if you opt to not add the new line tests in this PR.
| // was originally treated as a bitwise operator. This check changes the spacing requirement | ||
| // for that specific case when opting in to PER-CS 3.0 or higher. | ||
| if ($operator === '|' && version_compare($this->perCompatible, '3.0', '>=') === true) { | ||
| if (isset($tokens[$stackPtr]['nested_parenthesis']) === true) { |
There was a problem hiding this comment.
This condition can be added to the if above it.
|
|
||
| $operator = $tokens[$stackPtr]['content']; | ||
|
|
||
| // PER-CS 3.0: Exception to the rule for catch blocks (union types) where no space is required. |
There was a problem hiding this comment.
| // PER-CS 3.0: Exception to the rule for catch blocks (union types) where no space is required. | |
| // PER-CS 3.0: Exception to the rule for pipe operators in multi-catch blocks where no space is required. |
| && $tokens[$tokens[$bracket]['parenthesis_owner']]['code'] === T_CATCH | ||
| ) { | ||
| if ($tokens[($stackPtr - 1)]['code'] === T_WHITESPACE) { | ||
| $error = 'Expected 0 spaces before "%s"; %s found'; |
There was a problem hiding this comment.
| $error = 'Expected 0 spaces before "%s"; %s found'; | |
| $error = 'Expected 0 spaces before "%s" in multi-catch statement; %s found'; |
Not sure if this is better or not, but feels like it will prevent questions from end-users (who just follow a ruleset and don't maintain it) about why the sniff normally enforces at least 1 space and now suddenly enforces no spaces.
Might also be an idea to make a similar change to the error code and make it explicit that this applies to multi-catch pipe operators only.
I'm also thinking that making the message + code more specific will prevent it from blocking future changes if PHP thinks of some new syntax again and PER-CS makes yet another exception.
If you agree, please also apply these changes to the other error message/code.
| $error = 'Expected 0 spaces before "%s"; %s found'; | ||
| $data = [ | ||
| $operator, | ||
| $tokens[($stackPtr - 1)]['length'], |
There was a problem hiding this comment.
$tokens[($stackPtr - 1)]['length'] will lead to confusing error messages if the whitespace token contents is a new line character...
(here and in the second message too)
|
|
||
| $fix = $phpcsFile->addFixableError($error, $stackPtr, 'SpaceBefore', $data); | ||
| if ($fix === true) { | ||
| $phpcsFile->fixer->replaceToken(($stackPtr - 1), ''); |
There was a problem hiding this comment.
See my remark about the fixer vs new lines in the test comments.
2 participants
PSR12.Operators.OperatorSpacing: add support for PER 2.0 catch block spacing
Description
This PR introduces a new public property
$perCompatibleto thePSR12.Operators.OperatorSpacingsniff.This property allows users to opt-in to PER Coding Style 3.0 behavior regarding union types in
catchblocks.According to PER-CS 3.0, the pipe operator
|incatchstatements should not be surrounded by spaces and should be treated as part of the type definition rather than a bitwise operator.When
$perCompatibleis set to3.0(or higher), the sniff will enforce no spaces around the pipe operator withincatchblocks. The default value remains1.0, ensuring full backwards compatibility with PSR-12 behavior.Suggested changelog entry
PSR12.Operators.OperatorSpacing: new
perCompatibleproperty to support PER-CS 3.0.Related issues/external references
Fixes #660
Types of changes
PR checklist