From 649802651cfc969fd18ab38afd51b5016f11593c Mon Sep 17 00:00:00 2001 From: AndrolGenhald Date: Fri, 7 Jan 2022 16:16:36 -0600 Subject: [PATCH] Add support for references and improve UnusedVariable checks (fixes #7254). --- config.xsd | 1 + .../ReferenceReusedFromConfusingScope.md | 39 ++ src/Psalm/Codebase.php | 4 +- src/Psalm/Context.php | 114 +++++- .../Internal/Analyzer/ClosureAnalyzer.php | 1 + .../Analyzer/FunctionLikeAnalyzer.php | 37 +- .../Statements/Block/ForeachAnalyzer.php | 17 + .../Statements/Block/IfElse/ElseAnalyzer.php | 8 +- .../Block/IfElse/ElseIfAnalyzer.php | 8 +- .../Statements/Block/IfElse/IfAnalyzer.php | 6 + .../Statements/Block/IfElseAnalyzer.php | 2 +- .../Statements/Block/LoopAnalyzer.php | 14 +- .../Assignment/ArrayAssignmentAnalyzer.php | 23 ++ .../InstancePropertyAssignmentAnalyzer.php | 2 +- .../Expression/AssignmentAnalyzer.php | 167 ++++++--- .../Statements/Expression/CallAnalyzer.php | 3 +- .../Fetch/AtomicPropertyFetchAnalyzer.php | 1 + .../Fetch/VariableFetchAnalyzer.php | 30 +- .../Statements/ExpressionAnalyzer.php | 21 +- .../Analyzer/Statements/GlobalAnalyzer.php | 29 +- .../Analyzer/Statements/UnsetAnalyzer.php | 1 + .../Internal/Analyzer/StatementsAnalyzer.php | 9 +- .../Internal/Diff/FileStatementsDiffer.php | 3 +- .../Diff/NamespaceStatementsDiffer.php | 3 +- .../Type/SimpleAssertionReconciler.php | 4 - .../Type/SimpleNegatedAssertionReconciler.php | 3 - .../Type/TemplateStandinTypeReplacer.php | 4 +- .../ReferenceReusedFromConfusingScope.php | 9 + src/Psalm/Type/Union.php | 4 +- tests/AnnotationTest.php | 30 +- .../UnusedVariableManipulationTest.php | 21 +- tests/ReferenceTest.php | 337 ++++++++++++++++++ tests/UnusedVariableTest.php | 155 +++++++- 33 files changed, 954 insertions(+), 156 deletions(-) create mode 100644 docs/running_psalm/issues/ReferenceReusedFromConfusingScope.md create mode 100644 src/Psalm/Issue/ReferenceReusedFromConfusingScope.php create mode 100644 tests/ReferenceTest.php diff --git a/config.xsd b/config.xsd index 46840b0a3a9..9e5d5a24aff 100644 --- a/config.xsd +++ b/config.xsd @@ -403,6 +403,7 @@ + diff --git a/docs/running_psalm/issues/ReferenceReusedFromConfusingScope.md b/docs/running_psalm/issues/ReferenceReusedFromConfusingScope.md new file mode 100644 index 00000000000..b280e4948e2 --- /dev/null +++ b/docs/running_psalm/issues/ReferenceReusedFromConfusingScope.md @@ -0,0 +1,39 @@ +# ReferenceReusedFromConfusingScope + +Emitted when a reference assigned in a potentially confusing scope is reused later. +Since PHP doesn't scope loops and ifs the same way most other languages do, a common issue is the reuse of a variable +declared in such a scope. Usually it doesn't matter, because a reassignment to an already-defined variable will just +reassign it, but if the already-defined variable is a reference it will change the value of the referred to variable. + +```php +taint_flow_graph->addSource($source); - $expr_type->parent_nodes = [ - $source->id => $source, - ]; + $expr_type->parent_nodes[$source->id] = $source; } /** diff --git a/src/Psalm/Context.php b/src/Psalm/Context.php index 78b116ba034..73b559173d9 100644 --- a/src/Psalm/Context.php +++ b/src/Psalm/Context.php @@ -14,6 +14,7 @@ use Psalm\Type\Atomic\DependentType; use Psalm\Type\Atomic\TArray; use Psalm\Type\Union; +use RuntimeException; use function array_keys; use function array_search; @@ -41,6 +42,54 @@ class Context */ public $vars_possibly_in_scope = []; + /** + * Keeps track of how many times a var_in_scope has been referenced. May not be set for all $vars_in_scope. + * + * @var array> + */ + public $referenced_counts = []; + + /** + * Maps references to referenced variables for the current scope. + * With `$b = &$a`, this will contain `['$b' => '$a']`. + * + * All keys and values in this array are guaranteed to be set in $vars_in_scope. + * + * To check if a variable was passed or returned by reference, or + * references an object property or array item, see Union::$by_ref. + * + * @var array + */ + public $references_in_scope = []; + + /** + * Set of references to variables in another scope. These references will be marked as used if they are assigned to. + * + * @var array + */ + public $references_to_external_scope = []; + + /** + * A set of globals that are referenced somewhere. + * Ideally this shouldn't be needed and GlobalAnalyzer should add an edge to the + * DataFlowGraph pointing from the global to its use in another scope, but since that's + * difficult this is used as a workaround to always mark referenced globals as used. + * + * @internal May be removed if GlobalAnalyzer is improved. + * + * @var array + */ + public $referenced_globals = []; + + /** + * A set of references that might still be in scope from a scope likely to cause confusion. This applies + * to references set inside a loop or if statement, since it's easy to forget about PHP's weird scope + * rules, and assinging to a reference will change the referenced variable rather than shadowing it. + * + * @var array + */ + public $references_possibly_from_confusing_scope = []; + /** * Whether or not we're inside the conditional of an if/where etc. * @@ -375,11 +424,6 @@ class Context */ public $has_returned = false; - /** - * @var array - */ - public $vars_from_global = []; - public function __construct(?string $self = null) { $this->self = $self; @@ -458,6 +502,28 @@ public function update( } } + /** + * Updates the list of possible references from a confusing scope, + * such as a reference created in an if and then later reused. + */ + public function updateReferencesPossiblyFromConfusingScope( + Context $confusing_scope_context, + StatementsAnalyzer $statements_analyzer + ): void { + foreach ($confusing_scope_context->references_in_scope + $confusing_scope_context->references_to_external_scope + as $reference_id => $_ + ) { + if (!isset($this->references_in_scope[$reference_id]) + && !isset($this->references_to_external_scope[$reference_id]) + && $reference_location = $statements_analyzer->getFirstAppearance($reference_id) + ) { + $this->references_possibly_from_confusing_scope[$reference_id] = $reference_location; + } + } + $this->references_possibly_from_confusing_scope += + $confusing_scope_context->references_possibly_from_confusing_scope; + } + /** * @param array $new_vars_in_scope * @@ -505,19 +571,39 @@ public static function getNewOrUpdatedVarIds(Context $original_context, Context return $redefined_var_ids; } - public function remove(string $remove_var_id): void + public function remove(string $remove_var_id, bool $removeDescendents = true): void { - unset( - $this->referenced_var_ids[$remove_var_id], - $this->vars_possibly_in_scope[$remove_var_id] - ); - if (isset($this->vars_in_scope[$remove_var_id])) { $existing_type = $this->vars_in_scope[$remove_var_id]; unset($this->vars_in_scope[$remove_var_id]); - $this->removeDescendents($remove_var_id, $existing_type); + if ($removeDescendents) { + $this->removeDescendents($remove_var_id, $existing_type); + } + } + $this->removePossibleReference($remove_var_id); + unset($this->vars_possibly_in_scope[$remove_var_id]); + } + + /** + * Remove a variable from the context which might be a reference to another variable. + * Leaves the variable as possibly-in-scope, unlike remove(). + */ + public function removePossibleReference(string $remove_var_id): void + { + if (isset($this->references_in_scope[$remove_var_id])) { + $reference_count = &$this->referenced_counts[$this->references_in_scope[$remove_var_id]]; + if ($reference_count < 1) { + throw new RuntimeException("Incorrect referenced count found"); + } + --$reference_count; } + unset( + $this->vars_in_scope[$remove_var_id], + $this->referenced_var_ids[$remove_var_id], + $this->references_in_scope[$remove_var_id], + $this->references_to_external_scope[$remove_var_id], + ); } /** @@ -667,7 +753,7 @@ public function removeDescendents( foreach ($this->vars_in_scope as $var_id => $type) { if (preg_match('/' . preg_quote($remove_var_id, '/') . '[\]\[\-]/', $var_id)) { - unset($this->vars_in_scope[$var_id]); + $this->remove($var_id, false); } foreach ($type->getAtomicTypes() as $atomic_type) { @@ -698,7 +784,7 @@ public function removeMutableObjectVars(bool $methods_only = false): void } foreach ($vars_to_remove as $var_id) { - unset($this->vars_in_scope[$var_id], $this->vars_possibly_in_scope[$var_id]); + $this->remove($var_id, false); } $clauses_to_keep = []; diff --git a/src/Psalm/Internal/Analyzer/ClosureAnalyzer.php b/src/Psalm/Internal/Analyzer/ClosureAnalyzer.php index 1b36c9becf3..02c95ada4a4 100644 --- a/src/Psalm/Internal/Analyzer/ClosureAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/ClosureAnalyzer.php @@ -160,6 +160,7 @@ public static function analyzeExpression( if ($use->byRef) { $use_context->vars_in_scope[$use_var_id]->by_ref = true; + $use_context->references_to_external_scope[$use_var_id] = true; } $use_context->vars_possibly_in_scope[$use_var_id] = true; diff --git a/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php b/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php index f371bab0ec3..f6a452bba0b 100644 --- a/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php @@ -763,7 +763,7 @@ public function analyze( foreach ($context->vars_in_scope as $var => $_) { if (strpos($var, '$this->') !== 0 && $var !== '$this') { - unset($context->vars_in_scope[$var]); + $context->removePossibleReference($var); } } @@ -846,10 +846,6 @@ private function checkParamReferences( throw new UnexpectedValueException('$position should not be false here'); } - if ($storage->params[$position]->by_ref) { - continue; - } - if ($storage->params[$position]->promoted_property) { continue; } @@ -976,6 +972,7 @@ private function processParams( $project_analyzer = $statements_analyzer->getProjectAnalyzer(); foreach ($params as $offset => $function_param) { + $function_param_id = '$' . $function_param->name; $signature_type = $function_param->signature_type; $signature_type_location = $function_param->signature_type_location; @@ -1064,7 +1061,7 @@ private function processParams( && !$function_param->type->isBool()) ) { $param_assignment = DataFlowNode::getForAssignment( - '$' . $function_param->name, + $function_param_id, $function_param->location ); @@ -1082,16 +1079,6 @@ private function processParams( $statements_analyzer->data_flow_graph->addPath($type_source, $param_assignment, 'param'); } - if ($function_param->by_ref - && $codebase->find_unused_variables - ) { - $statements_analyzer->data_flow_graph->addPath( - $param_assignment, - new DataFlowNode('variable-use', 'variable use', null), - 'variable-use' - ); - } - if ($storage->variadic) { $this->param_nodes += [$param_assignment->id => $param_assignment]; } @@ -1100,18 +1087,19 @@ private function processParams( } } - $context->vars_in_scope['$' . $function_param->name] = $var_type; - $context->vars_possibly_in_scope['$' . $function_param->name] = true; + $context->vars_in_scope[$function_param_id] = $var_type; + $context->vars_possibly_in_scope[$function_param_id] = true; if ($function_param->by_ref) { - $context->vars_in_scope['$' . $function_param->name]->by_ref = true; + $context->vars_in_scope[$function_param_id]->by_ref = true; + $context->references_to_external_scope[$function_param_id] = true; } $parser_param = $this->function->getParams()[$offset] ?? null; if ($function_param->location) { $statements_analyzer->registerVariable( - '$' . $function_param->name, + $function_param_id, $function_param->location, null ); @@ -1147,7 +1135,7 @@ private function processParams( IssueBuffer::maybeAdd( new MismatchingDocblockParamType( - 'Parameter $' . $function_param->name . ' has wrong type \'' . $param_type . + 'Parameter ' . $function_param_id . ' has wrong type \'' . $param_type . '\', should be \'' . $signature_type . '\'', $function_param->type_location ), @@ -1248,13 +1236,6 @@ private function processParams( } } - 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 - // so that we don't have to do this - $context->hasVariable('$' . $function_param->name); - } - foreach ($function_param->attributes as $attribute) { AttributeAnalyzer::analyze( $this, diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/ForeachAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/ForeachAnalyzer.php index f03f3d0a0b5..31b465a10e6 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/ForeachAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/ForeachAnalyzer.php @@ -298,6 +298,15 @@ public static function analyze( $value_type->by_ref = true; } + if ($stmt->byRef + && $stmt->valueVar instanceof PhpParser\Node\Expr\Variable + && is_string($stmt->valueVar->name) + ) { + // When assigning as reference, it removes any previous + // reference, so it's no longer from a previous confusing scope + unset($foreach_context->references_possibly_from_confusing_scope['$' . $stmt->valueVar->name]); + } + AssignmentAnalyzer::analyze( $statements_analyzer, $stmt->valueVar, @@ -311,6 +320,14 @@ public static function analyze( : [] ); + if ($stmt->byRef + && $stmt->valueVar instanceof PhpParser\Node\Expr\Variable + && is_string($stmt->valueVar->name) + ) { + // TODO support references with destructuring + $foreach_context->references_to_external_scope['$' . $stmt->valueVar->name] = true; + } + foreach ($var_comments as $var_comment) { if (!$var_comment->var_id || !$var_comment->type) { continue; diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/ElseAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/ElseAnalyzer.php index 5944847fdff..dba64ad4ba6 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/ElseAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/ElseAnalyzer.php @@ -97,7 +97,7 @@ public static function analyze( if (preg_match('/' . preg_quote($changed_var_id, '/') . '[\]\[\-]/', $var_id) && !array_key_exists($var_id, $changed_var_ids) ) { - unset($else_context->vars_in_scope[$var_id]); + $else_context->removePossibleReference($var_id); } } } @@ -239,6 +239,12 @@ public static function analyze( $outer_context->mergeExceptions($else_context); } + // Track references set in the else to make sure they aren't reused later + $outer_context->updateReferencesPossiblyFromConfusingScope( + $else_context, + $statements_analyzer + ); + return null; } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/ElseIfAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/ElseIfAnalyzer.php index bac5b65fd84..9fb78a41cc7 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/ElseIfAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/ElseIfAnalyzer.php @@ -255,7 +255,7 @@ function (Clause $c) use ($assigned_in_conditional_var_ids, $elseif_cond_id): Cl && !array_key_exists($var_id, $newly_reconciled_var_ids) && !array_key_exists($var_id, $cond_referenced_var_ids) ) { - unset($elseif_context->vars_in_scope[$var_id]); + $elseif_context->removePossibleReference($var_id); } } } @@ -430,6 +430,12 @@ function (Clause $c) use ($assigned_in_conditional_var_ids, $elseif_cond_id): Cl $if_scope->negated_clauses = []; } + // Track references set in the elseif to make sure they aren't reused later + $outer_context->updateReferencesPossiblyFromConfusingScope( + $elseif_context, + $statements_analyzer + ); + return null; } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/IfAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/IfAnalyzer.php index f5e7e79f5fc..7af37ad3c35 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/IfAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/IfAnalyzer.php @@ -252,6 +252,12 @@ public static function analyze( $outer_context->mergeExceptions($if_context); } + // Track references set in the if to make sure they aren't reused later + $outer_context->updateReferencesPossiblyFromConfusingScope( + $if_context, + $statements_analyzer + ); + return null; } diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php index 221ab6ca49d..f12036fefa7 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php @@ -297,7 +297,7 @@ function (Clause $c) use ($mixed_var_ids, $cond_object_id): Clause { && !array_key_exists($var_id, $changed_var_ids) && !array_key_exists($var_id, $cond_referenced_var_ids) ) { - unset($if_context->vars_in_scope[$var_id]); + $if_context->removePossibleReference($var_id); } } } diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/LoopAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/LoopAnalyzer.php index a5ed096112a..099a581f4a4 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/LoopAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/LoopAnalyzer.php @@ -321,7 +321,7 @@ public static function analyze( // remove vars that were defined in the foreach foreach ($vars_to_remove as $var_id) { - unset($inner_context->vars_in_scope[$var_id]); + $inner_context->removePossibleReference($var_id); } $inner_context->clauses = $pre_loop_context->clauses; @@ -364,7 +364,7 @@ public static function analyze( if (isset($pre_loop_context->vars_in_scope[$var_id])) { $inner_context->vars_in_scope[$var_id] = clone $pre_loop_context->vars_in_scope[$var_id]; } else { - unset($inner_context->vars_in_scope[$var_id]); + $inner_context->removePossibleReference($var_id); } } } @@ -459,7 +459,7 @@ public static function analyze( if (!$does_always_break) { foreach ($loop_scope->loop_parent_context->vars_in_scope as $var_id => $type) { if (!isset($inner_context->vars_in_scope[$var_id])) { - unset($loop_scope->loop_parent_context->vars_in_scope[$var_id]); + $loop_scope->loop_parent_context->removePossibleReference($var_id); continue; } @@ -561,6 +561,12 @@ public static function analyze( $inner_context = $inner_do_context; } + // Track references set in the loop to make sure they aren't reused later + $loop_scope->loop_parent_context->updateReferencesPossiblyFromConfusingScope( + $inner_context, + $statements_analyzer + ); + return null; } @@ -638,7 +644,7 @@ private static function applyPreConditionToLoopContext( if (ExpressionAnalyzer::analyze($statements_analyzer, $pre_condition, $loop_context) === false) { $loop_context->inside_conditional = $was_inside_conditional; - + return []; } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/ArrayAssignmentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/ArrayAssignmentAnalyzer.php index 7652589699b..b4d7c3dace1 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/ArrayAssignmentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/ArrayAssignmentAnalyzer.php @@ -4,6 +4,7 @@ use InvalidArgumentException; use PhpParser; +use PhpParser\Node\Expr\Variable; use Psalm\CodeLocation; use Psalm\Codebase; use Psalm\Context; @@ -46,6 +47,7 @@ use function in_array; use function is_string; use function preg_match; +use function reset; use function strlen; /** @@ -690,6 +692,10 @@ private static function analyzeNestedArrayAssignment( $child_stmt = null; + if (!empty($child_stmts)) { + $root_var = reset($child_stmts)->var; + } + // First go from the root element up, and go as far as we can to figure out what // array types there are while ($child_stmts) { @@ -816,6 +822,23 @@ private static function analyzeNestedArrayAssignment( $parent_var_id = $array_var_id; } + if ($statements_analyzer->data_flow_graph instanceof VariableUseGraph + && $root_var_id !== null + && isset($context->references_to_external_scope[$root_var_id]) + && isset($root_var) && $root_var instanceof Variable && is_string($root_var->name) + && $root_var_id === '$' . $root_var->name + ) { + // Array is a reference to an external scope, mark it as used + $statements_analyzer->data_flow_graph->addPath( + DataFlowNode::getForAssignment( + $root_var_id, + new CodeLocation($statements_analyzer->getSource(), $root_var) + ), + new DataFlowNode('variable-use', 'variable use', null), + 'variable-use' + ); + } + if ($root_var_id && $full_var_id && $child_stmt diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php index 12a09c8d7b7..46449085797 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Assignment/InstancePropertyAssignmentAnalyzer.php @@ -1494,7 +1494,7 @@ private static function analyzeSetCall( $statements_analyzer ); - unset($context->vars_in_scope[$var_id]); + $context->removePossibleReference($var_id); } $old_data_provider = $statements_analyzer->node_data; diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php index 6cb610b2b1e..cca39e58457 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php @@ -49,6 +49,7 @@ use Psalm\Issue\PossiblyNullArrayAccess; use Psalm\Issue\PossiblyUndefinedArrayOffset; use Psalm\Issue\ReferenceConstraintViolation; +use Psalm\Issue\ReferenceReusedFromConfusingScope; use Psalm\Issue\UnnecessaryVarAnnotation; use Psalm\IssueBuffer; use Psalm\Node\Expr\BinaryOp\VirtualBitwiseAnd; @@ -77,6 +78,7 @@ use Psalm\Type\Atomic\TNonEmptyList; use Psalm\Type\Atomic\TNull; use Psalm\Type\Union; +use RuntimeException; use UnexpectedValueException; use function array_filter; @@ -847,20 +849,7 @@ public static function analyzeAssignmentRef( PhpParser\Node\Expr\AssignRef $stmt, Context $context ): bool { - $assignment_type = self::analyze( - $statements_analyzer, - $stmt->var, - $stmt->expr, - null, - $context, - $stmt->getDocComment() - ); - - if ($assignment_type === false) { - return false; - } - - $assignment_type->by_ref = true; + ExpressionAnalyzer::analyze($statements_analyzer, $stmt->expr, $context, false, null, false, true); $lhs_var_id = ExpressionIdentifier::getArrayVarId( $stmt->var, @@ -874,47 +863,86 @@ public static function analyzeAssignmentRef( $statements_analyzer ); - if ($lhs_var_id) { - $context->vars_in_scope[$lhs_var_id] = $assignment_type; - $context->hasVariable($lhs_var_id); - $context->byref_constraints[$lhs_var_id] = new ReferenceConstraint(); - - if ($statements_analyzer->data_flow_graph instanceof VariableUseGraph) { - foreach ($context->vars_in_scope[$lhs_var_id]->parent_nodes as $parent_node) { - $statements_analyzer->data_flow_graph->addPath( - $parent_node, - new DataFlowNode('variable-use', 'variable use', null), - 'variable-use' - ); - } + $doc_comment = $stmt->getDocComment(); + if ($doc_comment) { + try { + $var_comments = CommentAnalyzer::getTypeFromComment( + $doc_comment, + $statements_analyzer->getSource(), + $statements_analyzer->getAliases(), + ); + } catch (IncorrectDocblockException $e) { + IssueBuffer::maybeAdd( + new MissingDocblockType( + $e->getMessage(), + new CodeLocation($statements_analyzer->getSource(), $stmt) + ) + ); + } catch (DocblockParseException $e) { + IssueBuffer::maybeAdd( + new InvalidDocblock( + $e->getMessage(), + new CodeLocation($statements_analyzer->getSource(), $stmt) + ) + ); } - } - - if ($rhs_var_id) { - if (!isset($context->vars_in_scope[$rhs_var_id])) { - $context->vars_in_scope[$rhs_var_id] = Type::getMixed(); + if (!empty($var_comments)) { + IssueBuffer::maybeAdd( + new InvalidDocblock( + "Docblock type cannot be used for reference assignment", + new CodeLocation($statements_analyzer->getSource(), $stmt) + ) + ); } - - $context->byref_constraints[$rhs_var_id] = new ReferenceConstraint(); } - if ($statements_analyzer->data_flow_graph - && $lhs_var_id - && $rhs_var_id - && isset($context->vars_in_scope[$rhs_var_id]) - ) { - $rhs_type = $context->vars_in_scope[$rhs_var_id]; - - $data_flow_graph = $statements_analyzer->data_flow_graph; - - $lhs_location = new CodeLocation($statements_analyzer->getSource(), $stmt->var); + if ($lhs_var_id === null || $rhs_var_id === null) { + return false; + } - $lhs_node = DataFlowNode::getForAssignment($lhs_var_id, $lhs_location); + if (isset($context->references_in_scope[$lhs_var_id])) { + // Decrement old referenced variable's reference count + $reference_count = &$context->referenced_counts[$context->references_in_scope[$lhs_var_id]]; + if ($reference_count < 1) { + throw new RuntimeException("Incorrect referenced count found"); + } + --$reference_count; - foreach ($rhs_type->parent_nodes as $byref_destination_node) { - $data_flow_graph->addPath($lhs_node, $byref_destination_node, '='); + // Remove old reference parent node so previously referenced variable usage doesn't count as reference usage + $old_type = $context->vars_in_scope[$lhs_var_id]; + foreach ($old_type->parent_nodes as $old_parent_node_id => $_) { + if (strpos($old_parent_node_id, "$lhs_var_id-") === 0) { + unset($old_type->parent_nodes[$old_parent_node_id]); + } } } + // When assigning an existing reference as a reference it removes the + // old reference, so it's no longer potentially from a confusing scope. + unset($context->references_possibly_from_confusing_scope[$lhs_var_id]); + + $context->vars_in_scope[$lhs_var_id] = &$context->vars_in_scope[$rhs_var_id]; + $context->hasVariable($lhs_var_id); + $context->references_in_scope[$lhs_var_id] = $rhs_var_id; + $context->referenced_counts[$rhs_var_id] = ($context->referenced_counts[$rhs_var_id] ?? 0) + 1; + if (strpos($rhs_var_id, '[') !== false) { + // Reference to array item, we always consider array items to be an external scope for references + // TODO handle differently so it's detected as unused if the array is unused? + $context->references_to_external_scope[$lhs_var_id] = true; + } + if (strpos($rhs_var_id, '->') !== false) { + // Reference to object property, we always consider object properties to be an external scope for references + // TODO handle differently so it's detected as unused if the object is unused? + $context->references_to_external_scope[$lhs_var_id] = true; + } + + $lhs_location = new CodeLocation($statements_analyzer->getSource(), $stmt->var); + $statements_analyzer->registerVariableAssignment( + $lhs_var_id, + $lhs_location + ); + + $lhs_node = DataFlowNode::getForAssignment($lhs_var_id, $lhs_location); + $context->vars_in_scope[$lhs_var_id]->parent_nodes[$lhs_node->id] = $lhs_node; return true; } @@ -1595,6 +1623,7 @@ private static function analyzeAssignmentToVariable( ): void { if (is_string($assign_var->name)) { if ($var_id) { + $original_type = $context->vars_in_scope[$var_id] ?? null; $context->vars_in_scope[$var_id] = $assign_value_type; $context->vars_possibly_in_scope[$var_id] = true; @@ -1631,30 +1660,50 @@ private static function analyzeAssignmentToVariable( $assign_value_type->by_ref = true; } - if ($assign_value_type->by_ref) { - if ($statements_analyzer->data_flow_graph instanceof VariableUseGraph - && $assign_value_type->parent_nodes + if ($statements_analyzer->data_flow_graph instanceof VariableUseGraph + && $assign_value_type->parent_nodes + ) { + if (isset($context->references_to_external_scope[$var_id]) + || isset($context->references_in_scope[$var_id]) + || isset($context->referenced_counts[$var_id]) && $context->referenced_counts[$var_id] > 0 ) { $location = new CodeLocation($statements_analyzer, $assign_var); - - $byref_node = DataFlowNode::getForAssignment($var_id, $location); - - foreach ($assign_value_type->parent_nodes as $parent_node) { + $assignment_node = DataFlowNode::getForAssignment($var_id, $location); + $parent_nodes = $assign_value_type->parent_nodes; + if ($original_type !== null) { + $parent_nodes += $original_type->parent_nodes; + } + foreach ($parent_nodes as $parent_node) { $statements_analyzer->data_flow_graph->addPath( $parent_node, - new DataFlowNode('variable-use', 'variable use', null), - 'variable-use' + $assignment_node, + '&=' // Normal assignment to reference/referenced variable ); + } + if (isset($context->references_to_external_scope[$var_id])) { + // Mark reference to an external scope as used when a value is assigned to it $statements_analyzer->data_flow_graph->addPath( - $byref_node, - $parent_node, - 'byref-assignment' + $assignment_node, + new DataFlowNode('variable-use', 'variable use', null), + 'variable-use' ); } } } + if (isset($context->references_possibly_from_confusing_scope[$var_id])) { + IssueBuffer::maybeAdd( + new ReferenceReusedFromConfusingScope( + "$var_id is possibly a reference defined at" + . " {$context->references_possibly_from_confusing_scope[$var_id]->getShortSummary()}." + . " Reusing this variable may cause the referenced value to change.", + new CodeLocation($statements_analyzer, $assign_var) + ), + $statements_analyzer->getSuppressedIssues() + ); + } + if ($assign_value_type->getId() === 'bool' && ($assign_value instanceof PhpParser\Node\Expr\BinaryOp || ($assign_value instanceof PhpParser\Node\Expr\BooleanNot diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php index 941d88f7088..e40ce54eaf6 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php @@ -228,8 +228,7 @@ public static function collectSpecialInformation( if ($type->initialized) { $local_vars_in_scope[$var_id] = $context->vars_in_scope[$var_id]; - unset($context->vars_in_scope[$var_id]); - unset($context->vars_possibly_in_scope[$var_id]); + $context->remove($var_id, false); } } elseif ($var_id !== '$this') { $local_vars_in_scope[$var_id] = $context->vars_in_scope[$var_id]; diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php index ee9f642c186..ff04c3f4596 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/AtomicPropertyFetchAnalyzer.php @@ -74,6 +74,7 @@ class AtomicPropertyFetchAnalyzer { /** * @param array $invalid_fetch_types $invalid_fetch_types + * @psalm-suppress ComplexMethod */ public static function analyze( StatementsAnalyzer $statements_analyzer, diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/VariableFetchAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/VariableFetchAnalyzer.php index 3ecde22a7f9..331c818c709 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/VariableFetchAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Fetch/VariableFetchAnalyzer.php @@ -49,6 +49,8 @@ class VariableFetchAnalyzer /** * @param bool $from_global - when used in a global keyword + * @param bool $assigned_to_reference This is set to true when the expression being analyzed + * here is being assigned to another variable by reference. */ public static function analyze( StatementsAnalyzer $statements_analyzer, @@ -57,7 +59,8 @@ public static function analyze( bool $passed_by_reference = false, ?Union $by_ref_type = null, bool $array_assignment = false, - bool $from_global = false + bool $from_global = false, + bool $assigned_to_reference = false ): bool { $project_analyzer = $statements_analyzer->getFileAnalyzer()->project_analyzer; $codebase = $statements_analyzer->getCodebase(); @@ -220,11 +223,16 @@ public static function analyze( if (!isset($context->vars_possibly_in_scope[$var_name]) || !$statements_analyzer->getFirstAppearance($var_name) ) { - if ($array_assignment) { - // if we're in an array assignment, let's assign the variable - // because PHP allows it - - $context->vars_in_scope[$var_name] = Type::getArray(); + if ($array_assignment || $assigned_to_reference) { + if ($array_assignment) { + // if we're in an array assignment, let's assign the variable because PHP allows it + $stmt_type = Type::getArray(); + } else { + // If a variable is assigned by reference to a variable that + // does not exist, they are automatically initialized as `null` + $stmt_type = Type::getNull(); + } + $context->vars_in_scope[$var_name] = $stmt_type; $context->vars_possibly_in_scope[$var_name] = true; // it might have been defined first in another if/else branch @@ -235,6 +243,16 @@ public static function analyze( $context->branch_point ); } + $statements_analyzer->node_data->setType($stmt, $stmt_type); + + if ($assigned_to_reference) { + // Since this variable was created by being assigned to as a reference (ie for + // `$a = &$b` this variable is $b), we need to analyze it as an assignment to null. + AssignmentAnalyzer::analyze($statements_analyzer, $stmt, null, $stmt_type, $context, null); + + // Stop here, we don't want it to be considered possibly undefined like the array case. + return true; + } } elseif (!$context->inside_isset || $statements_analyzer->getSource() instanceof FunctionLikeAnalyzer ) { diff --git a/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php index 552a3ecaedf..69454eb0078 100644 --- a/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php @@ -64,13 +64,18 @@ */ class ExpressionAnalyzer { + /** + * @param bool $assigned_to_reference This is set to true when the expression being analyzed + * here is being assigned to another variable by reference. + */ public static function analyze( StatementsAnalyzer $statements_analyzer, PhpParser\Node\Expr $stmt, Context $context, bool $array_assignment = false, ?Context $global_context = null, - bool $from_stmt = false + bool $from_stmt = false, + bool $assigned_to_reference = false ): bool { $codebase = $statements_analyzer->getCodebase(); @@ -80,7 +85,8 @@ public static function analyze( $context, $array_assignment, $global_context, - $from_stmt + $from_stmt, + $assigned_to_reference ) === false ) { return false; @@ -138,13 +144,18 @@ public static function analyze( return true; } + /** + * @param bool $assigned_to_reference This is set to true when the expression being analyzed + * here is being assigned to another variable by reference. + */ private static function handleExpression( StatementsAnalyzer $statements_analyzer, PhpParser\Node\Expr $stmt, Context $context, bool $array_assignment, ?Context $global_context, - bool $from_stmt + bool $from_stmt, + bool $assigned_to_reference = false ): bool { if ($stmt instanceof PhpParser\Node\Expr\Variable) { return VariableFetchAnalyzer::analyze( @@ -153,7 +164,9 @@ private static function handleExpression( $context, false, null, - $array_assignment + $array_assignment, + false, + $assigned_to_reference ); } diff --git a/src/Psalm/Internal/Analyzer/Statements/GlobalAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/GlobalAnalyzer.php index 1781f242d72..3ef2635c8ff 100644 --- a/src/Psalm/Internal/Analyzer/Statements/GlobalAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/GlobalAnalyzer.php @@ -8,10 +8,12 @@ use Psalm\Internal\Analyzer\FunctionLikeAnalyzer; use Psalm\Internal\Analyzer\Statements\Expression\Fetch\VariableFetchAnalyzer; use Psalm\Internal\Analyzer\StatementsAnalyzer; +use Psalm\Internal\Codebase\VariableUseGraph; use Psalm\Internal\DataFlow\DataFlowNode; use Psalm\Internal\ReferenceConstraint; use Psalm\Issue\InvalidGlobal; use Psalm\IssueBuffer; +use RuntimeException; use function is_string; @@ -61,6 +63,7 @@ public static function analyze( $context->byref_constraints[$var_id] = new ReferenceConstraint(); } + $assignment_node = DataFlowNode::getForAssignment( $var_id, new CodeLocation($statements_analyzer, $var) @@ -68,7 +71,17 @@ public static function analyze( $context->vars_in_scope[$var_id]->parent_nodes = [ $assignment_node->id => $assignment_node, ]; - $context->vars_from_global[$var_id] = true; + $context->references_to_external_scope[$var_id] = true; + + if (isset($context->references_in_scope[$var_id])) { + // Global shadows existing reference + $reference_count = &$context->referenced_counts[$context->references_in_scope[$var_id]]; + if ($reference_count < 1) { + throw new RuntimeException("Incorrect referenced count found"); + } + --$reference_count; + unset($context->references_in_scope[$var_id]); + } $statements_analyzer->registerVariable( $var_id, new CodeLocation($statements_analyzer, $var), @@ -79,6 +92,20 @@ public static function analyze( $var, $var_id ); + + if ($statements_analyzer->data_flow_graph instanceof VariableUseGraph) { + if ($global_context !== null && $global_context->hasVariable($var_id)) { + // TODO get global CodeLocation so that global variable can be marked as unused if it is + // unused inside the function. Marking it as a referenced global causes it to be marked + // used if it's used as a global in any function, even if it's unused in that function. + $global_context->referenced_globals[$var_id] = true; + // $statements_analyzer->data_flow_graph->addPath( + // $assignment_node, + // $global_node, + // 'byref-assignment' + // ); + } + } } } } diff --git a/src/Psalm/Internal/Analyzer/Statements/UnsetAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/UnsetAnalyzer.php index b11ce758a63..d580f6c9938 100644 --- a/src/Psalm/Internal/Analyzer/Statements/UnsetAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/UnsetAnalyzer.php @@ -47,6 +47,7 @@ public static function analyze( if ($var_id) { $context->remove($var_id); + unset($context->references_possibly_from_confusing_scope[$var_id]); } if ($var instanceof PhpParser\Node\Expr\ArrayDimFetch && $var->dim) { diff --git a/src/Psalm/Internal/Analyzer/StatementsAnalyzer.php b/src/Psalm/Internal/Analyzer/StatementsAnalyzer.php index 10ea738065a..3eb0c6b95d0 100644 --- a/src/Psalm/Internal/Analyzer/StatementsAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/StatementsAnalyzer.php @@ -131,9 +131,9 @@ class StatementsAnalyzer extends SourceAnalyzer private $unused_var_locations = []; /** - * @var ?array + * @var array */ - public $byref_uses; + public $byref_uses = []; /** * @var ParsedDocblock|null @@ -214,7 +214,6 @@ public function analyze( && $codebase->find_unused_variables && $context->check_variables ) { - //var_dump($this->data_flow_graph); $this->checkUnreferencedVars($stmts, $context); } @@ -790,7 +789,7 @@ public function checkUnreferencedVars(array $stmts, Context $context): void $assignment_node = DataFlowNode::getForAssignment($var_id, $original_location); if (!isset($this->byref_uses[$var_id]) - && !isset($context->vars_from_global[$var_id]) + && !isset($context->referenced_globals[$var_id]) && !VariableFetchAnalyzer::isSuperGlobal($var_id) && $this->data_flow_graph instanceof VariableUseGraph && !$this->data_flow_graph->isVariableUsed($assignment_node) @@ -955,7 +954,7 @@ public function getFunctionAnalyzers(): array } /** - * @param array $byref_uses + * @param array $byref_uses */ public function setByRefUses(array $byref_uses): void { diff --git a/src/Psalm/Internal/Diff/FileStatementsDiffer.php b/src/Psalm/Internal/Diff/FileStatementsDiffer.php index 9580d7f634e..d5decb928d9 100644 --- a/src/Psalm/Internal/Diff/FileStatementsDiffer.php +++ b/src/Psalm/Internal/Diff/FileStatementsDiffer.php @@ -34,8 +34,7 @@ function ( PhpParser\Node\Stmt $a, PhpParser\Node\Stmt $b, string $a_code, - string $b_code, - bool &$body_change = false + string $b_code ): bool { if (get_class($a) !== get_class($b)) { return false; diff --git a/src/Psalm/Internal/Diff/NamespaceStatementsDiffer.php b/src/Psalm/Internal/Diff/NamespaceStatementsDiffer.php index 5ac1e8a9dcb..77eec387528 100644 --- a/src/Psalm/Internal/Diff/NamespaceStatementsDiffer.php +++ b/src/Psalm/Internal/Diff/NamespaceStatementsDiffer.php @@ -34,8 +34,7 @@ function ( PhpParser\Node\Stmt $a, PhpParser\Node\Stmt $b, string $a_code, - string $b_code, - bool &$body_change = false + string $b_code ): bool { if (get_class($a) !== get_class($b)) { return false; diff --git a/src/Psalm/Internal/Type/SimpleAssertionReconciler.php b/src/Psalm/Internal/Type/SimpleAssertionReconciler.php index baf3479524c..120aade190f 100644 --- a/src/Psalm/Internal/Type/SimpleAssertionReconciler.php +++ b/src/Psalm/Internal/Type/SimpleAssertionReconciler.php @@ -393,7 +393,6 @@ public static function reconcile( $negated, $code_location, $suppressed_issues, - $failed_reconciliation, $is_equality, null ); @@ -406,7 +405,6 @@ public static function reconcile( $negated, $code_location, $suppressed_issues, - $failed_reconciliation, $is_equality, (int) substr($assertion, 13) ); @@ -532,7 +530,6 @@ private static function reconcileIsset( /** * @param string[] $suppressed_issues - * @param 0|1|2 $failed_reconciliation */ private static function reconcileNonEmptyCountable( Union $existing_var_type, @@ -540,7 +537,6 @@ private static function reconcileNonEmptyCountable( bool $negated, ?CodeLocation $code_location, array $suppressed_issues, - int &$failed_reconciliation, bool $is_equality, ?int $min_count ): Union { diff --git a/src/Psalm/Internal/Type/SimpleNegatedAssertionReconciler.php b/src/Psalm/Internal/Type/SimpleNegatedAssertionReconciler.php index 72b5a11488d..28fbf4b8572 100644 --- a/src/Psalm/Internal/Type/SimpleNegatedAssertionReconciler.php +++ b/src/Psalm/Internal/Type/SimpleNegatedAssertionReconciler.php @@ -323,7 +323,6 @@ public static function reconcile( $negated, $code_location, $suppressed_issues, - $failed_reconciliation, $is_equality, null ); @@ -342,7 +341,6 @@ public static function reconcile( $negated, $code_location, $suppressed_issues, - $failed_reconciliation, $is_equality, (int) substr($assertion, 13) ); @@ -467,7 +465,6 @@ private static function reconcileNonEmptyCountable( bool $negated, ?CodeLocation $code_location, array $suppressed_issues, - int &$failed_reconciliation, bool $is_equality, ?int $min_count ): Union { diff --git a/src/Psalm/Internal/Type/TemplateStandinTypeReplacer.php b/src/Psalm/Internal/Type/TemplateStandinTypeReplacer.php index 939388c3429..81b64f9f434 100644 --- a/src/Psalm/Internal/Type/TemplateStandinTypeReplacer.php +++ b/src/Psalm/Internal/Type/TemplateStandinTypeReplacer.php @@ -213,8 +213,7 @@ private static function handleAtomicStandin( $add_lower_bound, $bound_equality_classlike, $depth, - $was_single, - $had_template + $was_single ); } } @@ -901,7 +900,6 @@ public static function handleTemplateParamClassStandin( ?string $bound_equality_classlike, int $depth, bool $was_single, - bool &$had_template ): array { if ($atomic_type->defining_class === $calling_class) { return [$atomic_type]; diff --git a/src/Psalm/Issue/ReferenceReusedFromConfusingScope.php b/src/Psalm/Issue/ReferenceReusedFromConfusingScope.php new file mode 100644 index 00000000000..31332b98bc0 --- /dev/null +++ b/src/Psalm/Issue/ReferenceReusedFromConfusingScope.php @@ -0,0 +1,9 @@ + [ ' [ ' [ ' [ ' [ + 'SKIPPED-removeUnusedVarAssignByRefToSubsequentlyUsedVariable' => [ ' [ + ' [ ',error_levels?:string[]}> + */ + public function providerValidCodeParse(): iterable + { + return [ + 'updateReferencedTypes' => [ + ' [ + '$a===' => '2', + '$b===' => '4', + '$c===' => '4', + ], + ], + 'SKIPPED-referenceToArrayItemChangesArrayType' => [ + ' */ + $arr = []; + + assert(isset($arr[0])); + $int = &$arr[0]; + $int = (string) $int; + ', + 'assertions' => [ + '$arr' => 'list', + ], + ], + 'referenceToReference' => [ + ' [ + '$a===' => '2', + '$b===' => '3', + '$c===' => '2', + '$d===' => '3', + ], + ], + 'referenceToSubsequentCatch' => [ + ' [ + ' [ + ' [ + '$a===' => 'null', + '$b===' => 'null', + ], + ], + 'referenceShadowedByGlobal' => [ + ' [ + '$a' => 'string', + ], + ], + 'unsetPreventsReferenceConfusion' => [ + ' [ + ' [ + ' + */ + public function providerInvalidCodeParse(): iterable + { + return [ + 'referenceReuseForeachValue' => [ + ' */ + $arr = []; + + foreach ($arr as &$var) { + $var += 1; + } + + $var = "foo"; + ', + 'error_message' => 'ReferenceReusedFromConfusingScope', + ], + 'referenceReuseDeclaredInForeach' => [ + ' */ + $arr = []; + + foreach ($arr as $val) { + $var = &$val; + $var += 1; + } + + $var = "foo"; + ', + 'error_message' => 'ReferenceReusedFromConfusingScope', + ], + 'referenceReuseDeclaredInFor' => [ + ' */ + $arr = []; + + for ($i = 0; $i < count($arr); ++$i) { + $var = &$arr[$i]; + $var += 1; + } + + $var = "foo"; + ', + 'error_message' => 'ReferenceReusedFromConfusingScope', + ], + 'referenceReuseDeclaredInIf' => [ + ' */ + $arr = []; + + if (isset($arr[0])) { + $var = &$arr[0]; + $var += 1; + } + + $var = "foo"; + ', + 'error_message' => 'ReferenceReusedFromConfusingScope', + ], + 'referenceReuseDeclaredInElseif' => [ + ' */ + $arr = []; + + if (random_int(0, 1)) { + } elseif (isset($arr[0])) { + $var = &$arr[0]; + $var += 1; + } + + $var = "foo"; + ', + 'error_message' => 'ReferenceReusedFromConfusingScope', + ], + 'referenceReuseDeclaredInElse' => [ + ' */ + $arr = []; + + if (!isset($arr[0])) { + } else { + $var = &$arr[0]; + $var += 1; + } + + $var = "foo"; + ', + 'error_message' => 'ReferenceReusedFromConfusingScope', + ], + 'referenceReuseDeeplyNested' => [ + '>> */ + $arr = []; + + for ($i = 0; $i < count($arr); ++$i) { + foreach ($arr[$i] as $inner_arr) { + if (isset($inner_arr[0])) { + $var = &$inner_arr[0]; + $var += 1; + } + } + } + + $var = "foo"; + ', + 'error_message' => 'ReferenceReusedFromConfusingScope', + ], + 'referencesIgnoreVarAnnotation' => [ + ' 'InvalidDocblock - src' . DIRECTORY_SEPARATOR . 'somefile.php:4:21 - Docblock type cannot be used for reference assignment', + ], + 'SKIPPED-referenceToTypedArrayConstrainsAssignment' => [ + ' */ + public array $arr = []; + + public function __construct() + { + assert(isset($this->arr[0])); + $int = &$this->arr[0]; + $int = (string) $int; + } + } + ', + 'error_message' => 'ReferenceConstraintViolation', + ], + 'SKIPPED-referenceToTypedArrayConstrainsAssignmentWithNullReferenceInitialization' => [ + ' */ + public array $arr = []; + + public function __construct() + { + $int = &$this->arr[0]; // If $this->arr[0] isn\'t set, this will set it to null. + } + } + ', + 'error_message' => 'PossiblyInvalidPropertyAssignmentValue', + ], + 'unsetOnlyPreventsReferenceConfusionAfterCall' => [ + ' 'ReferenceReusedFromConfusingScope', + ], + 'assignmentAsReferenceOnlyPreventsReferenceConfusionAfterAssignment' => [ + ' 'ReferenceReusedFromConfusingScope', + ], + ]; + } +} diff --git a/tests/UnusedVariableTest.php b/tests/UnusedVariableTest.php index 055be038db8..2c48e2eef52 100644 --- a/tests/UnusedVariableTest.php +++ b/tests/UnusedVariableTest.php @@ -1522,7 +1522,10 @@ function foo(array $arr) : void { } } - /** @param mixed $p */ + /** + * @param mixed $p + * @psalm-suppress UnusedParam + */ function takes_ref(&$p): void {}' ], 'passedByRefArrayOffset' => [ @@ -1952,6 +1955,23 @@ function foo(array &$arr): void { $arr[0] = $b; }' ], + 'byRefDeeplyNestedArrayParam' => [ + '> $arr */ + function foo(array &$arr): void { + $b = 5; + $arr[0][0] = $b; + }' + ], + 'nestedReferencesToByRefParam' => [ + '> $arr */ + function foo(array &$arr): void { + $a = &$arr[0]; + $b = &$a[0]; + $b = 5; + }' + ], 'byRefNestedArrayInForeach' => [ ' [ + ' [ + ' [ + '$b===' => '2', + '$a===' => '2', + ], + ], + 'referenceUsedAfterVariableReassignment' => [ + ' [ + ' [ + ' 'MixedArgumentTypeCoercion - src' . DIRECTORY_SEPARATOR . 'somefile.php:12:44 - Argument 1 of takesArrayOfString expects array, parent type array{mixed} provided. Consider improving the type at src' . DIRECTORY_SEPARATOR . 'somefile.php:10:41' ], + 'referenceReassignmentUnusedVariable' => [ + ' 'UnusedVariable - src' . DIRECTORY_SEPARATOR . 'somefile.php:3:21 - $c', + ], + 'referenceAssignmentIsNotUsed' => [ + ' 'UnusedVariable - src' . DIRECTORY_SEPARATOR . 'somefile.php:2:21 - $a', + ], + 'unusedReferenceToPreviouslyUsedVariable' => [ + ' 'UnusedVariable - src' . DIRECTORY_SEPARATOR . 'somefile.php:4:21 - $b', + ], + 'SKIPPED-unusedReferenceToSubsequentlyUsedVariable' => [ // Not easy to do the way it's currently set up + ' 'UnusedVariable - src' . DIRECTORY_SEPARATOR . 'somefile.php:3:21 - $b', + ], + 'unusedReferenceInForeach' => [ + ' 'UnusedForeachValue', + ], + 'SKIPPED-unusedReferenceInDestructuredForeach' => [ + ' 'UnusedForeachValue', + ], + 'unusedReturnByReference' => [ + ' 'UnusedVariable', + ], + 'unusedPassByReference' => [ + ' 'UnusedParam', + ], + 'SKIPPED-unusedGlobalVariable' => [ + ' 'UnusedVariable - src' . DIRECTORY_SEPARATOR . 'somefile.php:2:21 - $a', + ], + 'unusedUndeclaredGlobalVariable' => [ + ' 'UnusedVariable', + ], ]; } }