fix(eslint-plugin): [no-for-in-array] report on any type which may be an array or array-like #10535

ronami · 2024-12-21T16:00:53Z

PR Checklist

Addresses an existing open issue: fixes Bug: [no-for-in-array] fails to report on Array<T> | undefined #10534
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

See #10535 (comment) for the updated contents of this PR.

~~This PR tackles #10534 and reports on cases such as:~~

declare const maybeArr: string[] | undefined;
declare const arr: string[];

// fails correctly
for (const item in arr) {}

// should fail, same with `T[] | null`
for (const item in maybeArr) {}

typescript-eslint · 2024-12-21T16:00:57Z

Thanks for the PR, @ronami!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

nx-cloud · 2024-12-21T16:01:46Z

View your CI Pipeline Execution ↗ for commit 599ac75.

Command	Status	Duration	Result
`nx test eslint-plugin --coverage=false`	✅ Succeeded	7m 27s	View ↗
`nx test eslint-plugin`	✅ Succeeded	5m 46s	View ↗
`nx test typescript-eslint --coverage=false`	✅ Succeeded	22s	View ↗
`nx test visitor-keys --coverage=false`	✅ Succeeded	<1s	View ↗
`nx run types:build`	✅ Succeeded	<1s	View ↗
`nx run-many --target=build --exclude website --...`	✅ Succeeded	29s	View ↗
`nx test rule-schema-to-typescript-types --cover...`	✅ Succeeded	<1s	View ↗
`nx test typescript-estree --coverage=false`	✅ Succeeded	<1s	View ↗
`Additional runs (24)`	✅ Succeeded	...	View ↗

☁️ Nx Cloud last updated this comment at 2025-01-11 14:26:31 UTC

netlify · 2024-12-21T16:01:56Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`599ac75`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/67827b1af77a0900086da4c5
😎 Deploy Preview	https://deploy-preview-10535--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 94 (🟢 up 1 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-12-21T16:17:29Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.95%. Comparing base (3ae4148) to head (599ac75).
Report is 52 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10535      +/-   ##
==========================================
+ Coverage   86.86%   86.95%   +0.09%     
==========================================
  Files         445      446       +1     
  Lines       15455    15517      +62     
  Branches     4507     4519      +12     
==========================================
+ Hits        13425    13493      +68     
+ Misses       1675     1669       -6     
  Partials      355      355

