Improve @no-named-arguments support and variadics. (#5455)

* Improve @no-named-arguments support and variadics. Handling of argument unpacking and variadics still needs a pretty big makeover, but this is a good start. Fixes #5420 Improves #5453 (iterable works, array still causes issues) * Remove unneeded imports.
vimeo · Mar 22, 2021 · de5a031 · de5a031
1 parent 0789745
commit de5a031
Show file tree

Hide file tree

Showing 12 changed files with 275 additions and 15 deletions.
diff --git a/config.xsd b/config.xsd
@@ -310,6 +310,7 @@
             <xs:element name="MoreSpecificImplementedParamType" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="MoreSpecificReturnType" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="MutableDependency" type="PropertyIssueHandlerType" minOccurs="0" />
+            <xs:element name="NamedArgumentNotAllowed" type="ArgumentIssueHandlerType" minOccurs="0" />
             <xs:element name="NoInterfaceProperties" type="ClassIssueHandlerType" minOccurs="0" />
             <xs:element name="NonStaticSelfCall" type="IssueHandlerType" minOccurs="0" />
             <xs:element name="NoValue" type="IssueHandlerType" minOccurs="0" />

diff --git a/docs/running_psalm/issues/NamedArgumentNotAllowed.md b/docs/running_psalm/issues/NamedArgumentNotAllowed.md
@@ -0,0 +1,35 @@
+# NamedArgumentNotAllowed
+
+Emitted when a named argument is used when calling a function with `@no-named-arguments`.
+
+```php
+<?php
+
+/** @no-named-arguments */
+function foo(int $a, int $b): int {
+	return $a + $b;
+}
+
+foo(a: 0, b: 1);
+
+```
+
+## Why this is bad
+
+The `@no-named-arguments` annotation indicates that argument names may be changed in the future, and an update may break backwards compatibility with function calls using named arguments.
+
+## How to fix
+
+Avoid using named arguments for functions annotated with `@no-named-arguments`.
+
+```php
+<?php
+
+/** @no-named-arguments */
+function foo(int $a, int $b): int {
+	return $a + $b;
+}
+
+foo(0, 1);
+
+```
diff --git a/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php b/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php
@@ -987,9 +987,15 @@ private function processParams(
             $var_type = $param_type;
 
             if ($function_param->is_variadic) {
-                $var_type = new Type\Union([
-                    new Type\Atomic\TList($param_type),
-                ]);
+                if ($storage->allow_named_arg_calls) {
+                    $var_type = new Type\Union([
+                        new Type\Atomic\TArray([Type::getArrayKey(), $param_type]),
+                    ]);
+                } else {
+                    $var_type = new Type\Union([
+                        new Type\Atomic\TList($param_type),
+                    ]);
+                }
             }
 
             if ($statements_analyzer->data_flow_graph

diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentAnalyzer.php
@@ -34,6 +34,7 @@
 use Psalm\Issue\PossiblyInvalidArgument;
 use Psalm\Issue\PossiblyNullArgument;
 use Psalm\Issue\ArgumentTypeCoercion;
+use Psalm\Issue\NamedArgumentNotAllowed;
 use Psalm\IssueBuffer;
 use Psalm\Plugin\EventHandler\Event\AddRemoveTaintsEvent;
 use Psalm\Storage\FunctionLikeParameter;
@@ -42,6 +43,7 @@
 use Psalm\Type\Atomic\TClassString;
 use Psalm\Type\Atomic\TCallable;
 use Psalm\Type\Atomic\TList;
+
 use function strtolower;
 use function strpos;
 use function explode;
@@ -416,7 +418,7 @@ private static function checkFunctionLikeTypeMatches(
                 if (IssueBuffer::accepts(
                     new MixedArgument(
                         'Argument ' . ($argument_offset + 1) . ' of ' . $cased_method_id
-                            . ' cannot be ' . $arg_type->getId() . ', expecting array',
+                            . ' cannot unpack ' . $arg_type->getId() . ', expecting iterable',
                         new CodeLocation($statements_analyzer->getSource(), $arg->value),
                         $cased_method_id
                     ),
@@ -452,8 +454,13 @@ private static function checkFunctionLikeTypeMatches(
                  * @var Type\Atomic\TArray|Type\Atomic\TList|Type\Atomic\TKeyedArray
                  */
                 $unpacked_atomic_array = $arg_type->getAtomicTypes()['array'];
+                $arg_key_allowed = true;
 
                 if ($unpacked_atomic_array instanceof Type\Atomic\TKeyedArray) {
+                    if (!$allow_named_args && !$unpacked_atomic_array->getGenericKeyType()->isInt()) {
+                        $arg_key_allowed = false;
+                    }
+
                     if ($function_param->is_variadic) {
                         $arg_type = $unpacked_atomic_array->getGenericValueType();
                     } elseif ($codebase->php_major_version >= 8
@@ -483,29 +490,125 @@ private static function checkFunctionLikeTypeMatches(
                 } elseif ($unpacked_atomic_array instanceof Type\Atomic\TList) {
                     $arg_type = $unpacked_atomic_array->type_param;
                 } else {
+                    if (!$allow_named_args && !$unpacked_atomic_array->type_params[0]->isInt()) {
+                        $arg_key_allowed = false;
+                    }
                     $arg_type = $unpacked_atomic_array->type_params[1];
                 }
+
+                if (!$arg_key_allowed) {
+                    if (IssueBuffer::accepts(
+                        new NamedArgumentNotAllowed(
+                            'Method ' . $cased_method_id
+                                . ' called with named unpacked array ' . $unpacked_atomic_array->getId()
+                                . ' (array with string keys)',
+                            new CodeLocation($statements_analyzer->getSource(), $arg->value),
+                            $cased_method_id
+                        ),
+                        $statements_analyzer->getSuppressedIssues()
+                    )) {
+                        // fall through
+                    }
+                }
             } else {
+                $non_iterable = false;
+                $invalid_key = false;
+                $invalid_string_key = false;
+                $possibly_matches = false;
                 foreach ($arg_type->getAtomicTypes() as $atomic_type) {
                     if (!$atomic_type->isIterable($codebase)) {
+                        $non_iterable = true;
+                    } else {
+                        $key_type = $codebase->getKeyValueParamsForTraversableObject($atomic_type)[0];
+                        if (!UnionTypeComparator::isContainedBy(
+                            $codebase,
+                            $key_type,
+                            Type::getArrayKey()
+                        )) {
+                            $invalid_key = true;
+
+                            continue;
+                        }
+                        if (($codebase->php_major_version < 8 || !$allow_named_args) && !$key_type->isInt()) {
+                            $invalid_string_key = true;
+
+                            continue;
+                        }
+                        $possibly_matches = true;
+                    }
+                }
+
+                $issue_type = $possibly_matches ? PossiblyInvalidArgument::class : InvalidArgument::class;
+                if ($non_iterable) {
+                    if (IssueBuffer::accepts(
+                        new $issue_type(
+                            'Tried to unpack non-iterable ' . $arg_type->getId(),
+                            new CodeLocation($statements_analyzer->getSource(), $arg->value),
+                            $cased_method_id
+                        ),
+                        $statements_analyzer->getSuppressedIssues()
+                    )) {
+                        // fall through
+                    }
+                }
+                if ($invalid_key) {
+                    if (IssueBuffer::accepts(
+                        new $issue_type(
+                            'Method ' . $cased_method_id
+                                . ' called with unpacked iterable ' . $arg_type->getId()
+                                . ' with invalid key (must be '
+                                . ($codebase->php_major_version < 8 ? 'int' : 'int|string') . ')',
+                            new CodeLocation($statements_analyzer->getSource(), $arg->value),
+                            $cased_method_id
+                        ),
+                        $statements_analyzer->getSuppressedIssues()
+                    )) {
+                        // fall through
+                    }
+                }
+                if ($invalid_string_key) {
+                    if ($codebase->php_major_version < 8) {
                         if (IssueBuffer::accepts(
-                            new InvalidArgument(
-                                'Argument ' . ($argument_offset + 1) . ' of ' . $cased_method_id
-                                    . ' expects array, ' . $atomic_type->getId() . ' provided',
+                            new $issue_type(
+                                'String keys not supported in unpacked arguments',
+                                new CodeLocation($statements_analyzer->getSource(), $arg->value),
+                                $cased_method_id
+                            ),
+                            $statements_analyzer->getSuppressedIssues()
+                        )) {
+                            // fall through
+                        }
+                    } else {
+                        if (IssueBuffer::accepts(
+                            new NamedArgumentNotAllowed(
+                                'Method ' . $cased_method_id
+                                    . ' called with named unpacked iterable ' . $arg_type->getId()
+                                    . ' (iterable with string keys)',
                                 new CodeLocation($statements_analyzer->getSource(), $arg->value),
                                 $cased_method_id
                             ),
                             $statements_analyzer->getSuppressedIssues()
                         )) {
                             // fall through
                         }
-
-                        continue;
                     }
                 }
 
                 return null;
             }
+        } else {
+            if (!$allow_named_args && $arg->name !== null) {
+                if (IssueBuffer::accepts(
+                    new NamedArgumentNotAllowed(
+                        'Method ' . $cased_method_id. ' called with named argument ' . $arg->name->name,
+                        new CodeLocation($statements_analyzer->getSource(), $arg->value),
+                        $cased_method_id
+                    ),
+                    $statements_analyzer->getSuppressedIssues()
+                )) {
+                    // fall through
+                }
+            }
         }
 
         if (self::verifyType(

diff --git a/src/Psalm/Issue/NamedArgumentNotAllowed.php b/src/Psalm/Issue/NamedArgumentNotAllowed.php
@@ -0,0 +1,8 @@
+<?php
+namespace Psalm\Issue;
+
+class NamedArgumentNotAllowed extends ArgumentIssue
+{
+    public const ERROR_LEVEL = 7;
+    public const SHORTCODE = 268;
+}
diff --git a/tests/ArgTest.php b/tests/ArgTest.php
@@ -82,7 +82,7 @@ function f3(): array
                 '<?php
                     function Foo(string $a, string ...$b) : void {}
 
-                    /** @return array<int, string> */
+                    /** @return array<array-key, string> */
                     function Baz(string ...$c) {
                         Foo(...$c);
                         return $c;
@@ -201,10 +201,12 @@ function takeVariadicInts(int ...$inputs): void {}
             ],
             'iterableSplat' => [
                 '<?php
+                    /** @param iterable<int, mixed> $args */
                     function foo(iterable $args): int {
                         return intval(...$args);
                     }
 
+                    /** @param ArrayIterator<int, mixed> $args */
                     function bar(ArrayIterator $args): int {
                         return intval(...$args);
                     }',
@@ -289,6 +291,20 @@ function bar(string $p1, int $p3 = 10) : void {}'
                 [],
                 '8.0'
             ],
+            'variadicArgumentWithNoNamedArgumentsIsList' => [
+                '<?php
+                    class A {
+                        /**
+                         * @no-named-arguments
+                         * @psalm-return list<int>
+                         */
+                        public function foo(int ...$values): array
+                        {
+                            return $values;
+                        }
+                    }
+                ',
+            ],
         ];
     }
 
@@ -430,15 +446,15 @@ function foo(array $input) : CustomerData {
                             email: $input["email"],
                         );
                     }',
-                'error_message' => 'InvalidScalarArgument'
+                'error_message' => 'NamedArgumentNotAllowed',
             ],
             'noNamedArgsFunction' => [
                 '<?php
                     /** @no-named-arguments */
                     function takesArguments(string $name, int $age) : void {}
 
                     takesArguments(age: 5, name: "hello");',
-                'error_message' => 'InvalidScalarArgument'
+                'error_message' => 'NamedArgumentNotAllowed',
             ],
             'arrayWithoutAllNamedParameters' => [
                 '<?php
@@ -546,6 +562,94 @@ function test(int $param, int $param2): void {
                 false,
                 '8.0'
             ],
+            'variadicArgumentIsNotList' => [
+                '<?php
+                    /** @psalm-return list<int> */
+                    function foo(int ...$values): array
+                    {
+                        return $values;
+                    }
+                ',
+                'error_message' => 'MixedReturnTypeCoercion',
+            ],
+            'preventUnpackingPossiblyIterable' => [
+                '<?php
+                    function foo(int $arg1, int $arg2): void {}
+
+                    /** @var iterable<int, int>|object */
+                    $test = [1, 2];
+                    foo(...$test);
+                ',
+                'error_message' => 'PossiblyInvalidArgument'
+            ],
+            'SKIPPED-preventUnpackingPossiblyArray' => [
+                '<?php
+                    function foo(int $arg1, int $arg2): void {}
+
+                    /** @var array<int, int>|object */
+                    $test = [1, 2];
+                    foo(...$test);
+                ',
+                'error_message' => 'PossiblyInvalidArgument'
+            ],
+            'noNamedArguments' => [
+                '<?php
+                    /**
+                     * @psalm-suppress UnusedParam
+                     * @no-named-arguments
+                     */
+                    function foo(int $arg1, int $arg2): void {}
+
+                    foo(arg2: 0, arg1: 1);
+                ',
+                'error_message' => 'NamedArgumentNotAllowed',
+                [],
+                false,
+                '8.0',
+            ],
+            'noNamedArgumentsUnpackIterable' => [
+                '<?php
+                    /**
+                     * @psalm-suppress UnusedParam
+                     * @no-named-arguments
+                     */
+                    function foo(int $arg1, int $arg2): void {}
+
+                    /** @var iterable<string, int> */
+                    $test = ["arg1" => 1, "arg2" => 2];
+                    foo(...$test);
+                ',
+                'error_message' => 'NamedArgumentNotAllowed',
+                [],
+                false,
+                '8.0',
+            ],
+            'variadicArgumentWithNoNamedArgumentsPreventsPassingArrayWithStringKey' => [
+                '<?php
+                    /**
+                     * @no-named-arguments
+                     * @psalm-return list<int>
+                     */
+                    function foo(int ...$values): array
+                    {
+                        return $values;
+                    }
+
+                    foo(...["a" => 0]);
+                ',
+                'error_message' => 'NamedArgumentNotAllowed',
+            ],
+            'unpackNonArrayKeyIterable' => [
+                '<?php
+                    /** @psalm-suppress UnusedParam */
+                    function foo(string ...$args): void {}
+
+                    /** @var Iterator<float, string> */
+                    $test = null;
+                    foo(...$test);
+                ',
+                'error_message' => 'InvalidArgument',
+            ],
         ];
     }
 }