Skip to content

Commit

Permalink
Merge pull request #10419 from nicelocal/byref_closure_use
Browse files Browse the repository at this point in the history 
Implement by-ref closure use analysis
  • Loading branch information
@orklah
orklah authored Dec 3, 2023
2 parents 62f32f4 + 75633cb commit 1cca558
Show file tree
Hide file tree
Showing 10 changed files with 96 additions and 115 deletions.
2 changes: 1 addition & 1 deletion psalm-baseline.xml
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<?xml version="1.0" encoding="UTF-8"?>
<files psalm-version="5.x-dev@f9f8bacdf1d1931d10c208401aa3189d1958d182">
<files psalm-version="5.x-dev@18a6c0b6e9aade82a2f3cc36e3a644ba70eaf539">
<file src="examples/TemplateChecker.php">
<PossiblyUndefinedIntArrayOffset>
<code><![CDATA[$comment_block->tags['variablesfrom'][0]]]></code>
Expand Down
41 changes: 8 additions & 33 deletions src/Psalm/Internal/Analyzer/ClosureAnalyzer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
use Psalm\Issue\UndefinedVariable;
use Psalm\IssueBuffer;
use Psalm\Type;
use Psalm\Type\Atomic\TMixed;
use Psalm\Type\Atomic\TNamedObject;
use Psalm\Type\Union;

