diff --git a/src/Psalm/Checker/ClassLikeChecker.php b/src/Psalm/Checker/ClassLikeChecker.php index 1b83a02a8..d74051de5 100644 --- a/src/Psalm/Checker/ClassLikeChecker.php +++ b/src/Psalm/Checker/ClassLikeChecker.php @@ -440,7 +440,19 @@ abstract class ClassLikeChecker extends SourceChecker implements StatementsSourc $method_checker->check(clone $class_context); if (!$config->excludeIssueInFile('InvalidReturnType', $this->file_name)) { - $method_checker->checkReturnTypes($update_docblocks); + /** @var string */ + $method_id = $method_checker->getMethodId(); + $return_type_location = MethodChecker::getMethodReturnTypeLocation( + $method_id, + $secondary_return_type_location + ); + + $method_checker->checkReturnTypes( + $update_docblocks, + MethodChecker::getMethodReturnType($method_id), + $return_type_location, + $secondary_return_type_location + ); } } } @@ -531,9 +543,9 @@ abstract class ClassLikeChecker extends SourceChecker implements StatementsSourc $class_context->self . '::' . $this->getMappedMethodName(strtolower($stmt->name)), $method_id ); - - self::$class_methods[$class_context->self][strtolower($stmt->name)] = true; } + + self::$class_methods[$class_context->self][strtolower($stmt->name)] = true; } /** diff --git a/src/Psalm/Checker/FileChecker.php b/src/Psalm/Checker/FileChecker.php index 357fddf73..18e076e04 100644 --- a/src/Psalm/Checker/FileChecker.php +++ b/src/Psalm/Checker/FileChecker.php @@ -228,7 +228,8 @@ class FileChecker extends SourceChecker implements StatementsSource ); $this->namespace_aliased_classes[$namespace_name] = $namespace_checker->getAliasedClasses(); - $this->namespace_aliased_classes_flipped[$namespace_name] = $namespace_checker->getAliasedClassesFlipped(); + $this->namespace_aliased_classes_flipped[$namespace_name] = + $namespace_checker->getAliasedClassesFlipped(); $this->declared_classes = array_merge($namespace_checker->getDeclaredClasses()); } elseif ($stmt instanceof PhpParser\Node\Stmt\Function_ && $check_functions) { @@ -236,7 +237,24 @@ class FileChecker extends SourceChecker implements StatementsSource $function_checkers[$stmt->name]->check($function_context, $file_context); if (!$config->excludeIssueInFile('InvalidReturnType', $this->file_name)) { - $function_checkers[$stmt->name]->checkReturnTypes(); + /** @var string */ + $method_id = $function_checkers[$stmt->name]->getMethodId(); + + $return_type = FunctionChecker::getFunctionReturnType( + $method_id, + $this->file_name + ); + + $return_type_location = FunctionChecker::getFunctionReturnTypeLocation( + $method_id, + $this->file_name + ); + + $function_checkers[$stmt->name]->checkReturnTypes( + false, + $return_type, + $return_type_location + ); } } } else { @@ -333,7 +351,9 @@ class FileChecker extends SourceChecker implements StatementsSource $stmts = []; $root_cache_directory = Config::getInstance()->getCacheDirectory(); - $parser_cache_directory = $root_cache_directory ? $root_cache_directory . '/' . self::PARSER_CACHE_DIRECTORY : null; + $parser_cache_directory = $root_cache_directory + ? $root_cache_directory . '/' . self::PARSER_CACHE_DIRECTORY + : null; $from_cache = false; $cache_location = null; @@ -347,9 +367,10 @@ class FileChecker extends SourceChecker implements StatementsSource if (self::$file_content_hashes === null) { /** @var array */ - self::$file_content_hashes = $root_cache_directory && is_readable($root_cache_directory . '/' . self::FILE_HASHES) - ? unserialize((string)file_get_contents($root_cache_directory . '/' . self::FILE_HASHES)) - : []; + self::$file_content_hashes = $root_cache_directory && + is_readable($root_cache_directory . '/' . self::FILE_HASHES) + ? unserialize((string)file_get_contents($root_cache_directory . '/' . self::FILE_HASHES)) + : []; } if ($parser_cache_directory) { @@ -390,7 +411,10 @@ class FileChecker extends SourceChecker implements StatementsSource self::$file_content_hashes[$name_cache_key] = $file_content_hash; - file_put_contents($root_cache_directory . '/' . self::FILE_HASHES, serialize(self::$file_content_hashes)); + file_put_contents( + $root_cache_directory . '/' . self::FILE_HASHES, + serialize(self::$file_content_hashes) + ); } } @@ -660,8 +684,12 @@ class FileChecker extends SourceChecker implements StatementsSource if (self::$deleted_files === null) { self::$deleted_files = array_filter( array_keys(self::$file_references), + /** + * @param string $file_name + * @return bool + */ function ($file_name) { - return !file_exists((string)$file_name); + return !file_exists($file_name); } ); } diff --git a/src/Psalm/Checker/FunctionChecker.php b/src/Psalm/Checker/FunctionChecker.php index eaf363d9c..2c60f1c37 100644 --- a/src/Psalm/Checker/FunctionChecker.php +++ b/src/Psalm/Checker/FunctionChecker.php @@ -315,7 +315,7 @@ class FunctionChecker extends FunctionLikeChecker } } - self::$function_return_type_locations[$file_name][$function_id] = + self::$function_return_type_locations[$file_name][$function_id] = $return_type_location ?: false; self::$function_return_types[$file_name][$function_id] = $return_type ?: false; return null; @@ -619,15 +619,13 @@ class FunctionChecker extends FunctionLikeChecker if (isset($call_args[$function_index])) { $function_call_arg = $call_args[$function_index]; - if ($function_call_arg->value instanceof PhpParser\Node\Expr\Closure) { - $closure_yield_types = []; - $closure_return_types = EffectsAnalyser::getReturnTypes( - $function_call_arg->value->stmts, - $closure_yield_types, - true - ); + if ($function_call_arg->value instanceof PhpParser\Node\Expr\Closure && + isset($function_call_arg->value->inferredType) && + $function_call_arg->value->inferredType->types['Closure'] instanceof Type\Fn + ) { + $closure_return_type = $function_call_arg->value->inferredType->types['Closure']->return_type; - if (!$closure_return_types) { + if ($closure_return_type->isVoid()) { IssueBuffer::accepts( new InvalidReturnType( 'No return type could be found in the closure passed to ' . $call_map_key, @@ -640,7 +638,7 @@ class FunctionChecker extends FunctionLikeChecker } if ($call_map_key === 'array_map') { - $inner_type = new Type\Union($closure_return_types); + $inner_type = clone $closure_return_type; return new Type\Union([new Type\Generic('array', [Type::getInt(), $inner_type])]); } diff --git a/src/Psalm/Checker/FunctionLikeChecker.php b/src/Psalm/Checker/FunctionLikeChecker.php index 54cb05848..dabbf3720 100644 --- a/src/Psalm/Checker/FunctionLikeChecker.php +++ b/src/Psalm/Checker/FunctionLikeChecker.php @@ -108,233 +108,311 @@ abstract class FunctionLikeChecker extends SourceChecker implements StatementsSo */ public function check(Context $context, Context $global_context = null) { - if ($function_stmts = $this->function->getStmts()) { - $statements_checker = new StatementsChecker($this); + $function_stmts = $this->function->getStmts() ?: []; - $hash = null; + $statements_checker = new StatementsChecker($this); - if ($this instanceof MethodChecker) { - if (ClassLikeChecker::getThisClass()) { - $hash = $this->getMethodId() . json_encode([ + $hash = null; + + $closure_return_type = null; + $closure_return_type_location = null; + + if ($this instanceof MethodChecker) { + if (ClassLikeChecker::getThisClass()) { + $hash = $this->getMethodId() . json_encode([ + $context->vars_in_scope, + $context->vars_possibly_in_scope + ]); + + // if we know that the function has no effects on vars, we don't bother rechecking + if (isset(self::$no_effects_hashes[$hash])) { + list( $context->vars_in_scope, - $context->vars_possibly_in_scope - ]); + $context->vars_possibly_in_scope + ) = self::$no_effects_hashes[$hash]; - // if we know that the function has no effects on vars, we don't bother rechecking - if (isset(self::$no_effects_hashes[$hash])) { - list( - $context->vars_in_scope, - $context->vars_possibly_in_scope - ) = self::$no_effects_hashes[$hash]; + return null; + } + } elseif ($context->self) { + $context->vars_in_scope['$this'] = new Type\Union([new Type\Atomic($context->self)]); + } - return null; + $function_params = MethodChecker::getMethodParams((string)$this->getMethodId()); + + if ($function_params === null) { + throw new \InvalidArgumentException('Cannot get params for own method'); + } + + $implemented_method_ids = MethodChecker::getOverriddenMethodIds((string)$this->getMethodId()); + + if ($implemented_method_ids) { + $have_emitted = false; + + foreach ($implemented_method_ids as $implemented_method_id) { + if ($have_emitted) { + break; } - } elseif ($context->self) { - $context->vars_in_scope['$this'] = new Type\Union([new Type\Atomic($context->self)]); - } - $function_params = MethodChecker::getMethodParams((string)$this->getMethodId()); + if ($implemented_method_id === 'ArrayObject::__construct') { + continue; + } - if ($function_params === null) { - throw new \InvalidArgumentException('Cannot get params for own method'); - } + $implemented_params = MethodChecker::getMethodParams($implemented_method_id); - $implemented_method_ids = MethodChecker::getOverriddenMethodIds((string)$this->getMethodId()); + if ($implemented_params === null) { + continue; + } - if ($implemented_method_ids) { - $have_emitted = false; + foreach ($implemented_params as $i => $implemented_param) { + if (!isset($function_params[$i])) { + $cased_method_id = MethodChecker::getCasedMethodId((string)$this->getMethodId()); + $parent_method_id = MethodChecker::getCasedMethodId($implemented_method_id); - foreach ($implemented_method_ids as $implemented_method_id) { - if ($have_emitted) { + if (IssueBuffer::accepts( + new MethodSignatureMismatch( + 'Method ' . $cased_method_id .' has fewer arguments than parent method ' . + $parent_method_id, + new CodeLocation($this, $this->function) + ) + )) { + return false; + } + + $have_emitted = true; break; } - if ($implemented_method_id === 'ArrayObject::__construct') { - continue; - } + if ((string)$function_params[$i]->signature_type !== + (string)$implemented_param->signature_type + ) { + $cased_method_id = MethodChecker::getCasedMethodId((string)$this->getMethodId()); + $parent_method_id = MethodChecker::getCasedMethodId($implemented_method_id); - $implemented_params = MethodChecker::getMethodParams($implemented_method_id); - - if ($implemented_params === null) { - continue; - } - - foreach ($implemented_params as $i => $implemented_param) { - if (!isset($function_params[$i])) { - $cased_method_id = MethodChecker::getCasedMethodId((string)$this->getMethodId()); - $parent_method_id = MethodChecker::getCasedMethodId($implemented_method_id); - - if (IssueBuffer::accepts( - new MethodSignatureMismatch( - 'Method ' . $cased_method_id .' has fewer arguments than parent method ' . - $parent_method_id, - new CodeLocation($this, $this->function) - ) - )) { - return false; - } - - $have_emitted = true; - break; + if (IssueBuffer::accepts( + new MethodSignatureMismatch( + 'Argument ' . ($i + 1) . ' of ' . $cased_method_id .' has wrong type \'' . + $function_params[$i]->signature_type . '\', expecting \'' . + $implemented_param->signature_type . '\' as defined by ' . + $parent_method_id, + new CodeLocation($this, $this->function) + ) + )) { + return false; } - if ((string)$function_params[$i]->signature_type !== - (string)$implemented_param->signature_type - ) { - $cased_method_id = MethodChecker::getCasedMethodId((string)$this->getMethodId()); - $parent_method_id = MethodChecker::getCasedMethodId($implemented_method_id); - - if (IssueBuffer::accepts( - new MethodSignatureMismatch( - 'Argument ' . ($i + 1) . ' of ' . $cased_method_id .' has wrong type \'' . - $function_params[$i]->signature_type . '\', expecting \'' . - $implemented_param->signature_type . '\' as defined by ' . - $parent_method_id, - new CodeLocation($this, $this->function) - ) - )) { - return false; - } - - $have_emitted = true; - break; - } - } - } - } - } elseif ($this instanceof FunctionChecker) { - $function_params = FunctionChecker::getParams(strtolower((string)$this->getMethodId()), $this->file_name); - } else { // Closure - $function_params = []; - $function_param_names = []; - - foreach ($this->function->getParams() as $param) { - $param_array = self::getTranslatedParam( - $param, - $this - ); - - $function_params[] = $param_array; - $function_param_names[$param->name] = $param_array->type; - } - - $doc_comment = $this->function->getDocComment(); - - if ($doc_comment) { - try { - $docblock_info = CommentChecker::extractDocblockInfo( - (string)$doc_comment, - $doc_comment->getLine() - ); - } catch (DocblockParseException $e) { - if (IssueBuffer::accepts( - new InvalidDocblock( - 'Invalid type passed in docblock for ' . $this->getMethodId(), - new CodeLocation($this, $this->function, true) - ) - )) { - return false; - } - } - - if ($docblock_info) { - $this->suppressed_issues = $docblock_info->suppress; - - $config = \Psalm\Config::getInstance(); - - if ($config->use_docblock_types) { - if ($docblock_info->return_type) { - $return_type = - Type::parseString( - self::fixUpLocalType( - (string)$docblock_info->return_type, - null, - $this->namespace, - $this->getAliasedClasses() - ) - ); - } - - if ($docblock_info->params) { - $this->improveParamsFromDocblock( - $docblock_info->params, - $function_param_names, - $function_params, - new CodeLocation($this, $this->function, false) - ); - } + $have_emitted = true; + break; } } } } + } elseif ($this instanceof FunctionChecker) { + $function_params = FunctionChecker::getParams(strtolower((string)$this->getMethodId()), $this->file_name); + } else { // Closure + $function_params = []; + $function_param_names = []; - foreach ($function_params as $function_param) { - $param_type = ExpressionChecker::fleshOutTypes( - clone $function_param->type, - [], - $context->self, - $this->getMethodId() + foreach ($this->function->getParams() as $param) { + $param_array = self::getTranslatedParam( + $param, + $this ); - if (!$function_param->code_location) { - throw new \UnexpectedValueException('We should know where this code is'); + $function_params[] = $param_array; + $function_param_names[$param->name] = $param_array->type; + } + + $doc_comment = $this->function->getDocComment(); + + if ($this->function->returnType) { + $parser_return_type = $this->function->returnType; + + $suffix = ''; + + if ($parser_return_type instanceof PhpParser\Node\NullableType) { + $suffix = '|null'; + $parser_return_type = $parser_return_type->type; } - foreach ($param_type->types as $atomic_type) { - if ($atomic_type->isObjectType() - && !$atomic_type->isObject() - && $this->function instanceof PhpParser\Node - && ClassLikeChecker::checkFullyQualifiedClassLikeName( - $atomic_type->value, - $function_param->code_location, - $this->suppressed_issues - ) === false - ) { + $closure_return_type = Type::parseString( + (is_string($parser_return_type) + ? $parser_return_type + : ClassLikeChecker::getFQCLNFromNameObject( + $parser_return_type, + $this->namespace, + $this->getAliasedClasses() + ) + ) . $suffix + ); + + $closure_return_type_location = new CodeLocation( + $this->getSource(), + $this->function, + false, + self::RETURN_TYPE_REGEX + ); + } + + if ($doc_comment) { + try { + $docblock_info = CommentChecker::extractDocblockInfo( + (string)$doc_comment, + $doc_comment->getLine() + ); + } catch (DocblockParseException $e) { + if (IssueBuffer::accepts( + new InvalidDocblock( + 'Invalid type passed in docblock for ' . $this->getMethodId(), + new CodeLocation($this, $this->function, true) + ) + )) { return false; } } - $context->vars_in_scope['$' . $function_param->name] = $param_type; + if ($docblock_info) { + $this->suppressed_issues = $docblock_info->suppress; - $statements_checker->registerVariable($function_param->name, $function_param->code_location->line_number); - } + $config = \Psalm\Config::getInstance(); - $statements_checker->check($function_stmts, $context, null, $global_context); + if ($config->use_docblock_types) { + if ($docblock_info->return_type) { + $closure_return_type = + Type::parseString( + self::fixUpLocalType( + (string)$docblock_info->return_type, + null, + $this->namespace, + $this->getAliasedClasses() + ) + ); - if (isset($this->return_vars_in_scope[''])) { - $context->vars_in_scope = TypeChecker::combineKeyedTypes( - $context->vars_in_scope, - $this->return_vars_in_scope[''] - ); - } + if (!$closure_return_type_location) { + $closure_return_type_location = new CodeLocation( + $this->getSource(), + $this->function, + true + ); + } - if (isset($this->return_vars_possibly_in_scope[''])) { - $context->vars_possibly_in_scope = array_merge( - $context->vars_possibly_in_scope, - $this->return_vars_possibly_in_scope[''] - ); - } + $closure_return_type_location->setCommentLine($docblock_info->return_type_line_number); + } - foreach ($context->vars_in_scope as $var => $type) { - if (strpos($var, '$this->') !== 0) { - unset($context->vars_in_scope[$var]); + if ($docblock_info->params) { + $this->improveParamsFromDocblock( + $docblock_info->params, + $function_param_names, + $function_params, + new CodeLocation($this, $this->function, false) + ); + } + } } } - foreach ($context->vars_possibly_in_scope as $var => $type) { - if (strpos($var, '$this->') !== 0) { - unset($context->vars_possibly_in_scope[$var]); + $this->function->inferredType = new Type\Union([ + new Type\Fn( + 'Closure', + array_values($function_param_names), + $closure_return_type ?: Type::getMixed() + ) + ]); + } + + foreach ($function_params as $function_param) { + $param_type = ExpressionChecker::fleshOutTypes( + clone $function_param->type, + [], + $context->self, + $this->getMethodId() + ); + + if (!$function_param->code_location) { + throw new \UnexpectedValueException('We should know where this code is'); + } + + foreach ($param_type->types as $atomic_type) { + if ($atomic_type->isObjectType() + && !$atomic_type->isObject() + && $this->function instanceof PhpParser\Node + && ClassLikeChecker::checkFullyQualifiedClassLikeName( + $atomic_type->value, + $function_param->code_location, + $this->suppressed_issues + ) === false + ) { + return false; } } - if ($hash && ClassLikeChecker::getThisClass() && $this instanceof MethodChecker) { - self::$no_effects_hashes[$hash] = [ - $context->vars_in_scope, - $context->vars_possibly_in_scope - ]; + $context->vars_in_scope['$' . $function_param->name] = $param_type; + + $statements_checker->registerVariable( + $function_param->name, + $function_param->code_location->line_number + ); + } + + $statements_checker->check($function_stmts, $context, null, $global_context); + + if ($this->function instanceof Closure) { + $closure_yield_types = []; + + $this->checkReturnTypes( + false, + $closure_return_type, + $closure_return_type_location + ); + + if (!$closure_return_type || $closure_return_type->isMixed()) { + $closure_yield_types = []; + $closure_return_types = EffectsAnalyser::getReturnTypes( + $this->function->stmts, + $closure_yield_types, + true + ); + + if ($closure_return_types) { + $this->function->inferredType->types['Closure']->return_type = + new Type\Union($closure_return_types); + } } } + + if (isset($this->return_vars_in_scope[''])) { + $context->vars_in_scope = TypeChecker::combineKeyedTypes( + $context->vars_in_scope, + $this->return_vars_in_scope[''] + ); + } + + if (isset($this->return_vars_possibly_in_scope[''])) { + $context->vars_possibly_in_scope = array_merge( + $context->vars_possibly_in_scope, + $this->return_vars_possibly_in_scope[''] + ); + } + + foreach ($context->vars_in_scope as $var => $type) { + if (strpos($var, '$this->') !== 0) { + unset($context->vars_in_scope[$var]); + } + } + + foreach ($context->vars_possibly_in_scope as $var => $type) { + if (strpos($var, '$this->') !== 0) { + unset($context->vars_possibly_in_scope[$var]); + } + } + + if ($hash && ClassLikeChecker::getThisClass() && $this instanceof MethodChecker) { + self::$no_effects_hashes[$hash] = [ + $context->vars_in_scope, + $context->vars_possibly_in_scope + ]; + } + return null; } @@ -457,12 +535,23 @@ abstract class FunctionLikeChecker extends SourceChecker implements StatementsSo } /** - * @param bool $update_docblock + * @param bool $update_docblock + * @param Type\Union|null $return_type + * @param CodeLocation|null $return_type_location + * @param CodeLocation|null $secondary_return_type_location * @return false|null */ - public function checkReturnTypes($update_docblock = false) - { - if (!$this->function->getStmts()) { + public function checkReturnTypes( + $update_docblock = false, + Type\Union $return_type = null, + CodeLocation $return_type_location = null, + CodeLocation $secondary_return_type_location = null + ) { + if (!$this->function->getStmts() && + ($this->function instanceof ClassMethod && + ($this->getSource() instanceof InterfaceChecker || $this->function->isAbstract()) + ) + ) { return null; } @@ -474,21 +563,6 @@ abstract class FunctionLikeChecker extends SourceChecker implements StatementsSo $method_id = (string)$this->getMethodId(); $cased_method_id = $this instanceof MethodChecker ? MethodChecker::getCasedMethodId($method_id) : $method_id; - $secondary_return_type_location = null; - - if ($this instanceof MethodChecker) { - $return_type = MethodChecker::getMethodReturnType($method_id); - $return_type_location = MethodChecker::getMethodReturnTypeLocation($method_id, $secondary_return_type_location); - } else { - try { - $return_type = FunctionChecker::getFunctionReturnType($method_id, $this->file_name); - $return_type_location = FunctionChecker::getFunctionReturnTypeLocation($method_id, $this->file_name); - } catch (\Exception $e) { - $return_type = null; - $return_type_location = null; - } - } - if (!$return_type_location) { $return_type_location = new CodeLocation($this, $this->function, true); } @@ -530,8 +604,16 @@ abstract class FunctionLikeChecker extends SourceChecker implements StatementsSo $this->file_name, $this->function->getLine(), (string)$this->function->getDocComment(), - $inferred_return_type->toNamespacedString($this->getAliasedClassesFlipped(), $this->getFQCLN(), false), - $inferred_return_type->toNamespacedString($this->getAliasedClassesFlipped(), $this->getFQCLN(), true) + $inferred_return_type->toNamespacedString( + $this->getAliasedClassesFlipped(), + $this->getFQCLN(), + false + ), + $inferred_return_type->toNamespacedString( + $this->getAliasedClassesFlipped(), + $this->getFQCLN(), + true + ) ); } @@ -601,8 +683,16 @@ abstract class FunctionLikeChecker extends SourceChecker implements StatementsSo $this->file_name, $this->function->getLine(), (string)$this->function->getDocComment(), - $inferred_return_type->toNamespacedString($this->getAliasedClassesFlipped(), $this->getFQCLN(), false), - $inferred_return_type->toNamespacedString($this->getAliasedClassesFlipped(), $this->getFQCLN(), true) + $inferred_return_type->toNamespacedString( + $this->getAliasedClassesFlipped(), + $this->getFQCLN(), + false + ), + $inferred_return_type->toNamespacedString( + $this->getAliasedClassesFlipped(), + $this->getFQCLN(), + true + ) ); } diff --git a/src/Psalm/Checker/Statements/Block/TryChecker.php b/src/Psalm/Checker/Statements/Block/TryChecker.php index 34cc9fd02..8d7a133e8 100644 --- a/src/Psalm/Checker/Statements/Block/TryChecker.php +++ b/src/Psalm/Checker/Statements/Block/TryChecker.php @@ -59,6 +59,7 @@ class TryChecker array_map( /** * @param string $fq_catch_class + * @return Type\Atomic */ function ($fq_catch_class) { return new Type\Atomic($fq_catch_class); diff --git a/src/Psalm/Checker/Statements/Expression/CallChecker.php b/src/Psalm/Checker/Statements/Expression/CallChecker.php index dd4b99ca6..b207cf97f 100644 --- a/src/Psalm/Checker/Statements/Expression/CallChecker.php +++ b/src/Psalm/Checker/Statements/Expression/CallChecker.php @@ -118,6 +118,15 @@ class CallChecker } if (self::checkFunctionArguments( + $statements_checker, + $stmt->args, + $method_id, + $context + ) === false) { + // fall through + } + + if (self::checkFunctionArgumentsMatch( $statements_checker, $stmt->args, $method_id, @@ -225,13 +234,22 @@ class CallChecker $method_id = $fq_class_name . '::__construct'; if (self::checkFunctionArguments( + $statements_checker, + $stmt->args, + $method_id, + $context + ) === false) { + return false; + } + + if (self::checkFunctionArgumentsMatch( $statements_checker, $stmt->args, $method_id, $context, new CodeLocation($statements_checker->getSource(), $stmt) ) === false) { - return false; + // fall through } if ($fq_class_name === 'ArrayIterator' && isset($stmt->args[0]->value->inferredType)) { @@ -499,6 +517,15 @@ class CallChecker } if (self::checkFunctionArguments( + $statements_checker, + $stmt->args, + $method_id, + $context + ) === false) { + return false; + } + + if (self::checkFunctionArgumentsMatch( $statements_checker, $stmt->args, $method_id, @@ -692,6 +719,15 @@ class CallChecker } if (self::checkFunctionArguments( + $statements_checker, + $stmt->args, + $method_id, + $context + ) === false) { + return false; + } + + if (self::checkFunctionArgumentsMatch( $statements_checker, $stmt->args, $method_id, @@ -711,24 +747,16 @@ class CallChecker * @param array $args * @param string|null $method_id * @param Context $context - * @param CodeLocation $code_location - * @param boolean $is_mock * @return false|null */ protected static function checkFunctionArguments( StatementsChecker $statements_checker, array $args, $method_id, - Context $context, - CodeLocation $code_location, - $is_mock = false + Context $context ) { $function_params = null; - $is_variadic = false; - - $fq_class_name = null; - $in_call_map = $method_id ? FunctionChecker::inCallMap($method_id) : false; if ($method_id) { @@ -737,13 +765,6 @@ class CallChecker $args, $statements_checker->getFileName() ); - - if ($in_call_map || !strpos($method_id, '::')) { - $is_variadic = FunctionChecker::isVariadic(strtolower($method_id), $statements_checker->getFileName()); - } else { - $fq_class_name = explode('::', $method_id)[0]; - $is_variadic = $is_mock || MethodChecker::isVariadic($method_id); - } } foreach ($args as $argument_offset => $arg) { @@ -826,13 +847,48 @@ class CallChecker } } + return null; + } + + /** + * @param StatementsChecker $statements_checker + * @param array $args + * @param string|null $method_id + * @param Context $context + * @param CodeLocation $code_location + * @param boolean $is_mock + * @return false|null + */ + protected static function checkFunctionArgumentsMatch( + StatementsChecker $statements_checker, + array $args, + $method_id, + Context $context, + CodeLocation $code_location, + $is_mock = false + ) { // we need to do this calculation after the above vars have already processed $function_params = $method_id ? FunctionLikeChecker::getParamsById($method_id, $args, $statements_checker->getFileName()) : []; + $in_call_map = $method_id ? FunctionChecker::inCallMap($method_id) : false; + $cased_method_id = $method_id; + $is_variadic = false; + + $fq_class_name = null; + + if ($method_id) { + if ($in_call_map || !strpos($method_id, '::')) { + $is_variadic = FunctionChecker::isVariadic(strtolower($method_id), $statements_checker->getFileName()); + } else { + $fq_class_name = explode('::', $method_id)[0]; + $is_variadic = $is_mock || MethodChecker::isVariadic($method_id); + } + } + if ($method_id && strpos($method_id, '::') && !$in_call_map) { $cased_method_id = MethodChecker::getCasedMethodId($method_id); } @@ -902,70 +958,37 @@ class CallChecker : null; } - /** @var PhpParser\Node\Expr\Closure|null */ - $closure_arg = isset($args[$closure_index]) && - $args[$closure_index]->value instanceof PhpParser\Node\Expr\Closure - ? $args[$closure_index]->value + /** @var PhpParser\Node\Arg */ + $closure_arg = isset($args[$closure_index]) ? $args[$closure_index] : null; + + /** @var Type\Union|null */ + $closure_arg_type = $closure_arg && isset($closure_arg->value->inferredType) + ? $closure_arg->value->inferredType : null; - if ($closure_arg) { + if ($closure_arg_type) { $expected_closure_param_count = $method_id === 'array_filter' ? 1 : count($array_arg_types); - if (count($closure_arg->params) > $expected_closure_param_count) { - if (IssueBuffer::accepts( - new TooManyArguments( - 'Too many arguments in closure for ' . ($cased_method_id ?: $method_id), - new CodeLocation($statements_checker->getSource(), $closure_arg) - ), - $statements_checker->getSuppressedIssues() - )) { - return false; - } - } elseif (count($closure_arg->params) < $expected_closure_param_count) { - if (IssueBuffer::accepts( - new TooFewArguments( - 'You must supply a param in the closure for ' . ($cased_method_id ?: $method_id), - new CodeLocation($statements_checker->getSource(), $closure_arg) - ), - $statements_checker->getSuppressedIssues() - )) { - return false; - } - } - - foreach ($closure_arg->params as $i => $closure_param) { - if (!$array_arg_types[$i]) { + foreach ($closure_arg_type->types as $closure_type) { + if (!$closure_type instanceof Type\Fn) { continue; } - /** @var Type\Generic */ - $array_arg_type = $array_arg_types[$i]; - - $translated_param = FunctionLikeChecker::getTranslatedParam( - $closure_param, - $statements_checker->getSource() - ); - - $param_type = $translated_param->type; - $input_type = $array_arg_type->type_params[1]; - - if ($input_type->isMixed()) { - continue; - } - - $type_match_found = FunctionLikeChecker::doesParamMatch( - $input_type, - $param_type, - $scalar_type_match_found, - $coerced_type - ); - - if ($coerced_type) { + if (count($closure_type->parameters) > $expected_closure_param_count) { if (IssueBuffer::accepts( - new TypeCoercion( - 'First parameter of closure passed to function ' . $cased_method_id . ' expects ' . - $param_type . ', parent type ' . $input_type . ' provided', - new CodeLocation($statements_checker->getSource(), $closure_param) + new TooManyArguments( + 'Too many arguments in closure for ' . ($cased_method_id ?: $method_id), + new CodeLocation($statements_checker->getSource(), $closure_arg) + ), + $statements_checker->getSuppressedIssues() + )) { + return false; + } + } elseif (count($closure_type->parameters) < $expected_closure_param_count) { + if (IssueBuffer::accepts( + new TooFewArguments( + 'You must supply a param in the closure for ' . ($cased_method_id ?: $method_id), + new CodeLocation($statements_checker->getSource(), $closure_arg) ), $statements_checker->getSuppressedIssues() )) { @@ -973,27 +996,65 @@ class CallChecker } } - if (!$type_match_found) { - if ($scalar_type_match_found) { + $closure_params = $closure_type->parameters; + $closure_return_type = $closure_type->return_type; + + foreach ($closure_params as $i => $closure_param_type) { + if (!$array_arg_types[$i]) { + continue; + } + + /** @var Type\Generic */ + $array_arg_type = $array_arg_types[$i]; + + $input_type = $array_arg_type->type_params[1]; + + if ($input_type->isMixed()) { + continue; + } + + $type_match_found = FunctionLikeChecker::doesParamMatch( + $input_type, + $closure_param_type, + $scalar_type_match_found, + $coerced_type + ); + + if ($coerced_type) { if (IssueBuffer::accepts( - new InvalidScalarArgument( + new TypeCoercion( 'First parameter of closure passed to function ' . $cased_method_id . ' expects ' . - $param_type . ', ' . $input_type . ' provided', - new CodeLocation($statements_checker->getSource(), $closure_param) + $closure_param_type . ', parent type ' . $input_type . ' provided', + new CodeLocation($statements_checker->getSource(), $closure_arg) + ), + $statements_checker->getSuppressedIssues() + )) { + return false; + } + } + + if (!$type_match_found) { + if ($scalar_type_match_found) { + if (IssueBuffer::accepts( + new InvalidScalarArgument( + 'First parameter of closure passed to function ' . $cased_method_id . ' expects ' . + $closure_param_type . ', ' . $input_type . ' provided', + new CodeLocation($statements_checker->getSource(), $closure_arg) + ), + $statements_checker->getSuppressedIssues() + )) { + return false; + } + } elseif (IssueBuffer::accepts( + new InvalidArgument( + 'First parameter of closure passed to function ' . $cased_method_id . ' expects ' . + $closure_param_type . ', ' . $input_type . ' provided', + new CodeLocation($statements_checker->getSource(), $closure_arg) ), $statements_checker->getSuppressedIssues() )) { return false; } - } elseif (IssueBuffer::accepts( - new InvalidArgument( - 'First parameter of closure passed to function ' . $cased_method_id . ' expects ' . - $param_type . ', ' . $input_type . ' provided', - new CodeLocation($statements_checker->getSource(), $closure_param) - ), - $statements_checker->getSuppressedIssues() - )) { - return false; } } } @@ -1038,8 +1099,6 @@ class CallChecker } } } - - return null; } /** diff --git a/src/Psalm/Checker/Statements/ExpressionChecker.php b/src/Psalm/Checker/Statements/ExpressionChecker.php index 277e540f2..f2c26ddb7 100644 --- a/src/Psalm/Checker/Statements/ExpressionChecker.php +++ b/src/Psalm/Checker/Statements/ExpressionChecker.php @@ -231,7 +231,9 @@ class ExpressionChecker $closure_checker->check($use_context); - $stmt->inferredType = Type::getClosure(); + if (!isset($stmt->inferredType)) { + $stmt->inferredType = Type::getClosure(); + } } elseif ($stmt instanceof PhpParser\Node\Expr\ArrayDimFetch) { if (FetchChecker::checkArrayAccess( $statements_checker, diff --git a/src/Psalm/Checker/TypeChecker.php b/src/Psalm/Checker/TypeChecker.php index eb45aa380..5dd161178 100644 --- a/src/Psalm/Checker/TypeChecker.php +++ b/src/Psalm/Checker/TypeChecker.php @@ -1450,9 +1450,16 @@ class TypeChecker return ['mixed']; } - $array_types = array_filter($all_types, function ($type) { - return preg_match('/^array(\<|$)/', (string)$type); - }); + $array_types = array_filter( + $all_types, + /** + * @param string $type + * @return bool + */ + function ($type) { + return (bool)preg_match('/^array(\<|$)/', $type); + } + ); $all_types = array_flip($all_types); @@ -1476,6 +1483,10 @@ class TypeChecker public static function negateTypes(array $types) { return array_map( + /** + * @param string $type + * @return string + */ function ($type) { if ($type === 'mixed') { return $type; @@ -1513,6 +1524,10 @@ class TypeChecker $simple_declared_types = array_filter( array_keys($declared_type->types), + /** + * @param string $type_value + * @return bool + */ function ($type_value) { return $type_value !== 'null'; } @@ -1520,6 +1535,10 @@ class TypeChecker $simple_inferred_types = array_filter( array_keys($inferred_type->types), + /** + * @param string $type_value + * @return bool + */ function ($type_value) { return $type_value !== 'null'; } diff --git a/src/Psalm/Type.php b/src/Psalm/Type.php index 039576476..210cf5bfe 100644 --- a/src/Psalm/Type.php +++ b/src/Psalm/Type.php @@ -105,6 +105,9 @@ abstract class Type $generic_type = array_shift($parse_tree->children); $generic_params = array_map( + /** + * @return Union + */ function (ParseTree $child_tree) { $tree_type = self::getTypeFromTree($child_tree); return $tree_type instanceof Union ? $tree_type : new Union([$tree_type]); @@ -137,8 +140,19 @@ abstract class Type if ($parse_tree->value === ParseTree::UNION) { $union_types = array_map( + /** + * @return Atomic + */ function (ParseTree $child_tree) { - return self::getTypeFromTree($child_tree); + $atomic_type = self::getTypeFromTree($child_tree); + + if (!$atomic_type instanceof Atomic) { + throw new \UnexpectedValueException( + 'Was expecting an atomic type, got ' . get_class($atomic_type) + ); + } + + return $atomic_type; }, $parse_tree->children ); @@ -224,6 +238,9 @@ abstract class Type $class_chars = '[a-zA-Z\<\>\\\\_]+'; return preg_replace_callback( '/(' . $class_chars . '|' . '\((' . $class_chars . '(\|' . $class_chars . ')*' . ')\))((\[\])+)/', + /** + * @return string + */ function (array $matches) { $inner_type = str_replace(['(', ')'], '', (string)$matches[1]); diff --git a/src/Psalm/Type/Fn.php b/src/Psalm/Type/Fn.php new file mode 100644 index 000000000..0ab6ad368 --- /dev/null +++ b/src/Psalm/Type/Fn.php @@ -0,0 +1,33 @@ + + */ + public $parameters = []; + + /** + * @var Union + */ + public $return_type; + + /** + * Constructs a new instance of a generic type + * + * @param string $value + * @param array $parameters + * @param Union $return_type + */ + public function __construct($value, array $parameters, Union $return_type) + { + $this->parameters = $parameters; + $this->return_type = $return_type; + } +} diff --git a/src/Psalm/Type/Generic.php b/src/Psalm/Type/Generic.php index 62d4fc92c..83098909f 100644 --- a/src/Psalm/Type/Generic.php +++ b/src/Psalm/Type/Generic.php @@ -30,6 +30,9 @@ class Generic extends Atomic implode( ', ', array_map( + /** + * @return string + */ function (Union $type_param) { return (string)$type_param; }, @@ -72,6 +75,9 @@ class Generic extends Atomic implode( ', ', array_map( + /** + * @return string + */ function (Union $type_param) use ($aliased_classes, $this_class) { return $type_param->toNamespacedString($aliased_classes, $this_class, false); }, diff --git a/src/Psalm/Type/ObjectLike.php b/src/Psalm/Type/ObjectLike.php index bd000e952..cd56da47c 100644 --- a/src/Psalm/Type/ObjectLike.php +++ b/src/Psalm/Type/ObjectLike.php @@ -34,6 +34,9 @@ class ObjectLike extends Atomic implode( ', ', array_map( + /** + * @return string + */ function ($name, $type) { return $name . ':' . $type; }, @@ -61,6 +64,9 @@ class ObjectLike extends Atomic implode( ', ', array_map( + /** + * @return string + */ function ($name, Union $type) use ($aliased_classes, $this_class, $use_phpdoc_format) { return $name . ':' . $type->toNamespacedString($aliased_classes, $this_class, $use_phpdoc_format); }, diff --git a/src/Psalm/Type/Union.php b/src/Psalm/Type/Union.php index a20bbc3e0..bbf424c1b 100644 --- a/src/Psalm/Type/Union.php +++ b/src/Psalm/Type/Union.php @@ -33,8 +33,12 @@ class Union extends Type return implode( '|', array_map( + /** + * @param string $type + * @return string + */ function ($type) { - return (string)$type; + return $type; }, $this->types ) @@ -52,6 +56,9 @@ class Union extends Type return implode( '|', array_map( + /** + * @return string + */ function (Atomic $type) use ($aliased_classes, $this_class, $use_phpdoc_format) { return $type->toNamespacedString($aliased_classes, $this_class, $use_phpdoc_format); }, diff --git a/tests/ClosureTest.php b/tests/ClosureTest.php index bc438f96f..b18d84f14 100644 --- a/tests/ClosureTest.php +++ b/tests/ClosureTest.php @@ -38,6 +38,9 @@ class ClosureTest extends PHPUnit_Framework_TestCase /** @return void */ function fn() { run_function( + /** + * @return void + */ function() use(&$data) { $data = 1; } @@ -63,7 +66,7 @@ class ClosureTest extends PHPUnit_Framework_TestCase $bar = ["foo", "bar"]; $bam = array_map( - function(int $a) { + function(int $a) : int { return $a + 1; }, $bar @@ -74,4 +77,46 @@ class ClosureTest extends PHPUnit_Framework_TestCase $context = new Context('somefile.php'); $file_checker->check(true, true, $context); } + + /** + * @expectedException \Psalm\Exception\CodeException + * @expectedExceptionMessage InvalidReturnType + */ + public function testNoReturn() + { + $stmts = self::$parser->parse('check(true, true, $context); + } + + public function testInferredArg() + { + $stmts = self::$parser->parse('check(true, true, $context); + } } diff --git a/tests/InterfaceTest.php b/tests/InterfaceTest.php index 1dae83762..c4b467fde 100644 --- a/tests/InterfaceTest.php +++ b/tests/InterfaceTest.php @@ -38,6 +38,9 @@ class InterfaceTest extends PHPUnit_Framework_TestCase interface B { + /** + * @return string + */ public function bar(); } @@ -53,14 +56,17 @@ class InterfaceTest extends PHPUnit_Framework_TestCase { public function foo() { + return "hello"; } public function bar() { + return "goodbye"; } public function baz() { + return "hello again"; } } @@ -99,13 +105,19 @@ class InterfaceTest extends PHPUnit_Framework_TestCase { public function foo() { + return "hello"; } public function baz() { + return "goodbye"; } } + /** + * @param A $a + * @return void + */ function qux(A $a) { } diff --git a/tests/Php40Test.php b/tests/Php40Test.php index 6fba8687d..702f79e6e 100644 --- a/tests/Php40Test.php +++ b/tests/Php40Test.php @@ -29,8 +29,11 @@ class Php40Test extends PHPUnit_Framework_TestCase { $stmts = self::$parser->parse('parse('parse(' */ + /** + * @return array + */ function f(int ...$a_list) { - return array_map(function (int $a) { - return $a + 1; - }, $a_list); + return array_map( + /** + * @return int + */ + function (int $a) { + return $a + 1; + }, + $a_list + ); } f(1); diff --git a/tests/ReturnTypeTest.php b/tests/ReturnTypeTest.php index b5280cdc1..fa8de428e 100644 --- a/tests/ReturnTypeTest.php +++ b/tests/ReturnTypeTest.php @@ -475,9 +475,9 @@ class ReturnTypeTest extends PHPUnit_Framework_TestCase public function testOverrideReturnTypeInGrandparent() { $stmts = self::$parser->parse('parse('parse('parse('