Merge pull request #137 from PHPCSStandards/universal/new-constructor…

…destructorreturn-sniff

✨ New `Universal.CodeAnalysis.ConstructorDestructorReturn` sniff

Universal/Docs/CodeAnalysis/ConstructorDestructorReturnStandard.xml

@@ -0,0 +1,64 @@
+<?xml version="1.0"?>
+<documentation xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+    xsi:noNamespaceSchemaLocation="https://phpcsstandards.github.io/PHPCSDevTools/phpcsdocs.xsd"
+    title="Constructor Destructor Return"
+    >
+    <standard>
+    <![CDATA[
+    A class constructor/destructor can not have a return type declarations. This would result in a fatal error.
+    ]]>
+    </standard>
+    <code_comparison>
+        <code title="Valid: no return type declaration.">
+        <![CDATA[
+class Foo {
+    public function __construct() {}
+}
+        ]]>
+        </code>
+        <code title="Invalid: return type declaration.">
+        <![CDATA[
+class Foo {
+    public function __construct()<em>: int</em> {}
+}
+        ]]>
+        </code>
+    </code_comparison>
+
+    <standard>
+    <![CDATA[
+    A class constructor/destructor should not return anything.
+    ]]>
+    </standard>
+    <code_comparison>
+        <code title="Valid: class constructor/destructor doesn't return anything.">
+        <![CDATA[
+class Foo {
+    public function __construct() {
+        // Do something.
+    }
+
+    public function __destruct() {
+        // Do something.
+        return;
+    }
+}
+        ]]>
+        </code>
+        <code title="Invalid: class constructor/destructor returns a value.">
+        <![CDATA[
+class Foo {
+    public function __construct() {
+        // Do something.
+        return <em>$this</em>;
+    }
+
+    public function __destruct() {
+        // Do something.
+        return <em>false</em>;
+    }
+}
+        ]]>
+        </code>
+    </code_comparison>
+</documentation>

Universal/Sniffs/CodeAnalysis/ConstructorDestructorReturnSniff.php

@@ -0,0 +1,145 @@
+<?php
+/**
+ * PHPCSExtra, a collection of sniffs and standards for use with PHP_CodeSniffer.
+ *
+ * @package   PHPCSExtra
+ * @copyright 2020 PHPCSExtra Contributors
+ * @license   https://opensource.org/licenses/LGPL-3.0 LGPL3
+ * @link      https://github.com/PHPCSStandards/PHPCSExtra
+ */
+
+namespace PHPCSExtra\Universal\Sniffs\CodeAnalysis;
+
+use PHP_CodeSniffer\Files\File;
+use PHP_CodeSniffer\Sniffs\Sniff;
+use PHP_CodeSniffer\Util\Tokens;
+use PHPCSUtils\BackCompat\BCFile;
+use PHPCSUtils\Utils\FunctionDeclarations;
+use PHPCSUtils\Utils\GetTokensAsString;
+use PHPCSUtils\Utils\NamingConventions;
+use PHPCSUtils\Utils\ObjectDeclarations;
+use PHPCSUtils\Utils\Scopes;
+
+/**
+ * Verify that a class constructor/destructor does not return anything, nor has a
+ * return type declaration (fatal error).
+ *
+ * @since 1.0.0
+ */
+final class ConstructorDestructorReturnSniff implements Sniff
+{
+
+    /**
+     * Registers the tokens that this sniff wants to listen for.
+     *
+     * @since 1.0.0
+     *
+     * @return int[]
+     */
+    public function register()
+    {
+        return [\T_FUNCTION];
+    }
+
+    /**
+     * Processes this test, when one of its tokens is encountered.
+     *
+     * @since 1.0.0
+     *
+     * @param \PHP_CodeSniffer\Files\File $phpcsFile The file being scanned.
+     * @param int                         $stackPtr  The position of the current token
+     *                                               in the stack passed in $tokens.
+     *
+     * @return void
+     */
+    public function process(File $phpcsFile, $stackPtr)
+    {
+        $scopePtr = Scopes::validDirectScope($phpcsFile, $stackPtr, Tokens::$ooScopeTokens);
+        if ($scopePtr === false) {
+            // Not an OO method.
+            return;
+        }
+
+        $functionName   = FunctionDeclarations::getName($phpcsFile, $stackPtr);
+        $functionNameLC = \strtolower($functionName);
+
+        if ($functionNameLC === '__construct' || $functionNameLC === '__destruct') {
+            $functionType = \sprintf('A "%s()" magic method', $functionNameLC);
+        } else {
+            // This may be a PHP 4-style constructor.
+            $OOName = ObjectDeclarations::getName($phpcsFile, $scopePtr);
+
+            if (empty($OOName) === true) {
+                // Anonymous class or parse error. The function can't be a PHP 4-style constructor.
+                return;
+            }
+
+            if (NamingConventions::isEqual($functionName, $OOName) === false) {
+                // Class and function name not the same, so not a PHP 4-style constructor.
+                return;
+            }
+
+            $functionType = 'A PHP 4-style constructor';
+        }
+
+        /*
+         * OK, so now we know for sure that this is a constructor/destructor method.
+         */
+
+         // Check for a return type.
+        $properties = FunctionDeclarations::getProperties($phpcsFile, $stackPtr);
+        if ($properties['return_type'] !== '' && $properties['return_type_token'] !== false) {
+            $data = [
+                $functionType,
+                $properties['return_type'],
+            ];
+
+            $phpcsFile->addError(
+                '%s can not declare a return type. Found: %s',
+                $properties['return_type_token'],
+                'ReturnTypeFound',
+                $data
+            );
+        }
+
+        $tokens = $phpcsFile->getTokens();
+        if (isset($tokens[$stackPtr]['scope_opener'], $tokens[$stackPtr]['scope_closer']) === false) {
+            // Abstract/interface method, live coding or parse error.
+            return;
+        }
+
+        // Check for a value being returned.
+        $current = $tokens[$stackPtr]['scope_opener'];
+        $end     = $tokens[$stackPtr]['scope_closer'];
+
+        do {
+            $current = $phpcsFile->findNext(\T_RETURN, ($current + 1), $end);
+            if ($current === false) {
+                break;
+            }
+
+            $next = $phpcsFile->findNext(Tokens::$emptyTokens, ($current + 1), $end, true);
+            if ($next === false
+                || $tokens[$next]['code'] === \T_SEMICOLON
+                || $tokens[$next]['code'] === \T_CLOSE_TAG
+            ) {
+                // Return statement without value.
+                continue;
+            }
+
+            $endOfStatement = BCFile::findEndOfStatement($phpcsFile, $next);
+
+            $data = [
+                $functionType,
+                GetTokensAsString::compact($phpcsFile, $current, $endOfStatement, true),
+            ];
+
+            $phpcsFile->addWarning(
+                '%s can not return a value. Found: "%s"',
+                $current,
+                'ReturnValueFound',
+                $data
+            );
+        } while ($current < $end);
+    }
+}