Expand Down Expand Up @@ -133,12 +132,6 @@ public static function analyzeExpression(

$use_var_id = '$' . $use->var->name;

// insert the ref into the current context if passed by ref, as whatever we're passing
// the closure to could execute it straight away.
if ($use->byRef && !$context->hasVariable($use_var_id)) {
$context->vars_in_scope[$use_var_id] = new Union([new TMixed()], ['by_ref' => true]);
}

if ($statements_analyzer->data_flow_graph instanceof VariableUseGraph
&& $context->hasVariable($use_var_id)
) {
Expand All @@ -154,7 +147,7 @@ public static function analyzeExpression(
}

$use_context->vars_in_scope[$use_var_id] =
$context->hasVariable($use_var_id) && !$use->byRef
$context->hasVariable($use_var_id)
? $context->vars_in_scope[$use_var_id]
: Type::getMixed();

Expand Down Expand Up @@ -205,7 +198,12 @@ public static function analyzeExpression(
$use_context->calling_method_id = $context->calling_method_id;
$use_context->phantom_classes = $context->phantom_classes;

$closure_analyzer->analyze($use_context, $statements_analyzer->node_data, $context, false);
$byref_vars = [];
$closure_analyzer->analyze($use_context, $statements_analyzer->node_data, $context, false, $byref_vars);

foreach ($byref_vars as $key => $value) {
$context->vars_in_scope[$key] = $value;
}

if ($closure_analyzer->inferred_impure
&& $statements_analyzer->getSource() instanceof FunctionLikeAnalyzer
Expand All @@ -229,7 +227,7 @@ public static function analyzeExpression(
/**
* @return false|null
*/
public static function analyzeClosureUses(
private static function analyzeClosureUses(
StatementsAnalyzer $statements_analyzer,
PhpParser\Node\Expr\Closure $stmt,
Context $context
Expand Down Expand Up @@ -268,21 +266,6 @@ public static function analyzeClosureUses(
continue;
}

if ($use->byRef) {
$context->vars_in_scope[$use_var_id] = Type::getMixed();
$context->vars_possibly_in_scope[$use_var_id] = true;

if (!$statements_analyzer->hasVariable($use_var_id)) {
$statements_analyzer->registerVariable(
$use_var_id,
new CodeLocation($statements_analyzer, $use->var),
null,
);
}

return null;
}

if (!isset($context->vars_possibly_in_scope[$use_var_id])) {
if ($context->check_variables) {
if (IssueBuffer::accepts(
Expand Down Expand Up @@ -329,14 +312,6 @@ public static function analyzeClosureUses(

continue;
}
} elseif ($use->byRef) {
$new_type = new Union([new TMixed()], [
'parent_nodes' => $context->vars_in_scope[$use_var_id]->parent_nodes,
]);

$context->remove($use_var_id);

$context->vars_in_scope[$use_var_id] = $new_type;
}
}

Expand Down
36 changes: 33 additions & 3 deletions src/Psalm/Internal/Analyzer/FunctionLikeAnalyzer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,8 @@
use Psalm\Issue\UnusedDocblockParam;
use Psalm\Issue\UnusedParam;
use Psalm\IssueBuffer;
use Psalm\Node\Expr\VirtualVariable;
use Psalm\Node\Stmt\VirtualWhile;
use Psalm\Plugin\EventHandler\Event\AfterFunctionLikeAnalysisEvent;
use Psalm\Storage\ClassLikeStorage;
use Psalm\Storage\FunctionLikeParameter;
Expand Down Expand Up @@ -149,14 +151,18 @@ public function __construct($function, SourceAnalyzer $source, FunctionLikeStora

/**
* @param bool $add_mutations whether or not to add mutations to this method
* @param array<string, Union> $byref_vars
* @param-out array<string, Union> $byref_vars
* @return false|null
* @psalm-suppress PossiblyUnusedReturnValue unused but seems important
* @psalm-suppress ComplexMethod Unavoidably complex
*/
public function analyze(
Context $context,
NodeDataProvider $type_provider,
?Context $global_context = null,
bool $add_mutations = false
bool $add_mutations = false,
array &$byref_vars = []
): ?bool {
$storage = $this->storage;

Expand Down Expand Up @@ -235,9 +241,8 @@ public function analyze(

$statements_analyzer = new StatementsAnalyzer($this, $type_provider);

$byref_uses = [];
if ($this instanceof ClosureAnalyzer && $this->function instanceof Closure) {
$byref_uses = [];

foreach ($this->function->uses as $use) {
if (!is_string($use->var->name)) {
continue;
Expand Down Expand Up @@ -352,6 +357,31 @@ public function analyze(
(bool) $template_types,
);

if ($byref_uses) {
$ref_context = clone $context;
$var = '$__tmp_byref_closure_if__' . (int) $this->function->getAttribute('startFilePos');

$ref_context->vars_in_scope[$var] = Type::getBool();

$var = new VirtualVariable(
substr($var, 1),
);
$virtual_while = new VirtualWhile(
$var,
$function_stmts,
);

$statements_analyzer->analyze(
[$virtual_while],
$ref_context,
);

foreach ($byref_uses as $var_id => $_) {
$byref_vars[$var_id] = $ref_context->vars_in_scope[$var_id];
$context->vars_in_scope[$var_id] = $ref_context->vars_in_scope[$var_id];
}
}

if ($storage->pure) {
$context->pure = true;
}
Expand Down
7 changes: 0 additions & 7 deletions src/Psalm/Internal/Analyzer/Statements/Expression/Call/FunctionCallAnalyzer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -404,13 +404,6 @@ public static function analyze(
}
}

if ($function_call_info->byref_uses) {
foreach ($function_call_info->byref_uses as $byref_use_var => $_) {
$context->vars_in_scope['$' . $byref_use_var] = Type::getMixed();
$context->vars_possibly_in_scope['$' . $byref_use_var] = true;
}
}

if ($function_name instanceof PhpParser\Node\Name && $function_call_info->function_id) {
NamedFunctionCallHandler::handle(
$statements_analyzer,
Expand Down
2 changes: 0 additions & 2 deletions src/Psalm/Internal/Diff/ClassStatementsDiffer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,6 @@ static function (
$start_diff = $b_start - $a_start;
$line_diff = $b->getLine() - $a->getLine();

/** @psalm-suppress MixedArrayAssignment */
$diff_map[] = [$a_start, $a_end, $start_diff, $line_diff];

return true;
Expand Down Expand Up @@ -183,7 +182,6 @@ static function (
}

if (!$signature_change && !$body_change) {
/** @psalm-suppress MixedArrayAssignment */
$diff_map[] = [$a_start, $a_end, $b_start - $a_start, $b->getLine() - $a->getLine()];
}

Expand Down
7 changes: 0 additions & 7 deletions src/Psalm/Internal/LanguageServer/LanguageServer.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -777,11 +777,8 @@ function (IssueData $issue_data): Diagnostic {
//Process Baseline
$file = $issue_data->file_name;
$type = $issue_data->type;
/** @psalm-suppress MixedArrayAccess */
if (isset($issue_baseline[$file][$type]) && $issue_baseline[$file][$type]['o'] > 0) {
/** @psalm-suppress MixedArrayAccess, MixedArgument */
if ($issue_baseline[$file][$type]['o'] === count($issue_baseline[$file][$type]['s'])) {
/** @psalm-suppress MixedArrayAccess, MixedAssignment */
$position = array_search(
str_replace("\r\n", "\n", trim($issue_data->selected_text)),
$issue_baseline[$file][$type]['s'],
Expand All @@ -790,16 +787,12 @@ function (IssueData $issue_data): Diagnostic {

if ($position !== false) {
$issue_data->severity = IssueData::SEVERITY_INFO;
/** @psalm-suppress MixedArgument */
array_splice($issue_baseline[$file][$type]['s'], $position, 1);
/** @psalm-suppress MixedArrayAssignment, MixedOperand, MixedAssignment */
$issue_baseline[$file][$type]['o']--;
}
} else {
/** @psalm-suppress MixedArrayAssignment */
$issue_baseline[$file][$type]['s'] = [];
$issue_data->severity = IssueData::SEVERITY_INFO;
/** @psalm-suppress MixedArrayAssignment, MixedOperand, MixedAssignment */
$issue_baseline[$file][$type]['o']--;
}
}
Expand Down
12 changes: 1 addition & 11 deletions tests/CallableTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,9 @@ function run_function(\Closure $fnc) {
/**
* @return void
* @psalm-suppress MixedArgument
*/
function f() {
$data = 0;
run_function(
/**
* @return void
Expand Down Expand Up @@ -1786,16 +1786,6 @@ function takesCallable(callable $c) : void {}
takesCallable(function() { return; });',
],
'byRefUsesAlwaysMixed' => [
'code' => '<?php
$callback = function() use (&$isCalled) : void {
$isCalled = true;
};
$isCalled = false;
$callback();
if ($isCalled === true) {}',
],
'notCallableListNoUndefinedClass' => [
'code' => '<?php
/**
Expand Down
84 changes: 53 additions & 31 deletions tests/ClosureTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,28 +17,63 @@ public function providerValidCodeParse(): iterable
return [
'byRefUseVar' => [
'code' => '<?php
/** @return void */
function run_function(\Closure $fnc) {
$fnc();
}
$doNotContaminate = 123;
/**
* @return void
* @psalm-suppress MixedArgument
*/
function f() {
run_function(
/**
* @return void
*/
function() use(&$data) {
$data = 1;
$test = 123;
$testBefore = $test;
$testInsideBefore = null;
$testInsideAfter = null;
$v = function () use (&$test, &$testInsideBefore, &$testInsideAfter, $doNotContaminate): void {
$testInsideBefore = $test;
$test = "test";
$testInsideAfter = $test;
$doNotContaminate = "test";
};
',
'assertions' => [
'$testBefore===' => '123',
'$testInsideBefore===' => "'test'|123|null",
'$testInsideAfter===' => "'test'|null",
'$test===' => "'test'|123",

'$doNotContaminate===' => '123',
],
],
'byRefUseSelf' => [
'code' => '<?php
$external = random_int(0, 1);
$v = function (bool $callMe) use (&$v, $external): void {
echo($external.PHP_EOL);
if ($callMe) {
$v(false);
}
};
$v(true);',
],
'byRefUseVarChangeType' => [
'code' => '<?php
function a(string $arg): int {
$v = function() use (&$arg): void {
if (is_integer($arg)) {
echo $arg;
}
);
echo $data;
if (random_bytes(1)) {
$arg = 123;
}
};
$v();
$v();
return 0;
}
f();',
a("test");',
],
'inferredArg' => [
'code' => '<?php
Expand Down Expand Up @@ -1268,19 +1303,6 @@ function takesB(B $_b) : void {}
takesB($getAButReallyB());',
'error_message' => 'ArgumentTypeCoercion - src' . DIRECTORY_SEPARATOR . 'somefile.php:13:28 - Argument 1 of takesB expects B, but parent type A provided',
],
'closureByRefUseToMixed' => [
'code' => '<?php
function assertInt(int $int): int {
$s = static function() use(&$int): void {
$int = "42";
};
$s();
return $int;
}',
'error_message' => 'MixedReturnStatement',
],
'noCrashWhenComparingIllegitimateCallable' => [
'code' => '<?php
class C {}
Expand Down
13 changes: 0 additions & 13 deletions tests/ReferenceConstraintTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,19 +48,6 @@ function useString(string &$s) : void {}
useString($a->getString());',
],
'makeByRefUseMixed' => [
'code' => '<?php
function s(?string $p): void {}
$var = 1;
$callback = function() use(&$var): void {
s($var);
};
$var = null;
$callback();',
'assertions' => [],
'ignored_issues' => ['MixedArgument'],
],
'assignByRefToMixed' => [
'code' => '<?php
function testRef() : array {
Expand Down
Loading

0 comments on commit 1cca558

Please sign in to comment.