feat: port rule promise/always-return#938
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the promise/always-return rule, which ensures that callbacks within .then() blocks return a value or throw an error. The implementation includes support for ignoreLastCallback and ignoreAssignmentVariable options, along with comprehensive documentation and tests. Feedback suggests refining the isLastCallback logic to better align with ESLint's handling of AwaitExpression, .catch(), and .finally(). Additionally, the statementTerminates function should be updated to support SwitchStatement, and the checkFunction logic can be optimized to avoid redundant calls to isLastCallback.
elecmonkey
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
…e_always_return-20260514
- Introduce completion three-value enum (FallsThrough/Terminates/Stops) to distinguish break/continue from return/throw - Add statementCompletion/statementListCompletion replacing the old bool helpers - Handle KindSwitchStatement (default clause + fallthrough simulation) - Handle KindWhileStatement/KindForStatement infinite loops (literal-true condition) - Handle KindDoStatement (body runs at least once) - Handle KindLabeledStatement (recurse into inner statement) - Add break-escape detection so loop bodies containing break are not incorrectly treated as terminating - Lock all seven upstream false-positive cases into the test suite
Deploying rslint with
|
| Latest commit: |
3e8da66
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://02e770b2.rslint.pages.dev |
| Branch Preview URL: | https://feat-port-rule-promise-alway.rslint.pages.dev |
…e_always_return-20260514
…utils/flow - Add Completion enum (FallsThrough/Terminates/Stops) to internal/utils/flow.go - Add StatementCompletion, StatementListCompletion, IsFunctionLikeContainer as exported APIs; all private helpers move alongside them - Simplify promise/always-return to delegate to utils.StatementListCompletion, removing ~270 lines of private duplicated logic - Add FLOW_UTILS_MIGRATION.md tracking three remaining sites to migrate (no_fallthrough, jsx_max_depth, react_hooksutil)
elecmonkey
force-pushed
the
feat/port-rule-promise_always_return-20260514
branch
from
6ca6578 to
52a3914
Compare
|
ci passed. some functions (IsFunctionLikeContainer, Completion, StatementCompletion, StatementListCompletion) were extracted into if this pr is merged, maybe I could launch a new pr to update some other things about the functions extracted?
|
There was a problem hiding this comment.
@elecmonkey Thanks for the port! The 7 cases I flagged earlier are all fixed. Cross-checked against eslint-plugin-promise@7 and found three remaining divergences, all in internal/utils/flow.go:
- Critical: 4 false positives from infinite-loop body-completion check (one shared fix)
- Minor:
while (1)not recognized as truthy - Edge case: labeled
breakthat escapes a labeled block
Details on each in the inline comments below.
Once these are addressed (or the edge case explicitly deferred with a note), this is good to merge from my side. flow.go becoming shared infrastructure is also a nice win — the loop fix here will pay off for any future rule using StatementCompletion.
And feel free to open a follow-up PR for the other updates related to the extracted functions.
| func loopBodyTerminates(loop *ast.Node, body *ast.Node) bool { | ||
| return StatementCompletion(body) == CompletionTerminates && !containsBreakExitingLoop(loop, body) | ||
| } |
There was a problem hiding this comment.
These four are flagged here, valid upstream:
hey.then(function() { while (true) {} })
hey.then(function(x) { while (true) { if (x) return 1; } })
hey.then(function() { while (true) { x++; } })
hey.then(function() { do {} while (true) })For an infinite loop, "body must strictly terminate" is too strict — a body that just falls through and lets the loop iterate again is fine, since the only ways out are return / throw / a break that exits the loop. Replacing the body-terminates check with !containsBreakExitingLoop(stmt, body) at the three call sites matches upstream's CodePath model (where infinite loops have no finalSegments to complain about):
case ast.KindWhileStatement:
whileStmt := stmt.AsWhileStatement()
if whileStmt != nil && isLiteralTrue(whileStmt.Expression) &&
!containsBreakExitingLoop(stmt, whileStmt.Statement) {
return CompletionTerminates
}
return CompletionFallsThrough
// same shape for ForStatement
case ast.KindDoStatement:
doStmt := stmt.AsDoStatement()
if doStmt == nil { return CompletionFallsThrough }
if isLiteralTrue(doStmt.Expression) { // do { ... } while (true)
if !containsBreakExitingLoop(stmt, doStmt.Statement) {
return CompletionTerminates
}
return CompletionFallsThrough
}
if loopBodyTerminates(stmt, doStmt.Statement) { // non-infinite: body runs once
return CompletionTerminates
}
return CompletionFallsThroughVerified locally — all 62 existing tests still pass.
| func isLiteralTrue(expr *ast.Node) bool { | ||
| expr = ast.SkipOuterExpressions(expr, skipOEKParentheses) | ||
| return expr != nil && expr.Kind == ast.KindTrueKeyword | ||
| } |
There was a problem hiding this comment.
Minor: only KindTrueKeyword is matched, so this gets flagged:
hey.then(function() { while (1) { return 1; } })ESLint's CodePath also treats non-zero numeric literals and non-empty string literals as definitely-truthy. Easy extension.
| case ast.KindLabeledStatement: | ||
| labeledStmt := stmt.AsLabeledStatement() | ||
| if labeledStmt == nil { | ||
| return CompletionFallsThrough | ||
| } | ||
| return StatementCompletion(labeledStmt.Statement) |
There was a problem hiding this comment.
Edge case: silent here, reported upstream:
hey.then(function(x) { outer: { if (x) break outer; return 1; } })If x is truthy, break outer exits the labeled block and the function falls through. The three-state Completion collapses if (x) break outer; (no else) to FallsThrough and the "could-stop" signal is lost by the time the labeled block decides its completion — so it ends up looking like it always terminates via the trailing return 1.
Tricky to fix cleanly without enriching the model — flagging for awareness, not blocking.
Summary
Related Links
Checklist