Squiz/ScopeKeywordSpacing: check spaces after abstract and final keyword

[klausi]

The spacing after the final keyword should be checked by ScopeKeywordSpacingSniff.

Description

This should throw an error because there are 6 spaces after final:

class FinalTest {
    final      public static function create(ContainerInterface $container) {}
}

Suggested changelog entry

Fixed bug XYZ : Squiz.WhiteSpace.ScopeKeywordSpacing detect white space problems after final keyword.

Related issues/external references

Fir reported at Drupal's coder: https://www.drupal.org/project/coder/issues/3468410

Types of changes

Bug fix

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 @klausi, thanks for this PR!

While in principle I agree with this change, the change you've made does more than "it says on the box".
Previously, the sniff checked the spacing after visibility keywords, static and readonly.
Now, it will also check final and abstract.

So this PR is not just an addition of final, but also of abstract to the tokens which are being checked.

I think, originally, the sniff was only intended for method and property prefixes.
And even though abstract and final could be used on methods, they were not included in the sniff as they would also affect class declarations.

Checking of scope keywords for anonymous functions snuck in with the introduction of static closures in PHP 5.4 and then for arrow functions in PHP 7.4
Checking of scope keywords for class constant sort of snuck in with the introduction of constant visibility in PHP 7.1 (and final constants in PHP 8.1 if this change is accepted).
And the introduction of the readonly keyword for properties being handled in PHP 8.1, had a side-effect that as of PHP 8.2, the readonly keyword would also be checked for class declarations.

So, to come back to the proposed change in this PR. While it would now formally widen the scope of the sniff from "methods and properties" to "methods, properties, class constants and class declaration", I don't think that's a bad thing, so I have no objections against this PR.

I do, however, think that this means more tests are needed.

• Tests with final and abstract on class declarations.
• Tests with final on class constant declarations.

And if adding extra tests anyway, there should probably also be tests with the visibility keywords and class constants. And tests for static arrow functions also look to be missing.

Note: these last two test additions would not be a blocker for this PR, but would still need to be done anyhow, either in this PR or in a follow-up PR.

[fredden]

Given these keywords can be used in more than one context, perhaps we should be producing more than one error code (ie, one per context, possibly also one per keyword?). We currently only throw Incorrect.

To avoid a significant breaking change, we should endeavour to maintain the existing code for the intended/existing behaviour, and add a new code for new contexts (ie, class declaration). So, perhaps keep Incorrect for method / property / class constant contexts, and a new error code for class declaration context.

[jrfnl]

@klausi Just checking in. It would be great to include this change in the 3.11.0 release if it is ready in time. Will you have a chance to finish off the work on this ?

[klausi]

Hi, currently swamped with other work, so it could take some time until I can check back. Feel free to take over or postpone it for a future release!

[jrfnl]

Hi, currently swamped with other work, so it could take some time until I can check back. Feel free to take over or postpone it for a future release!

Hi @klausi Thanks for letting me know. I understand, just wanted to prevent this PR from being forgotten as it is definitely a good candidate for merge. Pretty swamped myself atm, so take your time and we'll touch base again in a while ;-)

[jrfnl]

@klausi Just wanted to check if you'll have time to finish this off soonish ?

[klausi]

Hey there, still busy with other work so don't have time to follow-up here. Sorry!

[klausi]

Sorry for the delay, now finally updated with more test cases.

@jrfnl : I did the minimum you asked for, keeping this change in scope for the final keyword only. I think further test cases should be done in a separate issue/PR.

@fredden I don't think this will be a controversial breaking change. The class declaration context was already checked with the readonly keyword, so we are merely extending that for abstract and final here. But I see your point, ideally we should emit different error codes for class declaration context, class property context, class constant context ...

Currently we only emit one error code, so nobody has configured that to be silent. That is good, so we could introduce new error codes without them being breaking changes.

I need to check if that is easy to do.

[klausi]

Done, now added better error codes.

I think we can't do test coverage for different error codes, but I tested it manually with all the automated test cases and checked that the correct error code is reported.

[jrfnl]

@klausi Thank you for making those updates.

I did the minimum you asked for, keeping this change in scope for the final keyword only. I think further test cases should be done in a separate issue/PR.

Not exactly. My point was that the Tokens::$methodPrefixes token group also includes the abstract keyword, which means the spacing after abstract keywords will now also be checked. However, there are still no tests documenting and safeguarding the new support for handling the abstract keyword for all supported contexts.

Could you please have another look ?

@fredden I don't think this will be a controversial breaking change. The class declaration context was already checked with the readonly keyword, so we are merely extending that for abstract and final here. But I see your point, ideally we should emit different error codes for class declaration context, class property context, class constant context ...

Currently we only emit one error code, so nobody has configured that to be silent. That is good, so we could introduce new error codes without them being breaking changes.

I appreciate the effort you've put into the error code toggling, but this is a breaking change and will block the PR until the 4.0 release.

While the sniff only has one error code, end-users will often not know whether a sniff has only one or multiple error codes, they will deal with what's in front of them, so the error code may well be excluded via a ruleset. These type of excludes would now break.
Additionally, the error code could be in use in inline ignore annotation (probably rare, but still) and again, those would now be broken.

Looking at the code for the error code toggling, it would also need some more work as:

• A number of type keywords are missing.
• The defensive coding for $targetContext potentially being false (during live coding/IDE use of PHPCS) is not defensive enough (false => 0 => T_OPEN_TAG => goes unnecessarily into the switch).

All in all, I'd like to suggest moving the change in the error codes to a separate PR to be actioned for the 4.0 release.

[klausi]

Sure thing - reverted the error code addition and added test case for abstract class and abstract method.

[jrfnl]

@klausi Thanks for your work on this. I've earmarked the PR for the 3.12.0 release (as it will result in new errors being thrown where they previously weren't).

Hope you don't mind, but I've also taken the liberty to add a few more tweaks to the tests. While the "should throw an error" case was tested, the "this is correct and shouldn't be flagged" case was not being tested for the abstract and the final keyword. That's sorted now.

[klausi]

OK, thanks a lot!