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 3 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
127 changes: 127 additions & 0 deletions WordPress/Sniffs/Functions/ReturnTypeSniff.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
<?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 void Return before end if return type declaration contains an invalid token.
*/
public function process_token( $stackPtr ) {
$colon = $this->phpcsFile->findPrevious( T_COLON, $stackPtr - 1, null, false, null, true );

if ( false === $colon ) {
// Shouldn't happen, but just in case.
return;
}

// Space before colon disallowed.
if ( isset( $this->tokens[ $colon - 1 ] ) && T_CLOSE_PARENTHESIS !== $this->tokens[ $colon - 1 ]['code'] ) {
$error = '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 ( isset( $this->tokens[ $token ] ) && T_CLOSE_PARENTHESIS !== $this->tokens[ $token ]['code'] );
$this->phpcsFile->fixer->endChangeset();
}
}

// Only one space after colon.
if ( T_WHITESPACE !== $this->tokens[ $colon + 1 ]['code'] ) {
$error = '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 between the colon and the return type. Found: %s';
$data = array( 'more than one' ); /* @var @todo Count actual whitespaces */
Copy link
Member Author

Choose a reason for hiding this comment

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

WIP :-)

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

$nullable = $this->phpcsFile->findNext( T_NULLABLE, $colon + 1, $stackPtr );
// Check if there is space after nullable operator.
if ( false !== $nullable && T_WHITESPACE === $this->tokens[ $nullable + 1 ]['code'] ) {
$error = 'Space is not allowed after nullable operator.';
$fix = $this->phpcsFile->addFixableError( $error, $nullable + 1, 'SpaceAfterNullable' );
if ( true === $fix ) {
$this->phpcsFile->fixer->replaceToken( $nullable + 1, '' );
}
}

$contentLC = strtolower( $this->tokens[ $stackPtr ]['content'] );
// Check if simple return type is in lowercase.
if ( isset( $this->simple_return_types[ $contentLC ] ) && $contentLC !== $this->tokens[ $stackPtr ]['content'] ) {
$error = 'Simple return type must be lowercase. Found "%s", expected "%s"';
$data = array(
$this->tokens[ $stackPtr ]['content'],
$contentLC,
);
$fix = $this->phpcsFile->addFixableError( $error, $stackPtr, 'LowerCaseSimpleType', $data );
if ( true === $fix ) {
$this->phpcsFile->fixer->replaceToken( $stackPtr, $contentLC );
}
}
}
}
83 changes: 83 additions & 0 deletions WordPress/Tests/Functions/ReturnTypeUnitTest.inc
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
<?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;
}
79 changes: 79 additions & 0 deletions WordPress/Tests/Functions/ReturnTypeUnitTest.inc.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
<?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;
}
77 changes: 77 additions & 0 deletions WordPress/Tests/Functions/ReturnTypeUnitTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
<?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,
);
}

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

}

}