From f9450656e1666fbf6ceb8b419b2d02d065fb76b5 Mon Sep 17 00:00:00 2001 From: AndrolGenhald Date: Fri, 7 Jan 2022 16:16:36 -0600 Subject: [PATCH 1/9] 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 | 10 +- .../Type/SimpleNegatedAssertionReconciler.php | 3 - .../Type/TemplateStandinTypeReplacer.php | 2 - .../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, 956 insertions(+), 158 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 634b3de26a1..e03e0b62369 100644 --- a/config.xsd +++ b/config.xsd @@ -413,6 +413,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 eba6e9a445c..5d624e17bbb 100644 --- a/src/Psalm/Context.php +++ b/src/Psalm/Context.php @@ -13,6 +13,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; @@ -40,6 +41,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. * @@ -362,11 +411,6 @@ class Context */ public $has_returned = false; - /** - * @var array - */ - public $vars_from_global = []; - /** * @var array */ @@ -449,6 +493,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 * @@ -496,19 +562,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], + ); } /** @@ -645,7 +731,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) { @@ -676,7 +762,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 aabb0f43f1b..cbed8f768f3 100644 --- a/src/Psalm/Internal/Analyzer/ClosureAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/ClosureAnalyzer.php @@ -159,6 +159,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 d329e7f8cc9..8e767dd32b1 100644 --- a/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php @@ -771,7 +771,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); } } @@ -854,10 +854,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; } @@ -984,6 +980,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; @@ -1087,7 +1084,7 @@ private function processParams( && !$function_param->type->isBool()) ) { $param_assignment = DataFlowNode::getForAssignment( - '$' . $function_param->name, + $function_param_id, $function_param->location ); @@ -1105,16 +1102,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]; } @@ -1123,18 +1110,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 ); @@ -1170,7 +1158,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 ), @@ -1271,13 +1259,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 ad7ba5c7dcc..2b0be2e173a 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); } } } @@ -243,6 +243,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 0a5ec037aea..ff10838d32a 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); } } } @@ -434,6 +434,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 51b8e80dd3d..c33eb147fac 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/IfAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/IfAnalyzer.php @@ -254,6 +254,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 a3bf7c90045..837873c3a3a 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 13683b2ac7a..c49155bad48 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/LoopAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/LoopAnalyzer.php @@ -317,7 +317,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; @@ -360,7 +360,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); } } } @@ -453,7 +453,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; } @@ -555,6 +555,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; } @@ -630,7 +636,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 976fafa45ce..f6d948d557a 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; @@ -78,6 +79,7 @@ use Psalm\Type\Atomic\TNonEmptyList; use Psalm\Type\Atomic\TNull; use Psalm\Type\Union; +use RuntimeException; use UnexpectedValueException; use function array_filter; @@ -852,20 +854,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, null, true); $lhs_var_id = ExpressionIdentifier::getArrayVarId( $stmt->var, @@ -879,47 +868,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; } @@ -1600,6 +1628,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; @@ -1636,30 +1665,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 df54b95d64f..4142828c744 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php @@ -227,8 +227,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 b0ffb87d273..39bfe8d2f8b 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 a1a4483b717..a33d2e3c973 100644 --- a/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php @@ -65,6 +65,10 @@ */ 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, @@ -72,7 +76,8 @@ public static function analyze( bool $array_assignment = false, ?Context $global_context = null, bool $from_stmt = false, - ?TemplateResult $template_result = null + ?TemplateResult $template_result = null, + bool $assigned_to_reference = false ): bool { $codebase = $statements_analyzer->getCodebase(); @@ -83,7 +88,8 @@ public static function analyze( $array_assignment, $global_context, $from_stmt, - $template_result + $template_result, + $assigned_to_reference, ) === false) { return false; } @@ -140,6 +146,10 @@ 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, @@ -147,7 +157,8 @@ private static function handleExpression( bool $array_assignment, ?Context $global_context, bool $from_stmt, - ?TemplateResult $template_result = null + ?TemplateResult $template_result = null, + bool $assigned_to_reference = false ): bool { if ($stmt instanceof PhpParser\Node\Expr\Variable) { return VariableFetchAnalyzer::analyze( @@ -156,7 +167,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 952d8c5ac3f..2881e5273bf 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 b2e1753500e..7ce33e72eaf 100644 --- a/src/Psalm/Internal/Type/SimpleAssertionReconciler.php +++ b/src/Psalm/Internal/Type/SimpleAssertionReconciler.php @@ -241,8 +241,7 @@ public static function reconcile( $negated, $code_location, $suppressed_issues, - $failed_reconciliation, - $is_equality + $is_equality, ); } @@ -254,8 +253,7 @@ public static function reconcile( $negated, $code_location, $suppressed_issues, - $failed_reconciliation, - $is_equality + $is_equality, ); } @@ -434,7 +432,7 @@ public static function reconcile( $code_location, $suppressed_issues, $failed_reconciliation, - $is_equality + $is_equality, ); } @@ -560,7 +558,6 @@ private static function reconcileIsset( /** * @param string[] $suppressed_issues - * @param Reconciler::RECONCILIATION_* $failed_reconciliation */ private static function reconcileNonEmptyCountable( Assertion $assertion, @@ -569,7 +566,6 @@ private static function reconcileNonEmptyCountable( bool $negated, ?CodeLocation $code_location, array $suppressed_issues, - int &$failed_reconciliation, bool $is_equality ): Union { $old_var_type_string = $existing_var_type->getId(); diff --git a/src/Psalm/Internal/Type/SimpleNegatedAssertionReconciler.php b/src/Psalm/Internal/Type/SimpleNegatedAssertionReconciler.php index 548ec187b79..fc8d6b62c98 100644 --- a/src/Psalm/Internal/Type/SimpleNegatedAssertionReconciler.php +++ b/src/Psalm/Internal/Type/SimpleNegatedAssertionReconciler.php @@ -204,7 +204,6 @@ public static function reconcile( $negated, $code_location, $suppressed_issues, - $failed_reconciliation, $is_equality, null ); @@ -218,7 +217,6 @@ public static function reconcile( $negated, $code_location, $suppressed_issues, - $failed_reconciliation, $is_equality, $assertion->count ); @@ -510,7 +508,6 @@ private static function reconcileNotNonEmptyCountable( 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 c3e336edc4e..67bc813020f 100644 --- a/src/Psalm/Internal/Type/TemplateStandinTypeReplacer.php +++ b/src/Psalm/Internal/Type/TemplateStandinTypeReplacer.php @@ -214,7 +214,6 @@ private static function handleAtomicStandin( $bound_equality_classlike, $depth, $was_single, - $had_template ); } } @@ -896,7 +895,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 @@ + [ 'code' => ' [ 'code' => ' [ 'code' => ' [ 'code' => ' true, ], - 'removeUnusedVarAssignByRef' => [ + 'SKIPPED-removeUnusedVarAssignByRefToSubsequentlyUsedVariable' => [ 'input' => ' ' '7.1', @@ -356,6 +355,20 @@ function foo() : void { 'safe_types' => true, ], + 'removeUnusedVarAssignByRef' => [ + 'input' => ' ' '7.1', + 'issues_to_fix' => ['UnusedVariable'], + 'safe_types' => true, + ], + 'removeUnusedVarAssignByRefPartial' => [ 'input' => ' ' '7.1', @@ -384,7 +397,7 @@ function foo() : void { 'output' => ' '7.1', diff --git a/tests/ReferenceTest.php b/tests/ReferenceTest.php new file mode 100644 index 00000000000..09a0f6d2a74 --- /dev/null +++ b/tests/ReferenceTest.php @@ -0,0 +1,337 @@ +,ignored_issues?:list,php_version?:string}> + */ + public function providerValidCodeParse(): iterable + { + return [ + 'updateReferencedTypes' => [ + 'code' => ' [ + '$a===' => '2', + '$b===' => '4', + '$c===' => '4', + ], + ], + 'SKIPPED-referenceToArrayItemChangesArrayType' => [ + 'code' => ' */ + $arr = []; + + assert(isset($arr[0])); + $int = &$arr[0]; + $int = (string) $int; + ', + 'assertions' => [ + '$arr' => 'list', + ], + ], + 'referenceToReference' => [ + 'code' => ' [ + '$a===' => '2', + '$b===' => '3', + '$c===' => '2', + '$d===' => '3', + ], + ], + 'referenceToSubsequentCatch' => [ + 'code' => ' [ + 'code' => ' [ + 'code' => ' [ + '$a===' => 'null', + '$b===' => 'null', + ], + ], + 'referenceShadowedByGlobal' => [ + 'code' => ' [ + '$a' => 'string', + ], + ], + 'unsetPreventsReferenceConfusion' => [ + 'code' => ' [ + 'code' => ' [ + 'code' => ',php_version?:string}> + */ + public function providerInvalidCodeParse(): iterable + { + return [ + 'referenceReuseForeachValue' => [ + 'code' => ' */ + $arr = []; + + foreach ($arr as &$var) { + $var += 1; + } + + $var = "foo"; + ', + 'error_message' => 'ReferenceReusedFromConfusingScope', + ], + 'referenceReuseDeclaredInForeach' => [ + 'code' => ' */ + $arr = []; + + foreach ($arr as $val) { + $var = &$val; + $var += 1; + } + + $var = "foo"; + ', + 'error_message' => 'ReferenceReusedFromConfusingScope', + ], + 'referenceReuseDeclaredInFor' => [ + 'code' => ' */ + $arr = []; + + for ($i = 0; $i < count($arr); ++$i) { + $var = &$arr[$i]; + $var += 1; + } + + $var = "foo"; + ', + 'error_message' => 'ReferenceReusedFromConfusingScope', + ], + 'referenceReuseDeclaredInIf' => [ + 'code' => ' */ + $arr = []; + + if (isset($arr[0])) { + $var = &$arr[0]; + $var += 1; + } + + $var = "foo"; + ', + 'error_message' => 'ReferenceReusedFromConfusingScope', + ], + 'referenceReuseDeclaredInElseif' => [ + 'code' => ' */ + $arr = []; + + if (random_int(0, 1)) { + } elseif (isset($arr[0])) { + $var = &$arr[0]; + $var += 1; + } + + $var = "foo"; + ', + 'error_message' => 'ReferenceReusedFromConfusingScope', + ], + 'referenceReuseDeclaredInElse' => [ + 'code' => ' */ + $arr = []; + + if (!isset($arr[0])) { + } else { + $var = &$arr[0]; + $var += 1; + } + + $var = "foo"; + ', + 'error_message' => 'ReferenceReusedFromConfusingScope', + ], + 'referenceReuseDeeplyNested' => [ + 'code' => '>> */ + $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' => [ + 'code' => ' 'InvalidDocblock - src' . DIRECTORY_SEPARATOR . 'somefile.php:4:21 - Docblock type cannot be used for reference assignment', + ], + 'SKIPPED-referenceToTypedArrayConstrainsAssignment' => [ + 'code' => ' */ + public array $arr = []; + + public function __construct() + { + assert(isset($this->arr[0])); + $int = &$this->arr[0]; + $int = (string) $int; + } + } + ', + 'error_message' => 'ReferenceConstraintViolation', + ], + 'SKIPPED-referenceToTypedArrayConstrainsAssignmentWithNullReferenceInitialization' => [ + 'code' => ' */ + 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' => [ + 'code' => ' 'ReferenceReusedFromConfusingScope', + ], + 'assignmentAsReferenceOnlyPreventsReferenceConfusionAfterAssignment' => [ + 'code' => ' 'ReferenceReusedFromConfusingScope', + ], + ]; + } +} diff --git a/tests/UnusedVariableTest.php b/tests/UnusedVariableTest.php index dc7fb38cac7..fac06242936 100644 --- a/tests/UnusedVariableTest.php +++ b/tests/UnusedVariableTest.php @@ -1525,7 +1525,10 @@ function foo(array $arr) : void { } } - /** @param mixed $p */ + /** + * @param mixed $p + * @psalm-suppress UnusedParam + */ function takes_ref(&$p): void {}' ], 'passedByRefArrayOffset' => [ @@ -1955,6 +1958,23 @@ function foo(array &$arr): void { $arr[0] = $b; }' ], + 'byRefDeeplyNestedArrayParam' => [ + 'code' => '> $arr */ + function foo(array &$arr): void { + $b = 5; + $arr[0][0] = $b; + }' + ], + 'nestedReferencesToByRefParam' => [ + 'code' => '> $arr */ + function foo(array &$arr): void { + $a = &$arr[0]; + $b = &$a[0]; + $b = 5; + }' + ], 'byRefNestedArrayInForeach' => [ 'code' => ' [ + 'code' => ' [ + 'code' => ' [ + '$b===' => '2', + '$a===' => '2', + ], + ], + 'referenceUsedAfterVariableReassignment' => [ + 'code' => ' [ + 'code' => ' [ + 'code' => ' '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' => [ + 'code' => ' 'UnusedVariable - src' . DIRECTORY_SEPARATOR . 'somefile.php:3:21 - $c', + ], + 'referenceAssignmentIsNotUsed' => [ + 'code' => ' 'UnusedVariable - src' . DIRECTORY_SEPARATOR . 'somefile.php:2:21 - $a', + ], + 'unusedReferenceToPreviouslyUsedVariable' => [ + 'code' => ' 'UnusedVariable - src' . DIRECTORY_SEPARATOR . 'somefile.php:4:21 - $b', + ], + 'SKIPPED-unusedReferenceToSubsequentlyUsedVariable' => [ // Not easy to do the way it's currently set up + 'code' => ' 'UnusedVariable - src' . DIRECTORY_SEPARATOR . 'somefile.php:3:21 - $b', + ], + 'unusedReferenceInForeach' => [ + 'code' => ' 'UnusedForeachValue', + ], + 'SKIPPED-unusedReferenceInDestructuredForeach' => [ + 'code' => ' 'UnusedForeachValue', + ], + 'unusedReturnByReference' => [ + 'code' => ' 'UnusedVariable', + ], + 'unusedPassByReference' => [ + 'code' => ' 'UnusedParam', + ], + 'SKIPPED-unusedGlobalVariable' => [ + 'code' => ' 'UnusedVariable - src' . DIRECTORY_SEPARATOR . 'somefile.php:2:21 - $a', + ], + 'unusedUndeclaredGlobalVariable' => [ + 'code' => ' 'UnusedVariable', + ], ]; } } From 3c7d176f8764a1fc0ea44d21e834b4d9b91b2dc3 Mon Sep 17 00:00:00 2001 From: AndrolGenhald Date: Fri, 7 Jan 2022 16:57:22 -0600 Subject: [PATCH 2/9] Minor CS fix, phrasing improvement, fix test. --- src/Psalm/Context.php | 2 +- .../Type/TemplateStandinTypeReplacer.php | 2 +- tests/ReferenceConstraintTest.php | 32 ++++++++++++++++ tests/ReferenceTest.php | 38 ++----------------- 4 files changed, 37 insertions(+), 37 deletions(-) diff --git a/src/Psalm/Context.php b/src/Psalm/Context.php index 5d624e17bbb..604346380ff 100644 --- a/src/Psalm/Context.php +++ b/src/Psalm/Context.php @@ -495,7 +495,7 @@ 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. + * such as a reference created in an if that might later be reused. */ public function updateReferencesPossiblyFromConfusingScope( Context $confusing_scope_context, diff --git a/src/Psalm/Internal/Type/TemplateStandinTypeReplacer.php b/src/Psalm/Internal/Type/TemplateStandinTypeReplacer.php index 67bc813020f..9d86881d75f 100644 --- a/src/Psalm/Internal/Type/TemplateStandinTypeReplacer.php +++ b/src/Psalm/Internal/Type/TemplateStandinTypeReplacer.php @@ -894,7 +894,7 @@ public static function handleTemplateParamClassStandin( bool $add_lower_bound, ?string $bound_equality_classlike, int $depth, - bool $was_single, + bool $was_single ): array { if ($atomic_type->defining_class === $calling_class) { return [$atomic_type]; diff --git a/tests/ReferenceConstraintTest.php b/tests/ReferenceConstraintTest.php index 892ec3997db..2cdec8413a4 100644 --- a/tests/ReferenceConstraintTest.php +++ b/tests/ReferenceConstraintTest.php @@ -299,6 +299,38 @@ function main(bool $a, string $b, string $c): void { }', 'error_message' => 'InvalidPassByReference', ], + 'SKIPPED-referenceToTypedArrayConstrainsAssignment' => [ + 'code' => ' */ + public array $arr = []; + + public function __construct() + { + assert(isset($this->arr[0])); + $int = &$this->arr[0]; + $int = (string) $int; + } + } + ', + 'error_message' => 'ReferenceConstraintViolation', + ], + 'SKIPPED-referenceToTypedArrayConstrainsAssignmentWithNullReferenceInitialization' => [ + 'code' => ' */ + 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', + ], ]; } } diff --git a/tests/ReferenceTest.php b/tests/ReferenceTest.php index 09a0f6d2a74..f92ac0dc741 100644 --- a/tests/ReferenceTest.php +++ b/tests/ReferenceTest.php @@ -106,11 +106,11 @@ function foo(): void $b = 1; $a = &$b; global $a; + takesString($a); } + + function takesString(string $str): void {} ', - 'assertions' => [ - '$a' => 'string', - ], ], 'unsetPreventsReferenceConfusion' => [ 'code' => ' 'InvalidDocblock - src' . DIRECTORY_SEPARATOR . 'somefile.php:4:21 - Docblock type cannot be used for reference assignment', ], - 'SKIPPED-referenceToTypedArrayConstrainsAssignment' => [ - 'code' => ' */ - public array $arr = []; - - public function __construct() - { - assert(isset($this->arr[0])); - $int = &$this->arr[0]; - $int = (string) $int; - } - } - ', - 'error_message' => 'ReferenceConstraintViolation', - ], - 'SKIPPED-referenceToTypedArrayConstrainsAssignmentWithNullReferenceInitialization' => [ - 'code' => ' */ - 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' => [ 'code' => ' Date: Fri, 7 Jan 2022 17:35:17 -0600 Subject: [PATCH 3/9] Remove impossible TODO. --- src/Psalm/Context.php | 5 ----- src/Psalm/Internal/Analyzer/Statements/GlobalAnalyzer.php | 8 -------- 2 files changed, 13 deletions(-) diff --git a/src/Psalm/Context.php b/src/Psalm/Context.php index 604346380ff..a484df80156 100644 --- a/src/Psalm/Context.php +++ b/src/Psalm/Context.php @@ -70,11 +70,6 @@ class Context /** * 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 */ diff --git a/src/Psalm/Internal/Analyzer/Statements/GlobalAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/GlobalAnalyzer.php index 3ef2635c8ff..7099db0f9ba 100644 --- a/src/Psalm/Internal/Analyzer/Statements/GlobalAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/GlobalAnalyzer.php @@ -95,15 +95,7 @@ public static function analyze( 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' - // ); } } } From 089700d1bef6a6df9e44299a89b13325bb4daada Mon Sep 17 00:00:00 2001 From: AndrolGenhald Date: Fri, 7 Jan 2022 17:40:09 -0600 Subject: [PATCH 4/9] Add more reference tests. --- tests/ReferenceTest.php | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/tests/ReferenceTest.php b/tests/ReferenceTest.php index f92ac0dc741..16839f047dd 100644 --- a/tests/ReferenceTest.php +++ b/tests/ReferenceTest.php @@ -33,6 +33,42 @@ public function providerValidCodeParse(): iterable '$c===' => '4', ], ], + 'changingReferencedVariableChangesReference' => [ + 'code' => ' [ + '$a===' => '2', + '$b===' => '2', + ], + ], + 'unsetReference' => [ + 'code' => ' [ + '$a===' => '2', + '$b===' => '3', + ], + ], + 'recursiveReference' => [ + 'code' => ' [ + '$a===' => '2', + '$b===' => '2', + ], + ], 'SKIPPED-referenceToArrayItemChangesArrayType' => [ 'code' => ' */ From 7694eb8d0881b5d2a033910f001e746de7557290 Mon Sep 17 00:00:00 2001 From: AndrolGenhald Date: Fri, 7 Jan 2022 17:41:39 -0600 Subject: [PATCH 5/9] Work around phpcs impossible syntax issue. phpcs want `as` to have a single space before it, but it also wants the line indented. Worked around by assigning to another variable. --- src/Psalm/Context.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Psalm/Context.php b/src/Psalm/Context.php index a484df80156..b5013feead6 100644 --- a/src/Psalm/Context.php +++ b/src/Psalm/Context.php @@ -496,9 +496,9 @@ 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 => $_ - ) { + $references = $confusing_scope_context->references_in_scope + + $confusing_scope_context->references_to_external_scope; + foreach ($references 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) From 57b99be519f21e9d965e67116e22af562aea3115 Mon Sep 17 00:00:00 2001 From: AndrolGenhald Date: Mon, 10 Jan 2022 17:45:29 -0600 Subject: [PATCH 6/9] Improve reference support for assertions, array offsets, and properties. --- .../Analyzer/Statements/Block/DoAnalyzer.php | 1 + .../Block/IfConditionalAnalyzer.php | 1 + .../Statements/Block/IfElse/ElseAnalyzer.php | 1 + .../Block/IfElse/ElseIfAnalyzer.php | 2 + .../Statements/Block/IfElse/IfAnalyzer.php | 1 + .../Statements/Block/IfElseAnalyzer.php | 2 + .../Statements/Block/LoopAnalyzer.php | 2 + .../Statements/Block/SwitchAnalyzer.php | 1 + .../Statements/Block/SwitchCaseAnalyzer.php | 1 + .../Expression/AssignmentAnalyzer.php | 30 +++++++ .../Expression/BinaryOp/AndAnalyzer.php | 1 + .../Expression/BinaryOp/OrAnalyzer.php | 2 + .../Expression/Call/FunctionCallAnalyzer.php | 1 + .../Call/NamedFunctionCallHandler.php | 1 + .../Statements/Expression/CallAnalyzer.php | 1 + .../Statements/Expression/MatchAnalyzer.php | 1 + .../Statements/Expression/TernaryAnalyzer.php | 2 + .../ArrayFilterReturnTypeProvider.php | 2 + src/Psalm/Type/Reconciler.php | 44 ++++++++++- tests/AssertAnnotationTest.php | 79 +++++++++++++++++-- tests/ReferenceTest.php | 16 ++++ tests/TypeReconciliation/ConditionalTest.php | 21 +++++ tests/UnusedVariableTest.php | 25 +++++- 23 files changed, 230 insertions(+), 8 deletions(-) diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/DoAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/DoAnalyzer.php index aeeb964f90a..2298b6cd41f 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/DoAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/DoAnalyzer.php @@ -129,6 +129,7 @@ function (Clause $c) use ($mixed_var_ids): bool { $negated_while_types, [], $inner_loop_context->vars_in_scope, + $inner_loop_context->references_in_scope, $changed_var_ids, [], $statements_analyzer, diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/IfConditionalAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/IfConditionalAnalyzer.php index 29a94d5c78e..7c672ca138d 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/IfConditionalAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/IfConditionalAnalyzer.php @@ -54,6 +54,7 @@ public static function analyze( $if_scope->negated_types, [], $outer_context->vars_in_scope, + $outer_context->references_in_scope, $changed_var_ids, [], $statements_analyzer, diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/ElseAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/ElseAnalyzer.php index 2b0be2e173a..4d878054555 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/ElseAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/ElseAnalyzer.php @@ -78,6 +78,7 @@ public static function analyze( $else_types, [], $else_context->vars_in_scope, + $else_context->references_in_scope, $changed_var_ids, [], $statements_analyzer, diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/ElseIfAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/ElseIfAnalyzer.php index ff10838d32a..9cda6b597b5 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/ElseIfAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/ElseIfAnalyzer.php @@ -227,6 +227,7 @@ function (Clause $c) use ($assigned_in_conditional_var_ids, $elseif_cond_id): Cl $reconcilable_elseif_types, $active_elseif_types, $elseif_context->vars_in_scope, + $elseif_context->references_in_scope, $newly_reconciled_var_ids, $cond_referenced_var_ids, $statements_analyzer, @@ -360,6 +361,7 @@ function (Clause $c) use ($assigned_in_conditional_var_ids, $elseif_cond_id): Cl $negated_elseif_types, [], $pre_conditional_context->vars_in_scope, + $pre_conditional_context->references_in_scope, $newly_reconciled_var_ids, [], $statements_analyzer, diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/IfAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/IfAnalyzer.php index c33eb147fac..fda3b851617 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/IfAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/IfElse/IfAnalyzer.php @@ -286,6 +286,7 @@ private static function handleMicDrop( $if_scope->negated_types, [], $post_if_context->vars_in_scope, + $post_if_context->references_in_scope, $newly_reconciled_var_ids, [], $statements_analyzer, diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php index 837873c3a3a..316b91a8961 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php @@ -267,6 +267,7 @@ function (Clause $c) use ($mixed_var_ids, $cond_object_id): Clause { $reconcilable_if_types, $active_if_types, $if_context->vars_in_scope, + $if_context->references_in_scope, $changed_var_ids, $cond_referenced_var_ids, $statements_analyzer, @@ -328,6 +329,7 @@ function (Clause $c) use ($mixed_var_ids, $cond_object_id): Clause { $if_scope->negated_types, [], $temp_else_context->vars_in_scope, + $temp_else_context->references_in_scope, $changed_var_ids, [], $statements_analyzer, diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/LoopAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/LoopAnalyzer.php index c49155bad48..bc11d8644c4 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/LoopAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/LoopAnalyzer.php @@ -503,6 +503,7 @@ public static function analyze( $negated_pre_condition_types, [], $inner_context->vars_in_scope, + $inner_context->references_in_scope, $changed_var_ids, [], $statements_analyzer, @@ -666,6 +667,7 @@ private static function applyPreConditionToLoopContext( $reconcilable_while_types, $active_while_types, $loop_context->vars_in_scope, + $loop_context->references_in_scope, $changed_var_ids, $new_referenced_var_ids, $statements_analyzer, diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/SwitchAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/SwitchAnalyzer.php index 570881470bb..6d286879b7c 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/SwitchAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/SwitchAnalyzer.php @@ -159,6 +159,7 @@ public static function analyze( $reconcilable_if_types, [], $original_context->vars_in_scope, + $original_context->references_in_scope, $changed_var_ids, [], $statements_analyzer, diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/SwitchCaseAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/SwitchCaseAnalyzer.php index 078fc3cd75a..49177b5b414 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/SwitchCaseAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/SwitchCaseAnalyzer.php @@ -405,6 +405,7 @@ public static function analyze( $reconcilable_if_types, [], $case_context->vars_in_scope, + $case_context->references_in_scope, $changed_var_ids, $case->cond && $switch_var_id ? [$switch_var_id => true] : [], $statements_analyzer, diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php index f6d948d557a..797de49243d 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php @@ -3,6 +3,9 @@ namespace Psalm\Internal\Analyzer\Statements\Expression; use PhpParser; +use PhpParser\Node\Expr\ArrayDimFetch; +use PhpParser\Node\Expr\PropertyFetch; +use PhpParser\Node\Expr\Variable; use Psalm\CodeLocation; use Psalm\CodeLocation\DocblockTypeLocation; use Psalm\Codebase; @@ -947,8 +950,35 @@ public static function analyzeAssignmentRef( ); $lhs_node = DataFlowNode::getForAssignment($lhs_var_id, $lhs_location); + $context->vars_in_scope[$lhs_var_id]->parent_nodes[$lhs_node->id] = $lhs_node; + if (($stmt->var instanceof ArrayDimFetch || $stmt->var instanceof PropertyFetch) + && $stmt->var->var instanceof Variable + && is_string($stmt->var->var->name) + ) { + // If left-hand-side is an array offset or object property, add an edge to the root + // variable, so if the root variable is used, the offset/property is also marked as used. + $root_var_id = "$" . $stmt->var->var->name; + foreach ($context->vars_in_scope[$root_var_id]->parent_nodes as $root_parent) { + $statements_analyzer->data_flow_graph->addPath( + $lhs_node, + $root_parent, + $stmt->var instanceof ArrayDimFetch + ? "offset-assignment-as-reference" + : "property-assignment-as-reference" + ); + } + } + + if ($stmt->var instanceof ArrayDimFetch) { + // Analyze offset so that variables in the offset get marked as used + $was_inside_general_use = $context->inside_general_use; + $context->inside_general_use = true; + ExpressionAnalyzer::analyze($statements_analyzer, $stmt->var->dim, $context); + $context->inside_general_use = $was_inside_general_use; + } + return true; } diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/AndAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/AndAnalyzer.php index 98ca259fb2b..7e3937c38ac 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/AndAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/AndAnalyzer.php @@ -138,6 +138,7 @@ public static function analyze( $left_type_assertions, $active_left_assertions, $context->vars_in_scope, + $context->references_in_scope, $changed_var_ids, $left_referenced_var_ids, $statements_analyzer, diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/OrAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/OrAnalyzer.php index cf71761e339..f5ef20d7a8f 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/OrAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/BinaryOp/OrAnalyzer.php @@ -223,6 +223,7 @@ public static function analyze( $negated_type_assertions, $active_negated_type_assertions, $right_context->vars_in_scope, + $right_context->references_in_scope, $changed_var_ids, $left_referenced_var_ids, $statements_analyzer, @@ -314,6 +315,7 @@ public static function analyze( $right_type_assertions, $active_right_type_assertions, $right_context->vars_in_scope, + $right_context->references_in_scope, $right_changed_var_ids, $right_referenced_var_ids, $statements_analyzer, diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php index bb07f369b3d..ce2198ac666 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php @@ -945,6 +945,7 @@ private static function processAssertFunctionEffects( $assert_type_assertions, $assert_type_assertions, $context->vars_in_scope, + $context->references_in_scope, $changed_var_ids, array_map( fn($_): bool => true, diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NamedFunctionCallHandler.php b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NamedFunctionCallHandler.php index 06d6f10c17b..4358c47f17c 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NamedFunctionCallHandler.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/Call/NamedFunctionCallHandler.php @@ -399,6 +399,7 @@ public static function handle( $assertions, $assertions, $context->vars_in_scope, + $context->references_in_scope, $changed_vars, $referenced_var_ids, $statements_analyzer, diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php index 4142828c744..c90c44fe83e 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php @@ -885,6 +885,7 @@ public static function applyAssertionsToContext( $type_assertions, $type_assertions, $context->vars_in_scope, + $context->references_in_scope, $changed_var_ids, $asserted_keys, $statements_analyzer, diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/MatchAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/MatchAnalyzer.php index 2e2ee100d94..d131d7b2737 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/MatchAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/MatchAnalyzer.php @@ -253,6 +253,7 @@ public static function analyze( $reconcilable_types, [], $context->vars_in_scope, + $context->references_in_scope, $changed_var_ids, [], $statements_analyzer, diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/TernaryAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/TernaryAnalyzer.php index 39905239d37..2bd6307fda8 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/TernaryAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/TernaryAnalyzer.php @@ -161,6 +161,7 @@ function (Clause $c) use ($mixed_var_ids, $cond_id): Clause { $reconcilable_if_types, $active_if_types, $if_context->vars_in_scope, + $if_context->references_in_scope, $changed_var_ids, $cond_referenced_var_ids, $statements_analyzer, @@ -199,6 +200,7 @@ function (Clause $c) use ($mixed_var_ids, $cond_id): Clause { $negated_if_types, $negated_if_types, $t_else_context->vars_in_scope, + $t_else_context->references_in_scope, $changed_var_ids, $cond_referenced_var_ids, $statements_analyzer, diff --git a/src/Psalm/Internal/Provider/ReturnTypeProvider/ArrayFilterReturnTypeProvider.php b/src/Psalm/Internal/Provider/ReturnTypeProvider/ArrayFilterReturnTypeProvider.php index f26ef15cbf1..fd07c16459c 100644 --- a/src/Psalm/Internal/Provider/ReturnTypeProvider/ArrayFilterReturnTypeProvider.php +++ b/src/Psalm/Internal/Provider/ReturnTypeProvider/ArrayFilterReturnTypeProvider.php @@ -217,6 +217,7 @@ static function ($keyed_type) use ($statements_source, $context) { $assertions, $assertions, ['$inner_type' => $inner_type], + [], $changed_var_ids, ['$inner_type' => true], $statements_source, @@ -296,6 +297,7 @@ static function ($keyed_type) use ($statements_source, $context) { $assertions, $assertions, ['$inner_type' => $inner_type], + [], $changed_var_ids, ['$inner_type' => true], $statements_source, diff --git a/src/Psalm/Type/Reconciler.php b/src/Psalm/Type/Reconciler.php index 442699f85a0..22c137320ec 100644 --- a/src/Psalm/Type/Reconciler.php +++ b/src/Psalm/Type/Reconciler.php @@ -88,16 +88,21 @@ class Reconciler * @param array>> $new_types * @param array>> $active_new_types - types we can complain about * @param array $existing_types + * @param array $existing_references Maps keys of $existing_types that are references to other + * keys of $existing_types that they are references to. * @param array $changed_var_ids * @param array $referenced_var_ids * @param array> $template_type_map * * @return array + * + * @psalm-suppress ComplexMethod */ public static function reconcileKeyedTypes( array $new_types, array $active_new_types, array $existing_types, + array $existing_references, array &$changed_var_ids, array $referenced_var_ids, StatementsAnalyzer $statements_analyzer, @@ -110,6 +115,32 @@ public static function reconcileKeyedTypes( return $existing_types; } + $reference_map = []; + if (!empty($existing_references)) { + // PHP behaves oddly when passing an array containing references: https://bugs.php.net/bug.php?id=20993 + // To work around the issue, if there are any references, we have to recreate the array and fix the + // references so they're properly scoped and won't affect the caller. Starting with a new array is + // required for some unclear reason, just cloning elements of the existing array doesn't work properly. + $old_existing_types = $existing_types; + $existing_types = []; + + $cloned_referenceds = []; + foreach ($existing_references as $reference => $referenced) { + if (!isset($cloned_referenceds[$referenced])) { + $existing_types[$referenced] = clone $old_existing_types[$referenced]; + $cloned_referenceds[$referenced] = true; + } + $existing_types[$reference] = &$existing_types[$referenced]; + } + $existing_types += $old_existing_types; + + // Build a map from reference/referenced variables to other variables with the same reference + foreach ($existing_references as $reference => $referenced) { + $reference_map[$reference][] = $referenced; + $reference_map[$referenced][] = $reference; + } + } + $suppressed_issues = $statements_analyzer->getSuppressedIssues(); $old_new_types = $new_types; @@ -214,6 +245,7 @@ public static function reconcileKeyedTypes( $data, $data, $existing_types, + $existing_references, $changed_var_ids, $referenced_var_ids, $statements_analyzer, @@ -285,11 +317,11 @@ public static function reconcileKeyedTypes( $type_changed = !$before_adjustment || !$result_type->equals($before_adjustment); + $key_parts = self::breakUpPathIntoParts($key); if ($type_changed || $failed_reconciliation) { $changed_var_ids[$key] = true; if (substr($key, -1) === ']' && !$has_inverted_isset && !$has_empty && !$is_equality) { - $key_parts = self::breakUpPathIntoParts($key); self::adjustTKeyedArrayType( $key_parts, $existing_types, @@ -321,6 +353,16 @@ public static function reconcileKeyedTypes( if (!$has_object_array_access) { $existing_types[$key] = $result_type; } + + if (!$did_type_exist && isset($existing_types[$key]) && isset($reference_map[$key_parts[0]])) { + // If key is new, create references for other variables that reference the root variable. + $reference_key_parts = $key_parts; + foreach ($reference_map[$key_parts[0]] as $reference) { + $reference_key_parts[0] = $reference; + $reference_key = implode("", $reference_key_parts); + $existing_types[$reference_key] = &$existing_types[$key]; + } + } } return $existing_types; diff --git a/tests/AssertAnnotationTest.php b/tests/AssertAnnotationTest.php index fcad01f90e0..b0f672ef163 100644 --- a/tests/AssertAnnotationTest.php +++ b/tests/AssertAnnotationTest.php @@ -1825,7 +1825,74 @@ function assertBarNotNull(Foo $foo): bool function requiresString(string $_str): void {} ', ], - 'SKIPPED-applyAssertionsToReferences' => [ // See #7254 + 'referencesDontBreakAssertions' => [ + 'code' => ' [ + 'code' => ' [ + 'code' => ' [ 'code' => ' [ // See #7254 + 'applyAssertionsOnPropertiesFromReferences' => [ 'code' => ' [ // See #7254 + 'applyAssertionsOnPropertiesToReferencesWithConditionalOperator' => [ 'code' => ' 'PossiblyNullArgument', ], - 'SKIPPED-forgetAssertionAfterRelevantNonMutationFreeCallOnReference' => [ // See #7254 + 'forgetAssertionAfterRelevantNonMutationFreeCallOnReference' => [ 'code' => ' 'PossiblyNullArgument', ], - 'SKIPPED-forgetAssertionAfterReferenceModification' => [ // See #7254 + 'forgetAssertionAfterReferenceModification' => [ 'code' => ' 'PossiblyNullArgument', + 'error_message' => 'NullArgument', ], 'assertionOnMagicPropertyWithoutMutationFreeGet' => [ 'code' => ' [ + 'code' => 'bar; + + $foo->bar = "bar"; + ', + 'assertions' => [ + '$bar===' => "'bar'", + ], + ], ]; } diff --git a/tests/TypeReconciliation/ConditionalTest.php b/tests/TypeReconciliation/ConditionalTest.php index 2ab475b5fd5..733743da3e7 100644 --- a/tests/TypeReconciliation/ConditionalTest.php +++ b/tests/TypeReconciliation/ConditionalTest.php @@ -3241,6 +3241,27 @@ function c(string $c): void { ', 'error_message' => 'RedundantCondition', ], + 'impossibleConditionWithReference' => [ + 'code' => ' 'TypeDoesNotContainType', + ], + 'redundantConditionWithReference' => [ + 'code' => ' 'RedundantCondition', + ], ]; } } diff --git a/tests/UnusedVariableTest.php b/tests/UnusedVariableTest.php index fac06242936..2e5ce623a71 100644 --- a/tests/UnusedVariableTest.php +++ b/tests/UnusedVariableTest.php @@ -2470,7 +2470,7 @@ function takesAnInt(): void { if (++$i > 10) { break; } else {} - } + } }', ], 'referenceUseUsesReferencedVariable' => [ @@ -2514,6 +2514,29 @@ function takesAnInt(): void { } ', ], + 'arrayWithReferenceIsUsed' => [ + 'code' => ' */ + $arr = [1]; + $arr[1] = &$arr[0]; + + takesArray($arr); + + function takesArray(array $_arr): void {} + ', + ], + 'arrayWithVariableOffsetAssignedToReferenceUsesVariableOffset' => [ + 'code' => ' */ + $arr = [1]; + $int = 1; + $arr[$int] = &$arr[0]; + + takesArray($arr); + + function takesArray(array $_arr): void {} + ', + ], ]; } From 139c3af9d18394802c483caaa13efcd03fddfd74 Mon Sep 17 00:00:00 2001 From: AndrolGenhald Date: Mon, 10 Jan 2022 18:08:51 -0600 Subject: [PATCH 7/9] Add reference changes to UPGRADING documentation. --- UPGRADING.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/UPGRADING.md b/UPGRADING.md index f278fe8b219..40251bc89e3 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -152,6 +152,8 @@ - [BC] Atomic::__toString() is now final - [BC] Atomic::__toString() now returns a more detailed version of the type (it calls getId() under the hood) - [BC] Atomic::getId() has now a first param $exact. Calling the method with false will return a less detailed version of the type in some cases (similarly to what __toString used to return) +- [BC] To remove a variable from the context, Context::remove(). Calling + `unset($context->vars_in_scope[$var_id])` can cause problems when using references. ## Removed - [BC] Property `Psalm\Codebase::$php_major_version` was removed, use @@ -194,3 +196,4 @@ - [BC] Method `Psalm\Issue\CodeIssue::getMessage()` was removed - [BC] Method `Psalm\DocComment::parse()` was removed - [BC] Class `Psalm\Type\Atomic\THtmlEscapedString` has been removed + - [BC] Property `Psalm\Context::$vars_from_global` has been removed From 740a10141dc5b5deb3679893e6a083408bc41ef4 Mon Sep 17 00:00:00 2001 From: AndrolGenhald Date: Tue, 11 Jan 2022 11:36:37 -0600 Subject: [PATCH 8/9] Fix crash when a reference is reassigned in a loop. --- src/Psalm/Context.php | 27 +++++++++++++++++++++++++-- tests/ReferenceTest.php | 16 ++++++++++++++++ 2 files changed, 41 insertions(+), 2 deletions(-) diff --git a/src/Psalm/Context.php b/src/Psalm/Context.php index b5013feead6..b1346e55e87 100644 --- a/src/Psalm/Context.php +++ b/src/Psalm/Context.php @@ -17,6 +17,8 @@ use function array_keys; use function array_search; +use function array_shift; +use function assert; use function count; use function in_array; use function is_int; @@ -572,11 +574,31 @@ public function remove(string $remove_var_id, bool $removeDescendents = true): v } /** - * Remove a variable from the context which might be a reference to another variable. - * Leaves the variable as possibly-in-scope, unlike remove(). + * Remove a variable from the context which might be a reference to another variable, or + * referenced by another variable. Leaves the variable as possibly-in-scope, unlike remove(). */ public function removePossibleReference(string $remove_var_id): void { + if (isset($this->referenced_counts[$remove_var_id]) && $this->referenced_counts[$remove_var_id] > 0) { + // If a referenced variable goes out of scope, we need to update the references. + // All of the references to this variable are still references to the same value, + // so we pick the first one and make the rest of the references point to it. + $references = []; + foreach ($this->references_in_scope as $reference => $referenced) { + if ($referenced === $remove_var_id) { + $references[] = $reference; + unset($this->references_in_scope[$reference]); + } + } + assert(!empty($references)); + $first_reference = array_shift($references); + if (!empty($references)) { + $this->referenced_counts[$first_reference] = count($references); + foreach ($references as $reference) { + $this->references_in_scope[$reference] = $first_reference; + } + } + } 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) { @@ -587,6 +609,7 @@ public function removePossibleReference(string $remove_var_id): void unset( $this->vars_in_scope[$remove_var_id], $this->referenced_var_ids[$remove_var_id], + $this->referenced_counts[$remove_var_id], $this->references_in_scope[$remove_var_id], $this->references_to_external_scope[$remove_var_id], ); diff --git a/tests/ReferenceTest.php b/tests/ReferenceTest.php index 0c243b4d016..13ae8ec17f3 100644 --- a/tests/ReferenceTest.php +++ b/tests/ReferenceTest.php @@ -202,6 +202,22 @@ class Foo '$bar===' => "'bar'", ], ], + 'referenceReassignedInLoop' => [ + 'code' => ' $keys */ + function &ensure_array(array &$what, array $keys): array + { + $arr = & $what; + while ($key = array_shift($keys)) { + if (!isset($arr[$key]) || !is_array($arr[$key])) { + $arr[$key] = array(); + } + $arr = & $arr[$key]; + } + return $arr; + } + ', + ], ]; } From e268a053070ca1272df727673ea4f750ab3212b6 Mon Sep 17 00:00:00 2001 From: AndrolGenhald Date: Wed, 26 Jan 2022 12:54:42 -0600 Subject: [PATCH 9/9] Minor fixes after rebasing. --- UPGRADING.md | 2 +- docs/running_psalm/issues.md | 1 + src/Psalm/Internal/Analyzer/Statements/GlobalAnalyzer.php | 7 ++----- src/Psalm/Internal/Type/TypeExpander.php | 2 ++ src/Psalm/Issue/ReferenceReusedFromConfusingScope.php | 2 +- src/Psalm/Type/Reconciler.php | 2 -- 6 files changed, 7 insertions(+), 9 deletions(-) diff --git a/UPGRADING.md b/UPGRADING.md index 40251bc89e3..8dafd844e3b 100644 --- a/UPGRADING.md +++ b/UPGRADING.md @@ -196,4 +196,4 @@ - [BC] Method `Psalm\Issue\CodeIssue::getMessage()` was removed - [BC] Method `Psalm\DocComment::parse()` was removed - [BC] Class `Psalm\Type\Atomic\THtmlEscapedString` has been removed - - [BC] Property `Psalm\Context::$vars_from_global` has been removed + - [BC] Property `Psalm\Context::$vars_from_global` has been renamed to `$referenced_globals` diff --git a/docs/running_psalm/issues.md b/docs/running_psalm/issues.md index a57d76b9e4f..a06d6349e7e 100644 --- a/docs/running_psalm/issues.md +++ b/docs/running_psalm/issues.md @@ -216,6 +216,7 @@ - [RedundantIdentityWithTrue](issues/RedundantIdentityWithTrue.md) - [RedundantPropertyInitializationCheck](issues/RedundantPropertyInitializationCheck.md) - [ReferenceConstraintViolation](issues/ReferenceConstraintViolation.md) + - [ReferenceReusedFromConfusingScope](issues/ReferenceReusedFromConfusingScope.md) - [ReservedWord](issues/ReservedWord.md) - [StringIncrement](issues/StringIncrement.md) - [TaintedCallable](issues/TaintedCallable.md) diff --git a/src/Psalm/Internal/Analyzer/Statements/GlobalAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/GlobalAnalyzer.php index 7099db0f9ba..a04e72d9740 100644 --- a/src/Psalm/Internal/Analyzer/Statements/GlobalAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/GlobalAnalyzer.php @@ -8,7 +8,6 @@ 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; @@ -93,10 +92,8 @@ public static function analyze( $var_id ); - if ($statements_analyzer->data_flow_graph instanceof VariableUseGraph) { - if ($global_context !== null && $global_context->hasVariable($var_id)) { - $global_context->referenced_globals[$var_id] = true; - } + if ($global_context !== null && $global_context->hasVariable($var_id)) { + $global_context->referenced_globals[$var_id] = true; } } } diff --git a/src/Psalm/Internal/Type/TypeExpander.php b/src/Psalm/Internal/Type/TypeExpander.php index 5901ced6e19..97c1a8854a8 100644 --- a/src/Psalm/Internal/Type/TypeExpander.php +++ b/src/Psalm/Internal/Type/TypeExpander.php @@ -125,6 +125,8 @@ public static function expandUnion( * @param string|TNamedObject|TTemplateParam|null $static_class_type * * @return non-empty-list + * + * @psalm-suppress ComplexMethod */ public static function expandAtomic( Codebase $codebase, diff --git a/src/Psalm/Issue/ReferenceReusedFromConfusingScope.php b/src/Psalm/Issue/ReferenceReusedFromConfusingScope.php index 31332b98bc0..6c88cf67b26 100644 --- a/src/Psalm/Issue/ReferenceReusedFromConfusingScope.php +++ b/src/Psalm/Issue/ReferenceReusedFromConfusingScope.php @@ -5,5 +5,5 @@ class ReferenceReusedFromConfusingScope extends CodeIssue { public const ERROR_LEVEL = 4; - public const SHORTCODE = 303; + public const SHORTCODE = 308; } diff --git a/src/Psalm/Type/Reconciler.php b/src/Psalm/Type/Reconciler.php index 22c137320ec..b4002e597a4 100644 --- a/src/Psalm/Type/Reconciler.php +++ b/src/Psalm/Type/Reconciler.php @@ -95,8 +95,6 @@ class Reconciler * @param array> $template_type_map * * @return array - * - * @psalm-suppress ComplexMethod */ public static function reconcileKeyedTypes( array $new_types,