diff --git a/config.xsd b/config.xsd index e3f6eeab3bb..4ee050a6500 100644 --- a/config.xsd +++ b/config.xsd @@ -467,6 +467,7 @@ + diff --git a/docs/running_psalm/issues.md b/docs/running_psalm/issues.md index 1411a772fb1..8b273822835 100644 --- a/docs/running_psalm/issues.md +++ b/docs/running_psalm/issues.md @@ -278,6 +278,7 @@ - [UnresolvableInclude](issues/UnresolvableInclude.md) - [UnsafeGenericInstantiation](issues/UnsafeGenericInstantiation.md) - [UnsafeInstantiation](issues/UnsafeInstantiation.md) + - [UnsupportedReferenceUsage](issues/UnsupportedReferenceUsage.md) - [UnusedClass](issues/UnusedClass.md) - [UnusedClosureParam](issues/UnusedClosureParam.md) - [UnusedConstructor](issues/UnusedConstructor.md) diff --git a/docs/running_psalm/issues/UnsupportedReferenceUsage.md b/docs/running_psalm/issues/UnsupportedReferenceUsage.md new file mode 100644 index 00000000000..1e1b42b0d2c --- /dev/null +++ b/docs/running_psalm/issues/UnsupportedReferenceUsage.md @@ -0,0 +1,34 @@ +# UnsupportedReferenceUsage + +Emitted when Psalm encounters a reference that it is not currently able to track (for instance a reference to an array +offset of an array offset: `$foo = &$bar[$baz[0]]`). When an unsupported reference is encountered, Psalm will issue this +warning and treat the variable as though it wasn't actually a reference. + +## How to fix + +This can sometimes be fixed by using a temporary variable: + +```php + */ +$bar = [1, 2, 3]; +/** @var non-empty-list */ +$baz = [1, 2, 3]; + +$foo = &$bar[$baz[0]]; +``` + +can be turned into + +```php + */ +$bar = [1, 2, 3]; +/** @var non-empty-list */ +$baz = [1, 2, 3]; + +$offset = $baz[0]; +$foo = &$bar[$offset]; +``` diff --git a/src/Psalm/Context.php b/src/Psalm/Context.php index 7c31e352cf9..286cbf72ee4 100644 --- a/src/Psalm/Context.php +++ b/src/Psalm/Context.php @@ -2,6 +2,7 @@ namespace Psalm; +use InvalidArgumentException; use Psalm\Internal\Analyzer\StatementsAnalyzer; use Psalm\Internal\Clause; use Psalm\Internal\ReferenceConstraint; @@ -608,11 +609,7 @@ 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; + $this->decrementReferenceCount($remove_var_id); } unset( $this->vars_in_scope[$remove_var_id], @@ -623,6 +620,24 @@ public function removePossibleReference(string $remove_var_id): void ); } + /** + * Decrement the reference count of the variable that $ref_id is referring to. This needs to + * be done before $ref_id is changed to no longer reference its currently referenced variable, + * for example by unsetting, reassigning to another reference, or being shadowed by a global. + */ + public function decrementReferenceCount(string $ref_id): void + { + if (!isset($this->referenced_counts[$this->references_in_scope[$ref_id]])) { + throw new InvalidArgumentException("$ref_id is not a reference"); + } + $reference_count = $this->referenced_counts[$this->references_in_scope[$ref_id]]; + if ($reference_count < 1) { + throw new RuntimeException("Incorrect referenced count found"); + } + --$reference_count; + $this->referenced_counts[$this->references_in_scope[$ref_id]] = $reference_count; + } + /** * @param Clause[] $clauses * @param array $changed_var_ids diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php index 17262341b2a..805ced5e129 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php @@ -5,7 +5,6 @@ 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; @@ -82,7 +81,6 @@ use Psalm\Type\Atomic\TNonEmptyList; use Psalm\Type\Atomic\TNull; use Psalm\Type\Union; -use RuntimeException; use UnexpectedValueException; use function array_filter; @@ -947,11 +945,7 @@ public static function analyzeAssignmentRef( 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; + $context->decrementReferenceCount($lhs_var_id); // 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]; @@ -981,44 +975,20 @@ public static function analyzeAssignmentRef( } $lhs_location = new CodeLocation($statements_analyzer->getSource(), $stmt->var); - $statements_analyzer->registerVariableAssignment( - $lhs_var_id, - $lhs_location - ); + if (!$stmt->var instanceof ArrayDimFetch && !$stmt->var instanceof PropertyFetch) { + // If left-hand-side is an array offset or object property, usage is too difficult to track, + // so it's not registered as an unused variable (this mirrors behavior for non-references). + $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; - if ($statements_analyzer->data_flow_graph !== null - && ($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 ($root_var_id === '$this') { - // Variables on `$this` are always used - $statements_analyzer->data_flow_graph->addPath( - $lhs_node, - new DataFlowNode('variable-use', 'variable use', null), - 'variable-use', - ); - } - } - - if ($stmt->var instanceof ArrayDimFetch) { + if ($stmt->var instanceof ArrayDimFetch && $stmt->var->dim !== null) { // 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; diff --git a/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php index 0d3c64c542f..994759611db 100644 --- a/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php @@ -50,6 +50,7 @@ use Psalm\Internal\Type\TemplateResult; use Psalm\Issue\ForbiddenCode; use Psalm\Issue\UnrecognizedExpression; +use Psalm\Issue\UnsupportedReferenceUsage; use Psalm\IssueBuffer; use Psalm\Plugin\EventHandler\Event\AfterExpressionAnalysisEvent; use Psalm\Storage\FunctionLikeParameter; @@ -174,26 +175,7 @@ private static function handleExpression( } if ($stmt instanceof PhpParser\Node\Expr\Assign) { - $assignment_type = AssignmentAnalyzer::analyze( - $statements_analyzer, - $stmt->var, - $stmt->expr, - null, - $context, - $stmt->getDocComment(), - [], - !$from_stmt ? $stmt : null - ); - - if ($assignment_type === false) { - return false; - } - - if (!$from_stmt) { - $statements_analyzer->node_data->setType($stmt, $assignment_type); - } - - return true; + return self::analyzeAssignment($statements_analyzer, $stmt, $context, $from_stmt); } if ($stmt instanceof PhpParser\Node\Expr\AssignOp) { @@ -372,7 +354,20 @@ private static function handleExpression( } if ($stmt instanceof PhpParser\Node\Expr\AssignRef) { - return AssignmentAnalyzer::analyzeAssignmentRef($statements_analyzer, $stmt, $context); + if (!AssignmentAnalyzer::analyzeAssignmentRef($statements_analyzer, $stmt, $context)) { + IssueBuffer::maybeAdd( + new UnsupportedReferenceUsage( + "This reference cannot be analyzed by Psalm", + new CodeLocation($statements_analyzer->getSource(), $stmt) + ), + $statements_analyzer->getSuppressedIssues(), + ); + + // Analyze as if it were a normal assignent and just pretend the reference doesn't exist + return self::analyzeAssignment($statements_analyzer, $stmt, $context, $from_stmt); + } + + return true; } if ($stmt instanceof PhpParser\Node\Expr\ErrorSuppress) { @@ -530,4 +525,35 @@ public static function isMock(string $fq_class_name): bool { return in_array(strtolower($fq_class_name), Config::getInstance()->getMockClasses(), true); } + + /** + * @param PhpParser\Node\Expr\Assign|PhpParser\Node\Expr\AssignRef $stmt + */ + private static function analyzeAssignment( + StatementsAnalyzer $statements_analyzer, + PhpParser\Node\Expr $stmt, + Context $context, + bool $from_stmt + ): bool { + $assignment_type = AssignmentAnalyzer::analyze( + $statements_analyzer, + $stmt->var, + $stmt->expr, + null, + $context, + $stmt->getDocComment(), + [], + !$from_stmt ? $stmt : null + ); + + if ($assignment_type === false) { + return false; + } + + if (!$from_stmt) { + $statements_analyzer->node_data->setType($stmt, $assignment_type); + } + + return true; + } } diff --git a/src/Psalm/Internal/Analyzer/Statements/GlobalAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/GlobalAnalyzer.php index 9b7624ee394..d2db43f23cd 100644 --- a/src/Psalm/Internal/Analyzer/Statements/GlobalAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/GlobalAnalyzer.php @@ -12,7 +12,6 @@ use Psalm\Internal\ReferenceConstraint; use Psalm\Issue\InvalidGlobal; use Psalm\IssueBuffer; -use RuntimeException; use function is_string; @@ -74,11 +73,7 @@ public static function analyze( 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; + $context->decrementReferenceCount($var_id); unset($context->references_in_scope[$var_id]); } $statements_analyzer->registerVariable( diff --git a/src/Psalm/Issue/UnsupportedReferenceUsage.php b/src/Psalm/Issue/UnsupportedReferenceUsage.php new file mode 100644 index 00000000000..d5adaf27c52 --- /dev/null +++ b/src/Psalm/Issue/UnsupportedReferenceUsage.php @@ -0,0 +1,9 @@ + 'ReferenceReusedFromConfusingScope', ], + 'unsupportedReferenceUsageWithReferenceToArrayOffsetOfArrayOffset' => [ + 'code' => ' */ + $arr = []; + + /** @var non-empty-list */ + $foo = ["foo"]; + + $bar = &$arr[$foo[0]]; + ', + 'error_message' => 'UnsupportedReferenceUsage', + ], + 'unsupportedReferenceUsageContinuesAnalysis' => [ + 'code' => ' */ + $arr = []; + + /** @var non-empty-list */ + $foo = ["foo"]; + + /** @psalm-suppress UnsupportedReferenceUsage */ + $bar = &$arr[$foo[0]]; + + /** @psalm-trace $bar */; + ', + 'error_message' => ' - $bar: string', + ], ]; } }