Adding a rule to check on docblocks

Author: moufmouf

.gitignore

@@ -4,3 +4,4 @@
 /composer.lock
 /phpunit.xml
 /tmp
+/build/

README.md

@@ -14,6 +14,14 @@ They are more "strict" than the default PHPStan rules and some may be controvers
 - You should not have empty catch statements
 - When throwing an exception inside a catch block, [you should pass the catched exception as the "previous" exception](http://bestpractices.thecodingmachine.com/php/error_handling.html#wrapping-an-exception-do-not-lose-the-previous-exception)
 
+### PHPDoc related rules
+
+This is a PHP 7.1+ rule:
+
+- You should use type-hinting when possible
+- If not possible, you should use a Docblock to specify the type
+- If type-hinting against an array, you should use a Docblock to further explain the content of the array
+
 
 ### Work-in-progress
 
@@ -48,4 +56,12 @@ services:
     class: TheCodingMachine\PHPStan\Rules\Exceptions\EmptyExceptionRule
     tags:
       - phpstan.rules.rule
+  -
+    class: TheCodingMachine\PHPStan\Rules\TypeHints\MissingTypeHintInFunctionRule
+    tags:
+      - phpstan.rules.rule
+  -
+    class: TheCodingMachine\PHPStan\Rules\TypeHints\MissingTypeHintInMethodRule
+    tags:
+      - phpstan.rules.rule
 ```

composer.json

@@ -10,7 +10,8 @@
         }
     ],
     "require": {
-        "phpstan/phpstan": "^0.7.0"
+        "phpstan/phpstan": "^0.7.0",
+        "roave/better-reflection": "^1.2.0"
     },
     "require-dev": {
         "phpunit/phpunit": "^6.2"

phpunit.xml.dist

@@ -9,6 +9,9 @@
 	beStrictAboutTodoAnnotatedTests="true"
 	failOnRisky="true"
 	failOnWarning="true"
+	convertErrorsToExceptions="true"
+	convertNoticesToExceptions="true"
+	convertWarningsToExceptions="true"
 >
 	<testsuites>
 		<testsuite name="Test suite">
@@ -27,6 +30,7 @@
 			showUncoveredFiles="true"
 			showOnlySummary="true"
 		/>
-		<log type="coverage-clover" target="tmp/clover.xml"/>
+		<log type="coverage-html" target="build/coverage" charset="UTF-8" yui="true" highlight="true"/>
+		<log type="coverage-clover" target="build/logs/clover.xml"/>
 	</logging>
 </phpunit>

src/Rules/TypeHints/AbstractMissingTypeHintRule.php

@@ -0,0 +1,229 @@
+<?php
+
+
+namespace TheCodingMachine\PHPStan\Rules\TypeHints;
+
+
+use BetterReflection\Reflection\ReflectionParameter;
+use BetterReflection\Util\FindReflectionOnLine;
+use phpDocumentor\Reflection\Type;
+use phpDocumentor\Reflection\Types\Array_;
+use phpDocumentor\Reflection\Types\Boolean;
+use phpDocumentor\Reflection\Types\Callable_;
+use phpDocumentor\Reflection\Types\Float_;
+use phpDocumentor\Reflection\Types\Integer;
+use phpDocumentor\Reflection\Types\Mixed;
+use phpDocumentor\Reflection\Types\Null_;
+use phpDocumentor\Reflection\Types\Object_;
+use phpDocumentor\Reflection\Types\Scalar;
+use phpDocumentor\Reflection\Types\String_;
+use PhpParser\Node;
+use PhpParser\Node\Stmt\Catch_;
+use PHPStan\Analyser\Scope;
+use PHPStan\Broker\Broker;
+use PHPStan\Rules\Rule;
+
+class AbstractMissingTypeHintRule implements Rule
+{
+
+    /**
+     * @var Broker
+     */
+    private $broker;
+
+    public function __construct(Broker $broker)
+    {
+
+        $this->broker = $broker;
+    }
+
+    public function getNodeType(): string
+    {
+        // FIXME: does this encompass the Method_?
+        return Node\Stmt\Function_::class;
+    }
+
+    /**
+     * @param \PhpParser\Node\Stmt\Function_ $node
+     * @param \PHPStan\Analyser\Scope $scope
+     * @return string[]
+     */
+    public function processNode(Node $node, Scope $scope): array
+    {
+        // TODO: improve performance by caching better reflection results.
+        $finder = new FindReflectionOnLine();
+
+        $reflection = $finder($scope->getFile(), $node->getLine());
+
+        $errors = [];
+
+        foreach ($reflection->getParameters() as $parameter) {
+            $result = $this->analyzeParameter($parameter);
+
+            if ($result !== null) {
+                $errors[] = $result;
+            }
+        }
+
+        return $errors;
+    }
+
+    /**
+     * Analyzes a parameter and returns the error string if xomething goes wrong or null if everything is ok.
+     *
+     * @param ReflectionParameter $parameter
+     * @return null|string
+     */
+    private function analyzeParameter(ReflectionParameter $parameter): ?string
+    {
+        $phpTypeHint = $parameter->getTypeHint();
+        $docBlockTypeHints = $parameter->getDocBlockTypes();
+
+        // If there is a type-hint, we have nothing to say unless it is an array.
+        if ($phpTypeHint !== null) {
+            return $this->analyzeParameterWithTypehint($parameter, $phpTypeHint, $docBlockTypeHints);
+        } else {
+            return $this->analyzeParameterWithoutTypehint($parameter, $docBlockTypeHints);
+        }
+    }
+
+    /**
+     * @param Type[] $docBlockTypeHints
+     */
+    private function analyzeParameterWithTypehint(ReflectionParameter $parameter, Type $phpTypeHint, array $docBlockTypeHints): ?string
+    {
+        $docblockWithoutNullable = $this->typesWithoutNullable($docBlockTypeHints);
+
+        if (!$phpTypeHint instanceof Array_) {
+            // Let's detect mismatches between docblock and PHP typehint
+            foreach ($docblockWithoutNullable as $docblockTypehint) {
+                if (get_class($docblockTypehint) !== get_class($phpTypeHint)) {
+                    return sprintf('Parameter $%s type is type-hinted to "%s" but the @param annotation says it is a "%s". Please fix the @param annotation.', $parameter->getName(), (string) $phpTypeHint, (string) $docblockTypehint);
+                }
+            }
+
+            return null;
+        }
+
+        if (empty($docblockWithoutNullable)) {
+            return sprintf('Parameter $%s type is "array". Please provide a @param annotation to further speciy the type of the array. For instance: @param int[] $%s', $parameter->getName(), $parameter->getName());
+        } else {
+            foreach ($docblockWithoutNullable as $docblockTypehint) {
+                if (!$docblockTypehint instanceof Array_) {
+                    return sprintf('Mismatching type-hints for parameter %s. PHP type hint is "array" and docblock type hint is %s.', $parameter->getName(), (string) $docblockTypehint);
+                }
+
+                if ($docblockTypehint->getValueType() instanceof Mixed) {
+                    return sprintf('Parameter $%s type is "array". Please provide a more specific @param annotation. For instance: @param int[] $%s', $parameter->getName(), $parameter->getName());
+                }
+            }
+        }
+
+        return null;
+    }
+
+    /**
+     * @param Type[] $docBlockTypeHints
+     * @return null|string
+     */
+    private function analyzeParameterWithoutTypehint(ReflectionParameter $parameter, array $docBlockTypeHints): ?string
+    {
+        if (empty($docBlockTypeHints)) {
+            return sprintf('Parameter $%s has no type-hint and no @param annotation.', $parameter->getName());
+        }
+
+        $nativeTypehint = $this->isNativelyTypehintable($docBlockTypeHints);
+
+        if ($nativeTypehint !== null) {
+            return sprintf('Parameter $%s can be type-hinted to "%s".', $parameter->getName(), $nativeTypehint);
+        }
+
+        // TODO: cancel if the method is implemented from an interface or overrides another method
+        // TODO: return types
+
+        return null;
+    }
+
+    /**
+     * @param Type[] $docBlockTypeHints
+     * @return string|null
+     */
+    private function isNativelyTypehintable(array $docBlockTypeHints): ?string
+    {
+        if (count($docBlockTypeHints) > 2) {
+            return null;
+        }
+        $isNullable = $this->isNullable($docBlockTypeHints);
+        if (count($docBlockTypeHints) === 2 && !$isNullable) {
+            return null;
+        }
+
+        $types = $this->typesWithoutNullable($docBlockTypeHints);
+        // At this point, there is at most one element here
+        if (empty($types)) {
+            return null;
+        }
+
+        $type = $types[0];
+
+        if ($this->isNativeType($type)) {
+            return ($isNullable?'?':'').((string)$type);
+        }
+
+        if ($type instanceof Array_) {
+            return ($isNullable?'?':'').'array';
+        }
+
+        // TODO: more definitions to add here
+        // Manage interface/classes
+        // Manage array of things => (cast to array)
+
+        if ($type instanceof Object_) {
+            return ($isNullable?'?':'').((string)$type);
+        }
+
+        return null;
+    }
+
+    private function isNativeType(Type $type): bool
+    {
+        if ($type instanceof String_
+            || $type instanceof Integer
+            || $type instanceof Boolean
+            || $type instanceof Float_
+            || $type instanceof Scalar
+            || $type instanceof Callable_
+            || ((string) $type) === 'iterable'
+        ) {
+            return true;
+        }
+        return false;
+    }
+
+    /**
+     * @param Type[] $docBlockTypeHints
+     * @return bool
+     */
+    private function isNullable(array $docBlockTypeHints): bool
+    {
+        foreach ($docBlockTypeHints as $docBlockTypeHint) {
+            if ($docBlockTypeHint instanceof Null_) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    /**
+     * Removes "null" from the list of types.
+     *
+     * @param Type[] $docBlockTypeHints
+     * @return array
+     */
+    private function typesWithoutNullable(array $docBlockTypeHints): array
+    {
+        return array_filter($docBlockTypeHints, function($item) {
+            return !$item instanceof Null_;
+        });
+    }
+}

src/Rules/TypeHints/MissingTypeHintInFunctionRule.php

@@ -0,0 +1,14 @@
+<?php
+
+
+namespace TheCodingMachine\PHPStan\Rules\TypeHints;
+
+use PhpParser\Node;
+
+class MissingTypeHintInFunctionRule extends AbstractMissingTypeHintRule
+{
+    public function getNodeType(): string
+    {
+        return Node\Stmt\Function_::class;
+    }
+}

src/Rules/TypeHints/MissingTypeHintInMethodRule.php

@@ -0,0 +1,14 @@
+<?php
+
+
+namespace TheCodingMachine\PHPStan\Rules\TypeHints;
+
+use PhpParser\Node;
+
+class MissingTypeHintInMethodRule extends AbstractMissingTypeHintRule
+{
+    public function getNodeType(): string
+    {
+        return Node\Stmt\ClassMethod::class;
+    }
+}

tests/Rules/TypeHints/MissingTypeHintRuleInFunctionTest.php

@@ -0,0 +1,46 @@
+<?php declare(strict_types = 1);
+
+namespace TheCodingMachine\PHPStan\Rules\TypeHints;
+
+class MissingTypeHintRuleInFunctionTest extends \PHPStan\Rules\AbstractRuleTest
+{
+
+	protected function getRule(): \PHPStan\Rules\Rule
+	{
+		return new MissingTypeHintInFunctionRule(
+		    $this->createBroker()
+        );
+	}
+
+	public function testCheckCatchedException()
+	{
+        require_once __DIR__.'/data/typehints.php';
+
+        $this->analyse([__DIR__ . '/data/typehints.php'], [
+			[
+				'Parameter $no_type_hint has no type-hint and no @param annotation.',
+				3,
+			],
+            [
+                'Parameter $type_hintable can be type-hinted to "?string".',
+                11,
+            ],
+            [
+                'Parameter $type_hintable can be type-hinted to "\DateTimeInterface".',
+                19,
+            ],
+            [
+                'Parameter $type_hintable can be type-hinted to "array".',
+                27,
+            ],
+            [
+                'Parameter $better_type_hint type is "array". Please provide a @param annotation to further speciy the type of the array. For instance: @param int[] $better_type_hint',
+                40,
+            ],
+            [
+                'Parameter $param type is type-hinted to "string" but the @param annotation says it is a "int". Please fix the @param annotation.',
+                48,
+            ],
+		]);
+	}
+}

tests/Rules/TypeHints/MissingTypeHintRuleInMethodTest.php

@@ -0,0 +1,26 @@
+<?php declare(strict_types = 1);
+
+namespace TheCodingMachine\PHPStan\Rules\TypeHints;
+
+class MissingTypeHintRuleInMethodTest extends \PHPStan\Rules\AbstractRuleTest
+{
+
+	protected function getRule(): \PHPStan\Rules\Rule
+	{
+		return new MissingTypeHintInMethodRule(
+		    $this->createBroker()
+        );
+	}
+
+	public function testCheckCatchedException()
+	{
+        require_once __DIR__.'/data/typehints.php';
+
+        $this->analyse([__DIR__ . '/data/typehints_in_methods.php'], [
+			[
+				'Parameter $no_type_hint has no type-hint and no @param annotation.',
+				5,
+			],
+		]);
+	}
+}