From 472cdf6bea1c6a2c427bf19d8861a5118baf1814 Mon Sep 17 00:00:00 2001 From: Matthew Brown Date: Sun, 3 Mar 2019 15:11:09 -0500 Subject: [PATCH] Fix #1379 - allow @param-out to change type --- .../Analyzer/FunctionLikeAnalyzer.php | 48 ++++++++++++++++--- .../Statements/Expression/CallAnalyzer.php | 14 ++++-- .../Fetch/VariableFetchAnalyzer.php | 2 +- .../Statements/ExpressionAnalyzer.php | 5 +- .../Analyzer/Statements/ReturnAnalyzer.php | 2 +- src/Psalm/Internal/Analyzer/TypeAnalyzer.php | 2 - tests/AnnotationTest.php | 16 ------- tests/ReferenceConstraintTest.php | 34 +++++++++++++ 8 files changed, 91 insertions(+), 32 deletions(-) diff --git a/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php b/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php index 8db7c18b5..a76ef60c7 100644 --- a/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php @@ -19,6 +19,7 @@ use Psalm\Issue\MismatchingDocblockParamType; use Psalm\Issue\MissingClosureParamType; use Psalm\Issue\MissingParamType; use Psalm\Issue\MissingThrowsDocblock; +use Psalm\Issue\ReferenceConstraintViolation; use Psalm\Issue\ReservedWord; use Psalm\Issue\UnusedParam; use Psalm\IssueBuffer; @@ -540,11 +541,6 @@ abstract class FunctionLikeAnalyzer extends SourceAnalyzer implements Statements } } - if ($function_param->by_ref) { - $context->byref_constraints['$' . $function_param->name] - = new \Psalm\Internal\ReferenceConstraint(!$param_type->hasMixed() ? $param_type : null); - } - if ($function_param->by_ref) { // register by ref params as having been used, to avoid false positives // @todo change the assignment analysis *just* for byref params @@ -598,7 +594,7 @@ abstract class FunctionLikeAnalyzer extends SourceAnalyzer implements Statements $statements_analyzer->analyze($function_stmts, $context, $global_context, true); - $this->addPossibleParamTypes($context, $codebase); + $this->examineParamTypes($statements_analyzer, $context, $codebase); foreach ($storage->params as $offset => $function_param) { // only complain if there's no type defined by a parent type @@ -981,7 +977,7 @@ abstract class FunctionLikeAnalyzer extends SourceAnalyzer implements Statements /** * @return void */ - public function addPossibleParamTypes(Context $context, Codebase $codebase) + public function examineParamTypes(StatementsAnalyzer $statements_analyzer, Context $context, Codebase $codebase) { if ($context->infer_types) { foreach ($context->possible_param_types as $var_id => $type) { @@ -995,6 +991,44 @@ abstract class FunctionLikeAnalyzer extends SourceAnalyzer implements Statements $this->possible_param_types[$var_id] = clone $type; } } + } else { + $storage = $this->getFunctionLikeStorage($statements_analyzer); + + foreach ($storage->params as $i => $param) { + if ($param->by_ref && isset($context->vars_in_scope['$' . $param->name])) { + $actual_type = $context->vars_in_scope['$' . $param->name]; + $param_out_type = $param->type; + + if (isset($storage->param_out_types[$i])) { + $param_out_type = $storage->param_out_types[$i]; + } + + if ($param_out_type && !$actual_type->isMixed() && $param->location) { + if (!TypeAnalyzer::isContainedBy( + $codebase, + $actual_type, + $param_out_type, + $actual_type->ignore_nullable_issues, + $actual_type->ignore_falsable_issues + ) + ) { + if (IssueBuffer::accepts( + new ReferenceConstraintViolation( + 'Variable ' . '$' . $param->name . ' is limited to values of type ' + . $param_out_type->getId() + . ' because it is passed by reference, ' + . $actual_type->getId() . ' type found. Use @param-out to specify ' + . 'a different output type', + $param->location + ), + $statements_analyzer->getSuppressedIssues() + )) { + // fall through + } + } + } + } + } } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php index de3ab4ad7..b10320d7f 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php @@ -588,6 +588,7 @@ class CallAnalyzer $statements_analyzer, $arg->value, $by_ref_type, + $by_ref_type, $context, false ); @@ -701,6 +702,7 @@ class CallAnalyzer $statements_analyzer, $array_arg, $by_ref_type, + $by_ref_type, $context, false ); @@ -804,6 +806,7 @@ class CallAnalyzer $statements_analyzer, $array_arg, $by_ref_type, + $by_ref_type, $context, false ); @@ -811,10 +814,13 @@ class CallAnalyzer return; } + $array_type = Type::getArray(); + ExpressionAnalyzer::assignByRefParam( $statements_analyzer, $array_arg, - Type::getArray(), + $array_type, + $array_type, $context, false ); @@ -1157,7 +1163,7 @@ class CallAnalyzer $codebase = $statements_analyzer->getCodebase(); if (!isset($arg->value->inferredType)) { - if ($function_param) { + if ($function_param && !$function_param->by_ref) { $codebase->analyzer->incrementMixedCount($statements_analyzer->getFilePath()); $param_type = $function_param->type; @@ -1270,6 +1276,7 @@ class CallAnalyzer true )) { $by_ref_type = null; + $by_ref_out_type = null; $check_null_ref = true; @@ -1282,7 +1289,7 @@ class CallAnalyzer } if (isset($function_storage->param_out_types[$argument_offset])) { - $by_ref_type = $function_storage->param_out_types[$argument_offset]; + $by_ref_out_type = $function_storage->param_out_types[$argument_offset]; } } else { $by_ref_type = $last_param->type; @@ -1322,6 +1329,7 @@ class CallAnalyzer $statements_analyzer, $arg->value, $by_ref_type, + $by_ref_out_type ?: $by_ref_type, $context, $method_id && (strpos($method_id, '::') !== false || !CallMap::inCallMap($method_id)), $check_null_ref diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/VariableFetchAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/VariableFetchAnalyzer.php index 5e5d37d61..53c2351da 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/VariableFetchAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/VariableFetchAnalyzer.php @@ -166,7 +166,7 @@ class VariableFetchAnalyzer } if ($passed_by_reference && $by_ref_type) { - ExpressionAnalyzer::assignByRefParam($statements_analyzer, $stmt, $by_ref_type, $context); + ExpressionAnalyzer::assignByRefParam($statements_analyzer, $stmt, $by_ref_type, $by_ref_type, $context); return null; } diff --git a/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php index b40aa17ba..16c68a6cf 100644 --- a/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php @@ -644,6 +644,7 @@ class ExpressionAnalyzer StatementsAnalyzer $statements_analyzer, PhpParser\Node\Expr $stmt, Type\Union $by_ref_type, + Type\Union $by_ref_out_type, Context $context, bool $constrain_type = true, bool $prevent_null = false @@ -712,7 +713,7 @@ class ExpressionAnalyzer } } - $context->vars_in_scope[$var_id] = $by_ref_type; + $context->vars_in_scope[$var_id] = $by_ref_out_type; if (!isset($stmt->inferredType) || $stmt->inferredType->isEmpty()) { $stmt->inferredType = clone $by_ref_type; @@ -1294,7 +1295,7 @@ class ExpressionAnalyzer if ($source instanceof FunctionLikeAnalyzer && !($source->getSource() instanceof TraitAnalyzer) ) { - $source->addPossibleParamTypes($context, $codebase); + $source->examineParamTypes($statements_analyzer, $context, $codebase); $storage = $source->getFunctionLikeStorage($statements_analyzer); diff --git a/src/Psalm/Internal/Analyzer/Statements/ReturnAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/ReturnAnalyzer.php index a98767a68..4d00a907a 100644 --- a/src/Psalm/Internal/Analyzer/Statements/ReturnAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/ReturnAnalyzer.php @@ -124,7 +124,7 @@ class ReturnAnalyzer $context ); - $source->addPossibleParamTypes($context, $codebase); + $source->examineParamTypes($statements_analyzer, $context, $codebase); $storage = $source->getFunctionLikeStorage($statements_analyzer); diff --git a/src/Psalm/Internal/Analyzer/TypeAnalyzer.php b/src/Psalm/Internal/Analyzer/TypeAnalyzer.php index 6a16af575..4dc2b882f 100644 --- a/src/Psalm/Internal/Analyzer/TypeAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/TypeAnalyzer.php @@ -1529,8 +1529,6 @@ class TypeAnalyzer * @param bool &$all_types_contain * * @return null|false - * - * @psalm-suppress ConflictingReferenceConstraint */ private static function compareCallable( Codebase $codebase, diff --git a/tests/AnnotationTest.php b/tests/AnnotationTest.php index 8d7fa2813..2a4168e73 100644 --- a/tests/AnnotationTest.php +++ b/tests/AnnotationTest.php @@ -827,22 +827,6 @@ class AnnotationTest extends TestCase } }' ], - 'paramOutChangeType' => [ - ' [ ' [ + ' [ + ' [ + '$a' => 'int', + ], + ], ]; }