From 52c443453f75cc1d83d367547eee0c4969289c42 Mon Sep 17 00:00:00 2001 From: fladolcetta Date: Fri, 6 Mar 2026 13:37:19 -0600 Subject: [PATCH 01/16] Add tests --- .../TestReturnCheck-all-paths-throw-fail.inc | 20 ++++++++++++++++ .../TestReturnCheck-all-paths-throw.inc | 23 +++++++++++++++++++ tests/units/Checks/TestReturnCheck.php | 2 +- 3 files changed, 44 insertions(+), 1 deletion(-) diff --git a/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw-fail.inc b/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw-fail.inc index 2796b426..f5655889 100644 --- a/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw-fail.inc +++ b/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw-fail.inc @@ -1,5 +1,25 @@ assertEquals(44, $this->runAnalyzerOnFile('-all-paths-throw-fail.inc', ErrorConstants::TYPE_SIGNATURE_RETURN), "Failed to catch functions where not all paths throw or return"); + $this->assertEquals(46, $this->runAnalyzerOnFile('-all-paths-throw-fail.inc', ErrorConstants::TYPE_SIGNATURE_RETURN), "Failed to catch functions where not all paths throw or return"); } public function testWhileIFConstant() { From 3f20252dcb21b47b0b4635af8cfec0032b8505f2 Mon Sep 17 00:00:00 2001 From: fladolcetta Date: Fri, 6 Mar 2026 13:37:25 -0600 Subject: [PATCH 02/16] Fix issue --- src/Checks/ReturnCheck.php | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/src/Checks/ReturnCheck.php b/src/Checks/ReturnCheck.php index 47124e15..36495479 100644 --- a/src/Checks/ReturnCheck.php +++ b/src/Checks/ReturnCheck.php @@ -205,6 +205,8 @@ private function statementsAllReturnOrThrow(array $stmts): bool { return true; } elseif ($lastStatement instanceof Node\Stmt\Expression && $lastStatement->expr instanceof Node\Expr\Exit_) { return true; + } elseif ($lastStatement instanceof Node\Stmt\Expression && $this->isCallToFunctionThatThrows($lastStatement->expr)) { + return true; } elseif ($lastStatement instanceof Node\Stmt\If_) { return $this->allIfBranchesReturnOrThrow($lastStatement); } elseif ($lastStatement instanceof Node\Stmt\Switch_) { @@ -371,4 +373,34 @@ private function whileLoopReturnsOrThrows(Node\Stmt\While_ $whileLoop): bool { private function doWhileLoopReturnsOrThrows(Node\Stmt\Do_ $doWhileLoop): bool { return $this->statementsAllReturnOrThrow($doWhileLoop->stmts); } + + /** + * Check if an expression is a call to a function that never returns + * + * @param Node\Expr $expr The expression to check + * + * @return bool + */ + private function isCallToFunctionThatThrows(Node\Expr $expr): bool { + if ($expr instanceof Node\Expr\FuncCall) { + $name = $expr->name; + if ($name instanceof Node\Name) { + $function = $this->symbolTable->getAbstractedFunction(strval($name)); + if ($function) { + // Check if the function body always throws/returns + if ($function instanceof \BambooHR\Guardrail\Abstractions\FunctionAbstraction) { + // FunctionAbstraction wraps a Function_ node, we need to check if it always throws + // We can use reflection to get the function property + $reflection = new \ReflectionClass($function); + $property = $reflection->getProperty('function'); + $functionNode = $property->getValue($function); + if ($functionNode instanceof Node\Stmt\Function_) { + return $this->allPathsReturnOrThrow($functionNode); + } + } + } + } + } + return false; + } } From 4ff6492966e212a883b2fe31df6a96df2fc113cb Mon Sep 17 00:00:00 2001 From: fladolcetta Date: Fri, 6 Mar 2026 13:45:02 -0600 Subject: [PATCH 03/16] Test only returns not throws --- .../TestData/TestReturnCheck-all-paths-throw-fail.inc | 9 +++++++++ .../Checks/TestData/TestReturnCheck-all-paths-throw.inc | 2 -- tests/units/Checks/TestReturnCheck.php | 2 +- 3 files changed, 10 insertions(+), 3 deletions(-) diff --git a/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw-fail.inc b/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw-fail.inc index f5655889..267b092d 100644 --- a/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw-fail.inc +++ b/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw-fail.inc @@ -20,6 +20,15 @@ function throwFunction(): void { throw new \Exception(); } +function functionCallingReturnFunction(): int { + returnFunction(); + // Missing return or throw +} + +function returnFunction(): void { + return; +} + // Function with no return and no throw - should fail function noReturnNoThrow(): int { $x = 5; diff --git a/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw.inc b/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw.inc index 4e6b8744..ee7ae297 100644 --- a/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw.inc +++ b/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw.inc @@ -21,8 +21,6 @@ function throwFunction(): void { throw new \Exception(); } - - function testFunction(): int { throw new \Exception(); } diff --git a/tests/units/Checks/TestReturnCheck.php b/tests/units/Checks/TestReturnCheck.php index 1b94a150..054114d9 100644 --- a/tests/units/Checks/TestReturnCheck.php +++ b/tests/units/Checks/TestReturnCheck.php @@ -71,7 +71,7 @@ public function testAllPathsThrowNoReturnNoError() { public function testAllPathsThrowFail() { // 44 invalid functions/methods where not all paths return or throw // Includes: nested incomplete structures, try-finally incomplete, deeply nested incomplete, fall through failures - $this->assertEquals(46, $this->runAnalyzerOnFile('-all-paths-throw-fail.inc', ErrorConstants::TYPE_SIGNATURE_RETURN), "Failed to catch functions where not all paths throw or return"); + $this->assertEquals(47, $this->runAnalyzerOnFile('-all-paths-throw-fail.inc', ErrorConstants::TYPE_SIGNATURE_RETURN), "Failed to catch functions where not all paths throw or return"); } public function testWhileIFConstant() { From f9231f29b3c0ef7168d2c68ba3b2377bced80724 Mon Sep 17 00:00:00 2001 From: fladolcetta Date: Fri, 6 Mar 2026 14:21:54 -0600 Subject: [PATCH 04/16] Add tests and additional coverage --- src/Checks/ReturnCheck.php | 160 +++++++++++++++++- .../TestReturnCheck-all-paths-throw-fail.inc | 61 +++++++ .../TestReturnCheck-all-paths-throw.inc | 92 +++++++++- tests/units/Checks/TestReturnCheck.php | 7 +- 4 files changed, 312 insertions(+), 8 deletions(-) diff --git a/src/Checks/ReturnCheck.php b/src/Checks/ReturnCheck.php index 36495479..e86218cb 100644 --- a/src/Checks/ReturnCheck.php +++ b/src/Checks/ReturnCheck.php @@ -387,7 +387,7 @@ private function isCallToFunctionThatThrows(Node\Expr $expr): bool { if ($name instanceof Node\Name) { $function = $this->symbolTable->getAbstractedFunction(strval($name)); if ($function) { - // Check if the function body always throws/returns + // Check if the function body always throws (not just returns) if ($function instanceof \BambooHR\Guardrail\Abstractions\FunctionAbstraction) { // FunctionAbstraction wraps a Function_ node, we need to check if it always throws // We can use reflection to get the function property @@ -395,12 +395,168 @@ private function isCallToFunctionThatThrows(Node\Expr $expr): bool { $property = $reflection->getProperty('function'); $functionNode = $property->getValue($function); if ($functionNode instanceof Node\Stmt\Function_) { - return $this->allPathsReturnOrThrow($functionNode); + return $this->allPathsThrow($functionNode); } } } } + } elseif ($expr instanceof Node\Expr\MethodCall && $expr->name instanceof Node\Identifier) { + // Handle instance method calls + $type = $expr->var->getAttribute(\BambooHR\Guardrail\TypeComparer::INFERRED_TYPE_ATTR); + if ($type) { + // Handle union types by checking each type + $allThrow = false; + TypeComparer::forEachType($type, function ($typeNode) use ($expr, &$allThrow) { + $method = \BambooHR\Guardrail\Util::findAbstractedMethod(strval($typeNode), $expr->name, $this->symbolTable); + if ($method && $method instanceof \BambooHR\Guardrail\Abstractions\ClassMethod) { + $reflection = new \ReflectionClass($method); + $property = $reflection->getProperty('method'); + $methodNode = $property->getValue($method); + if ($methodNode instanceof Node\Stmt\ClassMethod) { + if ($this->allPathsThrow($methodNode)) { + $allThrow = true; + } + } + } + }); + return $allThrow; + } + } elseif ($expr instanceof Node\Expr\StaticCall && $expr->name instanceof Node\Identifier) { + // Handle static method calls + if ($expr->class instanceof Node\Name) { + $className = strval($expr->class); + $method = \BambooHR\Guardrail\Util::findAbstractedMethod($className, $expr->name, $this->symbolTable); + if ($method && $method instanceof \BambooHR\Guardrail\Abstractions\ClassMethod) { + $reflection = new \ReflectionClass($method); + $property = $reflection->getProperty('method'); + $methodNode = $property->getValue($method); + if ($methodNode instanceof Node\Stmt\ClassMethod) { + return $this->allPathsThrow($methodNode); + } + } + } } return false; } + + /** + * Check if all code paths in a function throw an exception (not return) + * + * @param Node\FunctionLike $func The function to check + * + * @return bool + */ + private function allPathsThrow(Node\FunctionLike $func): bool { + $stmts = $func->getStmts(); + if (!$stmts) { + return false; + } + return $this->statementsAllThrow($stmts); + } + + /** + * Check if all code paths in a list of statements throw (not return) + * + * @param array $stmts List of statements + * + * @return bool + */ + private function statementsAllThrow(array $stmts): bool { + $lastStatement = $this->getLastNonNopStatement($stmts); + + if (!$lastStatement) { + return false; + } elseif ($lastStatement instanceof Node\Stmt\Throw_) { + return true; + } elseif ($lastStatement instanceof Node\Stmt\Expression && $this->isCallToFunctionThatThrows($lastStatement->expr)) { + return true; + } elseif ($lastStatement instanceof Node\Stmt\If_) { + return $this->allIfBranchesThrow($lastStatement); + } elseif ($lastStatement instanceof Node\Stmt\Switch_) { + return $this->allSwitchCasesThrow($lastStatement); + } elseif ($lastStatement instanceof Node\Stmt\TryCatch) { + return $this->allTryCatchBranchesThrow($lastStatement); + } else { + return false; + } + } + + /** + * Check if all branches of an if statement throw + * + * @param Node\Stmt\If_ $ifStatement Instance of If_ + * + * @return bool + */ + private function allIfBranchesThrow(Node\Stmt\If_ $ifStatement): bool { + if ($this->isConstantTrue($ifStatement->cond)) { + return $this->statementsAllThrow($ifStatement->stmts); + } + if (!$ifStatement->else) { + return false; + } + if (!$this->statementsAllThrow($ifStatement->stmts)) { + return false; + } + if (!$this->statementsAllThrow($ifStatement->else->stmts)) { + return false; + } + if ($ifStatement->elseifs) { + foreach ($ifStatement->elseifs as $elseIf) { + if (!$this->statementsAllThrow($elseIf->stmts)) { + return false; + } + } + } + return true; + } + + /** + * Check if all cases of a switch statement throw + * + * @param Node\Stmt\Switch_ $switchStatement Instance of Switch_ + * + * @return bool + */ + private function allSwitchCasesThrow(Node\Stmt\Switch_ $switchStatement): bool { + $hasDefault = false; + foreach ($switchStatement->cases as $case) { + if ($case->cond === null) { + $hasDefault = true; + } + $stmts = $case->stmts; + while (($last = end($stmts)) instanceof Node\Stmt\Break_ || $last instanceof Node\Stmt\Nop) { + $stmts = array_slice($stmts, 0, -1); + } + if ($stmts && !$this->statementsAllThrow($stmts)) { + return false; + } + } + return $hasDefault; + } + + /** + * Check if all branches of a try-catch statement throw + * + * @param Node\Stmt\TryCatch $tryCatch Instance of TryCatch + * + * @return bool + */ + private function allTryCatchBranchesThrow(Node\Stmt\TryCatch $tryCatch): bool { + // If finally block throws, it overrides try/catch + if ($tryCatch->finally && $this->statementsAllThrow($tryCatch->finally->stmts)) { + return true; + } + + // Otherwise, both try and all catch blocks must throw + if (!$this->statementsAllThrow($tryCatch->stmts)) { + return false; + } + foreach ($tryCatch->catches as $catch) { + if (!$this->statementsAllThrow($catch->stmts)) { + return false; + } + } + return true; + } } diff --git a/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw-fail.inc b/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw-fail.inc index 267b092d..588354ba 100644 --- a/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw-fail.inc +++ b/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw-fail.inc @@ -29,6 +29,67 @@ function returnFunction(): void { return; } +function whileFalseCallsThrowFunction(): int { + while (false) { + throwFunction(); + } + // Missing return or throw - loop never executes +} + +function switchWithoutDefaultCallsThrowFunction(int $x): int { + switch ($x) { + case 1: + throwFunction(); + case 2: + throwFunction(); + // Missing default case + } + // Missing return or throw +} + +function tryNormalCatchThrows(): int { + try { + someNormalFunction(); + // No throw or return in try + } catch (\Exception $e) { + throwFunction(); + } + // Missing return after try-catch +} + +// Method call that doesn't throw - should fail (methods not yet supported anyway) +class NonThrowingClass { + public function normalMethod(): void { + echo "test"; + } +} + +function callMethodThatDoesntThrow(): int { + $obj = new NonThrowingClass(); + $obj->normalMethod(); + // Missing return or throw +} + +class StaticNonThrowingClass { + public static function normalStaticMethod(): void { + echo "test"; + } +} + +function callStaticMethodThatDoesntThrow(): int { + StaticNonThrowingClass::normalStaticMethod(); + // Missing return or throw +} + +function functionCallingReturnFunction(): int { + returnFunction(); + // Missing return or throw +} + +function returnFunction(): void { + return; +} + // Function with no return and no throw - should fail function noReturnNoThrow(): int { $x = 5; diff --git a/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw.inc b/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw.inc index ee7ae297..f5467a30 100644 --- a/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw.inc +++ b/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw.inc @@ -21,6 +21,96 @@ function throwFunction(): void { throw new \Exception(); } +// Nested function calls that throw (3 levels deep) +function functionCallingNestedThrowFunction(): int { + functionCallingAFunctionThatCallsThrowFunction(); +} + +// Function that throws in one branch but returns in another - should pass +function mixedBranchesOneThrowsOneReturns(bool $flag): int { + if ($flag) { + throwFunction(); + } else { + return 42; + } +} + +// Switch statement with all cases calling throw functions +function switchAllCasesCallThrowFunction(int $x): int { + switch ($x) { + case 1: + throwFunction(); + case 2: + throwFunction(); + default: + throwFunction(); + } +} + +// While loop with constant true calling throw function +function whileTrueCallsThrowFunction(): int { + while (true) { + throwFunction(); + } +} + +// Exit after function call +function callFunctionThenExit(): int { + someNormalFunction(); + exit(1); +} + +function someNormalFunction(): void { + echo "test"; +} + +// Try-catch where try calls throw function and catch also calls throw function +function tryCatchBothCallThrowFunctions(): int { + try { + throwFunction(); + } catch (\Exception $e) { + throwFunction(); + } +} + +// Finally block calling throw function (overrides) +function finallyCallsThrowFunction(): int { + try { + return 42; + } finally { + throwFunction(); + } +} + +class ThrowingClass { + public function throwMethod(): void { + throw new \Exception(); + } + + public static function throwStaticMethod(): void { + throw new \Exception(); + } + + public function internalCallToThrowMethod(): void { + $this->throwMethod(); + } +} + +function callMethodThatThrows(): int { + $obj = new ThrowingClass(); + $obj->throwMethod(); +} + +function callStaticMethodThatThrows(): int { + ThrowingClass::throwStaticMethod(); +} + +function doWhileCallsThrowFunctionFalse(): int { + do { + throwFunction(); + } while (false); +} + function testFunction(): int { throw new \Exception(); } @@ -591,4 +681,4 @@ function switchInsideTryCatch(int $x): int { } catch (\Exception $e) { return 0; } -} \ No newline at end of file +} diff --git a/tests/units/Checks/TestReturnCheck.php b/tests/units/Checks/TestReturnCheck.php index 054114d9..d008b1fd 100644 --- a/tests/units/Checks/TestReturnCheck.php +++ b/tests/units/Checks/TestReturnCheck.php @@ -63,15 +63,12 @@ public function testStandardReturnTypesFail() { $this->assertEquals(20, $this->runAnalyzerOnFile('-standard-returns-fail.inc', ErrorConstants::TYPE_SIGNATURE_RETURN), "Failed to fail standard return types"); } public function testAllPathsThrowNoReturnNoError() { - // 45 valid functions/methods where all paths return or throw - // Includes: nested structures, die(), try-finally, deeply nested, switch variations + // All function that should pass $this->assertEquals(0, $this->runAnalyzerOnFile('-all-paths-throw.inc', ErrorConstants::TYPE_SIGNATURE_RETURN), "Failed to pass functions where all paths throw exceptions"); } public function testAllPathsThrowFail() { - // 44 invalid functions/methods where not all paths return or throw - // Includes: nested incomplete structures, try-finally incomplete, deeply nested incomplete, fall through failures - $this->assertEquals(47, $this->runAnalyzerOnFile('-all-paths-throw-fail.inc', ErrorConstants::TYPE_SIGNATURE_RETURN), "Failed to catch functions where not all paths throw or return"); + $this->assertEquals(53, $this->runAnalyzerOnFile('-all-paths-throw-fail.inc', ErrorConstants::TYPE_SIGNATURE_RETURN), "Failed to catch functions where not all paths throw or return"); } public function testWhileIFConstant() { From aa4567ceab609bba9fc660e951b53a055e04b453 Mon Sep 17 00:00:00 2001 From: fladolcetta Date: Fri, 6 Mar 2026 18:42:23 -0600 Subject: [PATCH 05/16] Clean --- src/Checks/ReturnCheck.php | 153 +++++++++++++++++++------------------ 1 file changed, 77 insertions(+), 76 deletions(-) diff --git a/src/Checks/ReturnCheck.php b/src/Checks/ReturnCheck.php index e86218cb..9013e107 100644 --- a/src/Checks/ReturnCheck.php +++ b/src/Checks/ReturnCheck.php @@ -247,26 +247,7 @@ private function getLastNonNopStatement(array $stmts): ?Node { * @return bool */ private function allIfBranchesReturnOrThrow(Node\Stmt\If_ $ifStatement): bool { - if ($this->isConstantTrue($ifStatement->cond)) { - return $this->statementsAllReturnOrThrow($ifStatement->stmts); - } - if (!$ifStatement->else) { - return false; - } - if (!$this->statementsAllReturnOrThrow($ifStatement->stmts)) { - return false; - } - if (!$this->statementsAllReturnOrThrow($ifStatement->else->stmts)) { - return false; - } - if ($ifStatement->elseifs) { - foreach ($ifStatement->elseifs as $elseIf) { - if (!$this->statementsAllReturnOrThrow($elseIf->stmts)) { - return false; - } - } - } - return true; + return $this->checkIfBranches($ifStatement, false); } /** @@ -277,20 +258,7 @@ private function allIfBranchesReturnOrThrow(Node\Stmt\If_ $ifStatement): bool { * @return bool */ private function allSwitchCasesReturnOrThrow(Node\Stmt\Switch_ $switchStatement): bool { - $hasDefault = false; - foreach ($switchStatement->cases as $case) { - if ($case->cond === null) { - $hasDefault = true; - } - $stmts = $case->stmts; - while (($last = end($stmts)) instanceof Node\Stmt\Break_ || $last instanceof Node\Stmt\Nop) { - $stmts = array_slice($stmts, 0, -1); - } - if ($stmts && !$this->statementsAllReturnOrThrow($stmts)) { - return false; - } - } - return $hasDefault; + return $this->checkSwitchCases($switchStatement, false); } /** @@ -320,21 +288,7 @@ protected function getFunctionName(Node\FunctionLike $insideFunc, ?ClassLike $in * @return bool */ private function allTryCatchBranchesReturnOrThrow(Node\Stmt\TryCatch $tryCatch): bool { - // If finally block returns or throws, it overrides try/catch - if ($tryCatch->finally && $this->statementsAllReturnOrThrow($tryCatch->finally->stmts)) { - return true; - } - - // Otherwise, both try and all catch blocks must return or throw - if (!$this->statementsAllReturnOrThrow($tryCatch->stmts)) { - return false; - } - foreach ($tryCatch->catches as $catch) { - if (!$this->statementsAllReturnOrThrow($catch->stmts)) { - return false; - } - } - return true; + return $this->checkTryCatchBranches($tryCatch, false); } /** @@ -404,18 +358,13 @@ private function isCallToFunctionThatThrows(Node\Expr $expr): bool { // Handle instance method calls $type = $expr->var->getAttribute(\BambooHR\Guardrail\TypeComparer::INFERRED_TYPE_ATTR); if ($type) { - // Handle union types by checking each type $allThrow = false; TypeComparer::forEachType($type, function ($typeNode) use ($expr, &$allThrow) { $method = \BambooHR\Guardrail\Util::findAbstractedMethod(strval($typeNode), $expr->name, $this->symbolTable); if ($method && $method instanceof \BambooHR\Guardrail\Abstractions\ClassMethod) { - $reflection = new \ReflectionClass($method); - $property = $reflection->getProperty('method'); - $methodNode = $property->getValue($method); - if ($methodNode instanceof Node\Stmt\ClassMethod) { - if ($this->allPathsThrow($methodNode)) { - $allThrow = true; - } + $methodNode = $this->extractMethodNode($method); + if ($methodNode && $this->allPathsThrow($methodNode)) { + $allThrow = true; } } }); @@ -427,10 +376,8 @@ private function isCallToFunctionThatThrows(Node\Expr $expr): bool { $className = strval($expr->class); $method = \BambooHR\Guardrail\Util::findAbstractedMethod($className, $expr->name, $this->symbolTable); if ($method && $method instanceof \BambooHR\Guardrail\Abstractions\ClassMethod) { - $reflection = new \ReflectionClass($method); - $property = $reflection->getProperty('method'); - $methodNode = $property->getValue($method); - if ($methodNode instanceof Node\Stmt\ClassMethod) { + $methodNode = $this->extractMethodNode($method); + if ($methodNode) { return $this->allPathsThrow($methodNode); } } @@ -489,21 +436,57 @@ private function statementsAllThrow(array $stmts): bool { * @return bool */ private function allIfBranchesThrow(Node\Stmt\If_ $ifStatement): bool { + return $this->checkIfBranches($ifStatement, true); + } + + /** + * Check if all cases of a switch statement throw + * + * @param Node\Stmt\Switch_ $switchStatement Instance of Switch_ + * + * @return bool + */ + private function allSwitchCasesThrow(Node\Stmt\Switch_ $switchStatement): bool { + return $this->checkSwitchCases($switchStatement, true); + } + + /** + * Check if all branches of a try-catch statement throw + * + * @param Node\Stmt\TryCatch $tryCatch Instance of TryCatch + * + * @return bool + */ + private function allTryCatchBranchesThrow(Node\Stmt\TryCatch $tryCatch): bool { + return $this->checkTryCatchBranches($tryCatch, true); + } + + /** + * Unified method to check if branches meet termination criteria + * + * @param Node\Stmt\If_ $ifStatement Instance of If_ + * @param bool $throwOnly If true, only throw counts; if false, return or throw counts + * + * @return bool + */ + private function checkIfBranches(Node\Stmt\If_ $ifStatement, bool $throwOnly): bool { + $checker = $throwOnly ? [$this, 'statementsAllThrow'] : [$this, 'statementsAllReturnOrThrow']; + if ($this->isConstantTrue($ifStatement->cond)) { - return $this->statementsAllThrow($ifStatement->stmts); + return $checker($ifStatement->stmts); } if (!$ifStatement->else) { return false; } - if (!$this->statementsAllThrow($ifStatement->stmts)) { + if (!$checker($ifStatement->stmts)) { return false; } - if (!$this->statementsAllThrow($ifStatement->else->stmts)) { + if (!$checker($ifStatement->else->stmts)) { return false; } if ($ifStatement->elseifs) { foreach ($ifStatement->elseifs as $elseIf) { - if (!$this->statementsAllThrow($elseIf->stmts)) { + if (!$checker($elseIf->stmts)) { return false; } } @@ -512,13 +495,16 @@ private function allIfBranchesThrow(Node\Stmt\If_ $ifStatement): bool { } /** - * Check if all cases of a switch statement throw + * Unified method to check if switch cases meet termination criteria * * @param Node\Stmt\Switch_ $switchStatement Instance of Switch_ + * @param bool $throwOnly If true, only throw counts; if false, return or throw counts * * @return bool */ - private function allSwitchCasesThrow(Node\Stmt\Switch_ $switchStatement): bool { + private function checkSwitchCases(Node\Stmt\Switch_ $switchStatement, bool $throwOnly): bool { + $checker = $throwOnly ? [$this, 'statementsAllThrow'] : [$this, 'statementsAllReturnOrThrow']; + $hasDefault = false; foreach ($switchStatement->cases as $case) { if ($case->cond === null) { @@ -528,7 +514,7 @@ private function allSwitchCasesThrow(Node\Stmt\Switch_ $switchStatement): bool { while (($last = end($stmts)) instanceof Node\Stmt\Break_ || $last instanceof Node\Stmt\Nop) { $stmts = array_slice($stmts, 0, -1); } - if ($stmts && !$this->statementsAllThrow($stmts)) { + if ($stmts && !$checker($stmts)) { return false; } } @@ -536,27 +522,42 @@ private function allSwitchCasesThrow(Node\Stmt\Switch_ $switchStatement): bool { } /** - * Check if all branches of a try-catch statement throw + * Unified method to check if try-catch branches meet termination criteria * - * @param Node\Stmt\TryCatch $tryCatch Instance of TryCatch + * @param Node\Stmt\TryCatch $tryCatch Instance of TryCatch + * @param bool $throwOnly If true, only throw counts; if false, return or throw counts * * @return bool */ - private function allTryCatchBranchesThrow(Node\Stmt\TryCatch $tryCatch): bool { - // If finally block throws, it overrides try/catch - if ($tryCatch->finally && $this->statementsAllThrow($tryCatch->finally->stmts)) { + private function checkTryCatchBranches(Node\Stmt\TryCatch $tryCatch, bool $throwOnly): bool { + $checker = $throwOnly ? [$this, 'statementsAllThrow'] : [$this, 'statementsAllReturnOrThrow']; + + if ($tryCatch->finally && $checker($tryCatch->finally->stmts)) { return true; } - // Otherwise, both try and all catch blocks must throw - if (!$this->statementsAllThrow($tryCatch->stmts)) { + if (!$checker($tryCatch->stmts)) { return false; } foreach ($tryCatch->catches as $catch) { - if (!$this->statementsAllThrow($catch->stmts)) { + if (!$checker($catch->stmts)) { return false; } } return true; } + + /** + * Extract method node from a ClassMethod abstraction + * + * @param \BambooHR\Guardrail\Abstractions\ClassMethod $method + * + * @return Node\Stmt\ClassMethod|null + */ + private function extractMethodNode(\BambooHR\Guardrail\Abstractions\ClassMethod $method): ?Node\Stmt\ClassMethod { + $reflection = new \ReflectionClass($method); + $property = $reflection->getProperty('method'); + $methodNode = $property->getValue($method); + return $methodNode instanceof Node\Stmt\ClassMethod ? $methodNode : null; + } } From 027c070287b7bbdc9ef37b220ce248b19c0fa0d5 Mon Sep 17 00:00:00 2001 From: fladolcetta Date: Fri, 6 Mar 2026 21:37:02 -0600 Subject: [PATCH 06/16] Fix lint failures --- src/Checks/ReturnCheck.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Checks/ReturnCheck.php b/src/Checks/ReturnCheck.php index 9013e107..ca84114f 100644 --- a/src/Checks/ReturnCheck.php +++ b/src/Checks/ReturnCheck.php @@ -465,7 +465,7 @@ private function allTryCatchBranchesThrow(Node\Stmt\TryCatch $tryCatch): bool { * Unified method to check if branches meet termination criteria * * @param Node\Stmt\If_ $ifStatement Instance of If_ - * @param bool $throwOnly If true, only throw counts; if false, return or throw counts + * @param bool $throwOnly If true, only throw counts; if false, return or throw counts * * @return bool */ @@ -498,7 +498,7 @@ private function checkIfBranches(Node\Stmt\If_ $ifStatement, bool $throwOnly): b * Unified method to check if switch cases meet termination criteria * * @param Node\Stmt\Switch_ $switchStatement Instance of Switch_ - * @param bool $throwOnly If true, only throw counts; if false, return or throw counts + * @param bool $throwOnly If true, only throw counts; if false, return or throw counts * * @return bool */ @@ -525,7 +525,7 @@ private function checkSwitchCases(Node\Stmt\Switch_ $switchStatement, bool $thro * Unified method to check if try-catch branches meet termination criteria * * @param Node\Stmt\TryCatch $tryCatch Instance of TryCatch - * @param bool $throwOnly If true, only throw counts; if false, return or throw counts + * @param bool $throwOnly If true, only throw counts; if false, return or throw counts * * @return bool */ From 1a26e1c30760037fc64cf58c2821178e49946e44 Mon Sep 17 00:00:00 2001 From: fladolcetta Date: Fri, 6 Mar 2026 21:40:41 -0600 Subject: [PATCH 07/16] Fix guardrail errors --- src/Checks/ReturnCheck.php | 64 ++++++++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 23 deletions(-) diff --git a/src/Checks/ReturnCheck.php b/src/Checks/ReturnCheck.php index ca84114f..ae7d4755 100644 --- a/src/Checks/ReturnCheck.php +++ b/src/Checks/ReturnCheck.php @@ -470,24 +470,38 @@ private function allTryCatchBranchesThrow(Node\Stmt\TryCatch $tryCatch): bool { * @return bool */ private function checkIfBranches(Node\Stmt\If_ $ifStatement, bool $throwOnly): bool { - $checker = $throwOnly ? [$this, 'statementsAllThrow'] : [$this, 'statementsAllReturnOrThrow']; - if ($this->isConstantTrue($ifStatement->cond)) { - return $checker($ifStatement->stmts); + return $throwOnly ? $this->statementsAllThrow($ifStatement->stmts) : $this->statementsAllReturnOrThrow($ifStatement->stmts); } if (!$ifStatement->else) { return false; } - if (!$checker($ifStatement->stmts)) { - return false; - } - if (!$checker($ifStatement->else->stmts)) { - return false; - } - if ($ifStatement->elseifs) { - foreach ($ifStatement->elseifs as $elseIf) { - if (!$checker($elseIf->stmts)) { - return false; + if ($throwOnly) { + if (!$this->statementsAllThrow($ifStatement->stmts)) { + return false; + } + if (!$this->statementsAllThrow($ifStatement->else->stmts)) { + return false; + } + if ($ifStatement->elseifs) { + foreach ($ifStatement->elseifs as $elseIf) { + if (!$this->statementsAllThrow($elseIf->stmts)) { + return false; + } + } + } + } else { + if (!$this->statementsAllReturnOrThrow($ifStatement->stmts)) { + return false; + } + if (!$this->statementsAllReturnOrThrow($ifStatement->else->stmts)) { + return false; + } + if ($ifStatement->elseifs) { + foreach ($ifStatement->elseifs as $elseIf) { + if (!$this->statementsAllReturnOrThrow($elseIf->stmts)) { + return false; + } } } } @@ -503,8 +517,6 @@ private function checkIfBranches(Node\Stmt\If_ $ifStatement, bool $throwOnly): b * @return bool */ private function checkSwitchCases(Node\Stmt\Switch_ $switchStatement, bool $throwOnly): bool { - $checker = $throwOnly ? [$this, 'statementsAllThrow'] : [$this, 'statementsAllReturnOrThrow']; - $hasDefault = false; foreach ($switchStatement->cases as $case) { if ($case->cond === null) { @@ -514,8 +526,11 @@ private function checkSwitchCases(Node\Stmt\Switch_ $switchStatement, bool $thro while (($last = end($stmts)) instanceof Node\Stmt\Break_ || $last instanceof Node\Stmt\Nop) { $stmts = array_slice($stmts, 0, -1); } - if ($stmts && !$checker($stmts)) { - return false; + if ($stmts) { + $allTerminate = $throwOnly ? $this->statementsAllThrow($stmts) : $this->statementsAllReturnOrThrow($stmts); + if (!$allTerminate) { + return false; + } } } return $hasDefault; @@ -530,17 +545,20 @@ private function checkSwitchCases(Node\Stmt\Switch_ $switchStatement, bool $thro * @return bool */ private function checkTryCatchBranches(Node\Stmt\TryCatch $tryCatch, bool $throwOnly): bool { - $checker = $throwOnly ? [$this, 'statementsAllThrow'] : [$this, 'statementsAllReturnOrThrow']; - - if ($tryCatch->finally && $checker($tryCatch->finally->stmts)) { - return true; + if ($tryCatch->finally) { + $finallyTerminates = $throwOnly ? $this->statementsAllThrow($tryCatch->finally->stmts) : $this->statementsAllReturnOrThrow($tryCatch->finally->stmts); + if ($finallyTerminates) { + return true; + } } - if (!$checker($tryCatch->stmts)) { + $tryTerminates = $throwOnly ? $this->statementsAllThrow($tryCatch->stmts) : $this->statementsAllReturnOrThrow($tryCatch->stmts); + if (!$tryTerminates) { return false; } foreach ($tryCatch->catches as $catch) { - if (!$checker($catch->stmts)) { + $catchTerminates = $throwOnly ? $this->statementsAllThrow($catch->stmts) : $this->statementsAllReturnOrThrow($catch->stmts); + if (!$catchTerminates) { return false; } } From e1460f7dda975bef4dc981b092c342c0e25052eb Mon Sep 17 00:00:00 2001 From: fladolcetta Date: Mon, 9 Mar 2026 10:06:06 -0500 Subject: [PATCH 08/16] Refactor --- src/Checks/ReturnCheck.php | 247 +++++++++++++++++++++++-------------- 1 file changed, 156 insertions(+), 91 deletions(-) diff --git a/src/Checks/ReturnCheck.php b/src/Checks/ReturnCheck.php index ae7d4755..1790865c 100644 --- a/src/Checks/ReturnCheck.php +++ b/src/Checks/ReturnCheck.php @@ -222,6 +222,35 @@ private function statementsAllReturnOrThrow(array $stmts): bool { } } + /** + * Helper method to check statements based on throw-only or return-or-throw criteria + * + * @param array $stmts List of statements + * @param bool $throwOnly If true, only throw counts; if false, return or throw counts + * + * @return bool + */ + private function checkStatements(array $stmts, bool $throwOnly): bool { + if ($throwOnly) { + return $this->statementsAllThrow($stmts); + } + return $this->statementsAllReturnOrThrow($stmts); + } + + /** + * Remove trailing break and nop statements from a list + * + * @param array $stmts The statements + * + * @return array + */ + private function removeTrailingBreaksAndNops(array $stmts): array { + while (($last = end($stmts)) instanceof Node\Stmt\Break_ || $last instanceof Node\Stmt\Nop) { + $stmts = array_slice($stmts, 0, -1); + } + return $stmts; + } + /** * Get the last non-Nop statement from a list of statements * @@ -337,53 +366,94 @@ private function doWhileLoopReturnsOrThrows(Node\Stmt\Do_ $doWhileLoop): bool { */ private function isCallToFunctionThatThrows(Node\Expr $expr): bool { if ($expr instanceof Node\Expr\FuncCall) { - $name = $expr->name; - if ($name instanceof Node\Name) { - $function = $this->symbolTable->getAbstractedFunction(strval($name)); - if ($function) { - // Check if the function body always throws (not just returns) - if ($function instanceof \BambooHR\Guardrail\Abstractions\FunctionAbstraction) { - // FunctionAbstraction wraps a Function_ node, we need to check if it always throws - // We can use reflection to get the function property - $reflection = new \ReflectionClass($function); - $property = $reflection->getProperty('function'); - $functionNode = $property->getValue($function); - if ($functionNode instanceof Node\Stmt\Function_) { - return $this->allPathsThrow($functionNode); - } - } - } - } - } elseif ($expr instanceof Node\Expr\MethodCall && $expr->name instanceof Node\Identifier) { - // Handle instance method calls - $type = $expr->var->getAttribute(\BambooHR\Guardrail\TypeComparer::INFERRED_TYPE_ATTR); - if ($type) { - $allThrow = false; - TypeComparer::forEachType($type, function ($typeNode) use ($expr, &$allThrow) { - $method = \BambooHR\Guardrail\Util::findAbstractedMethod(strval($typeNode), $expr->name, $this->symbolTable); - if ($method && $method instanceof \BambooHR\Guardrail\Abstractions\ClassMethod) { - $methodNode = $this->extractMethodNode($method); - if ($methodNode && $this->allPathsThrow($methodNode)) { - $allThrow = true; - } - } - }); - return $allThrow; - } - } elseif ($expr instanceof Node\Expr\StaticCall && $expr->name instanceof Node\Identifier) { - // Handle static method calls - if ($expr->class instanceof Node\Name) { - $className = strval($expr->class); - $method = \BambooHR\Guardrail\Util::findAbstractedMethod($className, $expr->name, $this->symbolTable); - if ($method && $method instanceof \BambooHR\Guardrail\Abstractions\ClassMethod) { - $methodNode = $this->extractMethodNode($method); - if ($methodNode) { - return $this->allPathsThrow($methodNode); - } + return $this->isFunctionCallThatThrows($expr); + } elseif ($expr instanceof Node\Expr\MethodCall) { + return $this->isMethodCallThatThrows($expr); + } elseif ($expr instanceof Node\Expr\StaticCall) { + return $this->isStaticCallThatThrows($expr); + } + return false; + } + + /** + * Check if a function call always throws + * + * @param Node\Expr\FuncCall $funcCall The function call to check + * + * @return bool + */ + private function isFunctionCallThatThrows(Node\Expr\FuncCall $funcCall): bool { + if (!$funcCall->name instanceof Node\Name) { + return false; + } + + $function = $this->symbolTable->getAbstractedFunction(strval($funcCall->name)); + if (!$function instanceof \BambooHR\Guardrail\Abstractions\FunctionAbstraction) { + return false; + } + + $functionNode = $this->getFunctionNodeFromAbstraction($function); + return $functionNode && $this->allPathsThrow($functionNode); + } + + /** + * Check if an instance method call always throws + * + * @param Node\Expr\MethodCall $methodCall The method call to check + * + * @return bool + */ + private function isMethodCallThatThrows(Node\Expr\MethodCall $methodCall): bool { + if (!$methodCall->name instanceof Node\Identifier) { + return false; + } + + $type = $methodCall->var->getAttribute(TypeComparer::INFERRED_TYPE_ATTR); + if (!$type) { + return false; + } + + $allThrow = false; + TypeComparer::forEachType($type, function ($typeNode) use ($methodCall, &$allThrow) { + $method = \BambooHR\Guardrail\Util::findAbstractedMethod( + strval($typeNode), + $methodCall->name, + $this->symbolTable + ); + if ($method instanceof \BambooHR\Guardrail\Abstractions\ClassMethod) { + $methodNode = $this->getMethodNodeFromAbstraction($method); + if ($methodNode && $this->allPathsThrow($methodNode)) { + $allThrow = true; } } + }); + return $allThrow; + } + + /** + * Check if a static method call always throws + * + * @param Node\Expr\StaticCall $staticCall The static call to check + * + * @return bool + */ + private function isStaticCallThatThrows(Node\Expr\StaticCall $staticCall): bool { + if (!$staticCall->name instanceof Node\Identifier || !$staticCall->class instanceof Node\Name) { + return false; } - return false; + + $method = \BambooHR\Guardrail\Util::findAbstractedMethod( + strval($staticCall->class), + $staticCall->name, + $this->symbolTable + ); + + if (!$method instanceof \BambooHR\Guardrail\Abstractions\ClassMethod) { + return false; + } + + $methodNode = $this->getMethodNodeFromAbstraction($method); + return $methodNode && $this->allPathsThrow($methodNode); } /** @@ -471,40 +541,29 @@ private function allTryCatchBranchesThrow(Node\Stmt\TryCatch $tryCatch): bool { */ private function checkIfBranches(Node\Stmt\If_ $ifStatement, bool $throwOnly): bool { if ($this->isConstantTrue($ifStatement->cond)) { - return $throwOnly ? $this->statementsAllThrow($ifStatement->stmts) : $this->statementsAllReturnOrThrow($ifStatement->stmts); + return $this->checkStatements($ifStatement->stmts, $throwOnly); } + if (!$ifStatement->else) { return false; } - if ($throwOnly) { - if (!$this->statementsAllThrow($ifStatement->stmts)) { - return false; - } - if (!$this->statementsAllThrow($ifStatement->else->stmts)) { - return false; - } - if ($ifStatement->elseifs) { - foreach ($ifStatement->elseifs as $elseIf) { - if (!$this->statementsAllThrow($elseIf->stmts)) { - return false; - } - } - } - } else { - if (!$this->statementsAllReturnOrThrow($ifStatement->stmts)) { - return false; - } - if (!$this->statementsAllReturnOrThrow($ifStatement->else->stmts)) { - return false; - } - if ($ifStatement->elseifs) { - foreach ($ifStatement->elseifs as $elseIf) { - if (!$this->statementsAllReturnOrThrow($elseIf->stmts)) { - return false; - } + + if (!$this->checkStatements($ifStatement->stmts, $throwOnly)) { + return false; + } + + if (!$this->checkStatements($ifStatement->else->stmts, $throwOnly)) { + return false; + } + + if ($ifStatement->elseifs) { + foreach ($ifStatement->elseifs as $elseIf) { + if (!$this->checkStatements($elseIf->stmts, $throwOnly)) { + return false; } } } + return true; } @@ -522,15 +581,10 @@ private function checkSwitchCases(Node\Stmt\Switch_ $switchStatement, bool $thro if ($case->cond === null) { $hasDefault = true; } - $stmts = $case->stmts; - while (($last = end($stmts)) instanceof Node\Stmt\Break_ || $last instanceof Node\Stmt\Nop) { - $stmts = array_slice($stmts, 0, -1); - } - if ($stmts) { - $allTerminate = $throwOnly ? $this->statementsAllThrow($stmts) : $this->statementsAllReturnOrThrow($stmts); - if (!$allTerminate) { - return false; - } + + $stmts = $this->removeTrailingBreaksAndNops($case->stmts); + if ($stmts && !$this->checkStatements($stmts, $throwOnly)) { + return false; } } return $hasDefault; @@ -545,34 +599,45 @@ private function checkSwitchCases(Node\Stmt\Switch_ $switchStatement, bool $thro * @return bool */ private function checkTryCatchBranches(Node\Stmt\TryCatch $tryCatch, bool $throwOnly): bool { - if ($tryCatch->finally) { - $finallyTerminates = $throwOnly ? $this->statementsAllThrow($tryCatch->finally->stmts) : $this->statementsAllReturnOrThrow($tryCatch->finally->stmts); - if ($finallyTerminates) { - return true; - } + if ($tryCatch->finally && $this->checkStatements($tryCatch->finally->stmts, $throwOnly)) { + return true; } - $tryTerminates = $throwOnly ? $this->statementsAllThrow($tryCatch->stmts) : $this->statementsAllReturnOrThrow($tryCatch->stmts); - if (!$tryTerminates) { + if (!$this->checkStatements($tryCatch->stmts, $throwOnly)) { return false; } + foreach ($tryCatch->catches as $catch) { - $catchTerminates = $throwOnly ? $this->statementsAllThrow($catch->stmts) : $this->statementsAllReturnOrThrow($catch->stmts); - if (!$catchTerminates) { + if (!$this->checkStatements($catch->stmts, $throwOnly)) { return false; } } + return true; } /** - * Extract method node from a ClassMethod abstraction + * Get the underlying function node from a FunctionAbstraction + * + * @param \BambooHR\Guardrail\Abstractions\FunctionAbstraction $function + * + * @return Node\Stmt\Function_|null + */ + private function getFunctionNodeFromAbstraction(\BambooHR\Guardrail\Abstractions\FunctionAbstraction $function): ?Node\Stmt\Function_ { + $reflection = new \ReflectionClass($function); + $property = $reflection->getProperty('function'); + $functionNode = $property->getValue($function); + return $functionNode instanceof Node\Stmt\Function_ ? $functionNode : null; + } + + /** + * Get the underlying method node from a ClassMethod abstraction * * @param \BambooHR\Guardrail\Abstractions\ClassMethod $method * * @return Node\Stmt\ClassMethod|null */ - private function extractMethodNode(\BambooHR\Guardrail\Abstractions\ClassMethod $method): ?Node\Stmt\ClassMethod { + private function getMethodNodeFromAbstraction(\BambooHR\Guardrail\Abstractions\ClassMethod $method): ?Node\Stmt\ClassMethod { $reflection = new \ReflectionClass($method); $property = $reflection->getProperty('method'); $methodNode = $property->getValue($method); From dde38ecfbaf1220e2cc1c4aaf48eb4815670973c Mon Sep 17 00:00:00 2001 From: fladolcetta Date: Mon, 9 Mar 2026 12:27:00 -0500 Subject: [PATCH 09/16] Get rid of junk tests --- .../TestReturnCheck-all-paths-throw-fail.inc | 9 --------- .../TestData/TestReturnCheck-all-paths-throw.inc | 15 --------------- 2 files changed, 24 deletions(-) diff --git a/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw-fail.inc b/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw-fail.inc index 588354ba..28873a25 100644 --- a/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw-fail.inc +++ b/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw-fail.inc @@ -20,15 +20,6 @@ function throwFunction(): void { throw new \Exception(); } -function functionCallingReturnFunction(): int { - returnFunction(); - // Missing return or throw -} - -function returnFunction(): void { - return; -} - function whileFalseCallsThrowFunction(): int { while (false) { throwFunction(); diff --git a/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw.inc b/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw.inc index f5467a30..0a8daca9 100644 --- a/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw.inc +++ b/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw.inc @@ -21,11 +21,6 @@ function throwFunction(): void { throw new \Exception(); } -// Nested function calls that throw (3 levels deep) -function functionCallingNestedThrowFunction(): int { - functionCallingAFunctionThatCallsThrowFunction(); -} - // Function that throws in one branch but returns in another - should pass function mixedBranchesOneThrowsOneReturns(bool $flag): int { if ($flag) { @@ -54,16 +49,6 @@ function whileTrueCallsThrowFunction(): int { } } -// Exit after function call -function callFunctionThenExit(): int { - someNormalFunction(); - exit(1); -} - -function someNormalFunction(): void { - echo "test"; -} - // Try-catch where try calls throw function and catch also calls throw function function tryCatchBothCallThrowFunctions(): int { try { From c6fa9dbc66e63cc3d63cda5ff33c103ec501c216 Mon Sep 17 00:00:00 2001 From: fladolcetta Date: Mon, 9 Mar 2026 12:30:40 -0500 Subject: [PATCH 10/16] Re-arrange --- src/Checks/ReturnCheck.php | 211 ++++++++++++------------------------- 1 file changed, 70 insertions(+), 141 deletions(-) diff --git a/src/Checks/ReturnCheck.php b/src/Checks/ReturnCheck.php index 1790865c..1a4b4855 100644 --- a/src/Checks/ReturnCheck.php +++ b/src/Checks/ReturnCheck.php @@ -208,11 +208,11 @@ private function statementsAllReturnOrThrow(array $stmts): bool { } elseif ($lastStatement instanceof Node\Stmt\Expression && $this->isCallToFunctionThatThrows($lastStatement->expr)) { return true; } elseif ($lastStatement instanceof Node\Stmt\If_) { - return $this->allIfBranchesReturnOrThrow($lastStatement); + return $this->checkIfBranches($lastStatement, false); } elseif ($lastStatement instanceof Node\Stmt\Switch_) { - return $this->allSwitchCasesReturnOrThrow($lastStatement); + return $this->checkSwitchCases($lastStatement, false); } elseif ($lastStatement instanceof Node\Stmt\TryCatch) { - return $this->allTryCatchBranchesReturnOrThrow($lastStatement); + return $this->checkTryCatchBranches($lastStatement, false); } elseif ($lastStatement instanceof Node\Stmt\While_) { return $this->whileLoopReturnsOrThrows($lastStatement); } elseif ($lastStatement instanceof Node\Stmt\Do_) { @@ -269,25 +269,83 @@ private function getLastNonNopStatement(array $stmts): ?Node { } /** - * Check if all branches of an if statement either return or throw + * Unified method to check if branches meet termination criteria * * @param Node\Stmt\If_ $ifStatement Instance of If_ + * @param bool $throwOnly If true, only throw counts; if false, return or throw counts * * @return bool */ - private function allIfBranchesReturnOrThrow(Node\Stmt\If_ $ifStatement): bool { - return $this->checkIfBranches($ifStatement, false); + private function checkIfBranches(Node\Stmt\If_ $ifStatement, bool $throwOnly): bool { + if ($this->isConstantTrue($ifStatement->cond)) { + return $this->checkStatements($ifStatement->stmts, $throwOnly); + } + if (!$ifStatement->else) { + return false; + } + if (!$this->checkStatements($ifStatement->stmts, $throwOnly)) { + return false; + } + if (!$this->checkStatements($ifStatement->else->stmts, $throwOnly)) { + return false; + } + if ($ifStatement->elseifs) { + foreach ($ifStatement->elseifs as $elseIf) { + if (!$this->checkStatements($elseIf->stmts, $throwOnly)) { + return false; + } + } + } + return true; } /** - * Check if all cases of a switch statement either return or throw + * Unified method to check if switch cases meet termination criteria * * @param Node\Stmt\Switch_ $switchStatement Instance of Switch_ + * @param bool $throwOnly If true, only throw counts; if false, return or throw counts + * + * @return bool + */ + private function checkSwitchCases(Node\Stmt\Switch_ $switchStatement, bool $throwOnly): bool { + $hasDefault = false; + foreach ($switchStatement->cases as $case) { + if ($case->cond === null) { + $hasDefault = true; + } + + $stmts = $this->removeTrailingBreaksAndNops($case->stmts); + if ($stmts && !$this->checkStatements($stmts, $throwOnly)) { + return false; + } + } + return $hasDefault; + } + + /** + * Unified method to check if try-catch branches meet termination criteria + * + * @param Node\Stmt\TryCatch $tryCatch Instance of TryCatch + * @param bool $throwOnly If true, only throw counts; if false, return or throw counts * * @return bool */ - private function allSwitchCasesReturnOrThrow(Node\Stmt\Switch_ $switchStatement): bool { - return $this->checkSwitchCases($switchStatement, false); + private function checkTryCatchBranches(Node\Stmt\TryCatch $tryCatch, bool $throwOnly): bool { + if ($tryCatch->finally && $this->checkStatements($tryCatch->finally->stmts, $throwOnly)) { + return true; + } + + if (!$this->checkStatements($tryCatch->stmts, $throwOnly)) { + return false; + } + + foreach ($tryCatch->catches as $catch) { + if (!$this->checkStatements($catch->stmts, $throwOnly)) { + return false; + } + } + + return true; } /** @@ -309,17 +367,6 @@ protected function getFunctionName(Node\FunctionLike $insideFunc, ?ClassLike $in return $functionName; } - /** - * Check if all branches of a try-catch statement either return or throw - * - * @param Node\Stmt\TryCatch $tryCatch Instance of TryCatch - * - * @return bool - */ - private function allTryCatchBranchesReturnOrThrow(Node\Stmt\TryCatch $tryCatch): bool { - return $this->checkTryCatchBranches($tryCatch, false); - } - /** * Check if a return type allows a function to have no return statement * @@ -488,134 +535,16 @@ private function statementsAllThrow(array $stmts): bool { } elseif ($lastStatement instanceof Node\Stmt\Expression && $this->isCallToFunctionThatThrows($lastStatement->expr)) { return true; } elseif ($lastStatement instanceof Node\Stmt\If_) { - return $this->allIfBranchesThrow($lastStatement); + return $this->checkIfBranches($lastStatement, true); } elseif ($lastStatement instanceof Node\Stmt\Switch_) { - return $this->allSwitchCasesThrow($lastStatement); + return $this->checkSwitchCases($lastStatement, true); } elseif ($lastStatement instanceof Node\Stmt\TryCatch) { - return $this->allTryCatchBranchesThrow($lastStatement); + return $this->checkTryCatchBranches($lastStatement, true); } else { return false; } } - /** - * Check if all branches of an if statement throw - * - * @param Node\Stmt\If_ $ifStatement Instance of If_ - * - * @return bool - */ - private function allIfBranchesThrow(Node\Stmt\If_ $ifStatement): bool { - return $this->checkIfBranches($ifStatement, true); - } - - /** - * Check if all cases of a switch statement throw - * - * @param Node\Stmt\Switch_ $switchStatement Instance of Switch_ - * - * @return bool - */ - private function allSwitchCasesThrow(Node\Stmt\Switch_ $switchStatement): bool { - return $this->checkSwitchCases($switchStatement, true); - } - - /** - * Check if all branches of a try-catch statement throw - * - * @param Node\Stmt\TryCatch $tryCatch Instance of TryCatch - * - * @return bool - */ - private function allTryCatchBranchesThrow(Node\Stmt\TryCatch $tryCatch): bool { - return $this->checkTryCatchBranches($tryCatch, true); - } - - /** - * Unified method to check if branches meet termination criteria - * - * @param Node\Stmt\If_ $ifStatement Instance of If_ - * @param bool $throwOnly If true, only throw counts; if false, return or throw counts - * - * @return bool - */ - private function checkIfBranches(Node\Stmt\If_ $ifStatement, bool $throwOnly): bool { - if ($this->isConstantTrue($ifStatement->cond)) { - return $this->checkStatements($ifStatement->stmts, $throwOnly); - } - - if (!$ifStatement->else) { - return false; - } - - if (!$this->checkStatements($ifStatement->stmts, $throwOnly)) { - return false; - } - - if (!$this->checkStatements($ifStatement->else->stmts, $throwOnly)) { - return false; - } - - if ($ifStatement->elseifs) { - foreach ($ifStatement->elseifs as $elseIf) { - if (!$this->checkStatements($elseIf->stmts, $throwOnly)) { - return false; - } - } - } - - return true; - } - - /** - * Unified method to check if switch cases meet termination criteria - * - * @param Node\Stmt\Switch_ $switchStatement Instance of Switch_ - * @param bool $throwOnly If true, only throw counts; if false, return or throw counts - * - * @return bool - */ - private function checkSwitchCases(Node\Stmt\Switch_ $switchStatement, bool $throwOnly): bool { - $hasDefault = false; - foreach ($switchStatement->cases as $case) { - if ($case->cond === null) { - $hasDefault = true; - } - - $stmts = $this->removeTrailingBreaksAndNops($case->stmts); - if ($stmts && !$this->checkStatements($stmts, $throwOnly)) { - return false; - } - } - return $hasDefault; - } - - /** - * Unified method to check if try-catch branches meet termination criteria - * - * @param Node\Stmt\TryCatch $tryCatch Instance of TryCatch - * @param bool $throwOnly If true, only throw counts; if false, return or throw counts - * - * @return bool - */ - private function checkTryCatchBranches(Node\Stmt\TryCatch $tryCatch, bool $throwOnly): bool { - if ($tryCatch->finally && $this->checkStatements($tryCatch->finally->stmts, $throwOnly)) { - return true; - } - - if (!$this->checkStatements($tryCatch->stmts, $throwOnly)) { - return false; - } - - foreach ($tryCatch->catches as $catch) { - if (!$this->checkStatements($catch->stmts, $throwOnly)) { - return false; - } - } - - return true; - } - /** * Get the underlying function node from a FunctionAbstraction * From 162953f5eed8f5dab8445816971e844e5b66667c Mon Sep 17 00:00:00 2001 From: fladolcetta Date: Mon, 9 Mar 2026 12:46:45 -0500 Subject: [PATCH 11/16] Refactor --- src/Checks/ReturnCheck.php | 40 +++++++++++--------------- tests/units/Checks/TestReturnCheck.php | 2 +- 2 files changed, 18 insertions(+), 24 deletions(-) diff --git a/src/Checks/ReturnCheck.php b/src/Checks/ReturnCheck.php index 1a4b4855..36cb0e2b 100644 --- a/src/Checks/ReturnCheck.php +++ b/src/Checks/ReturnCheck.php @@ -184,17 +184,18 @@ private function allPathsReturnOrThrow(Node\FunctionLike $func): bool { if (!$stmts) { return false; } - return $this->statementsAllReturnOrThrow($stmts); + return $this->statementsAllReturnOrThrow($stmts, false); } /** * Check if all code paths in a list of statements either return or throw * - * @param array $stmts List of statements + * @param array $stmts List of statements + * @param bool $throwOnly If true, only throw counts; if false, return or throw counts * * @return bool */ - private function statementsAllReturnOrThrow(array $stmts): bool { + private function statementsAllReturnOrThrow(array $stmts, bool $throwOnly): bool { $lastStatement = $this->getLastNonNopStatement($stmts); if (!$lastStatement) { @@ -208,15 +209,18 @@ private function statementsAllReturnOrThrow(array $stmts): bool { } elseif ($lastStatement instanceof Node\Stmt\Expression && $this->isCallToFunctionThatThrows($lastStatement->expr)) { return true; } elseif ($lastStatement instanceof Node\Stmt\If_) { - return $this->checkIfBranches($lastStatement, false); + return $this->allIfBranchesReturnOrThrow($lastStatement, $throwOnly); } elseif ($lastStatement instanceof Node\Stmt\Switch_) { - return $this->checkSwitchCases($lastStatement, false); + return $this->checkSwitchCases($lastStatement, $throwOnly); } elseif ($lastStatement instanceof Node\Stmt\TryCatch) { - return $this->checkTryCatchBranches($lastStatement, false); + return $this->checkTryCatchBranches($lastStatement, $throwOnly); } elseif ($lastStatement instanceof Node\Stmt\While_) { - return $this->whileLoopReturnsOrThrows($lastStatement); + if ($this->isConstantTrue($lastStatement->cond)) { + return $this->statementsAllReturnOrThrow($lastStatement->stmts, $throwOnly); + } + return false; } elseif ($lastStatement instanceof Node\Stmt\Do_) { - return $this->doWhileLoopReturnsOrThrows($lastStatement); + return $this->statementsAllReturnOrThrow($lastStatement->stmts, $throwOnly); } else { return false; } @@ -234,7 +238,7 @@ private function checkStatements(array $stmts, bool $throwOnly): bool { if ($throwOnly) { return $this->statementsAllThrow($stmts); } - return $this->statementsAllReturnOrThrow($stmts); + return $this->statementsAllReturnOrThrow($stmts, false); } /** @@ -269,14 +273,14 @@ private function getLastNonNopStatement(array $stmts): ?Node { } /** - * Unified method to check if branches meet termination criteria + * Check if all branches of an if statement either return or throws (with options for throw only) * * @param Node\Stmt\If_ $ifStatement Instance of If_ * @param bool $throwOnly If true, only throw counts; if false, return or throw counts * * @return bool */ - private function checkIfBranches(Node\Stmt\If_ $ifStatement, bool $throwOnly): bool { + private function allIfBranchesReturnOrThrow(Node\Stmt\If_ $ifStatement, bool $throwOnly): bool { if ($this->isConstantTrue($ifStatement->cond)) { return $this->checkStatements($ifStatement->stmts, $throwOnly); } @@ -322,6 +326,7 @@ private function checkSwitchCases(Node\Stmt\Switch_ $switchStatement, bool $thro return $hasDefault; } + /** * Unified method to check if try-catch branches meet termination criteria * @@ -393,17 +398,6 @@ private function isConstantTrue($expr): bool { return false; } - private function whileLoopReturnsOrThrows(Node\Stmt\While_ $whileLoop): bool { - if ($this->isConstantTrue($whileLoop->cond)) { - return $this->statementsAllReturnOrThrow($whileLoop->stmts); - } - return false; - } - - private function doWhileLoopReturnsOrThrows(Node\Stmt\Do_ $doWhileLoop): bool { - return $this->statementsAllReturnOrThrow($doWhileLoop->stmts); - } - /** * Check if an expression is a call to a function that never returns * @@ -535,7 +529,7 @@ private function statementsAllThrow(array $stmts): bool { } elseif ($lastStatement instanceof Node\Stmt\Expression && $this->isCallToFunctionThatThrows($lastStatement->expr)) { return true; } elseif ($lastStatement instanceof Node\Stmt\If_) { - return $this->checkIfBranches($lastStatement, true); + return $this->allIfBranchesReturnOrThrow($lastStatement, true); } elseif ($lastStatement instanceof Node\Stmt\Switch_) { return $this->checkSwitchCases($lastStatement, true); } elseif ($lastStatement instanceof Node\Stmt\TryCatch) { diff --git a/tests/units/Checks/TestReturnCheck.php b/tests/units/Checks/TestReturnCheck.php index d008b1fd..00e35451 100644 --- a/tests/units/Checks/TestReturnCheck.php +++ b/tests/units/Checks/TestReturnCheck.php @@ -68,7 +68,7 @@ public function testAllPathsThrowNoReturnNoError() { } public function testAllPathsThrowFail() { - $this->assertEquals(53, $this->runAnalyzerOnFile('-all-paths-throw-fail.inc', ErrorConstants::TYPE_SIGNATURE_RETURN), "Failed to catch functions where not all paths throw or return"); + $this->assertEquals(52, $this->runAnalyzerOnFile('-all-paths-throw-fail.inc', ErrorConstants::TYPE_SIGNATURE_RETURN), "Failed to catch functions where not all paths throw or return"); } public function testWhileIFConstant() { From fb6b6ac4ba8d507324ce287fafd1c608eddccb14 Mon Sep 17 00:00:00 2001 From: fladolcetta Date: Mon, 9 Mar 2026 13:08:33 -0500 Subject: [PATCH 12/16] More refactor --- src/Checks/ReturnCheck.php | 64 +++++++------------------------------- 1 file changed, 11 insertions(+), 53 deletions(-) diff --git a/src/Checks/ReturnCheck.php b/src/Checks/ReturnCheck.php index 36cb0e2b..9f5e7862 100644 --- a/src/Checks/ReturnCheck.php +++ b/src/Checks/ReturnCheck.php @@ -201,11 +201,11 @@ private function statementsAllReturnOrThrow(array $stmts, bool $throwOnly): bool if (!$lastStatement) { return false; } elseif ($lastStatement instanceof Node\Stmt\Return_) { - return true; + return !$throwOnly; } elseif ($lastStatement instanceof Node\Stmt\Throw_) { return true; } elseif ($lastStatement instanceof Node\Stmt\Expression && $lastStatement->expr instanceof Node\Expr\Exit_) { - return true; + return !$throwOnly; } elseif ($lastStatement instanceof Node\Stmt\Expression && $this->isCallToFunctionThatThrows($lastStatement->expr)) { return true; } elseif ($lastStatement instanceof Node\Stmt\If_) { @@ -226,21 +226,6 @@ private function statementsAllReturnOrThrow(array $stmts, bool $throwOnly): bool } } - /** - * Helper method to check statements based on throw-only or return-or-throw criteria - * - * @param array $stmts List of statements - * @param bool $throwOnly If true, only throw counts; if false, return or throw counts - * - * @return bool - */ - private function checkStatements(array $stmts, bool $throwOnly): bool { - if ($throwOnly) { - return $this->statementsAllThrow($stmts); - } - return $this->statementsAllReturnOrThrow($stmts, false); - } - /** * Remove trailing break and nop statements from a list * @@ -282,20 +267,20 @@ private function getLastNonNopStatement(array $stmts): ?Node { */ private function allIfBranchesReturnOrThrow(Node\Stmt\If_ $ifStatement, bool $throwOnly): bool { if ($this->isConstantTrue($ifStatement->cond)) { - return $this->checkStatements($ifStatement->stmts, $throwOnly); + return $this->statementsAllReturnOrThrow($ifStatement->stmts, $throwOnly); } if (!$ifStatement->else) { return false; } - if (!$this->checkStatements($ifStatement->stmts, $throwOnly)) { + if (!$this->statementsAllReturnOrThrow($ifStatement->stmts, $throwOnly)) { return false; } - if (!$this->checkStatements($ifStatement->else->stmts, $throwOnly)) { + if (!$this->statementsAllReturnOrThrow($ifStatement->else->stmts, $throwOnly)) { return false; } if ($ifStatement->elseifs) { foreach ($ifStatement->elseifs as $elseIf) { - if (!$this->checkStatements($elseIf->stmts, $throwOnly)) { + if (!$this->statementsAllReturnOrThrow($elseIf->stmts, $throwOnly)) { return false; } } @@ -319,7 +304,7 @@ private function checkSwitchCases(Node\Stmt\Switch_ $switchStatement, bool $thro } $stmts = $this->removeTrailingBreaksAndNops($case->stmts); - if ($stmts && !$this->checkStatements($stmts, $throwOnly)) { + if ($stmts && !$this->statementsAllReturnOrThrow($stmts, $throwOnly)) { return false; } } @@ -336,16 +321,16 @@ private function checkSwitchCases(Node\Stmt\Switch_ $switchStatement, bool $thro * @return bool */ private function checkTryCatchBranches(Node\Stmt\TryCatch $tryCatch, bool $throwOnly): bool { - if ($tryCatch->finally && $this->checkStatements($tryCatch->finally->stmts, $throwOnly)) { + if ($tryCatch->finally && $this->statementsAllReturnOrThrow($tryCatch->finally->stmts, $throwOnly)) { return true; } - if (!$this->checkStatements($tryCatch->stmts, $throwOnly)) { + if (!$this->statementsAllReturnOrThrow($tryCatch->stmts, $throwOnly)) { return false; } foreach ($tryCatch->catches as $catch) { - if (!$this->checkStatements($catch->stmts, $throwOnly)) { + if (!$this->statementsAllReturnOrThrow($catch->stmts, $throwOnly)) { return false; } } @@ -509,34 +494,7 @@ private function allPathsThrow(Node\FunctionLike $func): bool { if (!$stmts) { return false; } - return $this->statementsAllThrow($stmts); - } - - /** - * Check if all code paths in a list of statements throw (not return) - * - * @param array $stmts List of statements - * - * @return bool - */ - private function statementsAllThrow(array $stmts): bool { - $lastStatement = $this->getLastNonNopStatement($stmts); - - if (!$lastStatement) { - return false; - } elseif ($lastStatement instanceof Node\Stmt\Throw_) { - return true; - } elseif ($lastStatement instanceof Node\Stmt\Expression && $this->isCallToFunctionThatThrows($lastStatement->expr)) { - return true; - } elseif ($lastStatement instanceof Node\Stmt\If_) { - return $this->allIfBranchesReturnOrThrow($lastStatement, true); - } elseif ($lastStatement instanceof Node\Stmt\Switch_) { - return $this->checkSwitchCases($lastStatement, true); - } elseif ($lastStatement instanceof Node\Stmt\TryCatch) { - return $this->checkTryCatchBranches($lastStatement, true); - } else { - return false; - } + return $this->statementsAllReturnOrThrow($stmts, true); } /** From 53f3a4b6c8eb75bf63753b4387e3569216948079 Mon Sep 17 00:00:00 2001 From: fladolcetta Date: Mon, 9 Mar 2026 13:18:32 -0500 Subject: [PATCH 13/16] Undo moves --- src/Checks/ReturnCheck.php | 75 +++++++++++++++++--------------------- 1 file changed, 34 insertions(+), 41 deletions(-) diff --git a/src/Checks/ReturnCheck.php b/src/Checks/ReturnCheck.php index 9f5e7862..affad61e 100644 --- a/src/Checks/ReturnCheck.php +++ b/src/Checks/ReturnCheck.php @@ -211,14 +211,11 @@ private function statementsAllReturnOrThrow(array $stmts, bool $throwOnly): bool } elseif ($lastStatement instanceof Node\Stmt\If_) { return $this->allIfBranchesReturnOrThrow($lastStatement, $throwOnly); } elseif ($lastStatement instanceof Node\Stmt\Switch_) { - return $this->checkSwitchCases($lastStatement, $throwOnly); + return $this->allTryCatchBranchesReturnOrThrow($lastStatement, $throwOnly); } elseif ($lastStatement instanceof Node\Stmt\TryCatch) { return $this->checkTryCatchBranches($lastStatement, $throwOnly); } elseif ($lastStatement instanceof Node\Stmt\While_) { - if ($this->isConstantTrue($lastStatement->cond)) { - return $this->statementsAllReturnOrThrow($lastStatement->stmts, $throwOnly); - } - return false; + return $this->whileLoopReturnsOrThrows($lastStatement, $throwOnly); } elseif ($lastStatement instanceof Node\Stmt\Do_) { return $this->statementsAllReturnOrThrow($lastStatement->stmts, $throwOnly); } else { @@ -226,20 +223,6 @@ private function statementsAllReturnOrThrow(array $stmts, bool $throwOnly): bool } } - /** - * Remove trailing break and nop statements from a list - * - * @param array $stmts The statements - * - * @return array - */ - private function removeTrailingBreaksAndNops(array $stmts): array { - while (($last = end($stmts)) instanceof Node\Stmt\Break_ || $last instanceof Node\Stmt\Nop) { - $stmts = array_slice($stmts, 0, -1); - } - return $stmts; - } - /** * Get the last non-Nop statement from a list of statements * @@ -289,21 +272,24 @@ private function allIfBranchesReturnOrThrow(Node\Stmt\If_ $ifStatement, bool $th } /** - * Unified method to check if switch cases meet termination criteria + * Check if all cases of a switch statement either return or throw (with options for throw only) * * @param Node\Stmt\Switch_ $switchStatement Instance of Switch_ * @param bool $throwOnly If true, only throw counts; if false, return or throw counts * * @return bool */ - private function checkSwitchCases(Node\Stmt\Switch_ $switchStatement, bool $throwOnly): bool { + private function allTryCatchBranchesReturnOrThrow(Node\Stmt\Switch_ $switchStatement, bool $throwOnly): bool { $hasDefault = false; foreach ($switchStatement->cases as $case) { if ($case->cond === null) { $hasDefault = true; } - $stmts = $this->removeTrailingBreaksAndNops($case->stmts); + $stmts = $case->stmts; + while (($last = end($stmts)) instanceof Node\Stmt\Break_ || $last instanceof Node\Stmt\Nop) { + $stmts = array_slice($stmts, 0, -1); + } if ($stmts && !$this->statementsAllReturnOrThrow($stmts, $throwOnly)) { return false; } @@ -311,6 +297,25 @@ private function checkSwitchCases(Node\Stmt\Switch_ $switchStatement, bool $thro return $hasDefault; } + /** + * @param Node\FunctionLike $insideFunc The method we're inside of + * @param ?ClassLike $inside The class we're inside of (if any) + * + * @return string + */ + protected function getFunctionName(Node\FunctionLike $insideFunc, ?ClassLike $inside = null) { + $functionName = ""; + if ($insideFunc instanceof Node\Stmt\Function_) { + $functionName = strval($insideFunc->name); + } elseif ($insideFunc instanceof Node\Expr\Closure || $insideFunc instanceof Node\Expr\ArrowFunction) { + $functionName = "anonymous function"; + } elseif ($insideFunc instanceof Node\Stmt\ClassMethod) { + $class = isset($inside->namespacedName) ? strval($inside->namespacedName) : ""; + $functionName = "$class::" . strval($insideFunc->name); + } + return $functionName; + } + /** * Unified method to check if try-catch branches meet termination criteria @@ -338,25 +343,6 @@ private function checkTryCatchBranches(Node\Stmt\TryCatch $tryCatch, bool $throw return true; } - /** - * @param Node\FunctionLike $insideFunc The method we're inside of - * @param ?ClassLike $inside The class we're inside of (if any) - * - * @return string - */ - protected function getFunctionName(Node\FunctionLike $insideFunc, ?ClassLike $inside = null) { - $functionName = ""; - if ($insideFunc instanceof Node\Stmt\Function_) { - $functionName = strval($insideFunc->name); - } elseif ($insideFunc instanceof Node\Expr\Closure || $insideFunc instanceof Node\Expr\ArrowFunction) { - $functionName = "anonymous function"; - } elseif ($insideFunc instanceof Node\Stmt\ClassMethod) { - $class = isset($inside->namespacedName) ? strval($inside->namespacedName) : ""; - $functionName = "$class::" . strval($insideFunc->name); - } - return $functionName; - } - /** * Check if a return type allows a function to have no return statement * @@ -383,6 +369,13 @@ private function isConstantTrue($expr): bool { return false; } + private function whileLoopReturnsOrThrows(Node\Stmt\While_ $whileLoop, bool $throwOnly): bool { + if ($this->isConstantTrue($whileLoop->cond)) { + return $this->statementsAllReturnOrThrow($whileLoop->stmts, $throwOnly); + } + return false; + } + /** * Check if an expression is a call to a function that never returns * From aa31793019453bc87483f0cc49bd1184c5b021fe Mon Sep 17 00:00:00 2001 From: fladolcetta Date: Mon, 9 Mar 2026 14:47:32 -0500 Subject: [PATCH 14/16] Delete last dead function --- src/Checks/ReturnCheck.php | 40 ++++++++++++-------------------------- 1 file changed, 12 insertions(+), 28 deletions(-) diff --git a/src/Checks/ReturnCheck.php b/src/Checks/ReturnCheck.php index affad61e..abdd42e4 100644 --- a/src/Checks/ReturnCheck.php +++ b/src/Checks/ReturnCheck.php @@ -141,7 +141,7 @@ private function checkGeneratorFunction(string $fileName, Node\FunctionLike $nod } if ($returnType && !$this->returnTypeAllowsNoReturn($returnType)) { - if (!$this->allPathsReturnOrThrow($node)) { + if (!$this->allPathsReturnOrThrow($node, false)) { $functionName = $this->getFunctionName($node, $inside); $this->emitError( $fileName, @@ -179,12 +179,12 @@ protected function containsYield(Node\FunctionLike $func): bool { * * @return bool */ - private function allPathsReturnOrThrow(Node\FunctionLike $func): bool { + private function allPathsReturnOrThrow(Node\FunctionLike $func, $throwOnly): bool { $stmts = $func->getStmts(); if (!$stmts) { return false; } - return $this->statementsAllReturnOrThrow($stmts, false); + return $this->statementsAllReturnOrThrow($stmts, $throwOnly); } /** @@ -211,9 +211,9 @@ private function statementsAllReturnOrThrow(array $stmts, bool $throwOnly): bool } elseif ($lastStatement instanceof Node\Stmt\If_) { return $this->allIfBranchesReturnOrThrow($lastStatement, $throwOnly); } elseif ($lastStatement instanceof Node\Stmt\Switch_) { - return $this->allTryCatchBranchesReturnOrThrow($lastStatement, $throwOnly); + return $this->allSwitchCasesReturnOrThrow($lastStatement, $throwOnly); } elseif ($lastStatement instanceof Node\Stmt\TryCatch) { - return $this->checkTryCatchBranches($lastStatement, $throwOnly); + return $this->allTryCatchBranchesReturnOrThrow($lastStatement, $throwOnly); } elseif ($lastStatement instanceof Node\Stmt\While_) { return $this->whileLoopReturnsOrThrows($lastStatement, $throwOnly); } elseif ($lastStatement instanceof Node\Stmt\Do_) { @@ -279,13 +279,12 @@ private function allIfBranchesReturnOrThrow(Node\Stmt\If_ $ifStatement, bool $th * * @return bool */ - private function allTryCatchBranchesReturnOrThrow(Node\Stmt\Switch_ $switchStatement, bool $throwOnly): bool { + private function allSwitchCasesReturnOrThrow(Node\Stmt\Switch_ $switchStatement, bool $throwOnly): bool { $hasDefault = false; foreach ($switchStatement->cases as $case) { if ($case->cond === null) { $hasDefault = true; } - $stmts = $case->stmts; while (($last = end($stmts)) instanceof Node\Stmt\Break_ || $last instanceof Node\Stmt\Nop) { $stmts = array_slice($stmts, 0, -1); @@ -318,18 +317,19 @@ protected function getFunctionName(Node\FunctionLike $insideFunc, ?ClassLike $in /** - * Unified method to check if try-catch branches meet termination criteria + * Check if all branches of a try-catch statement either return or throw (with options for throws only) * * @param Node\Stmt\TryCatch $tryCatch Instance of TryCatch * @param bool $throwOnly If true, only throw counts; if false, return or throw counts * * @return bool */ - private function checkTryCatchBranches(Node\Stmt\TryCatch $tryCatch, bool $throwOnly): bool { + private function allTryCatchBranchesReturnOrThrow(Node\Stmt\TryCatch $tryCatch, bool $throwOnly): bool { if ($tryCatch->finally && $this->statementsAllReturnOrThrow($tryCatch->finally->stmts, $throwOnly)) { return true; } + // Otherwise, both try and all catch blocks must return or throw if (!$this->statementsAllReturnOrThrow($tryCatch->stmts, $throwOnly)) { return false; } @@ -339,7 +339,6 @@ private function checkTryCatchBranches(Node\Stmt\TryCatch $tryCatch, bool $throw return false; } } - return true; } @@ -412,7 +411,7 @@ private function isFunctionCallThatThrows(Node\Expr\FuncCall $funcCall): bool { } $functionNode = $this->getFunctionNodeFromAbstraction($function); - return $functionNode && $this->allPathsThrow($functionNode); + return $functionNode && $this->allPathsReturnOrThrow($functionNode, true); } /** @@ -441,7 +440,7 @@ private function isMethodCallThatThrows(Node\Expr\MethodCall $methodCall): bool ); if ($method instanceof \BambooHR\Guardrail\Abstractions\ClassMethod) { $methodNode = $this->getMethodNodeFromAbstraction($method); - if ($methodNode && $this->allPathsThrow($methodNode)) { + if ($methodNode && $this->allPathsReturnOrThrow($methodNode, true)) { $allThrow = true; } } @@ -472,22 +471,7 @@ private function isStaticCallThatThrows(Node\Expr\StaticCall $staticCall): bool } $methodNode = $this->getMethodNodeFromAbstraction($method); - return $methodNode && $this->allPathsThrow($methodNode); - } - - /** - * Check if all code paths in a function throw an exception (not return) - * - * @param Node\FunctionLike $func The function to check - * - * @return bool - */ - private function allPathsThrow(Node\FunctionLike $func): bool { - $stmts = $func->getStmts(); - if (!$stmts) { - return false; - } - return $this->statementsAllReturnOrThrow($stmts, true); + return $methodNode && $this->allPathsReturnOrThrow($methodNode, true); } /** From aff308742a4cc97d159c0328e3646b0955d7a961 Mon Sep 17 00:00:00 2001 From: fladolcetta Date: Wed, 13 May 2026 10:16:31 -0500 Subject: [PATCH 15/16] Add test from external file --- .../TestData/TestReturnCheck-all-paths-throw-import.inc | 6 ++++++ tests/units/Checks/TestData/ThrowHelper.php | 7 +++++++ tests/units/Checks/TestReturnCheck.php | 6 ++++++ 3 files changed, 19 insertions(+) create mode 100644 tests/units/Checks/TestData/TestReturnCheck-all-paths-throw-import.inc create mode 100644 tests/units/Checks/TestData/ThrowHelper.php diff --git a/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw-import.inc b/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw-import.inc new file mode 100644 index 00000000..6ede7ccb --- /dev/null +++ b/tests/units/Checks/TestData/TestReturnCheck-all-paths-throw-import.inc @@ -0,0 +1,6 @@ +assertEquals(20, $this->runAnalyzerOnFile('-standard-returns-fail.inc', ErrorConstants::TYPE_SIGNATURE_RETURN), "Failed to fail standard return types"); } + public function testAllPathsThrowNoReturnNoError() { // All function that should pass $this->assertEquals(0, $this->runAnalyzerOnFile('-all-paths-throw.inc', ErrorConstants::TYPE_SIGNATURE_RETURN), "Failed to pass functions where all paths throw exceptions"); } + public function testAllPathsThrowNoReturnImportNoError() { + // All function that should pass + $this->assertEquals(0, $this->runAnalyzerOnFile('-all-paths-throw-import.inc', ErrorConstants::TYPE_SIGNATURE_RETURN, ['additionalFilesToIndex' => [__DIR__ . '/TestData/ThrowHelper.php']]), "Failed to pass functions where all paths throw exceptions"); + } + public function testAllPathsThrowFail() { $this->assertEquals(52, $this->runAnalyzerOnFile('-all-paths-throw-fail.inc', ErrorConstants::TYPE_SIGNATURE_RETURN), "Failed to catch functions where not all paths throw or return"); } From b7acecb0d17872e4f09f3a294907741c38f9a3ab Mon Sep 17 00:00:00 2001 From: fladolcetta Date: Fri, 22 May 2026 10:24:31 -0500 Subject: [PATCH 16/16] Add mechanism to catch throws for Real --- src/Checks/ReturnCheck.php | 26 ++++++- src/NodeVisitors/ThrowDetector.php | 105 ++++++++++++++++++++++++++++ src/SymbolTable/JsonSymbolTable.php | 41 ++++++++++- 3 files changed, 166 insertions(+), 6 deletions(-) create mode 100644 src/NodeVisitors/ThrowDetector.php diff --git a/src/Checks/ReturnCheck.php b/src/Checks/ReturnCheck.php index b4518c07..7f3c4683 100644 --- a/src/Checks/ReturnCheck.php +++ b/src/Checks/ReturnCheck.php @@ -379,6 +379,26 @@ private function whileLoopReturnsOrThrows(Node\Stmt\While_ $whileLoop, bool $thr return false; } + /** + * Returns true if the given function/method always exits via throw, exit, etc. + * + * Prefers the cached `always_throws` attribute (set at indexing time when the + * function body has already been stripped). Falls back to walking the statements + * when the body is still present (in-memory symbol tables, or the function + * currently being analyzed). + * + * @param Node\FunctionLike $node The function/method node to check + * + * @return bool + */ + private function functionAlwaysThrows(Node\FunctionLike $node): bool { + $cached = $node->getAttribute('always_throws'); + if ($cached !== null) { + return (bool)$cached; + } + return $this->allPathsReturnOrThrow($node, true); + } + /** * Check if an expression is a call to a function that never returns * @@ -415,7 +435,7 @@ private function isFunctionCallThatThrows(Node\Expr\FuncCall $funcCall): bool { } $functionNode = $this->getFunctionNodeFromAbstraction($function); - return $functionNode && $this->allPathsReturnOrThrow($functionNode, true); + return $functionNode && $this->functionAlwaysThrows($functionNode); } /** @@ -444,7 +464,7 @@ private function isMethodCallThatThrows(Node\Expr\MethodCall $methodCall): bool ); if ($method instanceof \BambooHR\Guardrail\Abstractions\ClassMethod) { $methodNode = $this->getMethodNodeFromAbstraction($method); - if ($methodNode && $this->allPathsReturnOrThrow($methodNode, true)) { + if ($methodNode && $this->functionAlwaysThrows($methodNode)) { $allThrow = true; } } @@ -475,7 +495,7 @@ private function isStaticCallThatThrows(Node\Expr\StaticCall $staticCall): bool } $methodNode = $this->getMethodNodeFromAbstraction($method); - return $methodNode && $this->allPathsReturnOrThrow($methodNode, true); + return $methodNode && $this->functionAlwaysThrows($methodNode); } /** diff --git a/src/NodeVisitors/ThrowDetector.php b/src/NodeVisitors/ThrowDetector.php new file mode 100644 index 00000000..93d5c66d --- /dev/null +++ b/src/NodeVisitors/ThrowDetector.php @@ -0,0 +1,105 @@ +getStmts(); + if (!$stmts) { + return false; + } + return self::statementsAlwaysThrow($stmts); + } + + private static function statementsAlwaysThrow(array $stmts): bool { + $last = null; + for ($i = count($stmts) - 1; $i >= 0; $i--) { + if (!($stmts[$i] instanceof Node\Stmt\Nop)) { + $last = $stmts[$i]; + break; + } + } + if (!$last) { + return false; + } + if ($last instanceof Node\Stmt\Throw_) { + return true; + } + if ($last instanceof Node\Stmt\Expression && $last->expr instanceof Node\Expr\Exit_) { + return true; + } + if ($last instanceof Node\Stmt\If_) { + if (!$last->else) { + return false; + } + if (!self::statementsAlwaysThrow($last->stmts)) { + return false; + } + if (!self::statementsAlwaysThrow($last->else->stmts)) { + return false; + } + foreach ($last->elseifs as $elseIf) { + if (!self::statementsAlwaysThrow($elseIf->stmts)) { + return false; + } + } + return true; + } + if ($last instanceof Node\Stmt\Switch_) { + $hasDefault = false; + foreach ($last->cases as $case) { + if ($case->cond === null) { + $hasDefault = true; + } + $caseStmts = $case->stmts; + while (($tail = end($caseStmts)) instanceof Node\Stmt\Break_ || $tail instanceof Node\Stmt\Nop) { + $caseStmts = array_slice($caseStmts, 0, -1); + } + if ($caseStmts && !self::statementsAlwaysThrow($caseStmts)) { + return false; + } + } + return $hasDefault; + } + if ($last instanceof Node\Stmt\TryCatch) { + if ($last->finally && self::statementsAlwaysThrow($last->finally->stmts)) { + return true; + } + if (!self::statementsAlwaysThrow($last->stmts)) { + return false; + } + foreach ($last->catches as $catch) { + if (!self::statementsAlwaysThrow($catch->stmts)) { + return false; + } + } + return true; + } + if ($last instanceof Node\Stmt\While_) { + if ($last->cond instanceof Node\Expr\ConstFetch && strtolower($last->cond->name->toString()) === 'true') { + return self::statementsAlwaysThrow($last->stmts); + } + return false; + } + if ($last instanceof Node\Stmt\Do_) { + return self::statementsAlwaysThrow($last->stmts); + } + return false; + } +} diff --git a/src/SymbolTable/JsonSymbolTable.php b/src/SymbolTable/JsonSymbolTable.php index 74f61a12..3887916d 100644 --- a/src/SymbolTable/JsonSymbolTable.php +++ b/src/SymbolTable/JsonSymbolTable.php @@ -12,6 +12,7 @@ use BambooHR\Guardrail\Abstractions\ReflectedClass; use BambooHR\Guardrail\Abstractions\ReflectedFunction; use BambooHR\Guardrail\Evaluators\Expression\Scalar; +use BambooHR\Guardrail\NodeVisitors\ThrowDetector; use BambooHR\Guardrail\NodeVisitors\VariadicCheckVisitor; use BambooHR\Guardrail\TypeComparer; use BambooHR\Guardrail\TypeParser; @@ -330,6 +331,9 @@ public static function stripMethodContents(ClassLike $class) { if ($stmt->getAttribute('variadic_implementation', null) === null) { $stmt->setAttribute("variadic_implementation", VariadicCheckVisitor::isVariadic($stmt->stmts)); } + if ($stmt->getAttribute('always_throws', null) === null) { + $stmt->setAttribute('always_throws', ThrowDetector::functionAlwaysThrows($stmt)); + } $stmt->stmts = []; } } @@ -371,9 +375,26 @@ public function addClass($name, ClassLike $class, $file) { $usesTrait = 1; } } + self::annotateMethodAlwaysThrows($class); $this->addType($name, $file, self::TYPE_CLASS, $usesTrait, $this->serializeClass($class)); } + /** + * Set the `always_throws` attribute on every ClassMethod under this ClassLike + * before its body gets dropped during serialization. + * + * @param ClassLike $class The class/interface/trait to annotate + * + * @return void + */ + public static function annotateMethodAlwaysThrows(ClassLike $class): void { + foreach ($class->stmts as $stmt) { + if ($stmt instanceof ClassMethod && $stmt->getAttribute('always_throws', null) === null) { + $stmt->setAttribute('always_throws', ThrowDetector::functionAlwaysThrows($stmt)); + } + } + } + /** * isDefinedClass * @@ -440,6 +461,7 @@ public function getAbstractedClass($name) { * @return void */ public function addInterface($name, Interface_ $interface, $file) { + self::annotateMethodAlwaysThrows($interface); $this->addType($name, $file, self::TYPE_INTERFACE, 0, $this->serializeClass($interface)); } @@ -455,8 +477,9 @@ public function addInterface($name, Interface_ $interface, $file) { public function addFunction($name, Function_ $function, $file) { $clone = clone $function; $clone->setAttribute("variadic_implementation", VariadicCheckVisitor::isVariadic($function->stmts)); + $clone->setAttribute('always_throws', ThrowDetector::functionAlwaysThrows($function)); $clone->stmts = []; - $this->addType($name, $file, self::TYPE_FUNCTION, 0, $this->serializeFunction($function)); + $this->addType($name, $file, self::TYPE_FUNCTION, 0, $this->serializeFunction($clone)); } /** @@ -577,6 +600,9 @@ function serializeFunction(Function_ $function): string { if ($function->getAttribute("variadic_implementation")) { $ret .= "V"; } + if ($function->getAttribute("always_throws")) { + $ret .= "R"; + } $ret .= ";"; return $ret; } @@ -616,6 +642,9 @@ function serializeMethod(ClassMethod $method): string { if ($method->getAttribute('variadic_implementation')) { $ret .= "V"; } + if ($method->getAttribute('always_throws')) { + $ret .= "R"; + } $ret .= ";"; return $ret; } @@ -715,7 +744,7 @@ function unserializeClass(string $serializedClass): ClassLike { } function unserializeMethod(string $serializedMethod): ClassMethod { - preg_match('/^M([^ &]+)([ &])?([0-9]+)?\(([^)]*)\)(?::([0-9]+)?(?:@([0-9]+))?)?(?:T([0-9,]+))?(V)?$/', $serializedMethod, $matches); + preg_match('/^M([^ &]+)([ &])?([0-9]+)?\(([^)]*)\)(?::([0-9]+)?(?:@([0-9]+))?)?(?:T([0-9,]+))?(V)?(R)?$/', $serializedMethod, $matches); $name = $matches[1]; $returnsByRef = isset($matches[2]) && $matches[2] == "&"; $flags = !empty($matches[3]) ? intval($matches[3]) : 0; @@ -743,6 +772,9 @@ function unserializeMethod(string $serializedMethod): ClassMethod { if (!empty($matches[8])) { $method->setAttribute('variadic_implementation', true); } + if (!empty($matches[9])) { + $method->setAttribute('always_throws', true); + } return $method; } @@ -772,7 +804,7 @@ function unserializeConst(string $serializedConstant): ClassConst { function unserializeFunction(string $serializedFunction): Function_ { - preg_match('/^F([^ &]+)(&)?\(([^)]*)\)(?::([0-9]+)?(?:@([0-9]+))?)?(?:T([0-9,]+))?(V)?;$/', $serializedFunction, $matches); + preg_match('/^F([^ &]+)(&)?\(([^)]*)\)(?::([0-9]+)?(?:@([0-9]+))?)?(?:T([0-9,]+))?(V)?(R)?;$/', $serializedFunction, $matches); $name = new Node\Name\FullyQualified($matches[1]); $returnsByRef = $matches[2] == "&"; if (trim($matches[3]) !== "") { @@ -802,6 +834,9 @@ function unserializeFunction(string $serializedFunction): Function_ { if (!empty($matches[7])) { $func->setAttribute('variadic_implementation', true); } + if (!empty($matches[8])) { + $func->setAttribute('always_throws', true); + } return $func; }