narrower type inference of for..in loop variables

[scascketta]

Consider the following code:

type RawGoodType =
  | "Coal"
  | "Iron Ore"
  | "Copper Ore"
  | "Iron Bar"
  | "Copper Bar";

type Bundle = {[good: RawGoodType]: number};

class Producer {
  recipe: Bundle;
  output: Bundle;

  canProduce(input: Bundle): boolean {
    for (let good in this.recipe) {
      if (!input.hasOwnProperty(good) || input[good] < this.recipe[good]) {
        return false;
      }
    }
    return true;
  }
}

This produces the output:

src/RawGood.js:18
 18:       if (!input.hasOwnProperty(good) || input[good] < this.recipe[good])
                                                    ^^^^ string. This type is incompatible with
 10: type Bundle = {[good: RawGoodType]: number};
                           ^^^^^^^^^^^ string enum

src/RawGood.js:18
 18:       if (!input.hasOwnProperty(good) || input[good] < this.recipe[good])
                                                                        ^^^^ string. This type is incompatible with
 10: type Bundle = {[good: RawGoodType]: number};
                           ^^^^^^^^^^^ string enum

Which is saying that this fails to type check on input[good] and this.recipe[good] saying that the type of good (string) is incompatible with type string enum. Shouldn't Flow infer that good is a member of the RawGoodType string enum since it is a property of recipe which is an object whose properties must belong to RawGoodType?

[avikchaudhuri]

This is not an enum issue. It is a bug in our for-in handling: instead of looking up the key set of the object to loop over, we assume the key set is always string.

[avikchaudhuri]

Related to #935

[chris-lock]

+1

[aarondcohen]

+1

[mroch]

i tried briefly to fix this and it wasn't too hard to teach it a better type for the left-hand side than just string, but I ran into an issue. consider a case like this:

var x;
if (Math.random() > 0.5) {
  x = { a: 'foo' };
} else {
  x = { b: 'bar' };
}

for (var y in x) {
  x[y];
}

If we infer x to be { a: string } | { b: string}, then y should be 'a' | 'b'. but x[y] will try x['a'] on { b: string } (and x['b'] on { a: string }) and error.

Not sure how we would encode that the type of y depends on which x is chosen.

[mroch]

Facebook: see D4290615. If I have time, I'll try to move it to a (closed) PR for posterity.

[jedwards1211]

@mroch I guess we should also mention enums in the title?

[dxu]

Hi, I came across this when trying to figure out an issue I was coming across, hope you guys don't mind me chiming in!

Is this also the cause of errors when I do something like this:

function test(
  events: {
    [event: number]: {
    }
  }
): void {
  for (const event: number in events) {
  }
}

I'm getting the following error:

for (const event: number in events) {
                  ^ string. This type is incompatible with
for (const event: number in events) {
                  ^ number

From this section in the documentation, I was under the impression that Flow would be smart enough to figure out the explicit type when iterating over an object used as a map with explicit keys, but from @avikchaudhuri's comment, it seems it might be because of the for-in handling?

[vkurchatkin]

@dxu when you use some subtype of string (like 'foo' | 'bar') as indexer in theory type could be more precise in for..in loop. It won't be 100% safe though, but that's the nature of map objects.

But, it can never be a number or anything else, because it is string at runtime.

[jedwards1211]

@vkurchatkin

It won't be 100% safe though, but that's the nature of map objects.

Are you thinking it would be unsafe in really old versions of JS, or would there be edge cases that trick Flow's type system even in modern JS? Like something to do with prototype chains?

[vkurchatkin]

Something like this:

const x = {
  a: 1,
  b: 1,
  c: 1
};

const x1: { a: number, b: number } = x;
const map: { ['a' | 'b']: number } = x1;

map has 'a' | 'b' as indexer, but actually has other properties.

[jedwards1211]

@vkurchatkin oh, right. So it should work with exact shape types, right?

const x = {
  a: 1,
  b: 1,
};

const x1: {| a: number, b: number |} = x;
for (let num in x1) {
  const num2: 'a' | 'b' = num 
}

I also tried the following and Flow got confused:

const map: {| ['a' | 'b']: number |} = x;

13: const map: {| ['a' | 'b']: number |} = x;
                                           ^ property `a`. Property not found in
13: const map: {| ['a' | 'b']: number |} = x;
               ^ object type
13: const map: {| ['a' | 'b']: number |} = x;
                                           ^ property `b`. Property not found in
13: const map: {| ['a' | 'b']: number |} = x;
               ^ object type

[dxu]

@vkurchatkin, thanks for the response! I understand that at runtime, objects are string-keyed, but my understanding of the documentation (with the below example) was that you could specify key types for accessing objects that are being used as Maps during the static analysis.

So, for example, you could potentially use this for accessing objects as a map with keys being of a custom enum type (or maybe something similar to your | example), or in my case, a number.

I'm confused because this seems to work with my above reasoning:

/* @flow */
function test(
  events: {
    [event: number]: {
    }
  }
): void {
    events['10'];
}

It gives the following error. From my understanding, this is because events is expected to be a "Map" keyed with numbers, and so you can't access it with strings:

8:     events['10'];
       ^ property `10` is a string. This type is incompatible with
4:     [event: number]: {
               ^ number

Am I misunderstanding something here?

[vkurchatkin]

Am I misunderstanding something here?

No, you are not. For Flow events['10'] and events[10] are different, although they are the same for VM.

[dfleury]

Consider this sample code:

/* @flow */

type AllowedNames = 'John' | 'Marc' | 'Peter'

type Employees = {
  [ name: AllowedNames ]: { age: number }
}

const employees: Employees = {
  'Marc': { age: 25 },
  'Peter': { age : 32 }
}

for (const name: AllowedNames in employees) {
  console.log(employees[name].age)
}

It throws:

14: for (const name: AllowedNames in employees) {
               ^ string. This type is incompatible with
14: for (const name: AllowedNames in employees) {
                     ^ string enum

To fix this, we need to dynamically check the values:

/* @flow */

type AllowedNames = 'John' | 'Marc' | 'Peter'

type Employees = {
  [ name: AllowedNames ]: { age: number }
}

const employees: Employees = {
  'Marc': { age: 25 },
  'Peter': { age : 32 }
}

for (const name: string in employees) {
  if (name === 'John' || name === 'Marc' || name === 'Peter')
    console.log(employees[name].age)
}

Well, it isn't much DRY, right?

[jessebeach]

@dfleury this just worked for me. You can use a Map instead of an object, which allows non-string keys and it appears that the Iterator accounts for Generics and accepts the Enum key values.