Skip to content

Commit

Permalink
Better understand nesting loops and switches
Browse files Browse the repository at this point in the history 
Fixes #2700
  • Loading branch information
@muglug
muglug committed Jan 27, 2020
1 parent 563032c commit 1580845
Show file tree
Hide file tree
Showing 14 changed files with 103 additions and 39 deletions.
9 changes: 6 additions & 3 deletions src/Psalm/Context.php
View file
Expand Up @@ -266,11 +266,14 @@ class Context
public $branch_point;

/**
* If we're inside case statements we allow continue; statements as an alias of break;
* What does break mean in this context?
*
* @var bool
* 'loop' means we're breaking out of a loop,
* 'switch' means we're breaking out of a switch
*
* @var list<'loop'|'switch'>
*/
public $inside_case = false;
public $break_types = [];

/**
* @var bool
Expand Down
2 changes: 1 addition & 1 deletion src/Psalm/Internal/Analyzer/FunctionLike/ReturnTypeAnalyzer.php
View file
Expand Up @@ -188,7 +188,7 @@ public static function verifyReturnType(
$function_stmts,
$type_provider,
$codebase->config->exit_functions,
false,
[],
false
) !== [ScopeAnalyzer::ACTION_END]
) {
Expand Down
54 changes: 39 additions & 15 deletions src/Psalm/Internal/Analyzer/ScopeAnalyzer.php
View file
Expand Up @@ -22,6 +22,15 @@ class ScopeAnalyzer
const ACTION_NONE = 'NONE';
const ACTION_RETURN = 'RETURN';

private const ACTIONS = [
self::ACTION_END,
self::ACTION_BREAK,
self::ACTION_CONTINUE,
self::ACTION_LEAVE_SWITCH,
self::ACTION_NONE,
self::ACTION_RETURN
];

/**
* @param array<PhpParser\Node\Stmt> $stmts
*
Expand Down Expand Up @@ -62,16 +71,16 @@ public static function doesEverBreak(array $stmts)

/**
* @param array<PhpParser\Node> $stmts
* @param bool $in_switch when checking inside a switch statement, continue is an alias of break
* @param bool $return_is_exit Exit and Throw statements are treated differently from return if this is false
* @param list<'loop'|'switch'> $break_types
*
* @return string[] one or more of 'LEAVE', 'CONTINUE', 'BREAK' (or empty if no single action is found)
* @return list<value-of<self::ACTIONS>>
*/
public static function getFinalControlActions(
array $stmts,
?\Psalm\Internal\Provider\NodeDataProvider $nodes,
array $exit_functions,
$in_switch = false,
array $break_types = [],
$return_is_exit = true
) {
if (empty($stmts)) {
Expand Down Expand Up @@ -146,7 +155,8 @@ public static function getFinalControlActions(
}

if ($stmt instanceof PhpParser\Node\Stmt\Continue_) {
if ($in_switch
if ($break_types
&& end($break_types) === 'switch'
&& (!$stmt->num || !$stmt->num instanceof PhpParser\Node\Scalar\LNumber || $stmt->num->value < 2)
) {
return [self::ACTION_LEAVE_SWITCH];
Expand All @@ -156,7 +166,8 @@ public static function getFinalControlActions(
}

if ($stmt instanceof PhpParser\Node\Stmt\Break_) {
if ($in_switch
if ($break_types
&& end($break_types) === 'switch'
&& (!$stmt->num || !$stmt->num instanceof PhpParser\Node\Scalar\LNumber || $stmt->num->value < 2)
) {
return [self::ACTION_LEAVE_SWITCH];
Expand All @@ -166,9 +177,15 @@ public static function getFinalControlActions(
}

if ($stmt instanceof PhpParser\Node\Stmt\If_) {
$if_statement_actions = self::getFinalControlActions($stmt->stmts, $nodes, $exit_functions, $in_switch);
$if_statement_actions = self::getFinalControlActions(
$stmt->stmts,
$nodes,
$exit_functions,
$break_types
);

$else_statement_actions = $stmt->else
? self::getFinalControlActions($stmt->else->stmts, $nodes, $exit_functions, $in_switch)
? self::getFinalControlActions($stmt->else->stmts, $nodes, $exit_functions, $break_types)
: [];

$all_same = count($if_statement_actions) === 1
Expand All @@ -183,7 +200,7 @@ public static function getFinalControlActions(
$elseif->stmts,
$nodes,
$exit_functions,
$in_switch
$break_types
);

$all_same = $all_same && $elseif_control_actions == $if_statement_actions;
Expand Down Expand Up @@ -215,7 +232,7 @@ public static function getFinalControlActions(
for ($d = count($stmt->cases) - 1; $d >= 0; --$d) {
$case = $stmt->cases[$d];

$case_actions = self::getFinalControlActions($case->stmts, $nodes, $exit_functions, true);
$case_actions = self::getFinalControlActions($case->stmts, $nodes, $exit_functions, ['switch']);

if (array_intersect([
self::ACTION_LEAVE_SWITCH,
Expand Down Expand Up @@ -252,8 +269,15 @@ public static function getFinalControlActions(

if ($stmt instanceof PhpParser\Node\Stmt\Do_
|| $stmt instanceof PhpParser\Node\Stmt\While_
|| $stmt instanceof PhpParser\Node\Stmt\Foreach_
|| $stmt instanceof PhpParser\Node\Stmt\For_
) {
$do_actions = self::getFinalControlActions($stmt->stmts, $nodes, $exit_functions);
$do_actions = self::getFinalControlActions(
$stmt->stmts,
$nodes,
$exit_functions,
array_merge($break_types, ['loop'])
);

$control_actions = array_merge($control_actions, $do_actions);
}
Expand All @@ -263,7 +287,7 @@ public static function getFinalControlActions(
$stmt->stmts,
$nodes,
$exit_functions,
$in_switch
$break_types
);

if ($stmt->catches) {
Expand All @@ -274,7 +298,7 @@ public static function getFinalControlActions(
$catch->stmts,
$nodes,
$exit_functions,
$in_switch
$break_types
);

$all_same = $all_same && $try_statement_actions == $catch_actions;
Expand All @@ -295,7 +319,7 @@ public static function getFinalControlActions(
$stmt->finally->stmts,
$nodes,
$exit_functions,
$in_switch
$break_types
);

if (!in_array(self::ACTION_NONE, $finally_statement_actions, true)) {
Expand All @@ -308,13 +332,13 @@ public static function getFinalControlActions(
}
}

$control_actions = array_merge($control_actions, $try_statement_actions);
$control_actions = \array_merge($control_actions, $try_statement_actions);
}
}

$control_actions[] = self::ACTION_NONE;

return array_unique($control_actions);
return array_values(array_unique($control_actions));
}

/**
Expand Down
3 changes: 1 addition & 2 deletions src/Psalm/Internal/Analyzer/Statements/Block/DoAnalyzer.php
View file
Expand Up @@ -32,8 +32,7 @@ public static function analyze(
Context $context
) {
$do_context = clone $context;

$do_context->inside_case = false;
$do_context->break_types[] = 'loop';

$codebase = $statements_analyzer->getCodebase();

Expand Down
2 changes: 1 addition & 1 deletion src/Psalm/Internal/Analyzer/Statements/Block/ForAnalyzer.php
View file
Expand Up @@ -66,7 +66,7 @@ public static function analyze(
$for_context = clone $context;

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

$codebase = $statements_analyzer->getCodebase();

Expand Down
2 changes: 1 addition & 1 deletion src/Psalm/Internal/Analyzer/Statements/Block/ForeachAnalyzer.php
View file
Expand Up @@ -226,7 +226,7 @@ public static function analyze(
}

$foreach_context->inside_loop = true;
$foreach_context->inside_case = false;
$foreach_context->break_types[] = 'loop';

if ($codebase->alter_code) {
$foreach_context->branch_point =
Expand Down
6 changes: 3 additions & 3 deletions src/Psalm/Internal/Analyzer/Statements/Block/IfAnalyzer.php
View file
Expand Up @@ -746,7 +746,7 @@ protected static function analyzeIfBlock(
$stmt->stmts,
$statements_analyzer->node_data,
$codebase->config->exit_functions,
$outer_context->inside_case
$outer_context->break_types
);

$has_ending_statements = $final_actions === [ScopeAnalyzer::ACTION_END];
Expand Down Expand Up @@ -1254,7 +1254,7 @@ function ($carry, Clause $clause) {
$elseif->stmts,
$statements_analyzer->node_data,
$codebase->config->exit_functions,
$outer_context->inside_case
$outer_context->break_types
);
// has a return/throw at end
$has_ending_statements = $final_actions === [ScopeAnalyzer::ACTION_END];
Expand Down Expand Up @@ -1605,7 +1605,7 @@ protected static function analyzeElseBlock(
$else->stmts,
$statements_analyzer->node_data,
$codebase->config->exit_functions,
$outer_context->inside_case
$outer_context->break_types
)
: [ScopeAnalyzer::ACTION_NONE];
// has a return/throw at end
Expand Down
3 changes: 2 additions & 1 deletion src/Psalm/Internal/Analyzer/Statements/Block/LoopAnalyzer.php
View file
Expand Up @@ -88,7 +88,8 @@ public static function analyze(
$final_actions = ScopeAnalyzer::getFinalControlActions(
$stmts,
$statements_analyzer->node_data,
Config::getInstance()->exit_functions
Config::getInstance()->exit_functions,
$loop_scope->loop_context->break_types
);

$does_always_break = $final_actions === [ScopeAnalyzer::ACTION_BREAK];
Expand Down
4 changes: 2 additions & 2 deletions src/Psalm/Internal/Analyzer/Statements/Block/SwitchAnalyzer.php
View file
Expand Up @@ -77,7 +77,7 @@ public static function analyze(
$case->stmts,
$statements_analyzer->node_data,
$config->exit_functions,
true
['switch']
);

if (!in_array(ScopeAnalyzer::ACTION_NONE, $case_actions, true)) {
Expand Down Expand Up @@ -428,7 +428,7 @@ private static function handleCase(
);
}

$case_context->inside_case = true;
$case_context->break_types[] = 'switch';

$switch_scope->leftover_statements = [];
$switch_scope->leftover_case_equality_expr = null;
Expand Down
4 changes: 2 additions & 2 deletions src/Psalm/Internal/Analyzer/Statements/Block/TryAnalyzer.php
View file
Expand Up @@ -90,7 +90,7 @@ public static function analyze(
$stmt->stmts,
$statements_analyzer->node_data,
$codebase->config->exit_functions,
$context->inside_case
$context->break_types
);

/** @var array<string, bool> */
Expand Down Expand Up @@ -355,7 +355,7 @@ function ($fq_catch_class) use ($codebase) {
$catch->stmts,
$statements_analyzer->node_data,
$codebase->config->exit_functions,
$context->inside_case
$context->break_types
);

foreach ($issues_to_suppress as $issue_to_suppress) {
Expand Down
2 changes: 1 addition & 1 deletion src/Psalm/Internal/Analyzer/Statements/Block/WhileAnalyzer.php
View file
Expand Up @@ -40,7 +40,7 @@ public static function analyze(
$while_context = clone $context;

$while_context->inside_loop = true;
$while_context->inside_case = false;
$while_context->break_types[] = 'loop';

$codebase = $statements_analyzer->getCodebase();

Expand Down
33 changes: 27 additions & 6 deletions src/Psalm/Internal/Analyzer/StatementsAnalyzer.php
View file
Expand Up @@ -391,10 +391,21 @@ function ($line) {
SwitchAnalyzer::analyze($this, $stmt, $context);
} elseif ($stmt instanceof PhpParser\Node\Stmt\Break_) {
$loop_scope = $context->loop_scope;

$leaving_switch = true;

if ($loop_scope && $original_context) {
if ($context->inside_case && !$stmt->num) {
if ($context->break_types
&& \end($context->break_types) === 'switch'
&& (!$stmt->num
|| !$stmt->num instanceof PhpParser\Node\Scalar\LNumber
|| $stmt->num->value < 2
)
) {
$loop_scope->final_actions[] = ScopeAnalyzer::ACTION_LEAVE_SWITCH;
} else {
$leaving_switch = false;

$loop_scope->final_actions[] = ScopeAnalyzer::ACTION_BREAK;
}

Expand Down Expand Up @@ -432,7 +443,7 @@ function ($line) {
}
}

if ($context->collect_references && (!$context->case_scope || $stmt->num)) {
if ($context->collect_references && !$leaving_switch) {
foreach ($context->unreferenced_vars as $var_id => $locations) {
if (isset($loop_scope->unreferenced_vars[$var_id])) {
$loop_scope->unreferenced_vars[$var_id] += $locations;
Expand All @@ -446,7 +457,7 @@ function ($line) {
}

$case_scope = $context->case_scope;
if ($case_scope) {
if ($case_scope && $leaving_switch) {
foreach ($context->vars_in_scope as $var_id => $type) {
if ($case_scope->parent_context !== $context) {
if ($case_scope->break_vars === null) {
Expand Down Expand Up @@ -477,8 +488,11 @@ function ($line) {
$has_returned = true;
} elseif ($stmt instanceof PhpParser\Node\Stmt\Continue_) {
$loop_scope = $context->loop_scope;

$leaving_switch = true;

if ($loop_scope === null) {
if (!$context->inside_case) {
if (!$context->break_types) {
if (IssueBuffer::accepts(
new ContinueOutsideLoop(
'Continue call outside loop context',
Expand All @@ -490,9 +504,16 @@ function ($line) {
}
}
} elseif ($original_context) {
if ($context->inside_case && !$stmt->num) {
if ($context->break_types
&& \end($context->break_types) === 'switch'
&& (!$stmt->num
|| !$stmt->num instanceof PhpParser\Node\Scalar\LNumber
|| $stmt->num->value < 2
)
) {
$loop_scope->final_actions[] = ScopeAnalyzer::ACTION_LEAVE_SWITCH;
} else {
$leaving_switch = false;
$loop_scope->final_actions[] = ScopeAnalyzer::ACTION_CONTINUE;
}

Expand Down Expand Up @@ -546,7 +567,7 @@ function ($line) {
}

$case_scope = $context->case_scope;
if ($case_scope && $context->collect_references) {
if ($case_scope && $context->collect_references && $leaving_switch) {
foreach ($context->unreferenced_vars as $var_id => $locations) {
if (isset($case_scope->unreferenced_vars[$var_id])) {
$case_scope->unreferenced_vars[$var_id] += $locations;
Expand Down
2 changes: 1 addition & 1 deletion src/Psalm/Internal/Visitor/ReflectorVisitor.php
View file
Expand Up @@ -1919,7 +1919,7 @@ private function registerFunctionLike(PhpParser\Node\FunctionLike $stmt, $fake_m
$function_stmt->stmts,
null,
$this->config->exit_functions,
false,
[],
false
);

Expand Down

0 comments on commit 1580845

Please sign in to comment.