facebook / flow

Adds static typing to JavaScript to improve developer productivity and code quality.
https://flow.org/
MIT License
22.07k stars 1.85k forks source link

Issue with optional object value in array #6843

Open chrisdrackett opened 6 years ago

chrisdrackett commented 6 years ago

I'm trying to do the following and I'm confused as to why it isn't working. I see that the types are different, but it seems to me the type without dataLabel should still satisfy the type I have setup where that value is optional:

https://flow.org/try/#0C4TwDgpgBAksEFsoF4oG8BQUoBsCGARhDgFxQDOwATgJYB2A5gDRZQAmeweAMocQPxlKtRi2wcuZAIJUqeEAB40AXwB8GZRgyhIUAPLAAFhCpxEKdK3xFSFavWasJeabPlK1GrTuhmEMuRALAPc-dWxWDABjAHs6Sih4SgA1PBwAVwhyV0CFA2NTeARVCwBtAF1IgDN0uijgGjjErOAAMVr6xroAChoi7NgikJAASktsKghgdKo6KAAiKpiY+a9tFva6hrjupOBUjKyRoA

wchargin commented 6 years ago

Flow is correct here.

Your object types are not exact, so it is perfectly legal for them to have extra fields. For instance:

const example: OtherItem = {label: "hello", data: [], dataLabel: 123};  // OK

This demonstrates that OtherItem must not be considered a subtype of Item. If it were, and if your example were legal (it also has array variance issues, but I’ll ignore those), then you could implement the function as follows:

function testFunction(items: ItemArray) {
  for (const item of items) {
    if (item.dataLabel != null && typeof item.dataLabel !== "string") {
      throw new Error("Not possible: " + (item.dataLabel: empty));
    }
  }
}

This implementation is perfectly valid, according to the type of Item: if the dataLabel is present, it must be a string. Flow properly accepts this code.

But if example could be treated as an Item, then you could invoke testFunction([example]), which would hit the “impossible” branch. Once you have a value of type empty in reachable code, you’ve broken the entire type system.


This is all predicated on your object types being inexact. If you make them exact, this problem goes away, but another one appears. Suppose that we write the following perfectly normal function:

function addLabel(x: Item): void {
  x.dataLabel = "goodbye";  // no problem
}

If OtherItem were a subtype of Item, then we could write

const otherItem: OtherItem = {label: "hello", data: []};  // OK
const item: Item = otherItem;  // !!!
addLabel(item);

This now violates the exactness of OtherItem (and opens up a similar unsoundness vector to above).


If your types are both exact and read-only, then Flow still rejects the code, but I don’t see why this time. None of the usual reasons seems to apply.

ghost commented 6 years ago

@wchargin You are right that OtherItem has to be an exact type and that OtherItem is not the subtype of Item but the other way around.

const t = (a:OtherItem) => {
  const b:$Subtype<Item> = a; // error
}
const p = (a:Item) => {
  const b:$Subtype<OtherItem> = a; // fine
}

@chrisdrackett To fix, in addition to @wchargin's suggestions , you need to mark dataLabel property in Item as covariant. It's a little confusing because the docs say interface properties are marked invariant by default but it seems to be true for type aliases too. Probably related to #6783 .

wchargin commented 6 years ago

@bhandarijiwan: It is true that if the types are as in the OP, then Item is a subtype of OtherItem. But this is not really useful or relevant: the OP wants to pass an array of OtherItems to a function expecting an array of Items, not the other way around.

Using exact covariant types and read-only arrays does fix the original code, as I alluded to when mentioning the variance issues in my comment.

It's a little confusing because the docs say interface properties are marked invariant by default but it seems to be true for type aliases too.

It doesn’t have to do with the type alias. The type in question is an object type. For object types, covariance is equivalent to being read-only, and so being invariant is the natural default (in JavaScript-land, at least).