From c626b7d68abe446cc9a6192632974681c477307c Mon Sep 17 00:00:00 2001 From: Matt Brown Date: Mon, 14 Aug 2017 19:30:11 -0400 Subject: [PATCH] Fix #200 - allow mapping of more callable strings, and callable arrays --- src/Psalm/Checker/FileChecker.php | 5 +- src/Psalm/Checker/FunctionChecker.php | 120 ++++++++++-- src/Psalm/Checker/FunctionLikeChecker.php | 7 +- .../Statements/Expression/CallChecker.php | 171 ++++++++++++------ src/Psalm/Checker/StatementsChecker.php | 5 +- tests/ClosureTest.php | 46 ++++- .../FileCheckerValidCodeParseTestTrait.php | 7 +- 7 files changed, 272 insertions(+), 89 deletions(-) diff --git a/src/Psalm/Checker/FileChecker.php b/src/Psalm/Checker/FileChecker.php index 98db29e9a..2e3063087 100644 --- a/src/Psalm/Checker/FileChecker.php +++ b/src/Psalm/Checker/FileChecker.php @@ -207,9 +207,8 @@ class FileChecker extends SourceChecker implements StatementsSource $method_id = $function_checker->getMethodId(); $function_storage = FunctionChecker::getStorage( - $this->project_checker, - $method_id, - $this->file_path + $statements_checker, + $method_id ); if (!$function_storage->has_template_return_type) { diff --git a/src/Psalm/Checker/FunctionChecker.php b/src/Psalm/Checker/FunctionChecker.php index 80c1802b3..d1f23b884 100644 --- a/src/Psalm/Checker/FunctionChecker.php +++ b/src/Psalm/Checker/FunctionChecker.php @@ -42,13 +42,14 @@ class FunctionChecker extends FunctionLikeChecker /** * @param string $function_id - * @param string $file_path * * @return bool */ - public static function functionExists(ProjectChecker $project_checker, $function_id, $file_path) + public static function functionExists(StatementsChecker $statements_checker, $function_id) { - $file_storage = $project_checker->file_storage_provider->get($file_path); + $project_checker = $statements_checker->getFileChecker()->project_checker; + + $file_storage = $project_checker->file_storage_provider->get($statements_checker->getFilePath()); if (isset($file_storage->declaring_function_ids[$function_id])) { return true; @@ -66,6 +67,10 @@ class FunctionChecker extends FunctionLikeChecker return true; } + if (isset($statements_checker->getFunctionCheckers()[$function_id])) { + return true; + } + if (self::extractReflectionInfo($function_id) === false) { return false; } @@ -75,11 +80,10 @@ class FunctionChecker extends FunctionLikeChecker /** * @param string $function_id - * @param string $file_path * * @return FunctionLikeStorage */ - public static function getStorage(ProjectChecker $project_checker, $function_id, $file_path) + public static function getStorage(StatementsChecker $statements_checker, $function_id) { if (isset(self::$stubbed_functions[$function_id])) { return self::$stubbed_functions[$function_id]; @@ -89,8 +93,25 @@ class FunctionChecker extends FunctionLikeChecker return self::$builtin_functions[$function_id]; } + $project_checker = $statements_checker->getFileChecker()->project_checker; + $file_path = $statements_checker->getFilePath(); + $file_storage = $project_checker->file_storage_provider->get($file_path); + $function_checkers = $statements_checker->getFunctionCheckers(); + + if (isset($function_checkers[$function_id])) { + $function_id = $function_checkers[$function_id]->getMethodId(); + + if (!isset($file_storage->functions[$function_id])) { + throw new \UnexpectedValueException( + 'Expecting ' . $function_id . ' to have storage in ' . $file_path + ); + } + + return $file_storage->functions[$function_id]; + } + if (!isset($file_storage->declaring_function_ids[$function_id])) { throw new \UnexpectedValueException( 'Expecting ' . $function_id . ' to have storage in ' . $file_path @@ -338,6 +359,7 @@ class FunctionChecker extends FunctionLikeChecker * @return Type\Union */ public static function getReturnTypeFromCallMapWithArgs( + StatementsChecker $statements_checker, $function_id, array $call_args, CodeLocation $code_location, @@ -376,6 +398,7 @@ class FunctionChecker extends FunctionLikeChecker if (substr($call_map_key, 0, 6) === 'array_') { $array_return_type = self::getArrayReturnType( + $statements_checker, $call_map_key, $call_args, $code_location, @@ -433,13 +456,20 @@ class FunctionChecker extends FunctionLikeChecker * @return Type\Union|null */ protected static function getArrayReturnType( + StatementsChecker $statements_checker, $call_map_key, $call_args, CodeLocation $code_location, array $suppressed_issues ) { if ($call_map_key === 'array_map') { - return self::getArrayMapReturnType($call_map_key, $call_args, $code_location, $suppressed_issues); + return self::getArrayMapReturnType( + $statements_checker, + $call_map_key, + $call_args, + $code_location, + $suppressed_issues + ); } $first_arg = isset($call_args[0]->value) ? $call_args[0]->value : null; @@ -538,6 +568,7 @@ class FunctionChecker extends FunctionLikeChecker * @return Type\Union */ protected static function getArrayMapReturnType( + StatementsChecker $statements_checker, $call_map_key, $call_args, CodeLocation $code_location, @@ -599,24 +630,77 @@ class FunctionChecker extends FunctionLikeChecker ]), ]); } - } elseif ($function_call_arg->value instanceof PhpParser\Node\Scalar\String_) { - $mapped_function_id = strtolower($function_call_arg->value->value); + } elseif ($function_call_arg->value instanceof PhpParser\Node\Scalar\String_ + || $function_call_arg->value instanceof PhpParser\Node\Expr\Array_ + ) { + $mapping_function_ids = Statements\Expression\CallChecker::getFunctionIdsFromCallableArg( + $statements_checker, + $function_call_arg->value + ); $call_map = self::getCallMap(); - if (isset($call_map[$mapped_function_id][0])) { - if ($call_map[$mapped_function_id][0]) { - $mapped_function_return = Type::parseString($call_map[$mapped_function_id][0]); + $mapping_return_type = null; - return new Type\Union([ - new Type\Atomic\TArray([ - Type::getInt(), - $mapped_function_return, - ]), - ]); + $project_checker = $statements_checker->getFileChecker()->project_checker; + + foreach ($mapping_function_ids as $mapping_function_id) { + if (isset($call_map[$mapping_function_id][0])) { + if ($call_map[$mapping_function_id][0]) { + $mapped_function_return = Type::parseString($call_map[$mapping_function_id][0]); + + if ($mapping_return_type) { + $mapping_return_type = Type::combineUnionTypes( + $mapping_return_type, + $mapped_function_return + ); + } else { + $mapping_return_type = $mapped_function_return; + } + } + } else { + if (strpos($mapping_function_id, '::') !== false) { + $return_type = MethodChecker::getMethodReturnType( + $project_checker, + $mapping_function_id + ) ?: Type::getMixed(); + + if ($mapping_return_type) { + $mapping_return_type = Type::combineUnionTypes( + $mapping_return_type, + $return_type + ); + } else { + $mapping_return_type = $return_type; + } + } else { + $function_storage = FunctionChecker::getStorage( + $statements_checker, + $mapping_function_id + ); + + $return_type = $function_storage->return_type ?: Type::getMixed(); + + if ($mapping_return_type) { + $mapping_return_type = Type::combineUnionTypes( + $mapping_return_type, + $return_type + ); + } else { + $mapping_return_type = $return_type; + } + } } } - // @todo handle array_map('some_custom_function', $arr) + + if ($mapping_return_type) { + return new Type\Union([ + new Type\Atomic\TArray([ + Type::getInt(), + $mapping_return_type, + ]), + ]); + } } } diff --git a/src/Psalm/Checker/FunctionLikeChecker.php b/src/Psalm/Checker/FunctionLikeChecker.php index e9b0235af..36a9d92f4 100644 --- a/src/Psalm/Checker/FunctionLikeChecker.php +++ b/src/Psalm/Checker/FunctionLikeChecker.php @@ -325,8 +325,7 @@ abstract class FunctionLikeChecker extends SourceChecker implements StatementsSo } else { // Closure $file_storage = $file_storage_provider->get($this->source->getFilePath()); - $function_id = $cased_function_id = - $this->getFilePath() . ':' . $this->function->getLine() . ':' . 'closure'; + $function_id = $cased_function_id = $this->getMethodId(); if (!isset($file_storage->functions[$function_id])) { throw new \UnexpectedValueException('Closure function ' . $function_id . ' should exist'); @@ -640,7 +639,7 @@ abstract class FunctionLikeChecker extends SourceChecker implements StatementsSo /** * @param string|null $context_self * - * @return null|string + * @return string */ public function getMethodId($context_self = null) { @@ -655,7 +654,7 @@ abstract class FunctionLikeChecker extends SourceChecker implements StatementsSo strtolower($this->function->name); } - return null; + return $this->getFilePath() . ':' . $this->function->getLine() . ':' . 'closure'; } /** diff --git a/src/Psalm/Checker/Statements/Expression/CallChecker.php b/src/Psalm/Checker/Statements/Expression/CallChecker.php index 7b45409db..200a78bb0 100644 --- a/src/Psalm/Checker/Statements/Expression/CallChecker.php +++ b/src/Psalm/Checker/Statements/Expression/CallChecker.php @@ -247,9 +247,8 @@ class CallChecker } $function_exists = FunctionChecker::functionExists( - $project_checker, - strtolower($method_id), - $statements_checker->getFilePath() + $statements_checker, + strtolower($method_id) ); } else { $function_exists = true; @@ -258,9 +257,8 @@ class CallChecker if ($function_exists) { if (!$in_call_map || $is_stubbed) { $function_storage = FunctionChecker::getStorage( - $project_checker, - strtolower($method_id), - $statements_checker->getFilePath() + $statements_checker, + strtolower($method_id) ); $function_params = $function_storage->params; @@ -363,6 +361,7 @@ class CallChecker } } else { $stmt->inferredType = FunctionChecker::getReturnTypeFromCallMapWithArgs( + $statements_checker, $method_id, $stmt->args, $code_location, @@ -1867,6 +1866,10 @@ class CallChecker ? $closure_arg->value->inferredType : null; + $file_checker = $statements_checker->getFileChecker(); + + $project_checker = $file_checker->project_checker; + if ($closure_arg_type) { $min_closure_param_count = $max_closure_param_count = count($array_arg_types); @@ -1910,8 +1913,6 @@ class CallChecker $i = 0; - $project_checker = $statements_checker->getFileChecker()->project_checker; - foreach ($closure_params as $closure_param) { if (!isset($array_arg_types[$i])) { ++$i; @@ -2153,44 +2154,51 @@ class CallChecker )) { return false; } - } elseif ($input_expr instanceof PhpParser\Node\Scalar\String_) { + } elseif ($input_expr instanceof PhpParser\Node\Scalar\String_ + || $input_expr instanceof PhpParser\Node\Expr\Array_ + ) { foreach ($param_type->types as $param_type_part) { if ($param_type_part instanceof TCallable) { - $function_name = $input_expr->value; + $function_ids = self::getFunctionIdsFromCallableArg( + $statements_checker, + $input_expr + ); - if (strpos($function_name, '::') !== false) { - list($callable_fq_class_name) = explode('::', $function_name); + foreach ($function_ids as $function_id) { + if (strpos($function_id, '::') !== false) { + list($callable_fq_class_name) = explode('::', $function_id); - if ($callable_fq_class_name !== 'parent' && $callable_fq_class_name !== 'self') { - if (ClassLikeChecker::checkFullyQualifiedClassLikeName( + if ($callable_fq_class_name !== 'parent' && $callable_fq_class_name !== 'self') { + if (ClassLikeChecker::checkFullyQualifiedClassLikeName( + $project_checker, + $callable_fq_class_name, + $code_location, + $statements_checker->getSuppressedIssues() + ) === false + ) { + return false; + } + + if (MethodChecker::checkMethodExists( + $project_checker, + $function_id, + $code_location, + $statements_checker->getSuppressedIssues() + ) === false + ) { + return false; + } + } + } else { + if (self::checkFunctionExists( $project_checker, - $callable_fq_class_name, - $code_location, - $statements_checker->getSuppressedIssues() + $statements_checker, + $function_id, + $code_location ) === false ) { return false; } - - if (MethodChecker::checkMethodExists( - $project_checker, - $function_name, - $code_location, - $statements_checker->getSuppressedIssues() - ) === false - ) { - return false; - } - } - } else { - if (self::checkFunctionExists( - $project_checker, - $statements_checker, - $function_name, - $code_location - ) === false - ) { - return false; } } } @@ -2200,6 +2208,62 @@ class CallChecker return null; } + /** + * @param PhpParser\Node\Scalar\String_|PhpParser\Node\Expr\Array_ $callable_arg + * + * @return string[] + */ + public static function getFunctionIdsFromCallableArg( + StatementsChecker $statements_checker, + $callable_arg + ) { + if ($callable_arg instanceof PhpParser\Node\Scalar\String_) { + return [$callable_arg->value]; + } + + if (count($callable_arg->items) !== 2) { + return []; + } + + $class_arg = $callable_arg->items[0]->value; + $method_name_arg = $callable_arg->items[1]->value; + + if (!$method_name_arg instanceof PhpParser\Node\Scalar\String_) { + return []; + } + + if ($class_arg instanceof PhpParser\Node\Scalar\String_) { + return [$class_arg->value . '::' . $method_name_arg->value]; + } + + if ($class_arg instanceof PhpParser\Node\Expr\ClassConstFetch + && is_string($class_arg->name) + && strtolower($class_arg->name) === 'class' + && $class_arg->class instanceof PhpParser\Node\Name + ) { + $fq_class_name = ClassLikeChecker::getFQCLNFromNameObject( + $class_arg->class, + $statements_checker->getAliases() + ); + + return [$fq_class_name . '::' . $method_name_arg->value]; + } + + if (!isset($class_arg->inferredType) || !$class_arg->inferredType->hasObjectType()) { + return []; + } + + $method_ids = []; + + foreach ($class_arg->inferredType->types as $type_part) { + if ($type_part instanceof TNamedObject) { + $method_ids[] = $type_part . '::' . $method_name_arg->value; + } + } + + return $method_ids; + } + /** * @param StatementsChecker $statements_checker * @param string $function_id @@ -2216,34 +2280,25 @@ class CallChecker $cased_function_id = $function_id; $function_id = strtolower($function_id); - if (!FunctionChecker::functionExists($project_checker, $function_id, $statements_checker->getFilePath())) { + if (!FunctionChecker::functionExists($statements_checker, $function_id)) { $root_function_id = preg_replace('/.*\\\/', '', $function_id); if ($function_id !== $root_function_id && - FunctionChecker::functionExists( - $project_checker, - $root_function_id, - $statements_checker->getFilePath() - ) + FunctionChecker::functionExists($statements_checker, $root_function_id) ) { $function_id = $root_function_id; } else { - $existing_function_checkers = $statements_checker->getFunctionCheckers(); - - // check whether it was defined inline - if (!isset($existing_function_checkers[$function_id])) { - if (IssueBuffer::accepts( - new UndefinedFunction( - 'Function ' . $cased_function_id . ' does not exist', - $code_location - ), - $statements_checker->getSuppressedIssues() - )) { - // fall through - } - - return false; + if (IssueBuffer::accepts( + new UndefinedFunction( + 'Function ' . $cased_function_id . ' does not exist', + $code_location + ), + $statements_checker->getSuppressedIssues() + )) { + // fall through } + + return false; } } diff --git a/src/Psalm/Checker/StatementsChecker.php b/src/Psalm/Checker/StatementsChecker.php index 295dfc13c..275262cd9 100644 --- a/src/Psalm/Checker/StatementsChecker.php +++ b/src/Psalm/Checker/StatementsChecker.php @@ -214,9 +214,8 @@ class StatementsChecker extends SourceChecker implements StatementsSource $method_id = $this->function_checkers[$function_id]->getMethodId(); $function_storage = FunctionChecker::getStorage( - $project_checker, - $method_id, - $this->getFilePath() + $this, + $method_id ); $return_type = $function_storage->return_type; diff --git a/tests/ClosureTest.php b/tests/ClosureTest.php index 26141a157..48a84c6fc 100644 --- a/tests/ClosureTest.php +++ b/tests/ClosureTest.php @@ -113,7 +113,51 @@ class ClosureTest extends TestCase public function foo(callable $c) : void {} - foo("A::bar");', + foo("A::bar"); + foo(["A", "bar"]); + foo([A::class, "bar"]); + $a = new A(); + foo([$a, "bar"]);', + ], + 'arrayMapCallableMethod' => [ + ' [ + '$a' => 'array', + '$b' => 'array', + '$c' => 'array', + '$d' => 'array', + '$e' => 'array', + '$f' => 'array', + ], + ], + 'arrayCallableMethod' => [ + ' [ 'project_checker); $file_checker->visitAndAnalyzeMethods($context); - foreach ($assertions as $var => $expected) { - $this->assertSame($expected, (string)$context->vars_in_scope[$var]); + $actual_vars = []; + foreach ($assertions as $var => $_) { + $actual_vars[$var] = (string)$context->vars_in_scope[$var]; } + + $this->assertSame($assertions, $actual_vars); } }