feat(lint): add checkForEach option to useIterableCallbackReturn#8637
feat(lint): add checkForEach option to useIterableCallbackReturn#8637tt-a1i wants to merge 6 commits intobiomejs:nextfrom
Conversation
🦋 Changeset detectedLatest commit: b9c55fa 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
CodSpeed Performance ReportMerging #8637 will not alter performanceComparing Summary
Footnotes
|
WalkthroughAdds a Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.rs📄 CodeRabbit inference engine (CONTRIBUTING.md)
Files:
🧠 Learnings (20)📓 Common learnings📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-22T09:26:56.943ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-11-24T18:05:20.371ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
📚 Learning: 2025-12-19T12:53:30.413ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
🔇 Additional comments (5)
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 |
dyc3
left a comment
dyc3
left a comment
There was a problem hiding this comment.
Looks good, mostly just housekeeping notes
.changeset/check-for-each-option.md
Outdated
| @@ -0,0 +1,5 @@ | |||
| --- | |||
| "@biomejs/biome": patch | |||
There was a problem hiding this comment.
Since its a new feature:
| "@biomejs/biome": patch | |
| "@biomejs/biome": minor |
This will also need to point to the next branch
| /// Default: `false` | ||
| /// | ||
| /// Example configuration: | ||
| /// | ||
| /// ```json | ||
| /// { | ||
| /// "linter": { | ||
| /// "rules": { | ||
| /// "suspicious": { | ||
| /// "useIterableCallbackReturn": { | ||
| /// "level": "error", | ||
| /// "options": { | ||
| /// "checkForEach": true | ||
| /// } | ||
| /// } | ||
| /// } | ||
| /// } | ||
| /// } | ||
| /// } | ||
| /// ``` |
There was a problem hiding this comment.
grep for json,options to see how we usually document options
| /// | ||
| /// Default: `false` | ||
| #[serde(default)] | ||
| pub check_for_each: bool, |
There was a problem hiding this comment.
This needs to be an Option<bool> so that options merging makes sense. You can add an impl block to provide the value of the option so you don't have to write .unwrap_or_default() when you access it.
| @@ -0,0 +1,26 @@ | |||
| // These forEach cases should be invalid when checkForEach is true | |||
There was a problem hiding this comment.
Should start with the magic comment /* should generate diagnostics */ so our infra knows and can assert for it
github-actions
bot
added
A-CLI
Add a new `checkForEach` option to the `useIterableCallbackReturn` rule. When set to `true`, the rule will check that `forEach` callbacks do not return a value, matching ESLint's `array-callback-return` rule behavior. The default is `false`, meaning `forEach` callbacks are not checked by default (behavior change from previous versions). Closes biomejs#8024
Distinguish between explicit `return` statements and implicit returns (arrow expression body) to provide more accurate diagnostic messages: - Explicit return: "Either remove this return or remove the returned value." - Implicit return: "Use a block body with no return, or wrap the expression with void."
- changeset: patch → minor - options doc: use json,options format - check_for_each: bool → Option<bool> with getter method - test file: add magic comment
tt-a1i
force-pushed
the
feat/check-for-each-option
branch
from
1d63a0c to
3e79e37
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rs (1)
60-67: Useuse_optionscode block property for option-dependent examples.The invalid example should use the
use_optionsproperty andexpect_diagnosticinstead ofignore. This allows the example to be properly tested with the specified options.🔎 Proposed fix
-/// The following code is invalid when `checkForEach` is `true`: -/// -/// ```js,ignore -/// // biome.json: { "linter": { "rules": { "suspicious": { "useIterableCallbackReturn": { "options": { "checkForEach": true } } } } } } +/// ```js,expect_diagnostic,use_options /// [].forEach(() => { /// return 1; // Should not return a value /// });Then ensure the
use_optionscode block is preceded by the options configuration block (lines 96-102).Based on learnings, invalid code examples must use
expect_diagnosticand emit exactly one diagnostic.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/invalidCheckForEach.js.snapis excluded by!**/*.snapand included by**
📒 Files selected for processing (4)
.changeset/check-for-each-option.mdcrates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rscrates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/invalidCheckForEach.jscrates/biome_rule_options/src/use_iterable_callback_return.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/biome_rule_options/src/use_iterable_callback_return.rs
- crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/invalidCheckForEach.js
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Use inline rustdoc documentation for rules, assists, and their options
Use thedbg!()macro for debugging output in Rust tests and code
Use doc tests (doctest) format with code blocks in rustdoc comments; ensure assertions pass in tests
Files:
crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rs
🧠 Learnings (17)
📚 Learning: 2025-12-19T12:53:30.413Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-12-19T12:53:30.413Z
Learning: Commit rule work with message format `feat(biome_<language>_analyze): <ruleName>`
Applied to files:
.changeset/check-for-each-option.md
📚 Learning: 2025-12-21T21:15:03.796Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-21T21:15:03.796Z
Learning: For promoting rules from nursery or implementing new features affecting end users, send PRs to the `next` branch
Applied to files:
.changeset/check-for-each-option.md
📚 Learning: 2025-12-19T12:53:30.413Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-12-19T12:53:30.413Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/**/*.rs : Assist rules should detect refactoring opportunities and emit code action signals
Applied to files:
crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rs
📚 Learning: 2025-12-19T12:53:30.413Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-12-19T12:53:30.413Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/**/*.rs : Implement `action` function in Rule trait to provide code actions
Applied to files:
crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rs
📚 Learning: 2025-12-22T09:26:56.943Z
Learnt from: ematipico
Repo: biomejs/biome PR: 8537
File: crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs:167-210
Timestamp: 2025-12-22T09:26:56.943Z
Learning: When defining lint rules (declare_lint_rule!), only specify fix_kind if the rule implements an action(...) function. Rules that only emit diagnostics without a code fix should omit fix_kind. This applies to all Rust lint rule definitions under crates/.../src/lint (e.g., crates/biome_js_analyze/src/lint/...).
Applied to files:
crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rs
📚 Learning: 2025-12-19T12:53:30.413Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-12-19T12:53:30.413Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/**/*.rs : Use generic rule names if the rule could potentially be implemented for multiple languages
Applied to files:
crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rs
📚 Learning: 2025-12-19T12:53:30.413Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-12-19T12:53:30.413Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Lint rules should perform static analysis of source code to detect invalid or error-prone patterns and emit diagnostics with proposed fixes
Applied to files:
crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rs
📚 Learning: 2025-12-19T12:53:30.413Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-12-19T12:53:30.413Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/**/*.rs : Use helper functions like `map`, `filter`, and `and_then` to avoid deep indentation
Applied to files:
crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rs
📚 Learning: 2025-12-19T12:53:30.413Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-12-19T12:53:30.413Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/**/*.rs : Use `let else` trick when `run` function returns `Vec` to reduce code branching
Applied to files:
crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rs
📚 Learning: 2025-12-19T12:53:30.413Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-12-19T12:53:30.413Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/**/*.rs : Use language-specific rule names if the rule is meant for a specific language only
Applied to files:
crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rs
📚 Learning: 2025-12-19T12:53:30.413Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-12-19T12:53:30.413Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/**/*.rs : Use `RuleSource::Eslint(...).same()` when implementing a rule that matches the behavior of an ESLint rule
Applied to files:
crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rs
📚 Learning: 2025-12-19T12:53:30.413Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-12-19T12:53:30.413Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/**/*.rs : Specify `fix_kind: FixKind::Unsafe` in `declare_lint_rule!` for unsafe code actions
Applied to files:
crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rs
📚 Learning: 2025-12-19T12:53:30.413Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-12-19T12:53:30.413Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/**/*.rs : Use `use_options` code block property for code examples that follow an options configuration in documentation
Applied to files:
crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rs
📚 Learning: 2025-12-19T12:53:30.413Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-12-19T12:53:30.413Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/**/*.rs : Use `full_options` code block property for complete biome.json configuration snippets in documentation
Applied to files:
crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rs
📚 Learning: 2025-11-24T18:05:20.371Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-11-24T18:05:20.371Z
Learning: Applies to crates/biome_formatter/**/biome_*_formatter/tests/specs/**/options.json : Create an `options.json` file (formatted as `biome.json`) in test specification folders to apply non-default formatting options to all test files in that folder
Applied to files:
crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rs
📚 Learning: 2025-12-19T12:53:30.413Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-12-19T12:53:30.413Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/**/*.rs : Each invalid code example in rule documentation must emit exactly one diagnostic
Applied to files:
crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rs
📚 Learning: 2025-12-19T12:53:30.413Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-12-19T12:53:30.413Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/**/*.rs : Invalid code examples in rule documentation must be marked with `expect_diagnostic` code block property
Applied to files:
crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rs
🧬 Code graph analysis (1)
crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rs (1)
crates/biome_service/src/workspace.rs (1)
markup(1194-1196)
🔇 Additional comments (7)
crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rs (6)
96-102: LGTM!The
json,optionsformat correctly documents the rule option as suggested in past review comments.
161-164: LGTM!The skip logic correctly bypasses forEach checks when
checkForEachis false, implementing the desired default behaviour.
185-208: LGTM!The implicit return handling correctly treats arrow expression bodies as return-with-value paths and emits appropriate diagnostics for the non-return-value-required case.
268-275: LGTM!The diagnostic message clearly guides users on how to fix implicit returns, suggesting either a block body or wrapping with
void.
303-304: LGTM!The new
UnexpectedImplicitReturnvariant andimplicit_returnfield properly extend the type system to track implicit returns separately from explicit return statements.Also applies to: 317-319
345-346: LGTM!Implicit returns are correctly distinguished from explicit returns and void expressions, enabling proper diagnostic emission for option-dependent behaviour.
.changeset/check-for-each-option.md (1)
1-5: LGTM!The changeset correctly declares this as a minor version bump for the new
checkForEachoption. The description clearly explains the feature and its alignment with ESLint's behaviour. Past review comments have been addressed.
crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rs
Outdated
Show resolved
Hide resolved
github-actions
bot
removed
A-Project
|
All fixed, thanks for the review!
|
crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rs
Outdated
Show resolved
Hide resolved
tt-a1i
force-pushed
the
feat/check-for-each-option
branch
from
87b82c5 to
d7fcc32
Compare
|
Removed the ignored example and changed the second one to use |
|
Please, next time, comment on the issue. There's already a PR #8289 |
3 participants
Summary
checkForEachoption to theuseIterableCallbackReturnruletrue, the rule will check thatforEachcallbacks do not return a valuefalse, matching ESLint'sarray-callback-returnrule behaviorforEachcallbacks are no longer checked by defaultCloses #8024
Test plan
checkForEach: trueoption ininvalidCheckForEach.jsinvalid.jsto remove forEach cases (now valid by default)cargo t -p biome_js_analyze use_iterable_callback_returnAI Assistance Disclosure
I used Codex to review the changes, sanity-check the implementation against existing patterns, and help spot potential edge cases.