Add ReturnType Sniff #1084

GaryJones · 2017-08-04T11:44:25Z

Only slightly adapted from the work Michał Bundyra (@webimpress) did for the Zend Coding Standard.

Required format for WordPress is the same as most others in the PHP community:

no space before the colon
exactly one space after colon before return type
no space after nullable operator (hasn't been discussed yet, but I see no reason to vary)
simple types should be given as lowercase

End result:

function foo(): bool { // Simple return type.
};

function foo(): ?string { // Nullable simple return type.
};

function foo( $a ): \MyClass { // Return type of a class.
};

❗️ Note: Not yet added to WordPress-Core/ruleset.xml.

See #547.

GaryJones · 2017-08-04T11:56:21Z

My own notes:

To get the line numbers that have errors in tests:

# In WPCS directory.
$ phpcs --standard=WordPress -s WordPress/Tests/Functions/ReturnTypeUnitTest.inc --sniffs=WordPress.Functions.ReturnType

To run tests:

# In WPCS directory.
$ ../phpcs/vendor/bin/phpunit --bootstrap="./Test/phpcs3-bootstrap.php" --filter WordPress ../phpcs/tests/AllTests.php

To check code standards for this new code:

# In WPCS directory.
$ phpcs WordPress/Sniffs/Functions --standard=bin/phpcs.xml
$ phpcs WordPress/Tests/Functions --standard=bin/phpcs.xml

jrfnl · 2017-08-04T12:02:38Z

There is an open PR for return type declarations upstream: squizlabs/PHP_CodeSniffer#1219
Would that implementation work for us ? and if so, why not use that as basis to make it easier to use the upstream version eventually ?

GaryJones · 2017-08-04T12:08:56Z

The one I based mine on is referenced in an issue labelled with 3.2.0, whereas the one you've highlighted isn't :-)

My base also handles nullable return types as well.

I'm happy to defer to upstream if/as/when something gets there, but in the meantime, we could be using this today.

GaryJones · 2017-08-04T12:21:24Z

Rebased to hopefully fix the test failures.

Further test cases to add and apply fixes for:

function fooh(): // a comment
array {}
function fooi() // a comment
: array {}

should be fixed to become:

function fooh(): array {} // a comment
function fooi(): array {} // a comment

Only slightly adapted from the [work](https://github.com/zendframework/zend-coding-standard/blob/52b435c714879609262aa36b004c4075cd967acc/src/ZendCodingStandard/Sniffs/Formatting/ReturnTypeSniff.php) Michał Bundyra (@webimpress) did for the Zend Coding Standard. Required format for WordPress is the same as most others in the PHP community: - no space before the colon - exactly one space after colon before return type - no space after nullable operator (hasn't been discussed yet, but I see no reason to vary) - simple types should be given as lowercase End result: ```php function foo(): bool { // Simple return type. }; function foo(): ?string { // Nullable simple return type. }; function foo( $a ): \MyClass { // Return type of a class. }; ``` ❗️ Note: Not yet added to `WordPress-Core/ruleset.xml`. See #547.

JDGrimes · 2017-08-04T13:06:01Z

WordPress/Sniffs/Functions/ReturnTypeSniff.php

+		$end     = $this->phpcsFile->findNext( array( T_SEMICOLON, T_OPEN_CURLY_BRACKET ), $stackPtr + 1 );
+		$last    = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $end - 1, null, true );
+		$invalid = (int) $this->phpcsFile->findNext( array( T_STRING, T_NS_SEPARATOR, T_RETURN_TYPE ), $first, $last + 1, true );
+		if ( $invalid ) {


Is there a particular reason to check for invalid tokens here? Doesn't that just indicate a parse error/live coding? If so, we shouldn't be giving an error here, just bowing out if we can't continue.

Maybe, but if we can determine that it appears to be not invalid, then we can also then check and fix any of the simple return types not being in lowercase as well.

If it's an invalid token, then it appears to try and remove it (but may leave the colon - I don't see any unit tests covering this).

Possible test cases:

