diff --git a/src/Checks/ReturnCheck.php b/src/Checks/ReturnCheck.php index cde3cb31..7f3c4683 100644 --- a/src/Checks/ReturnCheck.php +++ b/src/Checks/ReturnCheck.php @@ -145,7 +145,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, @@ -183,42 +183,45 @@ 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); + return $this->statementsAllReturnOrThrow($stmts, $throwOnly); } /** * 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) { 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 !$throwOnly; + } elseif ($lastStatement instanceof Node\Stmt\Expression && $this->isCallToFunctionThatThrows($lastStatement->expr)) { return true; } elseif ($lastStatement instanceof Node\Stmt\If_) { - return $this->allIfBranchesReturnOrThrow($lastStatement); + return $this->allIfBranchesReturnOrThrow($lastStatement, $throwOnly); } elseif ($lastStatement instanceof Node\Stmt\Switch_) { - return $this->allSwitchCasesReturnOrThrow($lastStatement); + return $this->allSwitchCasesReturnOrThrow($lastStatement, $throwOnly); } elseif ($lastStatement instanceof Node\Stmt\TryCatch) { - return $this->allTryCatchBranchesReturnOrThrow($lastStatement); + return $this->allTryCatchBranchesReturnOrThrow($lastStatement, $throwOnly); } elseif ($lastStatement instanceof Node\Stmt\While_) { - return $this->whileLoopReturnsOrThrows($lastStatement); + return $this->whileLoopReturnsOrThrows($lastStatement, $throwOnly); } elseif ($lastStatement instanceof Node\Stmt\Do_) { - return $this->doWhileLoopReturnsOrThrows($lastStatement); + return $this->statementsAllReturnOrThrow($lastStatement->stmts, $throwOnly); } else { return false; } @@ -242,28 +245,29 @@ private function getLastNonNopStatement(array $stmts): ?Node { } /** - * Check if all branches of an if statement either return or throw + * 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 allIfBranchesReturnOrThrow(Node\Stmt\If_ $ifStatement): bool { + private function allIfBranchesReturnOrThrow(Node\Stmt\If_ $ifStatement, bool $throwOnly): bool { if ($this->isConstantTrue($ifStatement->cond)) { - return $this->statementsAllReturnOrThrow($ifStatement->stmts); + return $this->statementsAllReturnOrThrow($ifStatement->stmts, $throwOnly); } if (!$ifStatement->else) { return false; } - if (!$this->statementsAllReturnOrThrow($ifStatement->stmts)) { + if (!$this->statementsAllReturnOrThrow($ifStatement->stmts, $throwOnly)) { return false; } - if (!$this->statementsAllReturnOrThrow($ifStatement->else->stmts)) { + if (!$this->statementsAllReturnOrThrow($ifStatement->else->stmts, $throwOnly)) { return false; } if ($ifStatement->elseifs) { foreach ($ifStatement->elseifs as $elseIf) { - if (!$this->statementsAllReturnOrThrow($elseIf->stmts)) { + if (!$this->statementsAllReturnOrThrow($elseIf->stmts, $throwOnly)) { return false; } } @@ -272,13 +276,14 @@ private function allIfBranchesReturnOrThrow(Node\Stmt\If_ $ifStatement): bool { } /** - * Check if all cases of a switch statement either return or throw + * 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 allSwitchCasesReturnOrThrow(Node\Stmt\Switch_ $switchStatement): bool { + private function allSwitchCasesReturnOrThrow(Node\Stmt\Switch_ $switchStatement, bool $throwOnly): bool { $hasDefault = false; foreach ($switchStatement->cases as $case) { if ($case->cond === null) { @@ -288,7 +293,7 @@ private function allSwitchCasesReturnOrThrow(Node\Stmt\Switch_ $switchStatement) while (($last = end($stmts)) instanceof Node\Stmt\Break_ || $last instanceof Node\Stmt\Nop) { $stmts = array_slice($stmts, 0, -1); } - if ($stmts && !$this->statementsAllReturnOrThrow($stmts)) { + if ($stmts && !$this->statementsAllReturnOrThrow($stmts, $throwOnly)) { return false; } } @@ -314,25 +319,27 @@ protected function getFunctionName(Node\FunctionLike $insideFunc, ?ClassLike $in return $functionName; } + /** - * Check if all branches of a try-catch statement either return or throw + * 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 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 allTryCatchBranchesReturnOrThrow(Node\Stmt\TryCatch $tryCatch): bool { - // If finally block returns or throws, it overrides try/catch - if ($tryCatch->finally && $this->statementsAllReturnOrThrow($tryCatch->finally->stmts)) { + 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)) { + if (!$this->statementsAllReturnOrThrow($tryCatch->stmts, $throwOnly)) { return false; } + foreach ($tryCatch->catches as $catch) { - if (!$this->statementsAllReturnOrThrow($catch->stmts)) { + if (!$this->statementsAllReturnOrThrow($catch->stmts, $throwOnly)) { return false; } } @@ -365,14 +372,157 @@ private function isConstantTrue($expr): bool { return false; } - private function whileLoopReturnsOrThrows(Node\Stmt\While_ $whileLoop): bool { + private function whileLoopReturnsOrThrows(Node\Stmt\While_ $whileLoop, bool $throwOnly): bool { if ($this->isConstantTrue($whileLoop->cond)) { - return $this->statementsAllReturnOrThrow($whileLoop->stmts); + return $this->statementsAllReturnOrThrow($whileLoop->stmts, $throwOnly); } return false; } - private function doWhileLoopReturnsOrThrows(Node\Stmt\Do_ $doWhileLoop): bool { - return $this->statementsAllReturnOrThrow($doWhileLoop->stmts); + /** + * 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 + * + * @param Node\Expr $expr The expression to check + * + * @return bool + */ + private function isCallToFunctionThatThrows(Node\Expr $expr): bool { + if ($expr instanceof Node\Expr\FuncCall) { + 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->functionAlwaysThrows($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->functionAlwaysThrows($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; + } + + $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->functionAlwaysThrows($methodNode); + } + + /** + * 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 getMethodNodeFromAbstraction(\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; } } 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; } 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..28873a25 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,86 @@ 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-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 @@ +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(); } @@ -570,4 +666,4 @@ function switchInsideTryCatch(int $x): int { } catch (\Exception $e) { return 0; } -} \ No newline at end of file +} diff --git a/tests/units/Checks/TestData/ThrowHelper.php b/tests/units/Checks/TestData/ThrowHelper.php new file mode 100644 index 00000000..b9c8e6af --- /dev/null +++ b/tests/units/Checks/TestData/ThrowHelper.php @@ -0,0 +1,7 @@ +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 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() { - // 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(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(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() {