From d41a9a8dcc6f4b7052244808dcf369d7993cb9c9 Mon Sep 17 00:00:00 2001 From: Matthew Brown Date: Thu, 12 Jul 2018 23:26:08 -0400 Subject: [PATCH] Fix #806 - allow specification of functions and static methods that exit --- config.xsd | 7 ++ .../FunctionLike/ReturnTypeChecker.php | 10 ++- src/Psalm/Checker/ScopeChecker.php | 55 ++++++++++++--- .../Checker/Statements/Block/IfChecker.php | 22 ++++-- .../Checker/Statements/Block/LoopChecker.php | 3 +- .../Statements/Block/SwitchChecker.php | 8 ++- .../Checker/Statements/Block/TryChecker.php | 12 ++-- src/Psalm/Config.php | 16 ++++- src/Psalm/Visitor/DependencyFinderVisitor.php | 1 + tests/ConfigTest.php | 70 +++++++++++++++++++ 10 files changed, 180 insertions(+), 24 deletions(-) diff --git a/config.xsd b/config.xsd index 10cfb524e..b10a52224 100644 --- a/config.xsd +++ b/config.xsd @@ -14,6 +14,7 @@ + @@ -92,6 +93,12 @@ + + + + + + diff --git a/src/Psalm/Checker/FunctionLike/ReturnTypeChecker.php b/src/Psalm/Checker/FunctionLike/ReturnTypeChecker.php index 9aac59cc7..157a07e86 100644 --- a/src/Psalm/Checker/FunctionLike/ReturnTypeChecker.php +++ b/src/Psalm/Checker/FunctionLike/ReturnTypeChecker.php @@ -100,7 +100,10 @@ class ReturnTypeChecker ); if ((!$return_type || $return_type->from_docblock) - && ScopeChecker::getFinalControlActions($function_stmts) !== [ScopeChecker::ACTION_END] + && ScopeChecker::getFinalControlActions( + $function_stmts, + $project_checker->config->exit_functions + ) !== [ScopeChecker::ACTION_END] && !$inferred_yield_types && count($inferred_return_type_parts) ) { @@ -119,7 +122,10 @@ class ReturnTypeChecker && !$return_type->from_docblock && !$return_type->isVoid() && !$inferred_yield_types - && ScopeChecker::getFinalControlActions($function_stmts) !== [ScopeChecker::ACTION_END] + && ScopeChecker::getFinalControlActions( + $function_stmts, + $project_checker->config->exit_functions + ) !== [ScopeChecker::ACTION_END] ) { if (IssueBuffer::accepts( new InvalidReturnType( diff --git a/src/Psalm/Checker/ScopeChecker.php b/src/Psalm/Checker/ScopeChecker.php index f6c01d766..5854f4d4d 100644 --- a/src/Psalm/Checker/ScopeChecker.php +++ b/src/Psalm/Checker/ScopeChecker.php @@ -59,6 +59,7 @@ class ScopeChecker */ public static function getFinalControlActions( array $stmts, + array $exit_functions, $in_switch = false, $return_is_exit = true ) { @@ -82,6 +83,36 @@ class ScopeChecker return [self::ACTION_END]; } + if ($stmt instanceof PhpParser\Node\Stmt\Expression) { + if ($exit_functions) { + if ($stmt->expr instanceof PhpParser\Node\Expr\FuncCall + || $stmt->expr instanceof PhpParser\Node\Expr\StaticCall + ) { + if ($stmt->expr instanceof PhpParser\Node\Expr\FuncCall) { + /** @var string|null */ + $resolved_name = $stmt->expr->name->getAttribute('resolvedName'); + + if ($resolved_name && isset($exit_functions[strtolower($resolved_name)])) { + return [self::ACTION_END]; + } + } elseif ($stmt->expr->class instanceof PhpParser\Node\Name + && $stmt->expr->name instanceof PhpParser\Node\Identifier + ) { + /** @var string|null */ + $resolved_class_name = $stmt->expr->class->getAttribute('resolvedName'); + + if ($resolved_class_name + && isset($exit_functions[strtolower($resolved_class_name . '::' . $stmt->expr->name)]) + ) { + return [self::ACTION_END]; + } + } + } + } + + continue; + } + if ($stmt instanceof PhpParser\Node\Stmt\Continue_) { if ($in_switch && (!$stmt->num || !$stmt->num instanceof PhpParser\Node\Scalar\LNumber || $stmt->num->value < 2) @@ -103,9 +134,9 @@ class ScopeChecker } if ($stmt instanceof PhpParser\Node\Stmt\If_) { - $if_statement_actions = self::getFinalControlActions($stmt->stmts, $in_switch); + $if_statement_actions = self::getFinalControlActions($stmt->stmts, $exit_functions, $in_switch); $else_statement_actions = $stmt->else - ? self::getFinalControlActions($stmt->else->stmts, $in_switch) + ? self::getFinalControlActions($stmt->else->stmts, $exit_functions, $in_switch) : []; $all_same = count($if_statement_actions) === 1 @@ -116,7 +147,11 @@ class ScopeChecker if ($stmt->elseifs) { foreach ($stmt->elseifs as $elseif) { - $elseif_control_actions = self::getFinalControlActions($elseif->stmts, $in_switch); + $elseif_control_actions = self::getFinalControlActions( + $elseif->stmts, + $exit_functions, + $in_switch + ); $all_same = $all_same && $elseif_control_actions == $if_statement_actions; @@ -147,7 +182,7 @@ class ScopeChecker for ($d = count($stmt->cases) - 1; $d >= 0; --$d) { $case = $stmt->cases[$d]; - $case_actions = self::getFinalControlActions($case->stmts, true); + $case_actions = self::getFinalControlActions($case->stmts, $exit_functions, true); if (array_intersect([ self::ACTION_LEAVE_SWITCH, @@ -183,11 +218,14 @@ class ScopeChecker } if ($stmt instanceof PhpParser\Node\Stmt\While_) { - $control_actions = array_merge(self::getFinalControlActions($stmt->stmts), $control_actions); + $control_actions = array_merge( + self::getFinalControlActions($stmt->stmts, $exit_functions), + $control_actions + ); } if ($stmt instanceof PhpParser\Node\Stmt\Do_) { - $do_actions = self::getFinalControlActions($stmt->stmts); + $do_actions = self::getFinalControlActions($stmt->stmts, $exit_functions); if (count($do_actions) && !in_array(self::ACTION_NONE, $do_actions, true)) { return $do_actions; @@ -197,13 +235,13 @@ class ScopeChecker } if ($stmt instanceof PhpParser\Node\Stmt\TryCatch) { - $try_statement_actions = self::getFinalControlActions($stmt->stmts, $in_switch); + $try_statement_actions = self::getFinalControlActions($stmt->stmts, $exit_functions, $in_switch); if ($stmt->catches) { $all_same = count($try_statement_actions) === 1; foreach ($stmt->catches as $catch) { - $catch_actions = self::getFinalControlActions($catch->stmts, $in_switch); + $catch_actions = self::getFinalControlActions($catch->stmts, $exit_functions, $in_switch); $all_same = $all_same && $try_statement_actions == $catch_actions; @@ -221,6 +259,7 @@ class ScopeChecker if ($stmt->finally->stmts) { $finally_statement_actions = self::getFinalControlActions( $stmt->finally->stmts, + $exit_functions, $in_switch ); diff --git a/src/Psalm/Checker/Statements/Block/IfChecker.php b/src/Psalm/Checker/Statements/Block/IfChecker.php index e64a409f3..a1c64def9 100644 --- a/src/Psalm/Checker/Statements/Block/IfChecker.php +++ b/src/Psalm/Checker/Statements/Block/IfChecker.php @@ -536,7 +536,13 @@ class IfChecker Context $outer_context, array $pre_assignment_else_redefined_vars ) { - $final_actions = ScopeChecker::getFinalControlActions($stmt->stmts, $outer_context->inside_case); + $project_checker = $statements_checker->getFileChecker()->project_checker; + + $final_actions = ScopeChecker::getFinalControlActions( + $stmt->stmts, + $project_checker->config->exit_functions, + $outer_context->inside_case + ); $has_ending_statements = $final_actions === [ScopeChecker::ACTION_END]; @@ -553,8 +559,6 @@ class IfChecker $if_scope->final_actions = $final_actions; - $project_checker = $statements_checker->getFileChecker()->project_checker; - $assigned_var_ids = $if_context->assigned_var_ids; $possibly_assigned_var_ids = $if_context->possibly_assigned_var_ids; $if_context->assigned_var_ids = []; @@ -1051,7 +1055,11 @@ class IfChecker } } - $final_actions = ScopeChecker::getFinalControlActions($elseif->stmts, $outer_context->inside_case); + $final_actions = ScopeChecker::getFinalControlActions( + $elseif->stmts, + $project_checker->config->exit_functions, + $outer_context->inside_case + ); // has a return/throw at end $has_ending_statements = $final_actions === [ScopeChecker::ACTION_END]; $has_leaving_statements = $has_ending_statements @@ -1419,7 +1427,11 @@ class IfChecker } $final_actions = $else - ? ScopeChecker::getFinalControlActions($else->stmts, $outer_context->inside_case) + ? ScopeChecker::getFinalControlActions( + $else->stmts, + $project_checker->config->exit_functions, + $outer_context->inside_case + ) : [ScopeChecker::ACTION_NONE]; // has a return/throw at end $has_ending_statements = $final_actions === [ScopeChecker::ACTION_END]; diff --git a/src/Psalm/Checker/Statements/Block/LoopChecker.php b/src/Psalm/Checker/Statements/Block/LoopChecker.php index 76e5dfe21..228028f14 100644 --- a/src/Psalm/Checker/Statements/Block/LoopChecker.php +++ b/src/Psalm/Checker/Statements/Block/LoopChecker.php @@ -7,6 +7,7 @@ use Psalm\Checker\Statements\ExpressionChecker; use Psalm\Checker\StatementsChecker; use Psalm\Clause; use Psalm\CodeLocation; +use Psalm\Config; use Psalm\Context; use Psalm\IssueBuffer; use Psalm\Scope\LoopScope; @@ -72,7 +73,7 @@ class LoopChecker ); } - $final_actions = ScopeChecker::getFinalControlActions($stmts); + $final_actions = ScopeChecker::getFinalControlActions($stmts, Config::getInstance()->exit_functions); $has_break_statement = $final_actions === [ScopeChecker::ACTION_BREAK]; if ($assignment_map) { diff --git a/src/Psalm/Checker/Statements/Block/SwitchChecker.php b/src/Psalm/Checker/Statements/Block/SwitchChecker.php index 754b2ec44..709ed6377 100644 --- a/src/Psalm/Checker/Statements/Block/SwitchChecker.php +++ b/src/Psalm/Checker/Statements/Block/SwitchChecker.php @@ -58,11 +58,17 @@ class SwitchChecker $case_action_map = []; + $config = \Psalm\Config::getInstance(); + // create a map of case statement -> ultimate exit type for ($i = count($stmt->cases) - 1; $i >= 0; --$i) { $case = $stmt->cases[$i]; - $case_actions = $case_action_map[$i] = ScopeChecker::getFinalControlActions($case->stmts, true); + $case_actions = $case_action_map[$i] = ScopeChecker::getFinalControlActions( + $case->stmts, + $config->exit_functions, + true + ); if (!in_array(ScopeChecker::ACTION_NONE, $case_actions, true)) { if ($case_actions === [ScopeChecker::ACTION_END]) { diff --git a/src/Psalm/Checker/Statements/Block/TryChecker.php b/src/Psalm/Checker/Statements/Block/TryChecker.php index 6337a9602..b923fc3a4 100644 --- a/src/Psalm/Checker/Statements/Block/TryChecker.php +++ b/src/Psalm/Checker/Statements/Block/TryChecker.php @@ -31,15 +31,15 @@ class TryChecker $catch_actions = []; $all_catches_leave = true; - /** @var int $i */ - foreach ($stmt->catches as $i => $catch) { - $catch_actions[$i] = ScopeChecker::getFinalControlActions($catch->stmts); - $all_catches_leave = $all_catches_leave && !in_array(ScopeChecker::ACTION_NONE, $catch_actions[$i], true); - } - $project_checker = $statements_checker->getFileChecker()->project_checker; $codebase = $project_checker->codebase; + /** @var int $i */ + foreach ($stmt->catches as $i => $catch) { + $catch_actions[$i] = ScopeChecker::getFinalControlActions($catch->stmts, $codebase->config->exit_functions); + $all_catches_leave = $all_catches_leave && !in_array(ScopeChecker::ACTION_NONE, $catch_actions[$i], true); + } + $existing_thrown_exceptions = $context->possibly_thrown_exceptions; /** diff --git a/src/Psalm/Config.php b/src/Psalm/Config.php index 06408457b..5c4b8a445 100644 --- a/src/Psalm/Config.php +++ b/src/Psalm/Config.php @@ -277,6 +277,13 @@ class Config /** @var ClassLoader|null */ private $composer_class_loader; + /** + * Custom functions that always exit + * + * @var array + */ + public $exit_functions = []; + protected function __construct() { self::$instance = $this; @@ -564,7 +571,14 @@ class Config if (isset($config_xml->ignoreExceptions) && isset($config_xml->ignoreExceptions->class)) { /** @var \SimpleXMLElement $exception_class */ foreach ($config_xml->ignoreExceptions->class as $exception_class) { - $config->ignored_exceptions[(string)$exception_class ['name']] = true; + $config->ignored_exceptions[(string) $exception_class['name']] = true; + } + } + + if (isset($config_xml->exitFunctions) && isset($config_xml->exitFunctions->function)) { + /** @var \SimpleXMLElement $exit_function */ + foreach ($config_xml->exitFunctions->function as $exit_function) { + $config->exit_functions[strtolower((string) $exit_function['name'])] = true; } } diff --git a/src/Psalm/Visitor/DependencyFinderVisitor.php b/src/Psalm/Visitor/DependencyFinderVisitor.php index e15981ca1..ce4d19e44 100644 --- a/src/Psalm/Visitor/DependencyFinderVisitor.php +++ b/src/Psalm/Visitor/DependencyFinderVisitor.php @@ -891,6 +891,7 @@ class DependencyFinderVisitor extends PhpParser\NodeVisitorAbstract implements P if ($function_stmt instanceof PhpParser\Node\Stmt\If_) { $final_actions = \Psalm\Checker\ScopeChecker::getFinalControlActions( $function_stmt->stmts, + $this->config->exit_functions, false, false ); diff --git a/tests/ConfigTest.php b/tests/ConfigTest.php index 9b5a7bd99..665ef17b5 100644 --- a/tests/ConfigTest.php +++ b/tests/ConfigTest.php @@ -495,6 +495,76 @@ class ConfigTest extends TestCase $this->analyzeFile($file_path, new Context()); } + /** + * @return void + */ + public function testExitFunctions() + { + $this->project_checker = $this->getProjectCheckerWithConfig( + TestConfig::loadFromXML( + dirname(__DIR__), + ' + + + + + + + ' + ) + ); + + $file_path = getcwd() . '/src/somefile.php'; + + $this->addFile( + $file_path, + 'analyzeFile($file_path, new Context()); + } + /** * @return void */