feat(js_analyze): implement noForIn#8085
Conversation
🦋 Changeset detectedLatest commit: c1f03e3 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 |
WalkthroughAdds a new nursery lint rule Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
crates/biome_cli/src/execute/migrate/eslint_any_rule_to_biome.rsis excluded by!**/migrate/eslint_any_rule_to_biome.rsand included by**crates/biome_configuration/src/analyzer/linter/rules.rsis excluded by!**/rules.rsand included by**crates/biome_diagnostics_categories/src/categories.rsis excluded by!**/categories.rsand included by**crates/biome_js_analyze/src/lint/nursery.rsis excluded by!**/nursery.rsand included by**crates/biome_js_analyze/tests/specs/nursery/noForIn/invalid.js.snapis excluded by!**/*.snapand included by**crates/biome_js_analyze/tests/specs/nursery/noForIn/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 (5)
crates/biome_js_analyze/src/lint/nursery/no_for_in.rs(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noForIn/invalid.js(1 hunks)crates/biome_js_analyze/tests/specs/nursery/noForIn/valid.js(1 hunks)crates/biome_rule_options/src/lib.rs(1 hunks)crates/biome_rule_options/src/no_for_in.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-10-15T09:23:33.055Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:23:33.055Z
Learning: Applies to crates/biome_js_type_info/biome_module_graph/src/**/*.rs : Do not copy or clone data between module graph entries (including behind Arc). Each module must avoid holding duplicated data from another module to enable simple invalidation.
Applied to files:
crates/biome_rule_options/src/lib.rs
📚 Learning: 2025-10-15T09:22:15.851Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:15.851Z
Learning: Applies to crates/biome_formatter/src/lib.rs : Add the provided AsFormat, IntoFormat, and iterator plumbing code to lib.rs
Applied to files:
crates/biome_rule_options/src/lib.rs
📚 Learning: 2025-10-15T09:22:15.851Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:15.851Z
Learning: Applies to crates/biome_formatter/src/**/*.rs : After generation, remove usages of `format_verbatim_node` and implement real formatting with biome_formatter utilities
Applied to files:
crates/biome_rule_options/src/lib.rs
📚 Learning: 2025-10-15T09:23:33.055Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:23:33.055Z
Learning: Applies to crates/biome_js_type_info/src/{type_info,local_inference,resolver,flattening}.rs : Avoid recursive type structures and cross-module Arcs; represent links between types using TypeReference and TypeData::Reference.
Applied to files:
crates/biome_rule_options/src/lib.rscrates/biome_rule_options/src/no_for_in.rs
📚 Learning: 2025-10-15T09:24:31.042Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_parser/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:24:31.042Z
Learning: Applies to crates/biome_parser/crates/biome_*_{syntax,factory}/** : Create per-language crates biome_<lang>_syntax and biome_<lang>_factory under crates/
Applied to files:
crates/biome_rule_options/src/lib.rs
📚 Learning: 2025-10-15T09:22:46.002Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_formatter/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:22:46.002Z
Learning: Applies to crates/biome_js_formatter/**/*.rs : Import and use the `FormatNode` trait for AST nodes
Applied to files:
crates/biome_js_analyze/src/lint/nursery/no_for_in.rs
📚 Learning: 2025-10-15T09:23:33.055Z
Learnt from: CR
Repo: biomejs/biome PR: 0
File: crates/biome_js_type_info/CONTRIBUTING.md:0-0
Timestamp: 2025-10-15T09:23:33.055Z
Learning: Applies to crates/biome_js_type_info/src/type_info.rs : Add new TypeScript type support by extending the TypeData enum rather than introducing parallel structures.
Applied to files:
crates/biome_rule_options/src/no_for_in.rs
🧬 Code graph analysis (2)
crates/biome_js_analyze/tests/specs/nursery/noForIn/invalid.js (1)
crates/biome_js_analyze/tests/specs/nursery/noForIn/valid.js (2)
array(2-2)i(8-8)
crates/biome_js_analyze/src/lint/nursery/no_for_in.rs (1)
crates/biome_analyze/src/rule.rs (2)
sources(617-620)inspired(254-259)
⏰ 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). (13)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Check JS Files
- GitHub Check: Test Node.js API
- GitHub Check: Documentation
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: Bench (biome_configuration)
- GitHub Check: autofix
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Check Dependencies
🔇 Additional comments (4)
crates/biome_rule_options/src/lib.rs (1)
90-90: Module wiring looks good.Thanks for exposing the new options module here. It keeps the generated index tidy.
crates/biome_rule_options/src/no_for_in.rs (1)
1-6: Options stub is spot on.An empty options struct with the usual derives is exactly what we need whilst the rule has no configuration knobs.
crates/biome_js_analyze/tests/specs/nursery/noForIn/valid.js (1)
1-18: Nice coverage of the allowed patterns.These examples mirror the docs nicely and will protect the rule from accidental regressions.
crates/biome_js_analyze/tests/specs/nursery/noForIn/invalid.js (1)
1-9: Good to see both diagnostic cases covered.Both flavours of array iteration are here, so once the rule logic is fixed we’ll keep catching the intended misuse.
| fn run(_ctx: &RuleContext<Self>) -> Self::Signals { | ||
| Some(()) | ||
| } | ||
|
|
||
| fn diagnostic(ctx: &RuleContext<Self>, _state: &Self::State) -> Option<RuleDiagnostic> { | ||
| let node = ctx.query(); | ||
| Some( | ||
| RuleDiagnostic::new( | ||
| rule_category!(), | ||
| node.range(), | ||
| markup! { | ||
| "Unexpected for-in loop." | ||
| }, | ||
| ) | ||
| .note(markup! { | ||
| "To prevent unexpected behavior if used incorrectly, use a regular for loop or forEach instead." | ||
| }), | ||
| ) |
There was a problem hiding this comment.
Only report when the RHS is an array.
Line 74 always returns Some(()), so we flag every for…in—including perfectly valid object iteration like for (const key in obj). The TypeScript ESLint rule we’re mirroring (no-for-in-array) only trips when the RHS is statically an array (or any). At the moment we’ve created a blanket ban, which will be a noisy false positive factory and also clashes with our existing use_guard_for_in rule.
Could you gate the diagnostic on confirming the iterable is an array (e.g. via the type model) and bail out when we can’t prove that? Once that’s in place, please add a regression test that shows for (const key in object) stays quiet. Otherwise we’ll have unhappy users—and trust me, they shout in British.
CodSpeed Performance ReportMerging #8085 will not alter performanceComparing Summary
Footnotes
|
| } | ||
| ``` | ||
|
|
||
| **Valid:** |
There was a problem hiding this comment.
nit: IMO we should try to not be overly verbose in changesets. A really big changeset can indicates something really important that users need to pay attention to, when in reality it's not that big. I think it would be good to remove the Valid section from this.
| /// ```js,expect_diagnostic | ||
| /// for (const i in array) { | ||
| /// console.log(array[i]); | ||
| /// } | ||
| /// ``` | ||
| /// ```js,expect_diagnostic | ||
| /// for (const i in array) { | ||
| /// console.log(i, array[i]); | ||
| /// } | ||
| /// ``` |
There was a problem hiding this comment.
nit: these examples look a little too similar
| }, | ||
| ) | ||
| .note(markup! { | ||
| "To prevent unexpected behavior if used incorrectly, use a regular for loop or forEach instead." |
There was a problem hiding this comment.
nit: maybe something along the lines of "for-in loops are confusing and easy to misuse. you likely want to use a for-of loop instead."
no strong feelings though
There was a problem hiding this comment.
Changed to For-in loops are confusing and easy to misuse. You likely want to use a regular loop, for-of loop or forEach instead.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
crates/biome_js_analyze/src/lint/nursery/no_for_in.rs (1)
68-70: Only report when the RHS is an array.This already-flagged critical issue bears repeating:
rununconditionally returnsSome(()), so you'll flag every for-in loop—including perfectly valid object iteration likefor (const key in obj). The TypeScript ESLint rule you're aligning with only trips when the RHS is statically an array. You'll need to check the type of the iterable expression before reporting.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
crates/biome_configuration/src/analyzer/linter/rules.rsis excluded by!**/rules.rsand included by**crates/biome_js_analyze/tests/specs/nursery/noForIn/invalid.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 (2)
.changeset/five-rats-train.md(1 hunks)crates/biome_js_analyze/src/lint/nursery/no_for_in.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 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/src/lint/nursery/no_for_in.rs
🧬 Code graph analysis (1)
crates/biome_js_analyze/src/lint/nursery/no_for_in.rs (1)
crates/biome_analyze/src/rule.rs (3)
recommended(602-605)sources(617-620)inspired(254-259)
⏰ 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). (18)
- GitHub Check: Bench (biome_js_formatter)
- GitHub Check: Bench (biome_js_analyze)
- GitHub Check: Bench (biome_js_parser)
- GitHub Check: Validate rules documentation
- GitHub Check: Test Node.js API
- GitHub Check: Lint project (depot-windows-2022)
- GitHub Check: Documentation
- GitHub Check: Test (depot-ubuntu-24.04-arm-16)
- GitHub Check: Test (depot-windows-2022-16)
- GitHub Check: Lint project (depot-ubuntu-24.04-arm-16)
- GitHub Check: End-to-end tests
- GitHub Check: Check Dependencies
- GitHub Check: Bench (biome_configuration)
- GitHub Check: lint
- GitHub Check: Check JS Files
- GitHub Check: autofix
- GitHub Check: triage
- GitHub Check: Validate PR title
| "@biomejs/biome": patch | ||
| --- | ||
|
|
||
| Added the nursery rule [`noForIn`](https://biomejs.dev/linter/rules/no-for-in/). Disallow iterating using a for-in loop. |
There was a problem hiding this comment.
Clarify this is about arrays.
The description "Disallow iterating using a for-in loop" sounds like a blanket ban, but you're aligning with no-for-in-array. Update to something like "Disallow for-in loops over arrays" to set accurate expectations.
🤖 Prompt for AI Agents
In .changeset/five-rats-train.md around line 5, the description "Disallow
iterating using a for-in loop" is too broad; change it to explicitly say this
targets arrays (e.g., "Disallow for-in loops over arrays" or "Disallow iterating
over arrays with for-in loops") so it aligns with the no-for-in-array behavior
and sets correct expectations.
| use biome_rule_options::no_for_in::NoForInOptions; | ||
|
|
||
| declare_lint_rule! { | ||
| /// Disallow iterating using a for-in loop. |
There was a problem hiding this comment.
Clarify this is about arrays, not all for-in loops.
The title "Disallow iterating using a for-in loop" suggests a blanket ban, but your detailed explanation (lines 12-17) correctly focuses on array-specific problems. Update the title to something like "Disallow for-in loops over arrays" to match your implementation intent and the ESLint rule you're mirroring.
🤖 Prompt for AI Agents
In crates/biome_js_analyze/src/lint/nursery/no_for_in.rs around line 10, the doc
comment title "Disallow iterating using a for-in loop." is too broad; update it
to specify arrays (e.g., "Disallow for-in loops over arrays") so it matches the
implementation and ESLint mapping described in the following lines. Change the
comment line to the more specific phrasing and ensure any nearby comments/docs
are consistent with array-only behavior.
| pub NoForIn { | ||
| version: "next", | ||
| name: "noForIn", | ||
| language: "js", | ||
| recommended: false, | ||
| sources: &[RuleSource::EslintTypeScript("no-for-in-array").inspired()], | ||
| } |
There was a problem hiding this comment.
Rule name doesn't match its intended scope.
You've named the rule noForIn, but the sources reference no-for-in-array and your documentation explains array-specific problems. This creates confusion—users will think it bans all for-in loops (including valid object iteration). Consider renaming to noForInArray to match the ESLint rule you're aligning with.
🤖 Prompt for AI Agents
In crates/biome_js_analyze/src/lint/nursery/no_for_in.rs around lines 53–59, the
rule is declared as NoForIn with name "noForIn" but sources reference ESLint's
"no-for-in-array" and docs describe array-specific behavior; rename the rule to
NoForInArray and change the name string to "noForInArray" (and update any local
identifiers/variants, constructor usages, tests, and documentation references)
so the enum/struct variant, exported name, and references consistently reflect
the array-specific scope.
| fn diagnostic(ctx: &RuleContext<Self>, _state: &Self::State) -> Option<RuleDiagnostic> { | ||
| let node = ctx.query(); | ||
| Some( | ||
| RuleDiagnostic::new( | ||
| rule_category!(), | ||
| node.range(), | ||
| markup! { | ||
| "Unexpected for-in loop." | ||
| }, | ||
| ) | ||
| .note(markup! { | ||
| "For-in loops are confusing and easy to misuse. You likely want to use a regular loop, for-of loop or forEach instead." | ||
| }), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Diagnostic message should mention arrays.
Your message "Unexpected for-in loop" and note are generic, but this rule is specifically about arrays (per your sources and documentation). Once you fix the run logic to check for arrays, update the message to something like "Avoid for-in loops over arrays" so users immediately understand the problem.
🤖 Prompt for AI Agents
In crates/biome_js_analyze/src/lint/nursery/no_for_in.rs around lines 72 to 86,
the diagnostic text is generic but this rule targets for-in loops over arrays;
change the title and note to explicitly reference arrays (e.g. "Avoid for-in
loops over arrays") and update the note to recommend array-appropriate
alternatives such as for-of, Array.prototype.forEach, or a classical for loop;
ensure the diagnostic remains concise and the message matches the rule's run
logic which should already be checking for array iteration.
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Summary
Implement https://typescript-eslint.io/rules/no-for-in-array/
Closes #8067
Test Plan
Docs