Flag	Coverage Δ
unittest	`86.95% <100.00%> (+0.09%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...ackages/eslint-plugin/src/rules/no-for-in-array.ts	`100.00% <100.00%> (ø)`

... and 23 files with indirect coverage changes

kirkwaiblinger · 2024-12-26T08:37:18Z

packages/eslint-plugin/src/rules/no-for-in-array.ts

@@ -45,3 +45,20 @@ export default createRule({
    };
  },
 });
+
+function isTypeArrayTypeOrUnionOfArrayTypes(


(style) avoid having the identical name function but with different behavior compared to the isTypeArrayTypeOrUnionOfArrayTypes in utils?

kirkwaiblinger · 2024-12-26T08:53:35Z

packages/eslint-plugin/src/rules/no-for-in-array.ts

+  for (const t of tsutils.unionTypeParts(type)) {
+    if (tsutils.isTypeFlagSet(t, ts.TypeFlags.Undefined | ts.TypeFlags.Null)) {
+      continue;
+    }
+
+    if (!checker.isArrayType(t)) {
+      return false;
+    }
+  }
+
+  return true;


(optional) This fixes a sort-of bug where a purely nullish type is considered an array type... "sort-of", since TS reports an error for the cases where the rule would reach this bug.

Suggested change

for (const t of tsutils.unionTypeParts(type)) {

if (tsutils.isTypeFlagSet(t, ts.TypeFlags.Undefined | ts.TypeFlags.Null)) {

continue;

}

if (!checker.isArrayType(t)) {

return false;

}

}

return true;

const nonNullishParts = tsutils

.unionTypeParts(type)

.filter(

t =>

!tsutils.isTypeFlagSet(t, ts.TypeFlags.Undefined | ts.TypeFlags.Null),

);

return (

nonNullishParts.length > 0 &&

nonNullishParts.every(t => checker.isArrayType(t))

);

(test case)

Playground

declare const nullish: null | undefined; // @ts-expect-error for (const k in nullish) { }

kirkwaiblinger

Requesting changes here just to account for conversation on the backing issue #10534 (comment)

ronami · 2024-12-27T21:30:29Z

I've updated the rule to report on anything that may be an array (along with some array-likes). Some notes:

Array-likes currently include regex array's, as mentioned on Docs: Deprecate no-for-in-array #10558, along with the arguments object (also added some of the ones mentioned on mdn). There may be more that I've missed.
I've added tuples so the rule warns on these too, as mentioned on Docs: Deprecate no-for-in-array #10558 (comment).
I fixed the bug mentioned on fix(eslint-plugin): [no-for-in-array] report on any type which may be an array or array-like #10535 (comment), in which the rule reports in addition to TypeScript (along with a test case).
The rule currently reports on string-likes. Since no test covers this, and especially as TypeScript already reports this as an error, I removed this part (so no double reporting on a string-like).

Some of the changes above don't have a dedicated issue that's marked as open-PRs, but they made sense as the changes are minimal and somewhat relate to the original issue this PR addresses. Please let me know if I should revert anything to make the PR match the original issue.

Remaining things that I think may benefit the rule (I'll consider opening issues for these):

No suggestion fixer, even though most of the time (or so I think) it's a mistake and should instead be for..of.

Thanks @kirkwaiblinger 👍

ronami · 2024-12-27T22:22:08Z

packages/eslint-plugin/src/rules/no-for-in-array.ts

+function isArray(checker: ts.TypeChecker, type: ts.Type): boolean {
+  return isTypeRecurser(
+    type,
+    t => checker.isArrayType(t) || checker.isTupleType(t),
+  );
+}
+
+function isTypeRecurser(
+  type: ts.Type,
+  predicate: (t: ts.Type) => boolean,
+): boolean {
+  if (type.isUnionOrIntersection()) {
+    return type.types.some(t => isTypeRecurser(t, predicate));
+  }
+
+  return predicate(type);
+}


I've used this part from #10551.

packages/eslint-plugin/src/rules/no-for-in-array.ts

… object

ronami · 2024-12-28T12:38:47Z

packages/eslint-plugin/src/rules/no-for-in-array.ts

+      'IArguments',
+      'HTMLCollection',
+      'RegExpExecArray',
+      'NodeList',


I'm not an expert on array-likes, basing this on https://developer.mozilla.org/en-US/docs/Web/API/NodeList.

ronami · 2024-12-28T12:39:00Z

packages/eslint-plugin/src/rules/no-for-in-array.ts

+  return isTypeRecurser(type, t =>
+    isBuiltinSymbolLike(program, t, [
+      'IArguments',
+      'HTMLCollection',


I'm not an expert on array-likes, basing this on https://developer.mozilla.org/en-US/docs/Web/API/HTMLCollection.

ronami · 2024-12-28T12:45:38Z

packages/eslint-plugin/tests/fixtures/tsconfig.lib-dom.json

+{
+  "extends": "./tsconfig.json",
+  "compilerOptions": {
+    "lib": ["es2015", "es2017", "esnext", "dom"]
+  }
+}


Is there a better way to do this? Specifically dom is necessary for the two dom-array-likes.

Yep, this is good!

kirkwaiblinger · 2024-12-30T01:29:22Z

Remaining things that I think may benefit the rule (I'll consider opening issues for these):

No suggestion fixer, even though most of the time (or so I think) it's a mistake and should instead be for..of.

For this, the complexity is that

for (const a in x) {
  console.log(a);
}

can't be directly fixed to a for-of loop equivalent since the for of would iterate array values and not string keys. So we'd have to be vigilant about keys only used for indexing or something. Note that prefer-for-of is in a similar area and may have design and logic for that that we could reuse. In any case punting to a separate issue is a good idea 👍

kirkwaiblinger · 2025-01-07T23:13:26Z

packages/eslint-plugin/src/rules/no-for-in-array.ts

@@ -45,3 +42,32 @@ export default createRule({
    };
  },
 });
+
+function isArrayLike(program: ts.Program, type: ts.Type): boolean {


I wonder how much hard-coding we can cut down on...
for RegExpExecArray (and Array subclasses in general I think) checker.isArrayLikeType(type) will take care of it.

The rest, though, require a bit more thinking. (With a bit of inspiration from some NVIDIA GPUs,) I was able to get all test cases passing with this approach:

import * as ts from 'typescript'; import { createRule, getConstrainedTypeAtLocation, getParserServices, } from '../util'; import { getForStatementHeadLoc } from '../util/getForStatementHeadLoc'; export default createRule({ name: 'no-for-in-array', meta: { type: 'problem', docs: { description: 'Disallow iterating over an array with a for-in loop', recommended: 'recommended', requiresTypeChecking: true, }, messages: { forInViolation: 'For-in loops over arrays skips holes, returns indices as strings, and may visit the prototype chain or other enumerable properties. Use a more robust iteration method such as for-of or array.forEach instead.', }, schema: [], }, defaultOptions: [], create(context) { return { ForInStatement(node): void { const services = getParserServices(context); const type = getConstrainedTypeAtLocation(services, node.right); if (isArrayLike(services.program, type)) { context.report({ loc: getForStatementHeadLoc(context.sourceCode, node), messageId: 'forInViolation', }); } }, }; }, }); function isArrayLike(program: ts.Program, type: ts.Type): boolean { return isTypeRecurser(type, t => { const checker = program.getTypeChecker(); const hasNumberyLength = (() => { const lengthProperty = t.getProperty('length'); if (lengthProperty != null) { const lengthType = checker.getTypeOfSymbol(lengthProperty); if (lengthType.flags & ts.TypeFlags.NumberLike) { return true; } } return false; })(); const hasNumberyIndex = (() => { const indexSignature = t.getNumberIndexType(); return indexSignature != null; })(); return hasNumberyIndex && hasNumberyLength; }); } function isTypeRecurser( type: ts.Type, predicate: (t: ts.Type) => boolean, ): boolean { if (type.isUnionOrIntersection()) { return type.types.some(t => isTypeRecurser(t, predicate)); } return predicate(type); }

But I haven't thought deeply whether this is a good idea. What do you think?

Interesting approach, and it seems TypeScript does some similar checks internally too. My intuition is to be careful with a check like that, in case it may flag false positives, but testing this on various cases, it seems to work nicely (though it is possible I've missed something). It does report on objects such as the following, which I think is intended (in addition to the arguments and several other array-likes):

declare const obj: { [key: number]: number; length: 1; }; for (const a in obj) { // }

I agree that hard-coding specific array-likes isn't very optimal. In my opinion, we should either use the plain and safer checker.isArrayLikeType() and avoid reporting array-likes that it doesn't detect or use this custom way to check array-likes. I think I'm +1 to using this over the plain checker.isArrayLikeType() but still a bit hesitant in case it over-reports.

I've updated the PR to match this, thanks 👍

…ture

kirkwaiblinger

I'm happy!

JoshuaKGoldberg

+1, this is sweet. 🍬

@yeonjuan

##### [v8.21.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8210-2025-01-20) ##### 🩹 Fixes - **eslint-plugin:** \[no-duplicate-enum-values] handle template literal ([#10675](typescript-eslint/typescript-eslint#10675)) - **eslint-plugin:** \[no-base-to-string] don't crash for recursive array or tuple types ([#10633](typescript-eslint/typescript-eslint#10633)) - **eslint-plugin:** \[no-for-in-array] report on any type which may be an array or array-like ([#10535](typescript-eslint/typescript-eslint#10535)) - **eslint-plugin:** check JSX spread elements for misused spread usage ([#10653](typescript-eslint/typescript-eslint#10653)) - **eslint-plugin:** \[no-unnecessary-type-arguments] handle type args on jsx ([#10630](typescript-eslint/typescript-eslint#10630)) ##### ❤️ Thank You - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

@yeonjuan

##### [v8.21.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8210-2025-01-20) ##### 🩹 Fixes - **eslint-plugin:** \[no-duplicate-enum-values] handle template literal ([#10675](typescript-eslint/typescript-eslint#10675)) - **eslint-plugin:** \[no-base-to-string] don't crash for recursive array or tuple types ([#10633](typescript-eslint/typescript-eslint#10633)) - **eslint-plugin:** \[no-for-in-array] report on any type which may be an array or array-like ([#10535](typescript-eslint/typescript-eslint#10535)) - **eslint-plugin:** check JSX spread elements for misused spread usage ([#10653](typescript-eslint/typescript-eslint#10653)) - **eslint-plugin:** \[no-unnecessary-type-arguments] handle type args on jsx ([#10630](typescript-eslint/typescript-eslint#10630)) ##### ❤️ Thank You - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

@yeonjuan

| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.20.0 | 8.21.0 | | npm | @typescript-eslint/parser | 8.20.0 | 8.21.0 | ## [v8.21.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8210-2025-01-20) ##### 🩹 Fixes - **eslint-plugin:** \[no-duplicate-enum-values] handle template literal ([#10675](typescript-eslint/typescript-eslint#10675)) - **eslint-plugin:** \[no-base-to-string] don't crash for recursive array or tuple types ([#10633](typescript-eslint/typescript-eslint#10633)) - **eslint-plugin:** \[no-for-in-array] report on any type which may be an array or array-like ([#10535](typescript-eslint/typescript-eslint#10535)) - **eslint-plugin:** check JSX spread elements for misused spread usage ([#10653](typescript-eslint/typescript-eslint#10653)) - **eslint-plugin:** \[no-unnecessary-type-arguments] handle type args on jsx ([#10630](typescript-eslint/typescript-eslint#10630)) ##### ❤️ Thank You - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

@yeonjuan

| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.20.0 | 8.21.0 | | npm | @typescript-eslint/parser | 8.20.0 | 8.21.0 | ## [v8.21.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8210-2025-01-20) ##### 🩹 Fixes - **eslint-plugin:** \[no-duplicate-enum-values] handle template literal ([#10675](typescript-eslint/typescript-eslint#10675)) - **eslint-plugin:** \[no-base-to-string] don't crash for recursive array or tuple types ([#10633](typescript-eslint/typescript-eslint#10633)) - **eslint-plugin:** \[no-for-in-array] report on any type which may be an array or array-like ([#10535](typescript-eslint/typescript-eslint#10535)) - **eslint-plugin:** check JSX spread elements for misused spread usage ([#10653](typescript-eslint/typescript-eslint#10653)) - **eslint-plugin:** \[no-unnecessary-type-arguments] handle type args on jsx ([#10630](typescript-eslint/typescript-eslint#10630)) ##### ❤️ Thank You - Ronen Amiel - YeonJuan [@yeonjuan](https://github.com/yeonjuan) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

report on for-in on 'Array<T> | undefined'

75d1ecb

ronami marked this pull request as ready for review December 21, 2024 16:17

kirkwaiblinger reviewed Dec 26, 2024

View reviewed changes

kirkwaiblinger requested changes Dec 27, 2024

View reviewed changes

update the rule to report on anything that may be an array or array-like

4ded1c5

nested test cases

7efca69

ronami changed the title ~~fix(eslint-plugin): [no-for-in-array] report on for-in usage with 'Array<T> | undefined'~~ fix(eslint-plugin): [no-for-in-array] report on any type which may be an array or array-like Dec 27, 2024

ronami commented Dec 27, 2024

View reviewed changes

packages/eslint-plugin/src/rules/no-for-in-array.ts Show resolved Hide resolved

ronami requested a review from kirkwaiblinger December 27, 2024 22:31

ronami added 2 commits December 28, 2024 14:14

refactor code to use 'isBuiltinSymbolLike' for checking the arguments…

e126dab

… object

refactor, add some more array-likes

b612eef

ronami commented Dec 28, 2024

View reviewed changes

ronami added 2 commits December 28, 2024 14:39

undo unrelated change

6466fb5

use lib-dom tsconfig only for tests that need it

2ce3cea

ronami commented Dec 28, 2024

View reviewed changes

ronami added 2 commits December 28, 2024 15:31

Merge branch 'main' into no-for-in-array-nullish

006eb4c

fix failing tests

762934d

kirkwaiblinger reviewed Jan 7, 2025

View reviewed changes

kirkwaiblinger added the awaiting response Issues waiting for a reply from the OP or another party label Jan 7, 2025

check array-like based on it having a length and a number-index-signa…

599ac75

…ture

ronami requested a review from kirkwaiblinger January 11, 2025 14:27

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Jan 11, 2025

kirkwaiblinger approved these changes Jan 19, 2025

View reviewed changes

kirkwaiblinger added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Jan 19, 2025

JoshuaKGoldberg approved these changes Jan 19, 2025

View reviewed changes

JoshuaKGoldberg merged commit 74f1c5a into typescript-eslint:main Jan 19, 2025
64 of 65 checks passed

ronami deleted the no-for-in-array-nullish branch January 19, 2025 21:03

github-actions bot locked as resolved and limited conversation to collaborators Jan 27, 2025

Uh oh!

fix(eslint-plugin): [no-for-in-array] report on any type which may be an array or array-like #10535

fix(eslint-plugin): [no-for-in-array] report on any type which may be an array or array-like #10535

Uh oh!

Conversation

ronami commented Dec 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Overview

Uh oh!

typescript-eslint bot commented Dec 21, 2024

Uh oh!

nx-cloud bot commented Dec 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Dec 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for typescript-eslint ready!

Uh oh!

codecov bot commented Dec 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kirkwaiblinger Dec 26, 2024

Choose a reason for hiding this comment

Uh oh!

kirkwaiblinger Dec 26, 2024

Choose a reason for hiding this comment

Uh oh!

kirkwaiblinger Dec 26, 2024

Choose a reason for hiding this comment

Uh oh!

kirkwaiblinger left a comment

Choose a reason for hiding this comment

Uh oh!

ronami commented Dec 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ronami Dec 27, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ronami Dec 28, 2024 • edited by kirkwaiblinger Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronami Dec 28, 2024 • edited by kirkwaiblinger Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronami Dec 28, 2024

Choose a reason for hiding this comment

Uh oh!

kirkwaiblinger Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

kirkwaiblinger commented Dec 30, 2024

Uh oh!

kirkwaiblinger Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronami Jan 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kirkwaiblinger left a comment

Choose a reason for hiding this comment

Uh oh!

JoshuaKGoldberg left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ronami commented Dec 21, 2024 •

edited

Loading

nx-cloud bot commented Dec 21, 2024 •

edited

Loading

netlify bot commented Dec 21, 2024 •

edited

Loading

codecov bot commented Dec 21, 2024 •

edited

Loading

ronami commented Dec 27, 2024 •

edited

Loading

ronami Dec 28, 2024 •

edited by kirkwaiblinger

Loading

ronami Dec 28, 2024 •

edited by kirkwaiblinger

Loading

kirkwaiblinger Jan 7, 2025 •

edited

Loading

ronami Jan 11, 2025 •

edited

Loading

JoshuaKGoldberg left a comment •

edited

Loading