feat(useIterableCallbackReturn): add checkForEach option#8442
Closed
tt-a1i wants to merge 2 commits intobiomejs:mainfrom
Closed
feat(useIterableCallbackReturn): add checkForEach option#8442tt-a1i wants to merge 2 commits intobiomejs:mainfrom
tt-a1i wants to merge 2 commits intobiomejs:mainfrom
Conversation
🦋 Changeset detectedLatest commit: d492dab 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
Contributor
WalkthroughAdds a Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ 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 |
tt-a1i
force-pushed
the
feat/use-iterable-callback-return-check-foreach
branch
from
389fd27 to
d5e1ac9
Compare
Add `checkForEach` option to match ESLint's `array-callback-return` rule. When `true`, reports forEach callbacks that return values. Default: `false` (forEach not checked). Closes biomejs#8024
tt-a1i
force-pushed
the
feat/use-iterable-callback-return-check-foreach
branch
from
d5e1ac9 to
ec048cf
Compare
CodSpeed Performance ReportMerging #8442 will not alter performanceComparing Summary
Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.changeset/add-checkforeach-option.md (1)
1-5: Tighten changeset to 1–3 sentences + add a tiny config example.
Right now it’s a bit of a run-on, and adding a minimal snippet will help users copy/paste. Also: “return a value, since …” reads better.Add `checkForEach` option to [`useIterableCallbackReturn`](https://biomejs.dev/linter/rules/use-iterable-callback-return/) rule. This option allows checking `forEach` callbacks for unexpected return values. When `true`, the rule reports `forEach` callbacks that return a value since `forEach` ignores return values. Default: `false`, matching ESLint's `array-callback-return` rule behavior. +Added the `checkForEach` option to the `useIterableCallbackReturn` rule. +When enabled, Biome reports `forEach` callbacks that return a value, since `forEach` ignores return values. +It defaults to `false`, matching ESLint’s `array-callback-return`. + +```json +{ + "linter": { "rules": { "suspicious": { "useIterableCallbackReturn": { "options": { "checkForEach": true } } } } } +} +```crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEachValid.js (1)
1-6: Nice edge-case coverage forvoidexpression bodies.
Optional: consider also covering a block body withreturn undefined;(same spirit, different AST shape).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEach.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEachValid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/invalid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/valid.js.snapis excluded by!**/*.snapand included by**packages/@biomejs/backend-jsonrpc/src/workspace.tsis excluded by!**/backend-jsonrpc/src/workspace.tsand included by**packages/@biomejs/biome/configuration_schema.jsonis excluded by!**/configuration_schema.jsonand included by**
📒 Files selected for processing (9)
.changeset/add-checkforeach-option.md(1 hunks)crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rs(3 hunks)crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEach.js(1 hunks)crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEach.options.json(1 hunks)crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEachValid.js(1 hunks)crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEachValid.options.json(1 hunks)crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/invalid.js(0 hunks)crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/valid.js(1 hunks)crates/biome_rule_options/src/use_iterable_callback_return.rs(1 hunks)
💤 Files with no reviewable changes (1)
- crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/invalid.js
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/biome_js_analyze/src/lint/suspicious/use_iterable_callback_return.rs
- crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/valid.js
🧰 Additional context used
📓 Path-based instructions (2)
crates/**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Update inline rustdoc documentation for rules, assists, and their options when adding new features or changing existing features in Rust crates
Files:
crates/biome_rule_options/src/use_iterable_callback_return.rs
.changeset/*.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Write changesets that are concise (1-3 sentences), user-focused, use past tense for actions taken and present tense for Biome behavior, include code examples for rules, and end sentences with periods
Files:
.changeset/add-checkforeach-option.md
🧠 Learnings (20)
📓 Common learnings
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Commit rule changes with message format: `feat(biome_<crate>): <ruleName>` to follow Biome's conventional commit style
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/tests/specs/**/*.jsonc : Use `.jsonc` files in test specs with code snippets as array of strings to test rules in script environment (no import/export syntax)
Applied to files:
crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEachValid.jscrates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEach.jscrates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEachValid.options.jsoncrates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEach.options.json
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/tests/specs/**/*.{js,ts,tsx,jsx,json,css} : Test rules using snapshot tests via the `insta` library with test cases in `tests/specs/<group>/<rule_name>/` directories prefixed by `invalid` or `valid`
Applied to files:
crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEachValid.jscrates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEach.jscrates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEachValid.options.jsoncrates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEach.options.json
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/biome_rule_options/src/**/*.rs : Wrap optional rule option fields in `Option<_>` to properly track set vs unset options during configuration merging
Applied to files:
crates/biome_rule_options/src/use_iterable_callback_return.rscrates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEach.options.json
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/biome_rule_options/src/**/*.rs : Rule options struct fields should use `#[serde(rename_all = "camelCase")]`, `#[serde(deny_unknown_fields)]`, and `#[serde(default)]` attributes for proper JSON serialization
Applied to files:
crates/biome_rule_options/src/use_iterable_callback_return.rs
📚 Learning: 2025-12-12T10:11:05.549Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-12-12T10:11:05.549Z
Learning: Applies to crates/**/*.rs : Update inline rustdoc documentation for rules, assists, and their options when adding new features or changing existing features in Rust crates
Applied to files:
crates/biome_rule_options/src/use_iterable_callback_return.rs
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/biome_rule_options/src/**/*.rs : Use `Box<[T]>` instead of `Vec<T>` for rule options array fields to save memory (boxed slices and boxed str use 2 words instead of three words)
Applied to files:
crates/biome_rule_options/src/use_iterable_callback_return.rs
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/biome_rule_options/src/**/*.rs : Rule options must be defined in the `biome_rule_options` crate and implement traits: `Deserializable`, `Merge`, `Serialize`, `Deserialize`, and `JsonSchema`
Applied to files:
crates/biome_rule_options/src/use_iterable_callback_return.rscrates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEach.options.json
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/biome_rule_options/src/**/*.rs : Implement the `Merge` trait for rule options to define how options from extended configuration merge with user configuration (usually reset instead of extend)
Applied to files:
crates/biome_rule_options/src/use_iterable_callback_return.rs
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Document rules with a one-line brief description in the first paragraph of the doc comment, followed by detailed paragraphs, `## Examples` section with `### Invalid` and `### Valid` subsections, and optional `## Options` section
Applied to files:
crates/biome_rule_options/src/use_iterable_callback_return.rs
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Use the `useConsistent` prefix for rules that ensure consistency across the codebase (e.g., `useConsistentArrayType`)
Applied to files:
crates/biome_rule_options/src/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/tests/specs/suspicious/useIterableCallbackReturn/checkForEachValid.options.jsoncrates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEach.options.json
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Use language tags in documentation code blocks (js, ts, tsx, json, css) and order properties consistently as: language, then `expect_diagnostic`, then options modifiers, then `ignore`, then `file=path`
Applied to files:
crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEachValid.options.jsoncrates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEach.options.json
📚 Learning: 2025-08-05T14:43:29.581Z
Learnt from: dyc3
Repo: biomejs/biome PR: 7081
File: packages/@biomejs/biome/configuration_schema.json:7765-7781
Timestamp: 2025-08-05T14:43:29.581Z
Learning: The file `packages/biomejs/biome/configuration_schema.json` is auto-generated and should not be manually edited or reviewed for schema issues; any changes should be made at the code generation source.
Applied to files:
crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEachValid.options.jsoncrates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEach.options.json
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Biome linter is designed to work across languages, so rule naming should be generic if potentially implementable for multiple languages, or specific if meant for one language only
Applied to files:
crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEachValid.options.jsoncrates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEach.options.json
📚 Learning: 2025-11-21T01:10:53.059Z
Learnt from: dyc3
Repo: biomejs/biome PR: 8171
File: crates/biome_js_analyze/src/lint/nursery/no_leaked_render.rs:125-137
Timestamp: 2025-11-21T01:10:53.059Z
Learning: In the Biome codebase, each lint rule has its own options type declaration (e.g., `type Options = RuleNameOptions`) as part of the codegen process, even if the options struct is empty or unused. This is standard practice and should not be flagged as an issue.
Applied to files:
crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEachValid.options.jsoncrates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEach.options.json
📚 Learning: 2025-09-25T12:32:59.003Z
Learnt from: arendjr
Repo: biomejs/biome PR: 7593
File: crates/biome_service/src/workspace/server.rs:1306-1306
Timestamp: 2025-09-25T12:32:59.003Z
Learning: In the biomejs/biome project, do not flag compilation errors during code review as they are handled by the existing test infrastructure and CI. Focus on other code quality aspects instead.
Applied to files:
crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEachValid.options.jsoncrates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEach.options.json
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Framework-specific rules should be named using the `use` or `no` prefix followed by the framework name (e.g., `noVueReservedProps`)
Applied to files:
crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEachValid.options.jsoncrates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEach.options.json
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : For rules ported from other ecosystems like ESLint or Clippy, add a `sources` field with `RuleSource` metadata using `.same()` for identical behavior or `.inspired()` for different behavior
Applied to files:
crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEach.options.json
📚 Learning: 2025-11-27T23:04:02.022Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_analyze/CONTRIBUTING.md:0-0
Timestamp: 2025-11-27T23:04:02.022Z
Learning: Applies to crates/biome_analyze/**/*analyze/src/lint/**/*.rs : Set rule severity to `error` for correctness/security/a11y rules, `warn` for suspicious/performance rules, `info` for style/complexity rules, and `info` for actions
Applied to files:
crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEach.options.json
🪛 LanguageTool
.changeset/add-checkforeach-option.md
[uncategorized] ~5-~5: Possible missing comma found.
Context: ...ports forEach callbacks that return a value since forEach ignores return values. ...
(AI_HYDRA_LEO_MISSING_COMMA)
⏰ 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). (12)
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Check Dependencies
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: End-to-end tests
- GitHub Check: Documentation
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Test Node.js API
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Check JS Files
🔇 Additional comments (3)
crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEachValid.options.json (1)
1-15: LGTM — clear, minimal fixture enablingcheckForEach.crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEach.options.json (1)
1-15: LGTM — does exactly what it says on the tin.crates/biome_js_analyze/tests/specs/suspicious/useIterableCallbackReturn/checkForEach.js (1)
1-31: Solid set of “should be flagged”forEachreturn patterns.
Comment on lines
+6
to
+15
| pub struct UseIterableCallbackReturnOptions { | ||
| /// Whether to check `forEach` callbacks for unexpected return values. | ||
| /// | ||
| /// When `true`, the rule will report `forEach` callbacks that return a value, | ||
| /// since `forEach` ignores return values. | ||
| /// | ||
| /// Default: `false` | ||
| #[serde(default)] | ||
| pub check_for_each: bool, | ||
| } |
Contributor
There was a problem hiding this comment.
Config-merge bug risk: bool + serde(default) can’t represent “unset”.
If a parent/extended config sets checkForEach: true, a child config that omits the field will deserialize to false and can accidentally override the inherited true during merge. Per repo learnings, this should be an Option<bool> to preserve “unset vs explicitly false”.
pub struct UseIterableCallbackReturnOptions {
@@
- #[serde(default)]
- pub check_for_each: bool,
+ #[serde(default)]
+ pub check_for_each: Option<bool>,
}And in the rule logic, treat it as options.check_for_each.unwrap_or(false). (Based on learnings, optional rule option fields should be wrapped in Option<_> for config merging.)
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pub struct UseIterableCallbackReturnOptions { | |
| /// Whether to check `forEach` callbacks for unexpected return values. | |
| /// | |
| /// When `true`, the rule will report `forEach` callbacks that return a value, | |
| /// since `forEach` ignores return values. | |
| /// | |
| /// Default: `false` | |
| #[serde(default)] | |
| pub check_for_each: bool, | |
| } | |
| pub struct UseIterableCallbackReturnOptions { | |
| /// Whether to check `forEach` callbacks for unexpected return values. | |
| /// | |
| /// When `true`, the rule will report `forEach` callbacks that return a value, | |
| /// since `forEach` ignores return values. | |
| /// | |
| /// Default: `false` | |
| #[serde(default)] | |
| pub check_for_each: Option<bool>, | |
| } |
🤖 Prompt for AI Agents
In crates/biome_rule_options/src/use_iterable_callback_return.rs around lines
6–15, change the field type from bool to Option<bool> (e.g. pub check_for_each:
Option<bool>) so omitted configs deserialize as None instead of false, keep or
add #[serde(default)] so missing values become None, and then update the rule
logic to use options.check_for_each.unwrap_or(false) when evaluating the
behavior.
Member
|
Fyi: duplicate #8289 |
Contributor
Author
|
Oh thanks for pointing that out! I didn't notice the existing PR. I'll close this one in favor of #8289. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add
checkForEachoption touseIterableCallbackReturnrule to match ESLint'sarray-callback-returnrule behavior.checkForEach: true, reportsforEachcallbacks that return values (sinceforEachignores return values)false(forEach not checked)This aligns with ESLint's default behavior where
forEachis only checked when explicitly enabled.Test plan
checkForEach.jswithcheckForEach: trueoption to verify forEach checking when enabledinvalid.jstovalid.jssince forEach is not checked by defaultCloses #8024