From 8d1de7757d7c6d4619490ef7618bc06a40626b9e Mon Sep 17 00:00:00 2001 From: Matt Brown Date: Wed, 14 Oct 2020 18:51:15 -0400 Subject: [PATCH] Use more accurate arguments count --- examples/TemplateChecker.php | 1 + examples/plugins/FunctionCasingChecker.php | 1 + src/Psalm/Codebase.php | 3 + .../Expression/Call/ArgumentAnalyzer.php | 14 +- .../Expression/Call/ArgumentsAnalyzer.php | 97 ++++++++---- .../Type/Comparator/ScalarTypeComparator.php | 4 - tests/ArgTest.php | 141 ++++++++++++++++++ tests/Php56Test.php | 115 -------------- tests/UnusedVariableTest.php | 1 + 9 files changed, 225 insertions(+), 152 deletions(-) diff --git a/examples/TemplateChecker.php b/examples/TemplateChecker.php index 42afd7f3a..79f5b7100 100644 --- a/examples/TemplateChecker.php +++ b/examples/TemplateChecker.php @@ -45,6 +45,7 @@ class TemplateAnalyzer extends Psalm\Internal\Analyzer\FileAnalyzer throw new \InvalidArgumentException('Could not interpret doc comment correctly'); } + /** @psalm-suppress ArgumentTypeCoercion */ $method_id = new \Psalm\Internal\MethodIdentifier(...explode('::', $matches[1])); $this_params = $this->checkMethod($method_id, $first_stmt, $codebase); diff --git a/examples/plugins/FunctionCasingChecker.php b/examples/plugins/FunctionCasingChecker.php index 59ae5819b..5ea44578d 100644 --- a/examples/plugins/FunctionCasingChecker.php +++ b/examples/plugins/FunctionCasingChecker.php @@ -42,6 +42,7 @@ class FunctionCasingChecker implements AfterFunctionCallAnalysisInterface, After } try { + /** @psalm-suppress ArgumentTypeCoercion */ $method_id = new \Psalm\Internal\MethodIdentifier(...explode('::', $declaring_method_id)); $function_storage = $codebase->methods->getStorage($method_id); diff --git a/src/Psalm/Codebase.php b/src/Psalm/Codebase.php index 93b28ab9f..ea78ba3ff 100644 --- a/src/Psalm/Codebase.php +++ b/src/Psalm/Codebase.php @@ -902,6 +902,7 @@ class Codebase if (strpos($symbol, '()')) { $symbol = substr($symbol, 0, -2); + /** @psalm-suppress ArgumentTypeCoercion */ $method_id = new \Psalm\Internal\MethodIdentifier(...explode('::', $symbol)); $declaring_method_id = $this->methods->getDeclaringMethodId($method_id); @@ -987,6 +988,7 @@ class Codebase if (strpos($symbol, '()')) { $symbol = substr($symbol, 0, -2); + /** @psalm-suppress ArgumentTypeCoercion */ $method_id = new \Psalm\Internal\MethodIdentifier(...explode('::', $symbol)); $declaring_method_id = $this->methods->getDeclaringMethodId($method_id); @@ -1162,6 +1164,7 @@ class Codebase public function getSignatureInformation(string $function_symbol) : ?\LanguageServerProtocol\SignatureInformation { if (strpos($function_symbol, '::') !== false) { + /** @psalm-suppress ArgumentTypeCoercion */ $method_id = new \Psalm\Internal\MethodIdentifier(...explode('::', $function_symbol)); $declaring_method_id = $this->methods->getDeclaringMethodId($method_id); diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentAnalyzer.php index 1b646fbcc..da9fcb65d 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentAnalyzer.php @@ -60,6 +60,7 @@ class ArgumentAnalyzer CodeLocation $function_call_location, ?FunctionLikeParameter $function_param, int $argument_offset, + int $unpacked_argument_offset, bool $allow_named_args, PhpParser\Node\Arg $arg, ?Type\Union $arg_value_type, @@ -175,6 +176,7 @@ class ArgumentAnalyzer $allow_named_args, $arg_value_type, $argument_offset, + $unpacked_argument_offset, $arg, $context, $class_generic_params, @@ -205,6 +207,7 @@ class ArgumentAnalyzer bool $allow_named_args, Type\Union $arg_type, int $argument_offset, + int $unpacked_argument_offset, PhpParser\Node\Arg $arg, Context $context, ?array $class_generic_params, @@ -413,18 +416,19 @@ class ArgumentAnalyzer $unpacked_atomic_array = $arg_type->getAtomicTypes()['array']; if ($unpacked_atomic_array instanceof Type\Atomic\TKeyedArray) { - if ($codebase->php_major_version >= 8 + if ($function_param->is_variadic) { + $arg_type = $unpacked_atomic_array->getGenericValueType(); + } elseif ($codebase->php_major_version >= 8 && $allow_named_args && isset($unpacked_atomic_array->properties[$function_param->name]) ) { $arg_type = clone $unpacked_atomic_array->properties[$function_param->name]; } elseif ($unpacked_atomic_array->is_list - && $argument_offset === 0 - && isset($unpacked_atomic_array->properties[$argument_offset]) + && isset($unpacked_atomic_array->properties[$unpacked_argument_offset]) ) { - $arg_type = clone $unpacked_atomic_array->properties[$argument_offset]; + $arg_type = clone $unpacked_atomic_array->properties[$unpacked_argument_offset]; } else { - $arg_type = $unpacked_atomic_array->getGenericValueType(); + $arg_type = Type::getMixed(); } } elseif ($unpacked_atomic_array instanceof Type\Atomic\TList) { $arg_type = $unpacked_atomic_array->type_param; diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentsAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentsAnalyzer.php index 6b5af0356..5799b52db 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentsAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/ArgumentsAnalyzer.php @@ -498,8 +498,41 @@ class ArgumentsAnalyzer $has_packed_var = false; + $packed_var_definite_args = 0; + foreach ($args as $arg) { - $has_packed_var = $has_packed_var || $arg->unpack; + if ($arg->unpack) { + $arg_value_type = $statements_analyzer->node_data->getType($arg->value); + + if (!$arg_value_type + || !$arg_value_type->isSingle() + || !$arg_value_type->hasArray() + ) { + $has_packed_var = true; + break; + } + + foreach ($arg_value_type->getAtomicTypes() as $atomic_arg_type) { + if (!$atomic_arg_type instanceof TKeyedArray) { + $has_packed_var = true; + break 2; + } + + $packed_var_definite_args = 0; + + foreach ($atomic_arg_type->properties as $property_type) { + if ($property_type->possibly_undefined) { + $has_packed_var = true; + } else { + $packed_var_definite_args++; + } + } + } + } + } + + if (!$has_packed_var) { + $packed_var_definite_args = \max(0, $packed_var_definite_args - 1); } $last_param = $function_params @@ -600,6 +633,7 @@ class ArgumentsAnalyzer $code_location, $function_params[$i], $i, + $i, $function_storage ? $function_storage->allow_named_arg_calls : true, new PhpParser\Node\Arg( StubsGenerator::getExpressionFromType($function_params[$i]->default_type) @@ -620,17 +654,21 @@ class ArgumentsAnalyzer } foreach ($args as $argument_offset => $arg) { - $function_param = null; + $arg_function_params = []; - if ($arg->name && $function_storage && $function_storage->allow_named_arg_calls) { + if ($arg->unpack && $function_param_count > $argument_offset) { + for ($i = $argument_offset; $i < $function_param_count; $i++) { + $arg_function_params[] = $function_params[$i]; + } + } elseif ($arg->name && $function_storage && $function_storage->allow_named_arg_calls) { foreach ($function_params as $candidate_param) { if ($candidate_param->name === $arg->name->name) { - $function_param = $candidate_param; + $arg_function_params = [$candidate_param]; break; } } - if (!$function_param) { + if (!$arg_function_params) { if (IssueBuffer::accepts( new InvalidNamedArgument( 'Parameter $' . $arg->name->name . ' does not exist on function ' @@ -644,13 +682,13 @@ class ArgumentsAnalyzer } } } elseif ($function_param_count > $argument_offset) { - $function_param = $function_params[$argument_offset]; + $arg_function_params = [$function_params[$argument_offset]]; } elseif ($last_param && $last_param->is_variadic) { - $function_param = $last_param; + $arg_function_params = [$last_param]; } - if ($function_param - && $function_param->by_ref + if ($arg_function_params + && $arg_function_params[0]->by_ref && $method_id !== 'extract' ) { if (self::handlePossiblyMatchingByRefParam( @@ -671,24 +709,27 @@ class ArgumentsAnalyzer $arg_value_type = $statements_analyzer->node_data->getType($arg->value); - if (ArgumentAnalyzer::checkArgumentMatches( - $statements_analyzer, - $cased_method_id, - $self_fq_class_name, - $static_fq_class_name, - $code_location, - $function_param, - $argument_offset, - $function_storage ? $function_storage->allow_named_arg_calls : true, - $arg, - $arg_value_type, - $context, - $class_generic_params, - $template_result, - $function_storage ? $function_storage->specialize_call : true, - $in_call_map - ) === false) { - return false; + foreach ($arg_function_params as $i => $function_param) { + if (ArgumentAnalyzer::checkArgumentMatches( + $statements_analyzer, + $cased_method_id, + $self_fq_class_name, + $static_fq_class_name, + $code_location, + $function_param, + $argument_offset + $i, + $i, + $function_storage ? $function_storage->allow_named_arg_calls : true, + $arg, + $arg_value_type, + $context, + $class_generic_params, + $template_result, + $function_storage ? $function_storage->specialize_call : true, + $in_call_map + ) === false) { + return false; + } } } @@ -767,7 +808,7 @@ class ArgumentsAnalyzer $expected_param_count = $i; } - for ($i = count($args), $j = count($function_params); $i < $j; ++$i) { + for ($i = count($args) + $packed_var_definite_args, $j = count($function_params); $i < $j; ++$i) { $param = $function_params[$i]; if (!$param->is_optional diff --git a/src/Psalm/Internal/Type/Comparator/ScalarTypeComparator.php b/src/Psalm/Internal/Type/Comparator/ScalarTypeComparator.php index a678f6adc..19d1efef6 100644 --- a/src/Psalm/Internal/Type/Comparator/ScalarTypeComparator.php +++ b/src/Psalm/Internal/Type/Comparator/ScalarTypeComparator.php @@ -153,10 +153,6 @@ class ScalarTypeComparator } if ($container_type_part instanceof TDependentGetDebugType) { - if ($container_type_part instanceof TClassString || $container_type_part instanceof TLiteralClassString) { - return true; - } - return $input_type_part instanceof TString; } diff --git a/tests/ArgTest.php b/tests/ArgTest.php index c37999934..e785b09b0 100644 --- a/tests/ArgTest.php +++ b/tests/ArgTest.php @@ -12,6 +12,94 @@ class ArgTest extends TestCase public function providerValidCodeParse(): iterable { return [ + 'argumentUnpackingLiteral' => [ + ' [ + ' [ + '$a' => 'non-empty-list', + ], + ], + 'arrayMergeArgumentUnpacking' => [ + ' [ + '$b' => 'array{0: int, 1: int}', + ], + ], + 'preserveTypesWhenUnpacking' => [ + '> + */ + function getData(): array + { + return [ + ["a", "b"], + ["c", "d"] + ]; + } + + /** + * @return array + */ + function f1(): array + { + $data = getData(); + return array_merge($data[0], $data[1]); + } + + /** + * @return array + */ + function f2(): array + { + $data = getData(); + return array_merge(...$data); + } + + /** + * @return array + */ + function f3(): array + { + $data = getData(); + return array_merge([], ...$data); + }', + ], + 'unpackArg' => [ + ' */ + function Baz(string ...$c) { + Foo(...$c); + return $c; + }', + ], + 'unpackByRefArg' => [ + ' [ + '$y' => 'int', + '$z' => 'array', + ], + ], 'callMapClassOptionalArg' => [ ' [ + ' 'InvalidArgument', + ], 'possiblyInvalidArgument' => [ ' 'InvalidScalarArgument' ], + 'arrayWithoutAllNamedParameters' => [ + ' 'MixedArgument', + [], + false, + '8.0' + ], + 'arrayWithoutAllNamedParametersSuppressMixed' => [ + ' 'TooFewArguments', + [], + false, + '8.0' + ], ]; } } diff --git a/tests/Php56Test.php b/tests/Php56Test.php index 93353d0ac..3ca808941 100644 --- a/tests/Php56Test.php +++ b/tests/Php56Test.php @@ -4,7 +4,6 @@ namespace Psalm\Tests; class Php56Test extends TestCase { use Traits\ValidCodeAnalysisTestTrait; - use Traits\InvalidCodeAnalysisTestTrait; /** * @return iterable,error_levels?:string[]}> @@ -70,101 +69,6 @@ class Php56Test extends TestCase '$two' => 'int', ], ], - 'argumentUnpacking' => [ - ' [ - ' [ - '$a' => 'non-empty-list', - ], - ], - 'arrayMergeArgumentUnpacking' => [ - ' [ - '$b' => 'array{0: int, 1: int}', - ], - ], - 'preserveTypesWhenUnpacking' => [ - '> - */ - function getData(): array - { - return [ - ["a", "b"], - ["c", "d"] - ]; - } - - /** - * @return array - */ - function f1(): array - { - $data = getData(); - return array_merge($data[0], $data[1]); - } - - /** - * @return array - */ - function f2(): array - { - $data = getData(); - return array_merge(...$data); - } - - /** - * @return array - */ - function f3(): array - { - $data = getData(); - return array_merge([], ...$data); - }', - ], - 'unpackArg' => [ - ' */ - function Baz(string ...$c) { - Foo(...$c); - return $c; - }', - ], - 'unpackByRefArg' => [ - ' [ - '$y' => 'int', - '$z' => 'array', - ], - ], 'exponentiation' => [ ' - */ - public function providerInvalidCodeParse(): iterable - { - return [ - 'arrayPushArgumentUnpackingWithBadArg' => [ - ' 'InvalidArgument', - ], - ]; - } } diff --git a/tests/UnusedVariableTest.php b/tests/UnusedVariableTest.php index c0be5aaf6..7a2bb6c4a 100644 --- a/tests/UnusedVariableTest.php +++ b/tests/UnusedVariableTest.php @@ -2184,6 +2184,7 @@ class UnusedVariableTest extends TestCase 'funcGetArgs' => [ '