Solve the for-in / indexer mess

[RyanCavanaugh]

/cc @vladima @ahejlsberg @billti

Motivating Examples

var arr = [1, 2, 3];

This should be an error (k is always a string):

for(var k in arr) {
  if(k === 0) { ... }
}

This should not be an error, even under no implicit any:

for(var k in arr) {
  if(arr[k] === 0) { ... }
}

This should somehow? not be an error, even though it involves a coercion we would normally disallow

for(var k in arr) {
  if (k > 0) { ... }
}

[DanielRosenwasser]

If there was some way of communicating that your string type was actually the domain of strings whose text was numeric (i.e. ToString(ToNumber(text) === text), that would possibly go a long way here.

[mhegazy]

I had an offline discussion with @ahejlsberg about this. I think we have a proposal. given:

for (var x in a) {...}

• if type of a has a numeric index signature, type of x is NumericString
• otherwise, typeof x is string

Where NumericString is an builtin type that can not be named, and is only given to for..in indexers. It is identical to string for all type system operations except that it is treated as number when indexing. e.g.:

var a = [1, 2, 3];
for (var x in a) {
    y = a[x];  // y is number
    z = x + 1 // z is string;
}

[ahejlsberg]

An alternative is to check whether the expression used to index an array is exactly an identifier declared as a for-in variable for an array that has a numeric indexer. This avoids having a new type and, importantly, avoids having to "erase" that type when it becomes in inferred type of a variable. I will try to work on it and put up a PR.

[DanielRosenwasser]

You mean if it only has a numeric index signature? Or even if it has a string index signature as well?

[mhegazy]

@DanielRosenwasser you are correct. it has to have only a numeric index signature.

[jeffreymorlan]

An object can satisfy a numeric index signature type and still have other non-numeric properties:

var obj: {[n: number]: string};
obj = { 0: 'a', 1: 'b', 2: 'c', length: 3 };
for (var k in obj) {
    console.log(k, obj[k].toUpperCase()); // This throws when k === 'length'
}

Even on a plain array, for/in may include properties that have been added to Array.prototype.

[ahejlsberg]

@jeffreymorlan Perhaps a better check is to see if the object being for-in'ed is array-like (specifically, if it is assignable to any[]). That is the same check we apply to a spread expression.

[jeffreymorlan]

One library I'm using returns array-like objects that inherit from Array.prototype but add additional (enumerable) methods that will show up in for/in, making it unsafe to use the for/in keys as indexes.

There are so many ways that for/in on array-likes may return properties other than array indexes...

• extra properties on the array object itself
• extra properties on prototype inheriting from Array
• some library "conveniently" adding extra properties onto Array.prototype itself (MooTools is an infamous example of this)
• old JS implementations where .length was enumerable

...not to mention the other surprising behaviors (indexes are strings instead of numbers, indexes not guaranteed to be in numeric order pre-ES6), that for/in on an array-like should just be considered a bug. At any rate it shouldn't be given special treatment from the type system to pretend that it's safe.

[ahejlsberg]

@jeffreymorlan Completely agree for-in is a messed up construct--but we don't get to change that. The objective is to provide the best possible type checking of for-in as it exists, and unquestionably it is a common pattern to use for-in with arrays and expect indexing with the for-in variable to produce the elements of the array. I agree that there are situations where non-numeric property names may appear because applications or libraries have added extra properties to arrays, but we have to balance that against existing (and more common) code that just uses plain JavaScript arrays. The operating principle here is to find the best possible compromise. I think the changes in #6379 bring us closer to that.

[jeffreymorlan]

it is a common pattern to use for-in with arrays and expect indexing with the for-in variable to produce the elements of the array

It's a common mistake; all the more reason not to allow it in TS, so people can find places where they have made that mistake and replace it with the correct for/of, .forEach, or good-old-fashioned for (var i = 0; i < arr.length; i++).

Monkey patching of Array.prototype isn't just something obscure that you'll never see in the real world (unfortunately). According to http://w3techs.com/technologies/overview/javascript_library/all MooTools is used by 3.8% of websites, and Prototype by 2.2%. It's a controversial practice; I don't like it myself, but many people take the moderate position that it's fine for polyfilling newer methods like .filter for older browsers - which still breaks for/in. On the other hand, for/in on arrays is nearly universally castigated; google for "for in array" and results # 1, 3, 4, and 7 are all about why you shouldn't do it.

Even TypeScript itself used to add properties to Array.prototype that would globally break for/in on arrays. From TypeScript 1.3 src/services/syntax/syntaxList.ts:

module TypeScript.Syntax {
    ...
    Array.prototype.kind = function () { ... }
    Array.prototype.separatorCount = function (): number { ... }
    Array.prototype.separatorAt = function (index: number): ISyntaxToken { ... }
    ...
}

[RyanCavanaugh]

There are two questions here:

• Should we disallow constructs that aren't part of the Good Parts of JS?
• Should we improve typechecking for people who use those not-great constructs anyway?

If people want to disallow a construct entirely, there are good tools for that -- TSLint, ESHint, etc.. I know there are tools that will disallow for / in without a hasOwnProperty check as well, which is a fine compromise in most cases. There's nothing stopping people from using a restricted subset of JavaScript's control flow keywords while using TypeScript. Certainly we run our own code through a stricter set of lint checks than many people might like.

If people think they're in an environment where for / in is safe... they may very well be right! Maybe they have unit tests to verify there are no non-own enumerable properties of arrays. Maybe they have strict build steps that verify no one is modifying Array.prototype. It's not unfathomable at all for this to happen.

Our goal has always been to not be more prescriptive than we have to be. We regularly get issues filed here asking us to be more permissive of things that are much more error-prone (e.g. overloading valueOf in shady ways) -- it's a balancing act. Completely disallowing for / in would be way over the line.

[jeffreymorlan]

@RyanCavanaugh I'm certainly not asking for for/in to be disallowed - I use it all the time myself - all I'm asking is that it shouldn't bend the indexing rules to make it look like it's safer for arrays than it actually is:

var map: {[key: string]: string};
for (var key in map) { // good
    // should be allowed, map has a string index signature
    console.log(map[key].toUpperCase());
}

var arr: string[];
for (var key in arr) { // probably a mistake; key is a string but you can't assume much else about it
    // this should be an error, because arr does not have a string index signature
    console.log(arr[key].toUpperCase()); 
}

[RyanCavanaugh]

We bent the rules pretty severely by making the for / in indexer type any, which was motivated by the huge amount of code we found that works like the second block in your example. Believe me that if we thought it were feasible to "correctly" type the iteration variable, we would have done so from the beginning.

A surprising thing we've discovered in practice is that people strongly disagree about what indexing behavior should be considered "safe" or not -- see the fact that the --suppressImplicitAnyIndexErrors switch exists for the strongest evidence of this. Many people did (or would) tell you that the second block in your example is totally fine code; certainly it works (for now), which is hard to argue with.

Of course, the real lie here is the existence of numeric index signatures at all, but that cat is out of the bag (and never really fit in the bag in the first place).

This change straightens the rules a bit so that the really egregious stuff (key << 1, etc) gets caught. It is a compromise.

[jeffreymorlan]

It's an easy fix to change for (var key in arr) to one of the numerous correct ways to iterate over an array. (And, if even that's too much, you can always keep the incorrect for/in loop and do arr[<any>k].)

