Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
212 changes: 181 additions & 31 deletions src/Checks/ReturnCheck.php
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
}
Expand All @@ -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;
}
}
Expand All @@ -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) {
Expand All @@ -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;
}
}
Expand All @@ -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;
}
}
Expand Down Expand Up @@ -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 {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what we're doing. We're trying to grab the body of the function that is being called in order to look up whether is contains a throw statement. That's going to work if the function is in the same file, but if it is a function in a different file then it won't be able to get the method body the way that we do here. There is a way to grab the file contents and parse it, but it is a pretty big performance hit. I have a branch where I do some work around declared throws where we don't have to have the method body handy to know if a function might throw. We don't have anything for a method that ALWAYS throws, which I think is what we're looking for here.

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;
}
}
Loading
Loading