function a(): invalidtoken {} function b(): invalid_token {} function c() :invalid_token {}

But what should they be fixed to (if anything)?

I don't think that we should attempt to fix them. We don't attempt to fix or even warn about syntax errors elsewhere. See #793

Also, those examples are valid tokens, aren't they? (T_STRING is valid, might be a class name.) It would have to be something like this:

function a(): $foo {}

I'm in agreement with @JDGrimes here. I'd actually say that this whole check for invalid tokens should be removed, as in line 109 -122.

JDGrimes · 2017-08-04T13:08:21Z

WordPress/Sniffs/Functions/ReturnTypeSniff.php

+
+		if ( $first === $last
+			&& ! in_array( $returnType, $this->simple_return_types, true )
+			&& in_array( strtolower( $returnType ), $this->simple_return_types, true )


This array could be flipped and isset() used instead. Although I guess we're trying to stay close to the upstream version.

There is no upstream as such, so flipped array in 7bcea29.

jrfnl

Ok, I've had a closer look at this sniff.

There are a couple of pertinent questions I'd like to pose:

Licensing

We've had discussions about licensing issues with copying code from other standards before. You've indicated that the sniff is largely based on work from @webimpress for the Zend Framework standard which is copyrighted and licensed under BSD-3. I'm not a legal copyright expert, so is this usage permitted or would it be better/preferred to use Composer to add the ZF standard to a project and just include that one sniff in a custom ruleset ?
That way you could also already use the sniff today without threading on other people's work.

Code style

I did a compare between this version and the original and - aside from code style -, there are hardly any differences. Should in that case the original code style be preserved to make compare & updating based on the original easier and the sniff be excluded from our own code style check ? The same as we previously did with the ArrayDeclaration sniff. In that case, we also did not unit test the sniff in WPCS as that was the responsibility of the original library and was done extensively there.
See #703

Basic principle of the rules/sniff:

What about function declarations with quite some variables/defaults making for a very long line ? By the looks of the unit tests, the return type declaration is now fixed to be on the same line as the closing parenthesis. Should new lines be allowed ? Possibly as a configurable property, like in several of the Squiz sniffs ignoreNewlines ?
Ref: https://github.com/squizlabs/PHP_CodeSniffer/wiki/Customisable-Sniff-Properties#squizstringsconcatenationspacing and several others.
I see no mention of "exactly one space between the end of the return type and the brace". While this is already checked via another sniff (Generic.Functions.OpeningFunctionBraceKernighanRitchie), it should possibly be mentioned in the WP Core coding standards handbook.
For that matter, the "brace should be on the same line as the class/function declaration" rule is implied in code samples used in the handbook in the "space usage" section, but it isn't actually made explicit either. /cc @pento

Code:

See inline comments. Gone through it with a fine toothcomb. Hope you'll find it useful. /cc @webimpress

jrfnl · 2017-08-06T00:40:01Z

WordPress/Sniffs/Functions/ReturnTypeSniff.php

+namespace WordPress\Sniffs\Functions;
+
+use PHP_CodeSniffer_Tokens as Tokens;
+use WordPress\Sniff;


Nitpicking: the order of the use statements everywhere else is:

the class which is being extended

any other class being used.

jrfnl · 2017-08-06T00:40:49Z

WordPress/Sniffs/Functions/ReturnTypeSniff.php

+use WordPress\Sniff;
+
+/**
+ * Enforces formatting of return types.


Maybe make the "formatting" expected a little more specific ?

jrfnl · 2017-08-06T01:28:44Z

WordPress/Sniffs/Functions/ReturnTypeSniff.php

+		'array'    => true,
+		'bool'     => true,
+		'callable' => true,
+		'double'   => true,


double is not a valid return type. (Same goes for real and integer for that matter.)
See: http://php.net/manual/en/functions.arguments.php#functions.arguments.type-declaration.types - especially the note about aliases not being accepted.

Test: https://3v4l.org/bqF4j

Interestingly enough parent is not in that list, but is an accepted return type.

Also: as of PHP 7.2, object will be added to the list of valid return types. See: https://wiki.php.net/rfc/object-typehint

jrfnl · 2017-08-06T01:39:08Z

WordPress/Tests/Functions/ReturnTypeUnitTest.inc.fixed

+function fooa(): array {} // Good.
+function foob(): array {} // Bad.
+function fooc(): array {} // Bad.
+function food(): array {}


Looks like the sniff has removed the comment instead of moving it around.

No, I took the comment out of the .fixed file to make the tests pass for now :-)

jrfnl · 2017-08-06T01:56:33Z

WordPress/Sniffs/Functions/ReturnTypeSniff.php

+				$this->phpcsFile->fixer->beginChangeset();
+				$token = $colon - 1;
+				do {
+					$this->phpcsFile->fixer->replaceToken( $token, '' );


The token type for tokens to be removed should be checked here to be T_WHITESPACE, otherwise comments could be accidentally removed.

This check should be done before the error is thrown so as not to throw an unfixable fixableError. See: https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/blob/master/WordPress/Sniffs/Arrays/ArrayDeclarationSpacingSniff.php#L101-L105 for some example code dealing with a similar situation.

Unit test:

function abc() /* Return declaration. */ : array { return array(); }

I'd come across the disappearing comments but removed them from the tests until I'd come up with a solution.

jrfnl · 2017-08-06T02:20:55Z

WordPress/Sniffs/Functions/ReturnTypeSniff.php

+			if ( T_WHITESPACE === $this->tokens[ $nullable + 1 ]['code'] ) {
+				$error = 'Space is not not allowed after nullable operator.';
+				$fix   = $this->phpcsFile->addFixableError( $error, $nullable + 1, 'SpaceAfterNullable' );
+				if ( $fix ) {


if ( true === $fix )

jrfnl · 2017-08-06T02:24:46Z

WordPress/Sniffs/Functions/ReturnTypeSniff.php

+		$first   = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $nullable ?: $colon ) + 1, null, true );
+		$end     = $this->phpcsFile->findNext( array( T_SEMICOLON, T_OPEN_CURLY_BRACKET ), $stackPtr + 1 );
+		$last    = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $end - 1, null, true );
+		$invalid = (int) $this->phpcsFile->findNext( array( T_STRING, T_NS_SEPARATOR, T_RETURN_TYPE ), $first, $last + 1, true );


