fix(no-for-loop): handle cached-length pattern in for loop init

[tranhoangtu-it]

Problem

The no-for-loop rule does not flag the cached-length pattern, which is commonly used as a micro-optimization:

for (let i = 0, j = arr.length; i < j; i += 1) {
  const element = arr[i];
  console.log(element);
}

This pattern exists widely in performance-sensitive codebases (notably eslint-plugin-react) and should be converted to for-of like the standard pattern.

Reported in #295.

Root Cause

getIndexIdentifierName returned early when the init VariableDeclaration had more than one declarator:

if (variableDeclaration.declarations.length !== 1) {
  return;
}

And getArrayIdentifierFromBinaryExpression only handled i < arr.length (a MemberExpression), not i < j where j was previously assigned arr.length.

Solution

1. Relax the single-declarator check to allow exactly 2 declarators
2. Add getCachedLengthIdentifier — detects when the second declarator is j = arr.length and returns both the length variable name and the array identifier
3. Update getArrayIdentifierFromBinaryExpression — when a cachedLength is provided, also matches i < j by resolving j back to the original array

Test Cases Added

Three new invalid cases covering:

• for (let i = 0, j = arr.length; i < j; i++)
• for (let i = 0, j = arr.length; i < j; i += 1) (with named element variable)
• for (let i = 0, len = arr.length; i < len; i++) (different length variable name)

The existing valid case for (let i = 0, j = 0; ...) continues to pass because the second declarator's init is 0 (a literal), not a MemberExpression with .length.

Fixes #295

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: dea0deac0f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Reject non-cached secondary declarators in loop init

Allowing declarations.length === 2 unconditionally causes false positives and unsafe autofixes for loops that have any extra initializer, not just cached length (for example for (let i = 0, tmp = sideEffect(); i < arr.length; i++)). The fixer replaces the whole header and drops the second declarator, which can remove side effects or leave tmp/j references in the body unresolved. This should be gated to the exact cached-length form before the rule proceeds.

Useful? React with 👍 / 👎.

[Copilot]

Pull request overview

Updates the unicorn/no-for-loop ESLint rule to also detect the “cached-length” for loop pattern (for (let i = 0, len = arr.length; i < len; i++)) so it can be converted to a for-of loop, aligning behavior with common performance-oriented codebases.

Changes:

• Extend getIndexIdentifierName to allow 2 declarators in for loop init.
• Add getCachedLengthIdentifier and update array detection logic to resolve i < len back to the original array.
• Add new invalid test cases covering cached-length variants.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
test/no-for-loop.js	Adds invalid fixtures for cached-length for loops and their expected for-of output.
rules/no-for-loop.js	Adds cached-length detection and extends array-resolution logic to support i < len comparisons.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

getCachedLengthIdentifier enables autofixing loops like for (let i = 0, len = arr.length; i < len; i++), but the fixer will remove the len = arr.length declarator entirely. If len is referenced anywhere in the loop body (for example to detect the last iteration), the autofix will break semantics. Add a guard before offering a fix (or before reporting) to ensure the cached-length variable has no reads/writes inside node.body (or preserve it in the fix).

[Copilot]

Allowing two declarators in the for init without further validation will make the rule start reporting loops that previously were intentionally ignored (e.g. for (let i = 0, j = 0; i < arr.length; i++) { ... }) and can also drop side effects/unrelated variables from the initializer when autofixing. Consider only accepting 2 declarators when the second declarator matches the cached-length pattern (<id> = <array>.length) and the loop test compares the index against that cached-length identifier; otherwise keep bailing out as before.

[sindresorhus]

I double-checked this manually and I think there are still two fixer-safety issues here.

1. The rule now accepts a seocnd declarator whenever its init looks like something.length, even if the loop condition is still using arr.length directly. In that case the autofix drops unrelated initializer work. For example:

for (let i = 0, len = sideEffect().length; i < arr.length; i++) {
	console.log(arr[i]);
}

gets fixed to:

for (const element of arr) {
	console.log(element);
}

which removes the sideEffect() call completely.

2. The new cached-length variable is not tracked when deciding whether autofix is safe. If that variable is used later in the loop body or leaks out via var, the fix removes its declaration and can break the code. For example:

for (var i = 0, len = arr.length; i < len; i++) {
	console.log(arr[i]);
}
console.log(len);

gets fixed to:

for (const element of arr) {
	console.log(element);
}
console.log(len);

so len is left undefined afterwards.

[tranhoangtu-it]

Great catches! Fixed both issues:

1. Unrelated second declarator: Added a check that the loop test actually references the cached-length variable before allowing autofix. When the test uses arr.length directly and there's an unrelated second declarator, the fix is now suppressed (the rule still reports, but won't autofix).

2. Leaked cached-length variable: Added the cached-length variable to the someVariablesLeakOutOfTheLoop check, so if it's declared with var and used after the loop, autofix is suppressed.

Added test cases for both scenarios (invalid(7) and invalid(8) in the snapshot tests). All 120 tests pass.

[sindresorhus]

Still issues. Please use a smarter AI to do the work.

---

1. The rule still autofixes when there is a seocnd declarator like len = sideEffect().length but the loop condition uses arr.length directly. I verified that this still fixes to for (const element of arr) { ... }, which drops the sideEffect() call entirely.

for (let i = 0, len = sideEffect().length; i < arr.length; i++) {
	console.log(arr[i]);
}

2. The cached-length variable is only checked for leaking outside the loop, not for being used inside the loop body. I verified that this still autofixes and leaves len behind in the body:

for (let i = 0, len = arr.length; i < len; i++) {
	console.log(len, arr[i]);
}

becomes:

for (const element of arr) {
	console.log(len, element);
}

So the remaining problem is broader than the var leak case. The fix should also bail out when the cached-length variable is referenced anywhere in the loop body.