Parse types for validity, and smarter checking

rules/phpdocs_basic.php

@@ -420,58 +420,75 @@ function local_moodlecheck_functiondescription(local_moodlecheck_file $file) {
  * @return array of found errors
  */
 function local_moodlecheck_functionarguments(local_moodlecheck_file $file) {
+    $typeparser = $file->get_type_parser();
     $errors = [];
 
     foreach ($file->get_functions() as $function) {
         if ($function->phpdocs !== false) {
-            $documentedarguments = $function->phpdocs->get_params();
-            $match = (count($documentedarguments) == count($function->arguments));
-            for ($i = 0; $match && $i < count($documentedarguments); $i++) {
-                if (count($documentedarguments[$i]) < 2) {
+            $ownername = $function->owner ? $function->owner->name : null;
+            $parentname = $function->owner && $function->owner->extends ? $function->owner->extends : null;
+            $errorlevel = 0;
+
+            $documentedarguments = $function->phpdocs->get_params('param', 2);
+            if (count($documentedarguments) != count($function->arguments)) {
+                $errorlevel = 2;
+            }
+            for ($i = 0; $errorlevel < 2 && $i < count($documentedarguments); $i++) {
+                if (count($documentedarguments[$i]) < 2 || $documentedarguments[$i][0] == null
+                        || $documentedarguments[$i][1] == null) {
                     // Must be at least type and parameter name.
-                    $match = false;
+                    $errorlevel = 2;
                 } else {
-                    $expectedtype = local_moodlecheck_normalise_function_type((string) $function->arguments[$i][0]);
+                    $expectedtype = $function->arguments[$i][0];
                     $expectedparam = (string)$function->arguments[$i][1];
-                    $documentedtype = local_moodlecheck_normalise_function_type((string) $documentedarguments[$i][0]);
+                    $expectednullable = $function->arguments[$i][2];
+                    $documentedtype = $documentedarguments[$i][0];
                     $documentedparam = $documentedarguments[$i][1];
 
-                    $typematch = $expectedtype === $documentedtype;
-                    $parammatch = $expectedparam === $documentedparam;
-                    if ($typematch && $parammatch) {
-                        continue;
-                    }
-
-                    // Documented types can be a collection (| separated).
-                    foreach (explode('|', $documentedtype) as $documentedtype) {
-                        // Ignore null. They cannot match any type in function.
-                        if (trim($documentedtype) === 'null') {
-                            continue;
-                        }
-
-                        if (strlen($expectedtype) && $expectedtype !== $documentedtype) {
-                            // It could be a type hinted array.
-                            if ($expectedtype !== 'array' || substr($documentedtype, -2) !== '[]') {
-                                $match = false;
-                            }
-                        } else if ($documentedtype === 'type') {
-                            $match = false;
-                        } else if ($expectedparam !== $documentedparam) {
-                            $match = false;
+                    if ($expectedparam !== $documentedparam) {
+                        $errorlevel = 2;
+                    } else if ($expectedtype != null) {
+                        $expectedtype = $typeparser->substitute_names($expectedtype, $ownername, $parentname);
+                        $documentedtype = $typeparser->substitute_names($documentedtype, $ownername, $parentname);
+                        if (!$typeparser->compare_types($expectedtype, $documentedtype)) {
+                            $errorlevel = 2;
+                        } else if ($expectednullable && strpos("|{$documentedtype}|", '|void|') === false) {
+                            $errorlevel = 1;
                         }
                     }
                 }
             }
-            $documentedreturns = $function->phpdocs->get_params('return');
-            for ($i = 0; $match && $i < count($documentedreturns); $i++) {
-                if (empty($documentedreturns[$i][0]) || $documentedreturns[$i][0] == 'type') {
-                    $match = false;
+
+            $documentedreturns = $function->phpdocs->get_params('return', 0);
+            if (count($documentedreturns) > 1) {
+                $errorlevel = 2;
+            } else if (count($documentedreturns) == 1) {
+                if (empty($documentedreturns[0][0])) {
+                    $errorlevel = 2;
+                } else if ($function->return != null) {
+                    $expectedtype = $function->return;
+                    $documentedtype = $documentedreturns[0][0];
+                    $expectedtype = $typeparser->substitute_names($expectedtype, $ownername, $parentname);
+                    $documentedtype = $typeparser->substitute_names($documentedtype, $ownername, $parentname);
+                    if (!$typeparser->compare_types($expectedtype, $documentedtype)) {
+                        $errorlevel = 2;
+                    } else if ($errorlevel < 2
+                            && strpos("|{$expectedtype}|", '|void|') !== false
+                            && strpos("|{$documentedtype}|", '|void|') === false
+                            && $documentedtype != 'never') {
+                        $errorlevel = 1;
+                    }
                 }
             }
-            if (!$match) {
-                $errors[] = [
+
+            if ($errorlevel > 0) {
+                $error = [
                     'line' => $function->phpdocs->get_line_number($file, '@param'),
                     'function' => $function->fullname, ];
+                if ($errorlevel < 2) {
+                    $error['severity'] = 'warning';
+                }
+                $errors[] = $error;
             }
         }
     }
@@ -482,36 +499,12 @@ function local_moodlecheck_functionarguments(local_moodlecheck_file $file) {
  * Normalise function type to be able to compare it.
  *
  * @param string $typelist
- * @return string
+ * @return ?string
  */
-function local_moodlecheck_normalise_function_type(string $typelist): string {
-    // Normalise a nullable type to `null|type` as these are just shorthands.
-    $typelist = str_replace(
-        '?',
-        'null|',
-        $typelist
-    );
-
-    // PHP 8 treats namespaces as single token. So we are going to undo this here
-    // and continue returning only the final part of the namespace. Someday we'll
-    // move to use full namespaces here, but not for now (we are doing the same,
-    // in other parts of the code, when processing phpdoc blocks).
-    $types = explode('|', $typelist);
-
-    // Namespaced typehint, potentially sub-namespaced.
-    // We need to strip namespacing as this area just isn't that smart.
-    $types = array_map(
-        function($type) {
-            if (strpos((string)$type, '\\') !== false) {
-                $type = substr($type, strrpos($type, '\\') + 1);
-            }
-            return $type;
-        },
-        $types
-    );
-    sort($types);
-
-    return implode('|', $types);
+function local_moodlecheck_normalise_function_type(string $typelist): ?string {
+    $typeparser = new \local_moodlecheck\type_parser();
+    list($type, $variable, $remainder) = $typeparser->parse_type_and_var(null, $typelist, 0, true);
+    return $type;
 }
 
 /**
@@ -521,14 +514,39 @@ function($type) {
  * @return array of found errors
  */
 function local_moodlecheck_variableshasvar(local_moodlecheck_file $file) {
+    $typeparser = $file->get_type_parser();
     $errors = [];
     foreach ($file->get_variables() as $variable) {
         if ($variable->phpdocs !== false) {
-            $documentedvars = $variable->phpdocs->get_params('var', 2);
-            if (!count($documentedvars) || $documentedvars[0][0] == 'type') {
-                $errors[] = [
+            $ownername = $variable->class ? $variable->class->name : null;
+            $parentname = $variable->class && $variable->class->extends ? $variable->class->extends : null;
+            $errorlevel = 0;
+
+            $documentedvars = $variable->phpdocs->get_params('var', 0);
+            if (count($documentedvars) != 1 || $documentedvars[0][0] == null) {
+                $errorlevel = 2;
+            } else if ($variable->type != null) {
+                $expectedtype = $variable->type;
+                $documentedtype = $documentedvars[0][0];
+                $expectedtype = $typeparser->substitute_names($expectedtype, $ownername, $parentname);
+                $documentedtype = $typeparser->substitute_names($documentedtype, $ownername, $parentname);
+                if (!$typeparser->compare_types($expectedtype, $documentedtype)) {
+                    $errorlevel = 2;
+                } else if ($errorlevel < 2
+                        && strpos("|{$expectedtype}|", '|void|') !== false
+                        && strpos("|{$documentedtype}|", '|void|') === false) {
+                    $errorlevel = 1;
+                }
+            }
+
+            if ($errorlevel > 0) {
+                $error = [
                     'line' => $variable->phpdocs->get_line_number($file, '@var'),
                     'variable' => $variable->fullname, ];
+                if ($errorlevel < 2) {
+                    $error['severity'] = 'warning';
+                }
+                $errors[] = $error;
             }
         }
     }

tests/fixtures/phpdoc_tags_general.php

@@ -254,7 +254,7 @@ public function namespaced_parameter_type(
      */
     public function builtin(
         ?\stdClass $data,
-        ?\core\test\something|\core\some\other_thing $moredata
+        \core\test\something|\core\some\other_thing|null $moredata
     ): \stdClass {
         return $user;
     }

tests/fixtures/phpdoc_types_invalid.php

@@ -0,0 +1,133 @@
+<?php
+// This file is part of Moodle - http://moodle.org/
+//
+// Moodle is free software: you can redistribute it and/or modify
+// it under the terms of the GNU General Public License as published by
+// the Free Software Foundation, either version 3 of the License, or
+// (at your option) any later version.
+//
+// Moodle is distributed in the hope that it will be useful,
+// but WITHOUT ANY WARRANTY; without even the implied warranty of
+// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+// GNU General Public License for more details.
+//
+// You should have received a copy of the GNU General Public License
+// along with Moodle.  If not, see <http://www.gnu.org/licenses/>.
+
+// phpcs:ignoreFile
+
+/**
+ * A collection of invalid types for testing
+ *
+ * Every type annotation should give an error either when checked with PHPStan or Psalm.
+ * Having just invalid types in here means the number of errors should match the number of type annotations.
+ *
+ * @package   local_moodlecheck
+ * @copyright 2023 Te Pūkenga – New Zealand Institute of Skills and Technology
+ * @author    James Calder
+ * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later (or CC BY-SA v4 or later)
+ */
+
+defined('MOODLE_INTERNAL') || die();
+
+/**
+ * A collection of invalid types for testing
+ *
+ * @package   local_moodlecheck
+ * @copyright 2023 Te Pūkenga – New Zealand Institute of Skills and Technology
+ * @author    James Calder
+ * @license   http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later (or CC BY-SA v4 or later)
+ */
+class types_invalid {
+
+    /**
+     * Expecting variable name, saw end
+     * @param int
+     */
+    public function expecting_var_saw_end(int $x): void {
+    }
+
+    /**
+     * Expecting variable name, saw other (passes Psalm)
+     * @param int int
+     */
+    public function expecting_var_saw_other(int $x): void {
+    }
+
+    // Expecting type, saw end.
+    /** @var */
+    public $expectingtypesawend;
+
+    /** @var $varname Expecting type, saw other */
+    public $expectingtypesawother;
+
+    // Unterminated string (passes Psalm).
+    /** @var " */
+    public $unterminatedstring;
+
+    // Unterminated string with escaped quote (passes Psalm).
+    /** @var "\"*/
+    public $unterminatedstringwithescapedquote;
+
+    // String has escape with no following character (passes Psalm).
+    /** @var "\*/
+    public $stringhasescapewithnofollowingchar;
+
+    /** @var array-key&(int|string) Non-DNF type (passes PHPStan) */
+    public $nondnftype;
+
+    /** @var int&string Invalid intersection */
+    public $invalidintersection;
+
+    /** @var int<0.0, 1> Invalid int min */
+    public $invalidintmin;
+
+    /** @var int<0, 1.0> Invalid int max */
+    public $invalidintmax;
+
+    /** @var int-mask<1.0, 2.0> Invalid int mask 1 */
+    public $invalidintmask1;
+
+    /** @var int-mask-of<string> Invalid int mask 2 */
+    public $invalidintmask2;
+
+    // Expecting class for class-string, saw end.
+    /** @var class-string< */
+    public $expectingclassforclassstringsawend;
+
+    /** @var class-string<int> Expecting class for class-string, saw other */
+    public $expectingclassforclassstringsawother;
+
+    /** @var list<int, string> List key */
+    public $listkey;
+
+    /** @var array<object, object> Invalid array key (passes Psalm) */
+    public $invalidarraykey;
+
+    /** @var non-empty-array{'a': int} Non-empty-array shape */
+    public $nonemptyarrayshape;
+
+    /** @var object{0.0: int} Invalid object key (passes Psalm) */
+    public $invalidobjectkey;
+
+    /** @var key-of<int> Can't get key of non-iterable */
+    public $cantgetkeyofnoniterable;
+
+    /** @var value-of<int> Can't get value of non-iterable */
+    public $cantgetvalueofnoniterable;
+
+    /**
+     * Class name has trailing slash
+     * @param types_invalid/ $x
+     */
+    public function class_name_has_trailing_slash(object $x): void {
+    }
+
+    // Expecting closing bracket, saw end.
+    /** @var (types_invalid */
+    public $expectingclosingbracketsawend;
+
+    /** @var (types_invalid int Expecting closing bracket, saw other*/
+    public $expectingclosingbracketsawother;
+
+}