Movatterモバイル変換

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

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

		function isTypeArrayTypeOrUnionOfArrayTypes(

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

kirkwaiblinger reviewed

Dec 26, 2024

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

Comment on lines 53 to 63

		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;

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

(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(consttoftsutils.unionTypeParts(type)){
	if(tsutils.isTypeFlagSet(t,ts.TypeFlags.Undefined\|ts.TypeFlags.Null)){
	continue;
	}

	if(!checker.isArrayType(t)){
	returnfalse;
	}
	}

	returntrue;
	constnonNullishParts=tsutils
	.unionTypeParts(type)
	.filter(
	t=>
	!tsutils.isTypeFlagSet(t,ts.TypeFlags.Undefined\|ts.TypeFlags.Null),
	);
	return(
	nonNullishParts.length>0&&
	nonNullishParts.every(t=>checker.isArrayType(t))
	);

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

(test case)

Playground

declareconstnullish:null|undefined;//@ts-expect-errorfor(constkinnullish){}

kirkwaiblinger requested changes

Copy link

Member

kirkwaiblinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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

4ded1c5

Copy link

MemberAuthor

ronami commentedDec 27, 2024•
edited
Loading

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 onDocs: Deprecateno-for-in-array #10558, along with thearguments object (also added some of the onesmentioned on mdn). There may be more that I've missed.
I've added tuples so the rule warns on these too, as mentioned onDocs: Deprecateno-for-in-array #10558 (comment).
I fixed the bug mentioned onfix(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 asopen-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 befor..of.

Thanks@kirkwaiblinger 👍

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

ronami commented

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

Comment on lines 62 to 78

		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);
		}

Copy link

MemberAuthor

ronamiDec 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I've used this part from#10551.

ronami commented

packages/eslint-plugin/src/rules/no-for-in-array.tsShow resolvedHide resolved

ronami requested a review fromkirkwaiblinger

December 27, 2024 22:31

ronami added2 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

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

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

Copy link

MemberAuthor

ronamiDec 28, 2024•
edited by kirkwaiblinger
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

ronami commented

Dec 28, 2024

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

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

Copy link

MemberAuthor

ronamiDec 28, 2024•
edited by kirkwaiblinger
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

ronami added2 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

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

Comment on lines +1 to +6

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

Copy link

MemberAuthor

ronamiDec 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Member

kirkwaiblingerJan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Yep, this is good!

ronami added2 commits

December 28, 2024 15:31

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

006eb4c

fix failing tests

762934d

Copy link

Member

kirkwaiblinger commentedDec 30, 2024

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 befor..of.

For this, the complexity is that

for(constainx){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 thatprefer-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 reviewed

Jan 7, 2025

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

		@@ -45,3 +42,32 @@ export default createRule({
		};
		},
		});

		function isArrayLike(program: ts.Program, type: ts.Type): boolean {

Copy link

Member

kirkwaiblingerJan 7, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I wonder how much hard-coding we can cut down on...
forRegExpExecArray (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*astsfrom'typescript';import{createRule,getConstrainedTypeAtLocation,getParserServices,}from'../util';import{getForStatementHeadLoc}from'../util/getForStatementHeadLoc';exportdefaultcreateRule({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{constservices=getParserServices(context);consttype=getConstrainedTypeAtLocation(services,node.right);if(isArrayLike(services.program,type)){context.report({loc:getForStatementHeadLoc(context.sourceCode,node),messageId:'forInViolation',});}},};},});functionisArrayLike(program:ts.Program,type:ts.Type):boolean{returnisTypeRecurser(type,t=>{constchecker=program.getTypeChecker();consthasNumberyLength=(()=>{constlengthProperty=t.getProperty('length');if(lengthProperty!=null){constlengthType=checker.getTypeOfSymbol(lengthProperty);if(lengthType.flags&ts.TypeFlags.NumberLike){returntrue;}}returnfalse;})();consthasNumberyIndex=(()=>{constindexSignature=t.getNumberIndexType();returnindexSignature!=null;})();returnhasNumberyIndex&&hasNumberyLength;});}functionisTypeRecurser(type:ts.Type,predicate:(t:ts.Type)=>boolean,):boolean{if(type.isUnionOrIntersection()){returntype.types.some(t=>isTypeRecurser(t,predicate));}returnpredicate(type);}

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

Copy link

MemberAuthor

ronamiJan 11, 2025•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Interesting approach, and it seems TypeScript doessome 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 thearguments and several other array-likes):

declareconstobj:{[key:number]:number;length:1;};for(constainobj){//}

I agree that hard-coding specific array-likes isn't very optimal. In my opinion, we should either use the plain and saferchecker.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 plainchecker.isArrayLikeType() but still a bit hesitant in case it over-reports.

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

kirkwaiblinger added the awaiting responseIssues 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 fromkirkwaiblinger

January 11, 2025 14:27

github-actionsbot removed the awaiting responseIssues waiting for a reply from the OP or another party label

Jan 11, 2025

kirkwaiblinger approved these changes

Copy link

Member

kirkwaiblinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I'm happy!

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

JoshuaKGoldberg approved these changes

Copy link

Member

JoshuaKGoldberg left a comment•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

+1, this is sweet. 🍬

JoshuaKGoldberg merged commit74f1c5a intotypescript-eslint:main