feat(lint/useIterableCallbackReturn): add allowImplicit option#9489
feat(lint/useIterableCallbackReturn): add allowImplicit option#9489mvanhorn wants to merge 7 commits intobiomejs:nextfrom
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: fa7c864 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 |
github-actions
bot
added
A-Linter
|
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)
📝 Coding Plan
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 Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rs
Show resolved
Hide resolved
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
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
| /// [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)
There was a problem hiding this comment.
Updated in a742f34: doc example now shows the .filter(Boolean) chaining pattern.
| @@ -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
There was a problem hiding this comment.
Done in a742f34: split into allowImplicitValid.js with the header.
| [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
There was a problem hiding this comment.
Split into allowImplicitValid.js and allowImplicitInvalid.js in a742f34. Also retargeted the PR to next per contribution guidelines.
- 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
| @@ -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.
There was a problem hiding this comment.
I read your CLAUDE.md and the analyzer CONTRIBUTING.md - thank you for maintaining those. On this comment: adding // should not generate diagnostics to the invalid file causes the test infrastructure to expect zero diagnostics, which fails since the file is meant to produce them. The existing invalid.js for this rule also has no such header. I kept the invalid file without the header so tests pass. Let me know if I'm misunderstanding the convention.
Re: AI disclosure - this PR uses Claude Code for implementation. I should have stated that upfront per your contribution guidelines. All tests verified locally.
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
There was a problem hiding this comment.
Added // should generate diagnostics to the invalid file in fa7c864. Thanks for clarifying the convention.
| @@ -0,0 +1,29 @@ | |||
| // should not generate diagnostics | |||
There was a problem hiding this comment.
Fixed in 629f724 - switched to /* should not generate diagnostics */ block comment to match the existing valid.js convention.
- 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>
3 participants
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).