Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve handling of unsupported references (fixes #8018). #8022

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions config.xsd
Expand Up @@ -467,6 +467,7 @@
<xs:element name="UnresolvableInclude" type="IssueHandlerType" minOccurs="0" />
<xs:element name="UnsafeGenericInstantiation" type="IssueHandlerType" minOccurs="0" />
<xs:element name="UnsafeInstantiation" type="IssueHandlerType" minOccurs="0" />
<xs:element name="UnsupportedReferenceUsage" type="IssueHandlerType" minOccurs="0" />
<xs:element name="UnusedClass" type="ClassIssueHandlerType" minOccurs="0" />
<xs:element name="UnusedClosureParam" type="IssueHandlerType" minOccurs="0" />
<xs:element name="UnusedConstructor" type="MethodIssueHandlerType" minOccurs="0" />
Expand Down
1 change: 1 addition & 0 deletions docs/running_psalm/issues.md
Expand Up @@ -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)
Expand Down
34 changes: 34 additions & 0 deletions 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
<?php

/** @var non-empty-list<int> */
$bar = [1, 2, 3];
/** @var non-empty-list<int> */
$baz = [1, 2, 3];

$foo = &$bar[$baz[0]];
```

can be turned into

```php
<?php

/** @var non-empty-list<int> */
$bar = [1, 2, 3];
/** @var non-empty-list<int> */
$baz = [1, 2, 3];

$offset = $baz[0];
$foo = &$bar[$offset];
```
25 changes: 20 additions & 5 deletions src/Psalm/Context.php
Expand Up @@ -2,6 +2,7 @@

namespace Psalm;

use InvalidArgumentException;
use Psalm\Internal\Analyzer\StatementsAnalyzer;
use Psalm\Internal\Clause;
use Psalm\Internal\ReferenceConstraint;
Expand Down Expand Up @@ -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],
Expand All @@ -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<string, bool> $changed_var_ids
Expand Down
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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];
Expand Down Expand Up @@ -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
);
}
Comment on lines +978 to +985
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It turns out the bug this pull request fixes actually affected this method, causing everything after $context->vars_in_scope[$lhs_var_id] = &$context->vars_in_scope[$rhs_var_id]; to be ignored.

After it was fixed, that same line was reported as an unused variable, so I finally gave up and just decided that putting a reference in an object or an array is a bad idea anyway, and it's not worth it to track usage in that case (this is basically increasing the scope of the change in #7684 from only ignoring unused references on $this to ignoring unused references on any object or array).


$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;
Expand Down
68 changes: 47 additions & 21 deletions src/Psalm/Internal/Analyzer/Statements/ExpressionAnalyzer.php
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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;
}
}
7 changes: 1 addition & 6 deletions src/Psalm/Internal/Analyzer/Statements/GlobalAnalyzer.php
Expand Up @@ -12,7 +12,6 @@
use Psalm\Internal\ReferenceConstraint;
use Psalm\Issue\InvalidGlobal;
use Psalm\IssueBuffer;
use RuntimeException;

use function is_string;

Expand Down Expand Up @@ -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(
Expand Down
9 changes: 9 additions & 0 deletions src/Psalm/Issue/UnsupportedReferenceUsage.php
@@ -0,0 +1,9 @@
<?php

namespace Psalm\Issue;

final class UnsupportedReferenceUsage extends CodeIssue
{
public const ERROR_LEVEL = 2;
public const SHORTCODE = 312;
}
27 changes: 27 additions & 0 deletions tests/ReferenceTest.php
Expand Up @@ -407,6 +407,33 @@ public function providerInvalidCodeParse(): iterable
',
'error_message' => 'ReferenceReusedFromConfusingScope',
],
'unsupportedReferenceUsageWithReferenceToArrayOffsetOfArrayOffset' => [
'code' => '<?php
/** @var array<string, string> */
$arr = [];

/** @var non-empty-list<string> */
$foo = ["foo"];

$bar = &$arr[$foo[0]];
',
'error_message' => 'UnsupportedReferenceUsage',
],
'unsupportedReferenceUsageContinuesAnalysis' => [
'code' => '<?php
/** @var array<string, string> */
$arr = [];

/** @var non-empty-list<string> */
$foo = ["foo"];

/** @psalm-suppress UnsupportedReferenceUsage */
$bar = &$arr[$foo[0]];

/** @psalm-trace $bar */;
',
'error_message' => ' - $bar: string',
],
];
}
}