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

Deduplicate loop analysis #10782

Open
wants to merge 3 commits into
base: 5.x
Choose a base branch
from
Open

Deduplicate loop analysis #10782

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
45dc937
Deduplicate setting loop vars
theodorejb Jan 19, 2024
bccec0c
Deduplicate loop analysis
theodorejb Jan 19, 2024
2176519
Merge remote-tracking branch 'upstream/5.x' into dedupe-loop-analysis
theodorejb Mar 20, 2024
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
23 changes: 1 addition & 22 deletions psalm-baseline.xml
View file
Open in desktop
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@a46a07c5ca8bd7b54d1ca814c1f5b5ed38a0ba90">
<files psalm-version="5.x-dev@0cb34f15bc851899e8aa4376522dd6126f44b41d">
<file src="examples/TemplateChecker.php">
<PossiblyUndefinedIntArrayOffset>
<code><![CDATA[$comment_block->tags['variablesfrom'][0]]]></code>
Expand Down Expand Up @@ -297,18 +297,10 @@
<code><![CDATA[$source_pos = strpos($source, '*')]]></code>
</RiskyTruthyFalsyComparison>
</file>
<file src="src/Psalm/Internal/Analyzer/Statements/Block/DoAnalyzer.php">
<RiskyTruthyFalsyComparison>
<code><![CDATA[$do_context->branch_point]]></code>
</RiskyTruthyFalsyComparison>
</file>
<file src="src/Psalm/Internal/Analyzer/Statements/Block/ForAnalyzer.php">
<ArgumentTypeCoercion>
<code><![CDATA[$stmt->cond]]></code>
</ArgumentTypeCoercion>
<RiskyTruthyFalsyComparison>
<code><![CDATA[$for_context->branch_point]]></code>
</RiskyTruthyFalsyComparison>
</file>
<file src="src/Psalm/Internal/Analyzer/Statements/Block/ForeachAnalyzer.php">
<ConflictingReferenceConstraint>
Expand All @@ -319,7 +311,6 @@
<code><![CDATA[!$var_comment->var_id]]></code>
<code><![CDATA[!$var_comment->var_id]]></code>
<code><![CDATA[$calling_type_params]]></code>
<code><![CDATA[$foreach_context->branch_point]]></code>
<code><![CDATA[$generic_storage->template_types]]></code>
<code><![CDATA[$statements_analyzer->getTemplateTypeMap()]]></code>
<code><![CDATA[$var_comment->line_number]]></code>
Expand Down Expand Up @@ -350,7 +341,6 @@
<RiskyTruthyFalsyComparison>
<code><![CDATA[$context->branch_point]]></code>
<code><![CDATA[$else_context->branch_point]]></code>
<code><![CDATA[$else_context->branch_point]]></code>
<code><![CDATA[$if_scope->assigned_var_ids]]></code>
<code><![CDATA[$if_scope->new_vars]]></code>
<code><![CDATA[$if_scope->redefined_vars]]></code>
Expand Down Expand Up @@ -378,24 +368,13 @@
<code><![CDATA[$traverser->traverse([$switch_condition])[0]]]></code>
</PossiblyUndefinedIntArrayOffset>
<RiskyTruthyFalsyComparison>
<code><![CDATA[$case_context->branch_point]]></code>
<code><![CDATA[$nested_or_options]]></code>
<code><![CDATA[$switch_var_id]]></code>
<code><![CDATA[$switch_var_id]]></code>
<code><![CDATA[$switch_var_id]]></code>
<code><![CDATA[$type_statements]]></code>
</RiskyTruthyFalsyComparison>
</file>
<file src="src/Psalm/Internal/Analyzer/Statements/Block/TryAnalyzer.php">
<RiskyTruthyFalsyComparison>
<code><![CDATA[$try_context->branch_point]]></code>
</RiskyTruthyFalsyComparison>
</file>
<file src="src/Psalm/Internal/Analyzer/Statements/Block/WhileAnalyzer.php">
<RiskyTruthyFalsyComparison>
<code><![CDATA[$while_context->branch_point]]></code>
</RiskyTruthyFalsyComparison>
</file>
<file src="src/Psalm/Internal/Analyzer/Statements/Expression/ArrayAnalyzer.php">
<RiskyTruthyFalsyComparison>
<code><![CDATA[$var_id]]></code>
Expand Down
2 changes: 1 addition & 1 deletion src/Psalm/Context.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ final class Context
public $is_global = false;

