fix: missed where opporunity false positive - #2
anurag6569201 wants to merge 1 commit into
Conversation
Source PR: github#22484 Source head: dd84d7f
⛔ Shipwright · BlockedRecommendation: do not merge PR #2 · Tier
Findings (7)
Fireworks usage: 10,428 input · 841 output · 11,269 total tokens · $0.0028 · 19s · 0 fix iteration(s) Open the Shipwright check for full evidence and the audit bundle. Use |
![shipwright-agent[bot]](https://avatars.githubusercontent.com/u/140162738?s=60&v=4){kind=small}
| else result = 1 | ||
| } | ||
|
|
||
| private predicate terminatesCallable(Stmt s) { |
There was a problem hiding this comment.
Shipwright · CRITICAL
terminatesCallable is recursive and has no base case for loops or switch statements.
Impact: terminatesCallable is recursive and has no base case for loops or switch statements. A foreach/while/for/do loop whose body contains a return will not be recognized as terminating, causing false positives. More critically, the recursion through BlockStmt.getLastStmt() and IfStmt branches can cycle or fail to terminate on malformed/cyclic ASTs, and the predicate lacks a recursion bound, risking evaluator non-terminat…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| or | ||
| stripped instanceof ThrowStmt | ||
| or | ||
| stripped instanceof BreakStmt |
There was a problem hiding this comment.
Shipwright · CRITICAL
The predicate treats BreakStmt as terminating the callable, but a break only exits the nearest loop/switch, not the method or iterator.
Impact: The predicate treats BreakStmt as terminating the callable, but a break only exits the nearest loop/switch, not the method or iterator. In a foreach nested inside another loop, a break in the filtered branch exits only the inner foreach and continues the outer loop, so the loop is still filtering work and should be flagged. This will suppress true positives.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| else result = 1 | ||
| } | ||
|
|
||
| private predicate terminatesCallable(Stmt s) { |
There was a problem hiding this comment.
Shipwright · HIGH
The name terminatesCallable is misleading: it returns true for BreakStmt, which does not terminate the callable, only the enclosing loop.
Impact: The name terminatesCallable is misleading: it returns true for BreakStmt, which does not terminate the callable, only the enclosing loop. A future maintainer reading missedWhereOpportunity will assume the predicate means method/iterator termination and will not realize break is included, leading to incorrect modifications.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| else result = 1 | ||
| } | ||
|
|
||
| private predicate terminatesCallable(Stmt s) { |
There was a problem hiding this comment.
Shipwright · HIGH
The recursive definition of terminatesCallable is hard to follow because it mixes structural recursion (BlockStmt, IfStmt) with leaf cases (ReturnStmt, YieldBreakStmt, ThrowStmt, B
Impact: The recursive definition of terminatesCallable is hard to follow because it mixes structural recursion (BlockStmt, IfStmt) with leaf cases (ReturnStmt, YieldBreakStmt, ThrowStmt, BreakStmt) without documenting the intended control-flow semantics. There is no comment explaining why BreakStmt is included or why loops are excluded, making the logic opaque to a newcomer.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| else result = 1 | ||
| } | ||
|
|
||
| private predicate terminatesCallable(Stmt s) { |
There was a problem hiding this comment.
Shipwright · HIGH
The predicate does not handle ContinueStmt, GotoStmt, or labeled statements.
Impact: The predicate does not handle ContinueStmt, GotoStmt, or labeled statements. A goto to a label outside the loop or a continue in a nested loop can terminate or alter control flow in ways the predicate misclassifies, leading to incorrect alert suppression or missed alerts.
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
| else result = 1 | ||
| } | ||
|
|
||
| private predicate terminatesCallable(Stmt s) { |
There was a problem hiding this comment.
Shipwright · HIGH
The change modifies a query that flags missed Where opportunities, but the new predicate's control-flow analysis is unsound for exception handling.
Impact: The change modifies a query that flags missed Where opportunities, but the new predicate's control-flow analysis is unsound for exception handling. A try/catch/finally block where the try contains a return but the finally throws or returns is not modeled, so the query may suppress alerts for loops whose filtered branch does not actually terminate the callable, or flag loops that do. This could hide real code-quality…
Suggested fix: Review the cited evidence, fix the risk if confirmed, and rerun Shipwright.
1 participant
fixes github#7936
Source merge-base:
7b2695bef3e42c51172ac398c147bdfe60ee3326Source head:
dd84d7f14473cb1f025259f8c4c935b7e86b8697