Universal/Tests/CodeAnalysis/ConstructorDestructorReturnUnitTest.inc

@@ -0,0 +1,125 @@
+<?php
+
+/*
+ * OK.
+ */
+
+// Global function, not a constructor/destructor.
+function __construct() : int {
+    return 123;
+}
+
+function __destruct() : float {
+    return 321;
+}
+
+// Methods which are not constructors can be disregarded.
+class NotAConstructor {
+    public function __set($name, $value): void {
+        // Do something.
+    }
+
+    public function Foo(): void {
+        // Do something.
+    }
+
+    public function Bar() {
+        return $this;
+    }
+}
+
+// Constructor/Destructor without a return statement or return type.
+class NoReturn {
+    protected function __construct()
+    {
+        // Do something.
+    }
+
+    protected function __destruct()
+    {
+        // Do something.
+    }
+
+    // Also applies to PHP4-style constructors/destructors.
+    function NoReturn()
+    {
+        // Do something.
+    }
+
+    function _NoReturn()
+    {
+        // Do something.
+    }
+}
+
+// Constructor/Destructor with return statement, but no value.
+$anon = new class extends ReturnNoValue {
+    public function __construct() {
+        if ($foo) {
+            return   ;
+        } else {
+            return /* comments are fine */
+            // Even when spread over multiple lines.
+            ;
+        }
+
+        return;
+    }
+
+    public function __destruct() {
+        // Do something.
+        return;
+    }
+
+    // Non-constructor/destructor method returning.
+    public function returnsavalue() {
+        return 'php4style';
+    }
+};
+
+
+/*
+ * Not OK.
+ */
+class ReturnsAValue {
+    public function __construct(): self {
+        return $this;
+    }
+
+    public function __destruct():string {
+        return 'destructed';
+    }
+
+    function returnsavalue()
+    {
+        return 'php4style';
+    }
+}
+
+$anon = new class() extends ReturnsAValue {
+    public function __Construct()
+    :    static|self
+    {
+        return $this;
+    }
+
+    public function __deStRucT() {
+        return 'destructed';
+    }
+};
+
+/*
+ * Return types are not allowed on constructor/destructor methods (fatal error).
+ */
+
+trait AbstractConstructorDestructorReturnTypes {
+    abstract public function __construct() : int;
+
+    abstract public function __destruct():string;
+}
+
+interface InterfaceMethodReturnTypes {
+    public function __construct(): void;
+
+    public function __destruct(): void;
+}

Universal/Tests/CodeAnalysis/ConstructorDestructorReturnUnitTest.php

@@ -0,0 +1,58 @@
+<?php
+/**
+ * PHPCSExtra, a collection of sniffs and standards for use with PHP_CodeSniffer.
+ *
+ * @package   PHPCSExtra
+ * @copyright 2020 PHPCSExtra Contributors
+ * @license   https://opensource.org/licenses/LGPL-3.0 LGPL3
+ * @link      https://github.com/PHPCSStandards/PHPCSExtra
+ */
+
+namespace PHPCSExtra\Universal\Tests\CodeAnalysis;
+
+use PHP_CodeSniffer\Tests\Standards\AbstractSniffUnitTest;
+
+/**
+ * Unit test class for the ConstructorDestructorReturn sniff.
+ *
+ * @covers PHPCSExtra\Universal\Sniffs\CodeAnalysis\ConstructorDestructorReturnSniff
+ *
+ * @since 1.0.0
+ */
+final class ConstructorDestructorReturnUnitTest extends AbstractSniffUnitTest
+{
+
+    /**
+     * Returns the lines where errors should occur.
+     *
+     * @return array <int line number> => <int number of errors>
+     */
+    public function getErrorList()
+    {
+        return [
+            85  => 1,
+            89  => 1,
+            101 => 1,
+            116 => 1,
+            118 => 1,
+            122 => 1,
+            124 => 1,
+        ];
+    }
+
+    /**
+     * Returns the lines where warnings should occur.
+     *
+     * @return array <int line number> => <int number of warnings>
+     */
+    public function getWarningList()
+    {
+        return [
+            86  => 1,
+            90  => 1,
+            95  => 1,
+            103 => 1,
+            107 => 1,
+        ];
+    }
+}