Conversation
🦋 Changeset detectedLatest commit: d7fc389 The changes in this PR will be included in the next version bump. This PR includes changesets to release 13 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Merging this PR will not alter performance
Comparing Footnotes
|
WalkthroughAdds a new JavaScript lint rule Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/biome_js_analyze/src/lint/nursery/no_nested_promises.rs`:
- Around line 1-79: The rule NoNestedPromises currently sets type Options = (),
but there is an existing options struct NoNestedPromisesOptions in
biome_rule_options; change the rule's Options associated type from () to
NoNestedPromisesOptions so the rule uses the proper config/schema and option
parsing. Update the impl Rule for NoNestedPromises to reference
NoNestedPromisesOptions (and adjust any uses of the options value in methods if
needed) so configuration is correctly wired.
In `@crates/biome_rule_options/src/no_nested_promises.rs`:
- Around line 1-6: Add a one-line inline rustdoc above the
NoNestedPromisesOptions struct describing what the options configure (e.g.,
"Options for the `no-nested-promises` rule."), matching the style used by other
rule option structs; ensure the doc is a single-line /// comment placed
immediately above the pub struct NoNestedPromisesOptions {} declaration so it
appears in generated docs and follows existing guidelines.
71616e9 to
a097ee0
Compare
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@crates/biome_js_analyze/src/lint/nursery/no_nested_promises.rs`:
- Around line 152-174: The ancestor walk in is_promise_callback currently
matches any ancestor call whose member name is "then"/"catch" even if the
function is nested deeper; update is_promise_callback to, after locating a
JsCallExpression whose callee is a .then/.catch name, verify that the function
node is a direct argument of that call (e.g., walk its JsCallExpression
arguments / JsCallArgument children and check one of their expression nodes
equals func.syntax() or the function's immediate JsCallArgument node) so only
direct promise callbacks return true; keep references to is_promise_callback,
AnyJsFunction, JsCallExpression, and JsCallArgument to locate the change.
In `@crates/biome_js_analyze/tests/specs/nursery/noNestedPromises/valid.js`:
- Around line 14-15: The two lines use an invalid two-argument form for
Promise.prototype.catch: locate the calls to doThing().catch(null, function() {
... }) and either remove them or change them to the intended form — if you meant
to register a rejection handler only, replace with doThing().catch(function() {
... }), or if you intended the then(null, onRejected) pattern, replace with
doThing().then(null, function() { ... }); update both occurrences accordingly
(the two doThing().catch(...) expressions).
🧹 Nitpick comments (2)
crates/biome_js_analyze/src/lint/nursery/no_nested_promises.rs (1)
227-237: Consider usingAstNode::ancestors()for cleaner traversal.The manual parent-walking loop works, but
biome_rowanprovidesancestors()which could simplify this:fn is_descendant_of(node: &JsSyntaxNode, potential_ancestor: &JsSyntaxNode) -> bool { node.ancestors().any(|ancestor| ancestor == *potential_ancestor) }crates/biome_js_analyze/tests/specs/nursery/noNestedPromises/invalid.js (1)
19-23: Clarify test comment.The comment says "references vars in closure but doesn't use them" which is a bit confusing. The outer callback receives
aand passes it togetB(a), but the inner.then(b => getC(b))only uses its own parameterb, not the outera. Perhaps clarify:-// references vars in closure but doesn't use them +// inner callback doesn't reference outer scope variable 'a'
| /// Check if a function is a direct callback to .then() or .catch() | ||
| fn is_promise_callback(func: &AnyJsFunction) -> bool { | ||
| // Walk up the tree to find a call expression | ||
| // The structure is: Function -> JsCallArgument (or similar) -> ... -> JsCallExpression | ||
| func.syntax() | ||
| .ancestors() | ||
| .skip(1) // skip self | ||
| .find_map(|node| { | ||
| JsCallExpression::cast_ref(&node)? | ||
| .callee() | ||
| .ok()? | ||
| .as_js_static_member_expression()? | ||
| .member() | ||
| .ok()? | ||
| .as_js_name()? | ||
| .value_token() | ||
| .ok() | ||
| }) | ||
| .is_some_and(|token| { | ||
| let text = token.text_trimmed(); | ||
| text == "then" || text == "catch" | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the actual code around lines 152-174
sed -n '140,190p' crates/biome_js_analyze/src/lint/nursery/no_nested_promises.rsRepository: biomejs/biome
Length of output: 1871
🏁 Script executed:
# Search for test files related to noNestedPromises
find crates/biome_js_analyze/tests -name "*noNestedPromises*" -o -name "*nested*promise*"Repository: biomejs/biome
Length of output: 117
🏁 Script executed:
# Check for test cases with nested or unrelated callbacks
rg -n "unrelated|setTimeout|forEach" crates/biome_js_analyze/tests/ --type rust -A 3 -B 3 | head -100Repository: biomejs/biome
Length of output: 778
🏁 Script executed:
# List all files in the test directory
ls -la crates/biome_js_analyze/tests/specs/nursery/noNestedPromises/Repository: biomejs/biome
Length of output: 420
🏁 Script executed:
# Read the test specification file
cat crates/biome_js_analyze/tests/specs/nursery/noNestedPromises.jsonc 2>/dev/null || cat crates/biome_js_analyze/tests/specs/nursery/noNestedPromises.json 2>/dev/null || find crates/biome_js_analyze/tests/specs/nursery -name "*noNestedPromises*" -type fRepository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Look for JavaScript test input files
find crates/biome_js_analyze/tests/specs/nursery/noNestedPromises -name "*.js" -o -name "*.jsx" -o -name "*.ts" -o -name "*.tsx"Repository: biomejs/biome
Length of output: 198
🏁 Script executed:
# Examine the test cases
cat crates/biome_js_analyze/tests/specs/nursery/noNestedPromises/invalid.jsRepository: biomejs/biome
Length of output: 616
🏁 Script executed:
cat crates/biome_js_analyze/tests/specs/nursery/noNestedPromises/valid.jsRepository: biomejs/biome
Length of output: 1523
🏁 Script executed:
# Check if there are any test snapshots that show unexpected behavior
cat crates/biome_js_analyze/tests/specs/nursery/noNestedPromises/invalid.js.snap | head -100Repository: biomejs/biome
Length of output: 3126
🏁 Script executed:
# Search for other test references to understand if this scenario is covered elsewhere
rg -n "unrelated\|nested.*forEach\|nested.*setTimeout" crates/biome_js_analyze/tests/specs/nursery/ -A 3Repository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Create a test case to verify the false positive concern
cat > /tmp/test_false_positive.js << 'EOF'
// This should NOT be flagged as a nested promise
something.then(() => {
unrelated(() => {
nested.then(() => {})
})
})
EOF
# Check what the rule currently does
cat /tmp/test_false_positive.jsRepository: biomejs/biome
Length of output: 183
🏁 Script executed:
# Let's trace through the ancestor logic more carefully by examining the actual code flow
# Look at how the function uses the results
sed -n '130,160p' crates/biome_js_analyze/src/lint/nursery/no_nested_promises.rsRepository: biomejs/biome
Length of output: 1146
🏁 Script executed:
# Check if there's a direct argument check anywhere
rg -n "direct\|argument\|parent" crates/biome_js_analyze/src/lint/nursery/no_nested_promises.rs -B 2 -A 2Repository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Let's understand the full rule logic to see if the concern is valid
# Look at where is_promise_callback is called and how it's used
rg -n "is_promise_callback\|find_parent_promise_callback" crates/biome_js_analyze/src/lint/nursery/no_nested_promises.rs -B 3 -A 3Repository: biomejs/biome
Length of output: 39
🏁 Script executed:
# Let's trace the visit logic - when is a nested promise detected?
sed -n '80,130p' crates/biome_js_analyze/src/lint/nursery/no_nested_promises.rsRepository: biomejs/biome
Length of output: 2144
🏁 Script executed:
# Let me trace through the exact scenario from the review to understand the AST structure
# Look at the full file to understand the complete context
wc -l crates/biome_js_analyze/src/lint/nursery/no_nested_promises.rsRepository: biomejs/biome
Length of output: 123
🏁 Script executed:
# Get the complete file content to understand the full logic
cat crates/biome_js_analyze/src/lint/nursery/no_nested_promises.rsRepository: biomejs/biome
Length of output: 8364
The concern is valid: is_promise_callback() can incorrectly match distant promise chain ancestors.
The ancestor walk in is_promise_callback doesn't verify the function is a direct argument of the matched call. For instance:
something.then(() => {
unrelated(() => {
nested.then(() => {})
})
})The inner arrow is passed to unrelated(), but its ancestor chain includes something.then() up the tree. The function would incorrectly identify it as a promise callback, flagging nested.then() as a false positive.
The existing test suite doesn't cover this nested non-promise-callback scenario, so the bug isn't caught. The fix would require verifying the function is a direct first argument of the matched .then()/.catch() call, not just an ancestor.
🤖 Prompt for AI Agents
In `@crates/biome_js_analyze/src/lint/nursery/no_nested_promises.rs` around lines
152 - 174, The ancestor walk in is_promise_callback currently matches any
ancestor call whose member name is "then"/"catch" even if the function is nested
deeper; update is_promise_callback to, after locating a JsCallExpression whose
callee is a .then/.catch name, verify that the function node is a direct
argument of that call (e.g., walk its JsCallExpression arguments /
JsCallArgument children and check one of their expression nodes equals
func.syntax() or the function's immediate JsCallArgument node) so only direct
promise callbacks return true; keep references to is_promise_callback,
AnyJsFunction, JsCallExpression, and JsCallArgument to locate the change.
| doThing().catch(null, function() { return 4 }) | ||
| doThing().catch(null, function() { throw 4 }) |
There was a problem hiding this comment.
Unusual catch syntax.
Promise.prototype.catch only accepts a single callback argument. The catch(null, function() {...}) pattern passes two arguments which is semantically incorrect (the second argument is ignored). This won't break the test but may confuse readers.
Was this intended to mirror the .then(null, onRejected) pattern? If so, consider removing these lines or updating them to valid catch usage.
🤖 Prompt for AI Agents
In `@crates/biome_js_analyze/tests/specs/nursery/noNestedPromises/valid.js` around
lines 14 - 15, The two lines use an invalid two-argument form for
Promise.prototype.catch: locate the calls to doThing().catch(null, function() {
... }) and either remove them or change them to the intended form — if you meant
to register a rejection handler only, replace with doThing().catch(function() {
... }), or if you intended the then(null, onRejected) pattern, replace with
doThing().then(null, function() { ... }); update both occurrences accordingly
(the two doThing().catch(...) expressions).
a097ee0 to
d7fc389
Compare
Summary
This PR adds a rule from
eslint-plugin-promise, a new rule source. This plugin provides a bunch of Promise-specific lints that can be pretty valuable, particularly to beginner programmers (IMO).Source rule: https://github.com/eslint-community/eslint-plugin-promise/blob/main/docs/rules/no-nesting.md
This rule specifically applies to Promises, but I intentionally chose not to use type information because the source rule also does not use type information to know if its flagging a promise.
Generated by sonnet 4.5, but I tweaked it a bit manually afterwards.
Test Plan
copied all the tests from the source rule.
Docs