From 7b1599d783767ec26faba5117136bdf8c66eef2b Mon Sep 17 00:00:00 2001 From: AndrolGenhald Date: Sun, 13 Feb 2022 15:02:46 -0600 Subject: [PATCH 1/3] Fix false positive for unused variable in try (fixes #7613). --- src/Psalm/Context.php | 7 ++++++ .../Analyzer/Statements/Block/TryAnalyzer.php | 2 ++ .../Expression/AssignmentAnalyzer.php | 8 +++++++ tests/TaintTest.php | 19 ++++++++++++++++ tests/UnusedVariableTest.php | 22 +++++++++++++++++++ 5 files changed, 58 insertions(+) diff --git a/src/Psalm/Context.php b/src/Psalm/Context.php index 3b9470d6f64..73e642af92e 100644 --- a/src/Psalm/Context.php +++ b/src/Psalm/Context.php @@ -108,6 +108,13 @@ class Context */ public $inside_assignment = false; + /** + * Whether or not we're inside a try block. + * + * @var bool + */ + public $inside_try = false; + /** * @var null|CodeLocation */ diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/TryAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/TryAnalyzer.php index 8ecffb85d89..f0600c7830a 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/TryAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/TryAnalyzer.php @@ -86,9 +86,11 @@ public static function analyze( $old_referenced_var_ids = $try_context->referenced_var_ids; + $context->inside_try = true; if ($statements_analyzer->analyze($stmt->stmts, $context) === false) { return false; } + $context->inside_try = false; if ($try_context->finally_scope) { foreach ($context->vars_in_scope as $var_id => $type) { diff --git a/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php index 407c6d0441e..50ccf068d07 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Expression/AssignmentAnalyzer.php @@ -315,6 +315,14 @@ public static function analyze( $assign_value_type->parent_nodes = [ $assignment_node->id => $assignment_node ]; + + if ($context->inside_try) { + // Copy previous assignment's parent nodes inside a try. Since an exception could be thrown at any + // point this is a workaround to ensure that use of a variable also uses all previous assignments. + if (isset($context->vars_in_scope[$array_var_id])) { + $assign_value_type->parent_nodes += $context->vars_in_scope[$array_var_id]->parent_nodes; + } + } } if ($array_var_id && isset($context->vars_in_scope[$array_var_id])) { diff --git a/tests/TaintTest.php b/tests/TaintTest.php index 75419f8a68d..302ca24348f 100644 --- a/tests/TaintTest.php +++ b/tests/TaintTest.php @@ -2285,6 +2285,25 @@ function sinkNotWorking($sink) : string {} echo sinkNotWorking($_GET["taint"]);', 'error_message' => 'TaintedHtml', ], + 'taintEscapedInTryMightNotWork' => [ + ' 'TaintedHtml', + ], ]; } diff --git a/tests/UnusedVariableTest.php b/tests/UnusedVariableTest.php index 61fdb840fce..46601956ed7 100644 --- a/tests/UnusedVariableTest.php +++ b/tests/UnusedVariableTest.php @@ -2436,6 +2436,28 @@ function setProxySettingsFromEnv(): void { $a = false; }' ], + 'usedInCatchIsAlwaysUsedInTry' => [ + ' [ + ' Date: Sun, 13 Feb 2022 15:30:06 -0600 Subject: [PATCH 2/3] Fix issue with nested `try` block and add more tests. --- .../Analyzer/Statements/Block/TryAnalyzer.php | 3 +- tests/UnusedVariableTest.php | 58 +++++++++++++++++++ 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/TryAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/TryAnalyzer.php index f0600c7830a..720f432680a 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/TryAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/TryAnalyzer.php @@ -86,11 +86,12 @@ public static function analyze( $old_referenced_var_ids = $try_context->referenced_var_ids; + $previously_inside_try = $context->inside_try; $context->inside_try = true; if ($statements_analyzer->analyze($stmt->stmts, $context) === false) { return false; } - $context->inside_try = false; + $context->inside_try = $previously_inside_try; if ($try_context->finally_scope) { foreach ($context->vars_in_scope as $var_id => $type) { diff --git a/tests/UnusedVariableTest.php b/tests/UnusedVariableTest.php index 46601956ed7..048025186ca 100644 --- a/tests/UnusedVariableTest.php +++ b/tests/UnusedVariableTest.php @@ -2458,6 +2458,21 @@ function setProxySettingsFromEnv(): void { } ', ], + 'usedInFinallyIsAlwaysUsedInTryWithNestedTry' => [ + ' 'MixedArgumentTypeCoercion - src' . DIRECTORY_SEPARATOR . 'somefile.php:12:44 - Argument 1 of takesArrayOfString expects array, parent type array{mixed} provided. Consider improving the type at src' . DIRECTORY_SEPARATOR . 'somefile.php:10:41' ], + 'warnAboutUnusedVariableInTryReassignedInCatch' => [ + ' 'UnusedVariable', + ], + 'warnAboutUnusedVariableInTryReassignedInFinally' => [ + ' 'UnusedVariable', + ], + 'SKIPPED-warnAboutVariableUsedInNestedTryNotUsedInOuterTry' => [ + ' 'UnusedVariable', + ], ]; } } From fd0ecf2528f13923e5eced65c996c4cb039e9749 Mon Sep 17 00:00:00 2001 From: AndrolGenhald Date: Sun, 13 Feb 2022 15:34:21 -0600 Subject: [PATCH 3/3] Rename variable to be consistent with existing convention. --- src/Psalm/Internal/Analyzer/Statements/Block/TryAnalyzer.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Psalm/Internal/Analyzer/Statements/Block/TryAnalyzer.php b/src/Psalm/Internal/Analyzer/Statements/Block/TryAnalyzer.php index 720f432680a..5074bd68a88 100644 --- a/src/Psalm/Internal/Analyzer/Statements/Block/TryAnalyzer.php +++ b/src/Psalm/Internal/Analyzer/Statements/Block/TryAnalyzer.php @@ -86,12 +86,12 @@ public static function analyze( $old_referenced_var_ids = $try_context->referenced_var_ids; - $previously_inside_try = $context->inside_try; + $was_inside_try = $context->inside_try; $context->inside_try = true; if ($statements_analyzer->analyze($stmt->stmts, $context) === false) { return false; } - $context->inside_try = $previously_inside_try; + $context->inside_try = $was_inside_try; if ($try_context->finally_scope) { foreach ($context->vars_in_scope as $var_id => $type) {