feat(lint/useIterableCallbackReturn): add allowImplicit option#9489
feat(lint/useIterableCallbackReturn): add allowImplicit option#9489mvanhorn wants to merge 10 commits into
Conversation
Add `allowImplicit` option that permits callbacks of methods requiring return values (map, filter, etc.) to use `return;` for implicit undefined returns. This matches ESLint's `allowImplicit` option for the `array-callback-return` rule. When enabled, `return;` in map/filter/etc. callbacks is accepted as intentional. When disabled (the default), the existing behavior is preserved. Fixes biomejs#9445 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 6c22bb1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces an Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rs`:
- Around line 217-233: The allow_implicit branch currently only reports
MissingReturnWithValue when there are exclusively fall-through paths and no
returns at all, missing cases where some explicit returns exist but other paths
fall through; update the block guarded by allow_implicit to mirror the logic
used in the non-allow branch for fall-throughs: if
returns_info.has_paths_without_returns then if
returns_info.returns_with_value.is_empty() push
RuleProblemKind::MissingReturnWithValue else push
RuleProblemKind::NotAllPathsReturnValue, leaving the treatment of
returns_without_value unchanged; adjust the code around allow_implicit,
returns_info, and problems to implement this behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0cce0f02-da88-4157-870f-901303f491ab
📒 Files selected for processing (2)
crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rscrates/biome_rule_options/src/use_iterable_callback_return.rs
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/warm-clouds-float.md:
- Around line 1-3: The changeset currently declares a patch release for the
package "@biomejs/biome" but adds a new option (a feature) so update the
changeset header in .changeset/warm-clouds-float.md: change the value from
"patch" to "minor" for the "@biomejs/biome" entry so the release type correctly
reflects a minor (feature) bump.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f503e0c3-b720-4a86-9f89-d91c479ee38e
📒 Files selected for processing (1)
.changeset/warm-clouds-float.md
Adding a new option is a feature addition under semver. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…cit mode When allowImplicit is true and some paths return values while others fall through without any return, report NotAllPathsReturnValue. Previously only the case where NO returns existed at all was caught. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/allowImplicit.js (1)
1-31: Good test coverage for the coreallowImplicitbehaviour.The test cases comprehensively cover the main scenarios. One small suggestion: the PR objectives mention the
.filter(Boolean)chaining pattern as a use case. You might consider adding a test demonstrating this idiom:// Valid: mapping with intentional undefined, then filtering [1, 2, 3].map((x) => { if (x > 2) return x; return; }).filter(Boolean);That said, functionally it's the same as case 1, so this is entirely optional.
,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/allowImplicit.js` around lines 1 - 31, Add a test case to the existing allowImplicit spec demonstrating the common `.map(...).filter(Boolean)` idiom: append a snippet that maps with an explicit empty `return;` on some paths (same pattern as the first valid case) and then chains `.filter(Boolean)` to show that intentional undefineds are filtered out; reference the arrow callback used in the existing tests (the `.map((x) => { if (x > 2) return x; return; })` pattern) so the new test mirrors that behavior but adds the `.filter(Boolean)` call to the expression.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/allowImplicit.js`:
- Around line 1-31: Add a test case to the existing allowImplicit spec
demonstrating the common `.map(...).filter(Boolean)` idiom: append a snippet
that maps with an explicit empty `return;` on some paths (same pattern as the
first valid case) and then chains `.filter(Boolean)` to show that intentional
undefineds are filtered out; reference the arrow callback used in the existing
tests (the `.map((x) => { if (x > 2) return x; return; })` pattern) so the new
test mirrors that behavior but adds the `.filter(Boolean)` call to the
expression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 26c43933-27c6-4b97-b354-3546e80348a5
⛔ Files ignored due to path filters (1)
crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/allowImplicit.js.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (3)
crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rscrates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/allowImplicit.jscrates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/allowImplicit.options.json
ematipico
left a comment
There was a problem hiding this comment.
Thank you! Still some work required
- tests needs to be separated
- the PR most go
next, as per contribution guidelines
| /// This is useful for patterns like mapping with early returns where some paths | ||
| /// intentionally return `undefined`. | ||
| /// | ||
| /// This matches ESLint's [`allowImplicit`](https://eslint.org/docs/latest/rules/array-callback-return#allowimplicit) option. |
There was a problem hiding this comment.
This information isn't needed
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
| /// [1, 2, 3].map((x) => { | ||
| /// if (x > 2) return x; | ||
| /// return; | ||
| /// }); |
There was a problem hiding this comment.
Can we have a snippet that is more real?
For example we should a .filter(Boolean)
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
| @@ -0,0 +1,31 @@ | |||
| // Valid: empty return is accepted with allowImplicit | |||
There was a problem hiding this comment.
The file should have // should not generate diagnostics comment on top
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
| [1, 2, 3].map((x) => { | ||
| if (x > 2) return x; | ||
| return; | ||
| }); | ||
|
|
||
| // Valid: all paths return values | ||
| [1, 2, 3].map((x) => { | ||
| if (x > 2) return x; | ||
| return 0; | ||
| }); | ||
|
|
||
| // Valid: implicit return (arrow expression) | ||
| [1, 2, 3].map((x) => x * 2); | ||
|
|
||
| // Invalid: some paths return values, others fall through without any return | ||
| [1, 2, 3].map((x) => { | ||
| if (x > 2) return x; | ||
| // falls through - likely a bug | ||
| }); | ||
|
|
||
| // Invalid: no returns at all | ||
| [1, 2, 3].map((x) => { | ||
| console.log(x); | ||
| }); | ||
|
|
||
| // Valid: only empty returns (all paths covered) | ||
| [1, 2, 3].forEach((x) => { | ||
| if (x > 2) return; | ||
| console.log(x); | ||
| }); |
There was a problem hiding this comment.
We don't mix valid and invalid cases. Valid in a file, invalid in another file
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
- Split allowImplicit tests into valid/invalid files per convention - Add "// should not generate diagnostics" header to valid file - Replace basic map example with .filter(Boolean) pattern in docs - Remove ESLint reference and inline comments from rule logic
| /// }).filter(Boolean); | ||
| /// ``` | ||
| /// | ||
| /// When `allowImplicit` is `true`, the above code will not trigger any diagnostic. |
There was a problem hiding this comment.
| /// When `allowImplicit` is `true`, the above code will not trigger any diagnostic. |
That's implied by the infrastructure
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
| @@ -0,0 +1,10 @@ | |||
| // some paths return values, others fall through without any return | |||
There was a problem hiding this comment.
Invalid files must have // should not generate diagnostics. You're using an AI agent, load the skills we provide.
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
There was a problem hiding this comment.
Yeah sorry, between the two files I got confused, but it's easily understandable:
- valid -> should not
- invalid -> should
Every file should have the top-level comments
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
| @@ -0,0 +1,29 @@ | |||
| // should not generate diagnostics | |||
This comment was marked as low quality.
This comment was marked as low quality.
Sorry, something went wrong.
- Remove "When allowImplicit is true..." line (implied by infrastructure) - Fix valid test file header: use /* */ block comment to match convention - Verified all 10 useIterableCallbackReturn tests pass locally
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Merging this PR will not alter performance
Comparing Footnotes
|
|
Yet another bot |
Summary
Adds an
allowImplicitoption to theuseIterableCallbackReturnrule, matching ESLint'sallowImplicitoption forarray-callback-return.When
allowImplicitistrue, callbacks for methods that require a return value (likemap,filter,find, etc.) can usereturn;to implicitly returnundefined. This supports patterns like:The default is
false, preserving the existing behavior.As discussed in the issue, this was identified as an enhancement rather than a bug fix. The maintainer confirmed this should match ESLint's behavior.
Fixes #9445
Test Plan
cargo check -p biome_js_analyzepassesDocs
Rule documentation is updated inline with the new
allowImplicitoption, including a JSON config example and a JavaScript code example.AI Assistance
This PR was written with AI assistance (Claude Code).