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.

Sign up for GitHub

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

Jump to bottom

Fix some minor issues with references. #7684

Merged
merged 2 commits into from Feb 20, 2022
Merged

Fix some minor issues with references. #7684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
580278d
Fix some minor issues with references.
AndrolGenhald Feb 17, 2022
8f710cc
Fix crash when data_flow_graph is null.
AndrolGenhald Feb 17, 2022
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
14 changes: 12 additions & 2 deletions src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php
View file
Open in desktop
Expand Up @@ -923,7 +923,7 @@ public static function analyzeAssignmentRef(
)
);
}
if (!empty($var_comments)) {
if (!empty($var_comments) && $var_comments[0]->type !== null && $var_comments[0]->var_id === null) {
IssueBuffer::maybeAdd(
new InvalidDocblock(
"Docblock type cannot be used for reference assignment",
Expand Down Expand Up @@ -988,7 +988,8 @@ public static function analyzeAssignmentRef(

$context->vars_in_scope[$lhs_var_id]->parent_nodes[$lhs_node->id] = $lhs_node;

if (($stmt->var instanceof ArrayDimFetch || $stmt->var instanceof PropertyFetch)
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)
) {
Expand All @@ -1004,6 +1005,15 @@ public static function analyzeAssignmentRef(
: "property-assignment-as-reference"
);
}

if ($root_var_id === '$this') {
// Variables on `$this` are always used
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mean that $this->whatever = $var constitutes a use of $var (but not necessarily $this->whatever)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it constitutes use of $this->whatever as well. We don't currently seem to track unused properties, and this caused me some issues.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reference assignment actually catches more than regular assignment for properties. I'm not too sure if it'd be better to just make it work the same or if non-reference assignment should be improved, so I decided to start small and make $this a special case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't currently seem to track unused properties,

We actually do, but you have to use the class itself and enable Detect unused classes and methods checkbox on psalm.dev: https://psalm.dev/r/031e79174f

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, I missed that the class had to be used. That means this PR causes this to not mark Foo::$bar as unused, but I think that's an acceptable tradeoff for now. To fix it we'd need to somehow mark the assignment as used, but still keep the property itself as unused.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems ok if it only involves references

$statements_analyzer->data_flow_graph->addPath(
$lhs_node,
new DataFlowNode('variable-use', 'variable use', null),
'variable-use',
);
}
}

if ($stmt->var instanceof ArrayDimFetch) {
Expand Down
10 changes: 10 additions & 0 deletions tests/ReferenceTest.php
View file
Open in desktop
Expand Up @@ -247,6 +247,16 @@ function func(array &$a): void
',
'assertions' => [],
],
'allowDocblockTypingOtherVariable' => [
'code' => '<?php
$a = 1;
/** @var string $a */
$b = &$a;
',
'assertions' => [
'$b' => 'string',
],
],
];
}

Expand Down
14 changes: 14 additions & 0 deletions tests/UnusedVariableTest.php
View file
Open in desktop
Expand Up @@ -2587,6 +2587,20 @@ function takesAnInt(): void {
}
}',
],
'referenceInPropertyIsNotUnused' => [
'code' => '<?php
class Foo
{
/** @var int|null */
public $bar = null;
public function setBarRef(int $ref): void
{
$this->bar = &$ref;
}
}
',
],
];
}

Expand Down