Don't cast to (int) and check in the if against false.

jrfnl · 2017-08-06T02:29:29Z

WordPress/Sniffs/Functions/ReturnTypeSniff.php

+
+		$first   = $this->phpcsFile->findNext( Tokens::$emptyTokens, ( $nullable ?: $colon ) + 1, null, true );
+		$end     = $this->phpcsFile->findNext( array( T_SEMICOLON, T_OPEN_CURLY_BRACKET ), $stackPtr + 1 );
+		$last    = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $end - 1, null, true );


Check $end !== false before using it. This would throw notices when code is checked in live coding situations in an IDE.

jrfnl · 2017-08-06T02:34:46Z

WordPress/Sniffs/Functions/ReturnTypeSniff.php

+		$end     = $this->phpcsFile->findNext( array( T_SEMICOLON, T_OPEN_CURLY_BRACKET ), $stackPtr + 1 );
+		$last    = $this->phpcsFile->findPrevious( Tokens::$emptyTokens, $end - 1, null, true );
+		$invalid = (int) $this->phpcsFile->findNext( array( T_STRING, T_NS_SEPARATOR, T_RETURN_TYPE ), $first, $last + 1, true );
+		if ( $invalid ) {


I'm in agreement with @JDGrimes here. I'd actually say that this whole check for invalid tokens should be removed, as in line 109 -122.

jrfnl · 2017-08-06T02:39:10Z

WordPress/Sniffs/Functions/ReturnTypeSniff.php

+		}
+
+		$return_type = trim( $this->phpcsFile->getTokensAsString( $first, $last - $first + 1 ) );
+


