-
-
Notifications
You must be signed in to change notification settings - Fork 492
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
Add ReturnType Sniff #1084
Changes from 2 commits
09100f7
7bcea29
177fd7c
333c97f
007e2d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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; | ||
|
||
/** | ||
* Enforces formatting of return types. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Test: https://3v4l.org/bqF4j Interestingly enough Also: as of PHP 7.2, |
||
'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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
*/ | ||
public function process_token( $stackPtr ) { | ||
$colon = (int) $this->phpcsFile->findPrevious( T_COLON, $stackPtr - 1 ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
if ( false === $colon ) {
// Shouldn't happen, but just in case.
return;
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
// Space before colon disallowed. | ||
if ( T_CLOSE_PARENTHESIS !== $this->tokens[ $colon - 1 ]['code'] ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
$error = 'There must be no space before colon before a return type.'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, '' ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 This check should be done before the error is thrown so as not to throw an unfixable Unit test: function abc() /* Return declaration. */ : array {
return array();
} There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'] ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably superfluous, but still: |
||
$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.'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't cast to |
||
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.'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
$fix = $this->phpcsFile->addFixableError( $error, $nullable + 1, 'SpaceAfterNullable' ); | ||
if ( $fix ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
$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 ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check |
||
$invalid = (int) $this->phpcsFile->findNext( array( T_STRING, T_NS_SEPARATOR, T_RETURN_TYPE ), $first, $last + 1, true ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? (
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ) ); | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This whole check can be simplified as we already received the $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 ) ); | ||
} | ||
} | ||
} | ||
} |
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; | ||
} |
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 {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} |
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(); | ||
|
||
} | ||
|
||
} |
There was a problem hiding this comment.
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: