Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ReturnType Sniff #1084

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 141 additions & 0 deletions WordPress/Sniffs/Functions/ReturnTypeSniff.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
<?php
/**
* WordPress Coding Standard.
*
* @package WPCS\WordPressCodingStandards
* @link https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards
* @license https://opensource.org/licenses/MIT MIT
*/

namespace WordPress\Sniffs\Functions;

use PHP_CodeSniffer_Tokens as Tokens;
use WordPress\Sniff;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. the class which is being extended
  2. any other class being used.


/**
* Enforces formatting of return types.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*
* @package WPCS\WordPressCodingStandards
*
* @since 0.14.0
*
* @link https://github.com/zendframework/zend-coding-standard/blob/52b435c714879609262aa36b004c4075cd967acc/src/ZendCodingStandard/Sniffs/Formatting/ReturnTypeSniff.php
*/
class ReturnTypeSniff extends Sniff {
/**
* Simple return types.
*
* @since 0.14.0
*
* @var string[]
*/
private $simple_return_types = array(
'array' => true,
'bool' => true,
'callable' => true,
'double' => true,
Copy link
Member

@jrfnl jrfnl Aug 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

'float' => true,
'int' => true,
'iterable' => true,
'parent' => true,
'self' => true,
'string' => true,
'void' => true,
);

/**
* Returns an array of tokens this test wants to listen for.
*
* @return array
*/
public function register() {
return array(
T_RETURN_TYPE,
);
}

/**
* Processes this test, when one of its tokens is encountered.
*
* @param int $stackPtr The position of the current token in the stack.
* @return null Return before end if Return type declaration contains an invalid token.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Return type => return type

@return null => @return void

*/
public function process_token( $stackPtr ) {
$colon = (int) $this->phpcsFile->findPrevious( T_COLON, $stackPtr - 1 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Don't cast the return to (int), token index 0 exists.
  2. You probably want to limit the search to the statement: $this->phpcsFile->findPrevious( T_COLON, $stackPtr - 1, null, false, null, true );
  3. Always check that the return is not false before using it. Shouldn't ever happen in this case, but it's better to be safe than sorry. I've come across too many tokenizer bugs not to.
if ( false === $colon ) {
	// Shouldn't happen, but just in case.
	return;
}

Copy link
Member Author

@GaryJones GaryJones Aug 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasons for the casts (here, and one or two other places in this PR), is that it is then treated as an int, but the fact it could be a bool returned, makes my IDE sad.

screenshot 2017-08-06 16 32 43


// Space before colon disallowed.
if ( T_CLOSE_PARENTHESIS !== $this->tokens[ $colon - 1 ]['code'] ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if ( isset( $this->tokens[ $colon - 1 ] ) && ...

$error = 'There must be no space before colon before a return type.';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: "There must be no space between the closing parenthesis and the colon when declaring a return type for a function."

$fix = $this->phpcsFile->addFixableError( $error, $colon - 1, 'SpaceBeforeColon' );

if ( true === $fix ) {
$this->phpcsFile->fixer->beginChangeset();
$token = $colon - 1;
do {
$this->phpcsFile->fixer->replaceToken( $token, '' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

-- $token;
} while ( T_CLOSE_PARENTHESIS !== $this->tokens[ $token ]['code'] );
Copy link
Member

@jrfnl jrfnl Aug 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably superfluous, but still: while ( $token > 0 && ... or while ( isset( $this->tokens[ $token ] ) && ....

$this->phpcsFile->fixer->endChangeset();
}
}

// Only one space after colon.
if ( T_WHITESPACE !== $this->tokens[ $colon + 1 ]['code'] ) {
$error = 'There must be a space after colon and before return type declaration.';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: "There must be one space between the colon and the return type. None found."

$fix = $this->phpcsFile->addFixableError( $error, $colon, 'NoSpaceAfterColon' );
if ( true === $fix ) {
$this->phpcsFile->fixer->addContent( $colon, ' ' );
}
} elseif ( ' ' !== $this->tokens[ $colon + 1 ]['content'] ) {
$error = 'There must be exactly one space after colon and before return type declaration.';
Copy link
Member

@jrfnl jrfnl Aug 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggestion: "There must be exactly one space between the colon and the return type. Found: %s" - where %s should be replaced with either "newline" or "x spaces", similar to how it's done here: https://github.com/squizlabs/PHP_CodeSniffer/blob/master/src/Standards/PSR2/Sniffs/ControlStructures/ControlStructureSpacingSniff.php#L81-L87

$fix = $this->phpcsFile->addFixableError( $error, $colon + 1, 'TooManySpacesAfterColon' );
if ( true === $fix ) {
$this->phpcsFile->fixer->replaceToken( $colon + 1, ' ' );
}
}

$nullable = (int) $this->phpcsFile->findNext( T_NULLABLE, $colon + 1, $stackPtr );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if ( $nullable ) {
// Check if there is space after nullable operator.
if ( T_WHITESPACE === $this->tokens[ $nullable + 1 ]['code'] ) {
$error = 'Space is not not allowed after nullable operator.';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not not

$fix = $this->phpcsFile->addFixableError( $error, $nullable + 1, 'SpaceAfterNullable' );
if ( $fix ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if ( true === $fix )

$this->phpcsFile->fixer->replaceToken( $nullable + 1, '' );
}
}
}

$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 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

$invalid = (int) $this->phpcsFile->findNext( array( T_STRING, T_NS_SEPARATOR, T_RETURN_TYPE ), $first, $last + 1, true );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if ( $invalid ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

@GaryJones GaryJones Aug 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possible test cases:

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

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$error = 'Return type declaration contains invalid token %s';
$data = array( $this->tokens[ $invalid ]['type'] );
$fix = $this->phpcsFile->addFixableError( $error, $invalid, 'InvalidToken', $data );
if ( true === $fix ) {
$this->phpcsFile->fixer->replaceToken( $invalid, '' );
}

return;
}

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...
}

if ( $first === $last
&& ! in_array( $return_type, $this->simple_return_types, true )
&& ! isset( $this->simple_return_types[ $return_type ] )
) {
$error = 'Simple return type must be lowercase. Found "%s", expected "%s"';
$data = array(
$return_type,
strtolower( $return_type ),
);
$fix = $this->phpcsFile->addFixableError( $error, $first, 'LowerCaseSimpleType', $data );
if ( true === $fix ) {
$this->phpcsFile->fixer->replaceToken( $stackPtr, strtolower( $return_type ) );
}
}
}
}
84 changes: 84 additions & 0 deletions WordPress/Tests/Functions/ReturnTypeUnitTest.inc
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
<?php

function fooa(): array {} // Good.
function foob() : array {} // Bad.
function fooc() : array {} // Bad.
function food() // Bad.
: array {}
function fooe():array {} // Bad.
function foof(): array {} // Bad.
function foog():
array {}

// Don't touch case statements here.
$v = 1;
switch ($v) {
case 1:
$x = $f1(-1 * $v);
break;
case (2) :
$x = $f2(-1 * $v, $v);
break;
default:
$x = $f1($v) + $f2(-1 * $v, $v);
break;
}

class ReturnType
{
public function method() :array // Bad.
{
return [];
}

private function priv(
$a,
$b
)
: int { // Bad.
return $a * $b;
}
}

$c = new class() {
public function method() :
float {
return mt_rand();
}
};

abstract class AbsCla
{
abstract public function x() :bool; // Bad.
}

interface MyInterface
{ // All below are bad.
public function a():vOid;
public function b() : Int;
public function c() :?int;
public function d() :Float;
public function e() :? float;
public function f():Double;
public function g():?double;
public function h():Array;
public function i() : ?array;
public function j():String;
public function k():?string;
public function l():Parent;
public function m():?parent;
public function n():Callable;
public function o():?callable;
public function p():Bool;
public function q():?bool;
public function r():Self;
public function s():?self;
public function t():Iterable;
public function u():?iterable;

public function v($a) : \DateTime;
public function w():?\DateTime;

public function y($a, $b, $c) : \My\TestClass;
public function z():? \ReturnType \MyType;
}
80 changes: 80 additions & 0 deletions WordPress/Tests/Functions/ReturnTypeUnitTest.inc.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
<?php

function fooa(): array {} // Good.
function foob(): array {} // Bad.
function fooc(): array {} // Bad.
function food(): array {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

function fooe(): array {} // Bad.
function foof(): array {} // Bad.
function foog(): array {}

// Don't touch case statements here.
$v = 1;
switch ($v) {
case 1:
$x = $f1(-1 * $v);
break;
case (2) :
$x = $f2(-1 * $v, $v);
break;
default:
$x = $f1($v) + $f2(-1 * $v, $v);
break;
}

class ReturnType
{
public function method(): array // Bad.
{
return [];
}

private function priv(
$a,
$b
): int { // Bad.
return $a * $b;
}
}

$c = new class() {
public function method(): float {
return mt_rand();
}
};

abstract class AbsCla
{
abstract public function x(): bool; // Bad.
}

interface MyInterface
{ // All below are bad.
public function a(): void;
public function b(): int;
public function c(): ?int;
public function d(): float;
public function e(): ?float;
public function f(): double;
public function g(): ?double;
public function h(): array;
public function i(): ?array;
public function j(): string;
public function k(): ?string;
public function l(): parent;
public function m(): ?parent;
public function n(): callable;
public function o(): ?callable;
public function p(): bool;
public function q(): ?bool;
public function r(): self;
public function s(): ?self;
public function t(): iterable;
public function u(): ?iterable;

public function v($a): \DateTime;
public function w(): ?\DateTime;

public function y($a, $b, $c): \My\TestClass;
public function z(): ?\ReturnType\MyType;
}
78 changes: 78 additions & 0 deletions WordPress/Tests/Functions/ReturnTypeUnitTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
<?php
/**
* Unit test class for WordPress Coding Standard.
*
* @package WPCS\WordPressCodingStandards
* @link https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards
* @license https://opensource.org/licenses/MIT MIT
*/

namespace WordPress\Tests\Functions;

use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest;

/**
* Unit test class for the ReturnType sniff.
*
* @package WPCS\WordPressCodingStandards
*
* @since 0.14.0
*/
class ReturnTypeUnitTest extends AbstractSniffUnitTest {

/**
* Returns the lines where errors should occur.
*
* @return array <int line number> => <int number of errors>
*/
public function getErrorList() {
return array(
4 => 1,
5 => 1,
6 => 1,
8 => 1,
9 => 1,
10 => 1,
29 => 2,
38 => 1,
44 => 2,
52 => 2,
57 => 2,
58 => 2,
59 => 2,
60 => 3,
61 => 3,
62 => 2,
63 => 1,
64 => 2,
65 => 1,
66 => 2,
67 => 1,
68 => 2,
69 => 1,
70 => 2,
71 => 1,
72 => 2,
73 => 1,
74 => 2,
75 => 1,
76 => 2,
77 => 1,
79 => 2,
80 => 1,
82 => 1,
83 => 3,
);
}

/**
* Returns the lines where warnings should occur.
*
* @return array <int line number> => <int number of warnings>
*/
public function getWarningList() {
return array();

}

}