This whole check can be simplified as we already received the T_RETURN_TYPE as the $stackPtr and simple return types are only one token:

$contentLC = strtolower( $this->tokens[ $stackPtr ]['content'] ); if ( isset( $this->simple_return_types[ $contentLC ] && $contentLC !== $this->tokens[ $stackPtr ]['content'] ) { // Throw fixable error. // ... reusing the $contentLC variable... }

GaryJones · 2017-08-06T16:06:46Z

WordPress/Sniffs/Functions/ReturnTypeSniff.php

-			$error = 'There must be exactly one space after colon and before return type declaration.';
-			$fix   = $this->phpcsFile->addFixableError( $error, $colon + 1, 'TooManySpacesAfterColon' );
+			$error = 'There must be exactly one space between the colon and the return type. Found: %s';
+			$data = array( 'more than one' ); /* @var @todo Count actual whitespaces */


- Remove double type, add object type. - Add more explicit feedback for too many spaces after colon.

If comments are found between closing parenthesis and colon, or colon and return type, then mark as an (unfixable) error.

GaryJones · 2017-08-06T21:22:55Z

We've had discussions about licensing issues with copying code from other standards before. You've indicated that the sniff is largely based on work from @webimpress for the Zend Framework standard which is copyrighted and licensed under BSD-3. I'm not a legal copyright expert, so is this usage permitted

My understanding is, that as long as we keep copyright / credits, BSD-3 can be used in MIT projects.

In this case (and it may not make a difference), their work is not yet on their master branch, or within a release, so it may not even count as being published.

With the PR feedback given, it's also had some significant changes to it, with sections deleted, and more test cases addressed )i.e. more than just syntax changes).

or would it be better/preferred to use Composer to add the ZF standard to a project and just include that one sniff in a custom ruleset ?

Unless @webimpress strongly dictates otherwise, I'd be against this. As mentioned, more test cases have been added and catered for, and even the error messages have more of WPCS flavour about them.

I'm more than happy to add more explicit credits as to the original source, but there is definitely some non-trivial changes made, and I'd hope that in the spirit of open source, these changes could be added back into the Zend Coding Standard, or otherwise make it upstream so everyone could benefit.

Code style

I did a compare between this version and the original and - aside from code style -, there are hardly any differences.

As above - there are more than just code style changes now (since you wrote the comment). Theirs is an unreleased Sniff, and ours handles things in a better way.

What about function declarations with quite some variables/defaults making for a very long line ? By the looks of the unit tests, the return type declaration is now fixed to be on the same line as the closing parenthesis. Should new lines be allowed ? Possibly as a configurable property, like in several of the Squiz sniffs ignoreNewlines ?

There's at least one test case that allows for ): array { to be on its own line. Happy to add more test cases if you feel a case is missing.

I see no mention of "exactly one space between the end of the return type and the brace". While this is already checked via another sniff (Generic.Functions.OpeningFunctionBraceKernighanRitchie), it should possibly be mentioned in the WP Core coding standards handbook.
For that matter, the "brace should be on the same line as the class/function declaration" rule is implied in code samples used in the handbook in the "space usage" section, but it isn't actually made explicit either. /cc @pento

As you said, it's not in the handbook, and covered by another sniff anyway, so I'm not sure what you need this PR to clarify here?

See inline comments. Gone through it with a fine toothcomb. Hope you'll find it useful.

Very useful, thanks - this was my first attempt at amending the internals of a Sniff (albeit starting from a very good base from @webimpress), so it was a case of jumping in at the deep-end and figuring out what the workflow should be. Definitely useful to have detailed feedback!

jrfnl · 2017-08-06T21:49:55Z

In this case (and it may not make a difference), their work is not yet on their master branch, or within a release, so it may not even count as being published.

Copyright starts at the moment something is written. Not when it is published.

If I understand it correctly, the sniff is not yet merged into the ZFCS ? In that case, it might well be that their license does not apply (yet) and that it falls under the ""normal" copyright, i.e. all rights reserved unless otherwise mentioned and the sniff itself did not contain any such mentions, so that would - for the moment - make it off-limits.

I'm more than happy to add more explicit credits as to the original source, but there is definitely some non-trivial changes made, and I'd hope that in the spirit of open source, these changes could be added back into the Zend Coding Standard, or otherwise make it upstream so everyone could benefit.

I'd like to hear from @webimpress here, but yes, as - since the original PR was made - the changes have become more extensive, it can be seen as a derivative work.
And yes, I, for one, would very much cheer you on for giving back the changes made here to the original source.

As you said, it's not in the handbook, and covered by another sniff anyway, so I'm not sure what you need this PR to clarify here?

Nothing for this PR here, but I did wonder if the handbook could do with clarification on that point and this sniff inspired that thought which is why I mentioned it here.

michalbundyra · 2017-08-08T11:38:38Z

@jrfnl @GaryJones
Sorry, I've been on holidays.
I had a look on your discussion here, and based on that I've made couple changes to sniff in ZendCodingStandard. The latest version is here:
https://github.com/zendframework/zend-coding-standard/blob/feature/sniffs/src/ZendCodingStandard/Sniffs/Formatting/ReturnTypeSniff.php

The main thing I have changed there is possibility to configure the sniff - you can set how many spaces must be before/after colon and after nullable operator.

Probably it still could be improved, but all tests provided here also pass with my sniff.
As you can see ZendCodingStandard is going to be big library with multiple sniffs.
I think it would be nice if you'd like to use the library thru composer than copy it across. Then you can also use another sniffs which I have implemented there. I know that it is not yet released and probably it will take some time, so it's your decision what to do with it.

GaryJones · 2017-08-08T11:48:48Z

@webimpress Thanks for following up :-)

Would your plan be to get this sniff into PHPCS repo itself?

GaryJones · 2017-08-08T11:49:38Z

Side note for this PR: WordPress.WhiteSpace.ControlStructureSpacing.NoSpaceAfterCloseParenthesis may need adjusting, as it complains of a "space being needed between opening control structure and closing parenthesis" when the space before the colon is missing.

jrfnl · 2017-08-08T11:55:41Z

Side note for this PR: WordPress.WhiteSpace.ControlStructureSpacing.NoSpaceAfterCloseParenthesis may need adjusting, as it complains of a "space being needed between opening control structure and closing parenthesis" when the space before the colon is missing.

Related to #1071 - The _T_FUNCTION and T_CLOSURE (and T_USE for that matter) tokens have been added to that sniff in WPCS, but are not control structures. They should be handled separately completely as the control structure sniff is not suitable for those.

jrfnl · 2017-08-14T18:39:41Z

Just thinking: should there be some unit tests covering closures in combination with return type declarations ?

See WordPress/WordPress-Coding-Standards#547 and WordPress/WordPress-Coding-Standards#1084

GaryJones added Status: Review ready Type: Enhancement labels Aug 4, 2017

GaryJones added this to the 0.14.0 milestone Aug 4, 2017

GaryJones force-pushed the feature/547-spacing-before-returntype branch 2 times, most recently from b9e86b1 to df7a043 Compare August 4, 2017 11:54

GaryJones force-pushed the feature/547-spacing-before-returntype branch from df7a043 to 26b4f84 Compare August 4, 2017 12:18

GaryJones force-pushed the feature/547-spacing-before-returntype branch from 26b4f84 to 09100f7 Compare August 4, 2017 12:29

JDGrimes suggested changes Aug 4, 2017

View reviewed changes

ReturnType: Flip array and use isset()

7bcea29

jrfnl requested changes Aug 6, 2017

View reviewed changes

ReturnType: Apply first round of feedback

177fd7c

GaryJones commented Aug 6, 2017

View reviewed changes

GaryJones added 2 commits August 6, 2017 17:49

ReturnType: Apply more of first round of feedback

333c97f

- Remove double type, add object type. - Add more explicit feedback for too many spaces after colon.

ReportType: Handle unusually placed comments

007e2d2

If comments are found between closing parenthesis and colon, or colon and return type, then mark as an (unfixable) error.

jrfnl mentioned this pull request Aug 14, 2017

Function declaration verification #1101

Closed

This was referenced Sep 22, 2017

Proposal for restructuring the rulesets & some of the sniffs #1157

Closed

Releasing 0.14.0 ? #1158

Closed

jrfnl removed this from the 0.14.0 milestone Sep 23, 2017

jrfnl added this to the Future Release milestone Sep 23, 2017

GaryJones removed the Status: Review ready label Dec 13, 2017

sirbrillig added a commit to Automattic/phpcs-neutron-ruleset that referenced this pull request Jan 26, 2018

Disable ContentAfterBrace, NoSpaceAfterCloseParenthesis

c427fa5

See WordPress/WordPress-Coding-Standards#547 and WordPress/WordPress-Coding-Standards#1084

jrfnl added the Focus: Modern PHP label Mar 1, 2018

ntwb mentioned this pull request Jul 12, 2018

Spacing of : for return declaration when including an argument #1420

Closed

GaryJones closed this Jul 23, 2018

GaryJones removed this from the Future Release milestone Aug 22, 2018

jrfnl mentioned this pull request Dec 7, 2022

Core: add sniffs to check formatting for type declarations #2132

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ReturnType Sniff #1084

Add ReturnType Sniff #1084

GaryJones commented Aug 4, 2017

GaryJones commented Aug 4, 2017

jrfnl commented Aug 4, 2017

GaryJones commented Aug 4, 2017 •

edited

Loading

GaryJones commented Aug 4, 2017 •

edited

Loading

JDGrimes Aug 4, 2017

GaryJones Aug 4, 2017

GaryJones Aug 4, 2017 •

edited

Loading

JDGrimes Aug 4, 2017

jrfnl Aug 6, 2017

JDGrimes Aug 4, 2017

GaryJones Aug 4, 2017

jrfnl left a comment •

edited

Loading

jrfnl Aug 6, 2017

jrfnl Aug 6, 2017

jrfnl Aug 6, 2017 •

edited

Loading

jrfnl Aug 6, 2017

GaryJones Aug 6, 2017

jrfnl Aug 6, 2017

GaryJones Aug 6, 2017

jrfnl Aug 6, 2017

jrfnl Aug 6, 2017

jrfnl Aug 6, 2017

jrfnl Aug 6, 2017

jrfnl Aug 6, 2017

GaryJones Aug 6, 2017

GaryJones commented Aug 6, 2017

jrfnl commented Aug 6, 2017

michalbundyra commented Aug 8, 2017

GaryJones commented Aug 8, 2017

GaryJones commented Aug 8, 2017

jrfnl commented Aug 8, 2017

jrfnl commented Aug 14, 2017

		}

		$return_type = trim( $this->phpcsFile->getTokensAsString( $first, $last - $first + 1 ) );

Add ReturnType Sniff #1084

Add ReturnType Sniff #1084

Conversation

GaryJones commented Aug 4, 2017

GaryJones commented Aug 4, 2017

jrfnl commented Aug 4, 2017

GaryJones commented Aug 4, 2017 • edited Loading

GaryJones commented Aug 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GaryJones Aug 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl left a comment • edited Loading

Choose a reason for hiding this comment

Licensing

Code style

Basic principle of the rules/sniff:

Code:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrfnl Aug 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GaryJones commented Aug 6, 2017

jrfnl commented Aug 6, 2017

michalbundyra commented Aug 8, 2017

GaryJones commented Aug 8, 2017

GaryJones commented Aug 8, 2017

jrfnl commented Aug 8, 2017

jrfnl commented Aug 14, 2017

GaryJones commented Aug 4, 2017 •

edited

Loading

GaryJones commented Aug 4, 2017 •

edited

Loading

GaryJones Aug 4, 2017 •

edited

Loading

jrfnl left a comment •

edited

Loading

jrfnl Aug 6, 2017 •

edited

Loading