Converting any nontrivial JS program to error-free TS, even without --noImplicitAny, already requires much larger changes. Here's one that I've encountered a lot:

var o = {};
if (foo) {
    o.x = ...; // Error: Property 'x' does not exist on type {}
} else {
    o.x = ...; // Error: Property 'x' does not exist on type {}
}
return o;

That's a common JS pattern and not even a dangerous one. But it wasn't considered important enough to have special, confusing properties-set-outside-the-object-literal rules made for it. You have to either declare o as any, or else restructure everything, turning the properties into temporary variables so you can build the whole object with one literal at the bottom.

Compared to that, fixing array for/ins both is easier, and is actually making bad code better (as opposed to just making already acceptable JS code more type-checkable).

[jods4]

I didn't read the full thread, but wanted to say that this change in TS 1.8 broke several places in our codebase when upgrading to the beta.

it seems @ahejlsberg has special-cased the array[i] access to continue to work when iterating an array's keys because "the string is know to be convertible to a number", but then there are several other valid code pattern that still breaks.

For instance, from our codebase:

for (let i in someArray) {
  // ... stuff
  someArray.splice(i, 1);
}

It fails because Array.splice takes a number, not a string. But by the same virtue as array[i] is OK because conversion is guaranteed to work, I'd argue lots of code breakage could be avoided by allowing the string -> number conversion in all places where the variable comes from an array for..in.