/**
* @var array<string, bool>
* @var array<string, bool|int>
*/
public $protected_var_ids = [];

Expand Down
22 changes: 3 additions & 19 deletions src/Psalm/Internal/Analyzer/Statements/Block/DoAnalyzer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,9 @@
use Psalm\Context;
use Psalm\Internal\Algebra;
use Psalm\Internal\Algebra\FormulaGenerator;
use Psalm\Internal\Analyzer\ScopeAnalyzer;
use Psalm\Internal\Analyzer\StatementsAnalyzer;
use Psalm\Internal\Clause;
use Psalm\Internal\Scope\LoopScope;
use Psalm\Type;
use Psalm\Type\Reconciler;
use UnexpectedValueException;

Expand Down Expand Up @@ -41,8 +39,8 @@ public static function analyze(

$codebase = $statements_analyzer->getCodebase();

if ($codebase->alter_code) {
$do_context->branch_point = $do_context->branch_point ?: (int) $stmt->getAttribute('startFilePos');
if ($codebase->alter_code && $do_context->branch_point === null) {
$do_context->branch_point = (int) $stmt->getAttribute('startFilePos');
}

$loop_scope = new LoopScope($do_context, $context);
Expand Down Expand Up @@ -138,21 +136,7 @@ static function (Clause $c) use ($mixed_var_ids): bool {
);
}

foreach ($inner_loop_context->vars_in_scope as $var_id => $type) {
// if there are break statements in the loop it's not certain
// that the loop has finished executing, so the assertions at the end
// the loop in the while conditional may not hold
if (in_array(ScopeAnalyzer::ACTION_BREAK, $loop_scope->final_actions, true)) {
if (isset($loop_scope->possibly_defined_loop_parent_vars[$var_id])) {
$context->vars_in_scope[$var_id] = Type::combineUnionTypes(
$type,
$loop_scope->possibly_defined_loop_parent_vars[$var_id],
);
}
} else {
$context->vars_in_scope[$var_id] = $type;
}
}
LoopAnalyzer::setLoopVars($inner_loop_context, $context, $loop_scope);

$do_context->loop_scope = null;

Expand Down
134 changes: 7 additions & 127 deletions src/Psalm/Internal/Analyzer/Statements/Block/ForAnalyzer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,10 @@

use PhpParser;
use Psalm\Context;
use Psalm\Internal\Analyzer\ScopeAnalyzer;
use Psalm\Internal\Analyzer\Statements\ExpressionAnalyzer;
use Psalm\Internal\Analyzer\StatementsAnalyzer;
use Psalm\Internal\Scope\LoopScope;
use Psalm\Type;
use UnexpectedValueException;

use function array_merge;
use function count;
use function in_array;
use function is_string;

/**
Expand Down Expand Up @@ -57,129 +51,15 @@ public static function analyze(

$while_true = !$stmt->cond && !$stmt->init && !$stmt->loop;

$pre_context = null;

if ($while_true) {
$pre_context = clone $context;
}

$for_context = clone $context;

$for_context->inside_loop = true;
$for_context->break_types[] = 'loop';

$codebase = $statements_analyzer->getCodebase();

if ($codebase->alter_code) {
$for_context->branch_point = $for_context->branch_point ?: (int) $stmt->getAttribute('startFilePos');
}

$loop_scope = new LoopScope($for_context, $context);

$loop_scope->protected_var_ids = array_merge(
$assigned_var_ids,
$context->protected_var_ids,
);

if (LoopAnalyzer::analyze(
return LoopAnalyzer::analyzeForOrWhile(
$statements_analyzer,
$stmt->stmts,
$stmt,
$context,
$while_true,
$init_var_types,
$assigned_var_ids,
$stmt->cond,
$stmt->loop,
$loop_scope,
$inner_loop_context,
) === false) {
return false;
}

if (!$inner_loop_context) {
throw new UnexpectedValueException('There should be an inner loop context');
}

$always_enters_loop = false;

foreach ($stmt->cond as $cond) {
if ($cond_type = $statements_analyzer->node_data->getType($cond)) {
$always_enters_loop = $cond_type->isAlwaysTruthy();
}

if (count($stmt->init) === 1
&& count($stmt->cond) === 1
&& $cond instanceof PhpParser\Node\Expr\BinaryOp
&& ($cond_value = $statements_analyzer->node_data->getType($cond->right))
&& ($cond_value->isSingleIntLiteral() || $cond_value->isSingleStringLiteral())
&& $cond->left instanceof PhpParser\Node\Expr\Variable
&& is_string($cond->left->name)
&& isset($init_var_types[$cond->left->name])
&& $init_var_types[$cond->left->name]->isSingleIntLiteral()
) {
$init_value = $init_var_types[$cond->left->name]->getSingleLiteral()->value;
$cond_value = $cond_value->getSingleLiteral()->value;

if ($cond instanceof PhpParser\Node\Expr\BinaryOp\Smaller && $init_value < $cond_value) {
$always_enters_loop = true;
break;
}

if ($cond instanceof PhpParser\Node\Expr\BinaryOp\SmallerOrEqual && $init_value <= $cond_value) {
$always_enters_loop = true;
break;
}

if ($cond instanceof PhpParser\Node\Expr\BinaryOp\Greater && $init_value > $cond_value) {
$always_enters_loop = true;
break;
}

if ($cond instanceof PhpParser\Node\Expr\BinaryOp\GreaterOrEqual && $init_value >= $cond_value) {
$always_enters_loop = true;
break;
}
}
}

if ($while_true) {
$always_enters_loop = true;
}

$can_leave_loop = !$while_true
|| in_array(ScopeAnalyzer::ACTION_BREAK, $loop_scope->final_actions, true);

if ($always_enters_loop && $can_leave_loop) {
foreach ($inner_loop_context->vars_in_scope as $var_id => $type) {
// if there are break statements in the loop it's not certain
// that the loop has finished executing, so the assertions at the end
// the loop in the while conditional may not hold
if (in_array(ScopeAnalyzer::ACTION_BREAK, $loop_scope->final_actions, true)
|| in_array(ScopeAnalyzer::ACTION_CONTINUE, $loop_scope->final_actions, true)
) {
if (isset($loop_scope->possibly_defined_loop_parent_vars[$var_id])) {
$context->vars_in_scope[$var_id] = Type::combineUnionTypes(
$type,
$loop_scope->possibly_defined_loop_parent_vars[$var_id],
);
}
} else {
$context->vars_in_scope[$var_id] = $type;
}
}
}

$for_context->loop_scope = null;

if ($can_leave_loop) {
$context->vars_possibly_in_scope = array_merge(
$context->vars_possibly_in_scope,
$for_context->vars_possibly_in_scope,
);
} elseif ($pre_context) {
$context->vars_possibly_in_scope = $pre_context->vars_possibly_in_scope;
}

if ($context->collect_exceptions) {
$context->mergeExceptions($for_context);
}

return null;
);
}
}
5 changes: 2 additions & 3 deletions src/Psalm/Internal/Analyzer/Statements/Block/ForeachAnalyzer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,9 +283,8 @@ public static function analyze(
$foreach_context->inside_loop = true;
$foreach_context->break_types[] = 'loop';

if ($codebase->alter_code) {
$foreach_context->branch_point =
$foreach_context->branch_point ?: (int) $stmt->getAttribute('startFilePos');
if ($codebase->alter_code && $foreach_context->branch_point === null) {
$foreach_context->branch_point = (int) $stmt->getAttribute('startFilePos');
}

if ($stmt->keyVar instanceof PhpParser\Node\Expr\Variable && is_string($stmt->keyVar->name)) {
Expand Down
5 changes: 2 additions & 3 deletions src/Psalm/Internal/Analyzer/Statements/Block/IfElseAnalyzer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,9 +292,8 @@ public static function analyze(
}

if ($stmt->else) {
if ($codebase->alter_code) {
$else_context->branch_point =
$else_context->branch_point ?: (int) $stmt->getAttribute('startFilePos');
if ($codebase->alter_code && $else_context->branch_point === null) {
$else_context->branch_point = (int) $stmt->getAttribute('startFilePos');
}
}

Expand Down