no-for-in-array rule

[danvk]

Fixes #1380

I would be interested in feedback on whether this is accurate:

const isArrayType = (type.flags & ts.TypeFlags.Reference) !== 0;

The typescript.d.ts file is quite terse about what a "Generic type reference" is, but in my testing it seems to correspond precisely to array types.

[jkillian]

Just a thought, what do you think about banning for-in all together? My thinking is that in the vast majority of cases, users only want the object's own properties and thus:

for (const key of Object.keys(someObj)) {
  // do stuff
}

is preferred. A for-in loop is really only beneficial if you want inherited properties, which is unlikely.

The other advantage is that this wouldn't need the type-checker, so it could be used in more situations and would be faster.

[jkillian]

On the other hand, a general ban of for-in wouldn't address the issue you described, as Object.keys(someArray) will have the same problem. That would lead me to think that a no-array-key-iteration (naming?) rule might be a better solution than just a no-for-in-array rule.

[jkillian]

I would be interested in feedback on whether this is accurate:

const isArrayType = (type.flags & ts.TypeFlags.Reference) !== 0;

Unfortunately I don't think so, check out the description of TypeFlags.Reference here. I think what's happening is that the Array type is inherently generic and thus always has that flag set? I think that potentially an instance of any generic class would have this flag set, but I haven't tested that out.

Maybe @weswigham could shed some insight on the best way to check if something is an Array type?

[danvk]

So is that saying that an object of a generic type (say Promise<T>) would also have the TypeFlags.Reference bit set? I tried adding a test:

function foo<T>(promise: Promise<T>) {
  for (var k in promise);
}

But promise in that for-in loop does not have the TypeFlags.reference flag set. An example of something which has this flag set but is not an array would be very helpful!

I typically use for-in for iterating over object literals that I've created myself, so inherited properties aren't a big concern. for-in is compact and seems to be faster than for-of-Object.keys in modern browsers and node.

I can see the argument for disallowing it entirely, though. Maybe there should be two options for a "for-in" rule: "for-in": [true, "never"] and "for-in": [true, "for-objects"]?

[jkillian]

When testing the type of b in this code:

class A<T> { }
let b: A<number>;

I get a result that shows that b does have the TypeFlags.Reference flag set (also included are the other variable declarations from your test case):

Reference: b:A<number>
Reference: array = [1, 2, 3, 4]
Reference: strArray = ['a', 'b', 'c']
Reference: objArray = [{a: 1}, {a: 2, b: 10}, {a: 3}]
Not Reference: o = {}
Not Reference: numArray = {
    1: "a",
    2: "b"
}
Not Reference: d = new Date()
Not Reference: str = "abc"
Not Reference: objectMap = {} as {[key: string]: number}
Not Reference: map = new Map([['a', 1], ['b', 2]])
Not Reference: set = new Set([1, 2, 3])

In case it helps, here's the method I added to your rule to get this output:

    public visitVariableDeclaration(node: ts.VariableDeclaration) {
        const tc = this.getTypeChecker();
        const type = tc.getTypeAtLocation(node);
        // tslint:disable
        const isReference = (type.flags & ts.TypeFlags.Reference) > 0;
        console.log(`${isReference ? "Reference" : "Not Reference"}: ${node.getText()}`);
        // tslint:enable
    }

[jkillian]

If we go your route for this rule, we could actually just enhance the current forin rule. (It really should be hyphenated, but it's not.) It could then have three options: "never", "guarded", "only-objects" where "guarded" and "only-objects" could be used together. I think this might be the best way to go. With no options provided, the rule would default to "guarded" since that is its current behavior.

I know I proposed it above in the thread, but after more thought, I don't think a rule that warns about for (const key of Object.keys(someArray)) is necessary. Although it's "wrong" in the same way that for (const key of someArray) is, it's not a mistake that would be accidentally made.

[danvk]

Sure, I could add an only-objects setting to the existing rule.

I pushed a new version of the array check. The logic is now:

const isArrayType = type.symbol && type.symbol.name === "Array";

This handles the A<number> case correctly.

[danvk]

(If this check seems correct then I'll add it to the existing rule.)

[jkillian]

I pushed a new version of the array check. The logic is now:

const isArrayType = type.symbol && type.symbol.name === "Array";

Your guess is as good as mine here, but this makes sense to me and can't think of any edge cases which would mess things up.

[weswigham]

@jkillian

function foo() {
  type Array = MyArray<T>;
  let x = new Array();
  // ...
}

It doesn't handle block scoped type declarations whose name is also Array.

[jkillian]

It doesn't handle block scoped type declarations whose name is also Array.

Ha, are there any better alternatives?

Let local type declarations named Array be anathema...

[danvk]

If you redefine Array locally and get a lint error, then I don't feel bad for you :)

[weswigham]

@jkillian You want to be able to lookup the global array type, then check if the type you find at that location is an instantiation of it. Inside the compiler, this wouldn't be too difficult, but we don't publicly expose the functions you need to do this check on the type checker instance right now. Heck, we don't even expose a way to lookup a global type!

I've known about this shortcoming in our public API since I started working on compiler extensions and have had it at the back of my mind. I've opened #9879 to track the issue.

[danvk]

@jkillian I pushed a new version which merges my no-for-in-array rule with the existing forin rule.

This introduces a new problem, though (and it's the reason that the tests will fail): the rule now requires type checking, whereas it didn't before. So existing configs will break.

What do you think? Is it still a good idea to merge the rules?

[jkillian]

@weswigham Thanks for the info, sounds like the current method we're using is best for now then. Excited for the Type Relationship API! (And the other ones, like the Type Builder API).

@danvk Shoot, wasn't thinking about that. We definitely don't want to break existing users. Let's just keep it as no-for-in-array as you suggested. (We can add an option to ban all for-in loops to the main non-typechecked forin rule down the road if we want to.) Apologies for the needless churn

[danvk]

@jkillian I've rolled this back to the version with a separate no-for-in-array rule. I believe this is ready to go.

[jkillian]

Thanks @danvk!