[RyanCavanaugh]

It fails because Array.splice takes a number, not a string

This is very much intentional. Consider if the splice implementation had code like if (i === 0) ... -- the wrong thing would happen.

Indexed access doesn't have this problem.

[jods4]

@RyanCavanaugh Interesting, I was frustrated by the errors and didn't think it through.

Clearly you are right and allowing all string -> number conversions is not safe.

For the splice case it is safe though, because ECMA specs step 5 says ToInteger(start). The same could be said for all standard array methods (I believe any native methods in fact. ECMA specs always convert to the desired type, as far as I know).

Maybe you could special case conversions to number on the Array built-ins?
That is both the most likely place such code will happen, where it will break, and where it actually is safe.

[NN---]

Shouldn't this work ?
I define explicitly type 'A' as a type with numeric indexer to have numeric key in 'for..in'.

interface A {
    [index:number]:number;
}

var a : A = {1:1,2:2};
for (var x in a) {
    var y = x * 2;
}

[jods4]

@nn look three comments above.

for (var x in a) automatically types x to string. Always.

As of today, TS has a built-in exception whereas if a is an array, then doing a[x] is fine although it should be a type error (because the indexer expects a number but x is a string).
There is no other implicit conversion to number.

Note that in your code above, TS is correct assuming x is a string. Try this:

var a = {1: 1, 2: 2};
for (var x in a)
  console.log(typeof x);

And you'll see string as output. What happens when you do x * 2 is JS implicit conversion to number. This "works" but is not accepted by TS type system.

[NN---]

@jods4 I understand and agree but I would like to have some 'string_index_as_number' type which allows only to be used as indexer and disallow everything else.
So this code will work but typeof will be string.

[RyanCavanaugh]

Shouldn't this work ?

⚠️ ⚠️ ⚠️ No! ⚠️ ⚠️ ⚠️

interface A {
    [index:number]:number;
}

var a : A = {1:1,2:2};
for (var x in a) {
    var y = x + 2; // y: '12', '22'
}

[NN---]

@RyanCavanaugh I get your point.
I just want to understand what is the better way to fix my code from older version.

I have something like this:

interface SubStates {
 [subStateId: number]: SubState;
}

class State {
 subStates: SubStates = {};
}

interface States {
 [id: number]: State;
}

// Map like object
states: States = {};

// Recursive traversal
function f(id: number) {
  var subStates = states[id].subState;
  if (subStates) {
    for (var subState in subStates) {
     f(subState);
   }
  }
}

Since I used the 'id' only as indexer it worked without any problem.
The only option to fix it currently is to create f2 which receives string and does the recursion.

[RyanCavanaugh]

I don't understand what doesn't work about your sample.

[mhegazy]

The issue is remove expects a number.

here are the options:

• remove takes number|string

• remove(+subState);

• remove(subState as any);

• define subState before hand:

  if (subStates) {
      var subState: any;
      for (subState in subStates) {
          remove(subState);
      }
  }

[NN---]

Sorry, my bad.
I meant f(subState) of course or any function expecting number as @mhegazy mentioned.

[mhegazy]

the last would be my recommendation, as it has no runtime impact (vs +subState), and easier to read.

[NN---]

@mhegazy Good idea. I totally forgot that I can put a variable outside of the for loop.
Never encountered in a real usage for this :)

[RyanCavanaugh]

if remove is expecting a number, you really ought to give it one (+subState) rather than hoping whatever its implementation is just happens to work when given a string

[NN---]

Thank you all.