facebook / flow

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

Error using union types in interface #4084

Open tengyifei opened 7 years ago

tengyifei commented 7 years ago

Minimal reproduction:

// @flow
interface Foo {
    type: string | string[];
}
function bar(): Foo {
    return { type: 'hello' };
}

Expected behavior: Flow type-checks without issue.

Actual behavior: Error

test.js:3
  3:     type: string | string[];
                        ^^^^^^^^ array type. This type is incompatible with the expected return type of
  6:     return { type: 'hello'  };
                        ^^^^^^^ string

Flow version: 0.47.0

AugustinLF commented 7 years ago

It's a variance problem, that I'll struggle to explain. But Foo can be mutated (since it's JS), if you want to use this as you're doing, you need to type Foo.type as covariant. See the docs, or this blog post.

// @flow
interface Foo {
    +type: string | string[];
}
function bar(): Foo {
    return { type: 'hello' };
}
ubershmekel commented 7 years ago

Thanks @AugustinLF - after reading the docs and the @samwgoldman blog post I feel I've learned something. I think flow is getting in my way here. Correct me if I'm wrong:

I personally find Interfaces with mutable union types useful. Using any hacks seems my only option which defeats the goal of type soundness.

There is no way to type-check the following code using flow:

interface Flexy {
  name: string | null;
}
// the following assignment is an error without the `+name`
let fobj: Flexy = { name: 'what' };
// the following assignment is an error with the `+name`
fobj.name = null;

For me this is a reason to prefer typescript. A potential solution for this would be to differentiate between assignment of referenced variables and non-referential objects. E.g. let fobj: Flexy = { name: 'what' }; is actually sound because { name: 'what' } has no referenced variables which is not the same as assigning to fobj a variable that is a interface Solid { name: string; }.

A flow try to describe the tradeoff with the simplest sample I could make: https://flow.org/try/#0JYOwLgpgTgZghgYwgAgMoHsA2wAmyDeAUMiciHALYQBcyAzmFKAOYDchAvoaJLIigDFMEAB4BPAMLoQDAsWQBqclVoMmIZsgA+ZAK6ZM7Lj2jwkyIaLEBZXWDgAjYXJLKa9Ri216DRwsLBkZkx0BzhMDGwcWkjcZABeAjJKdwByAAsIA3RU5A52AOQYBGkGWktxKRlAxODQ8NicdgB6ZuQAEQBBADkAcQBRACVkODo6YGYQLzB0ItCAKwA6N2QAd3R9PAp0HGAYMSCQsIisOIBaZARgKARdCgA3CHAvDahkMDEABxQ6MQYICiLQitYqlMDLFIJHyGQjAtpCdCrZAOCAvR5QLIHT5QdCQBBgYCPAD8cOQAAUcd8oB9kAADNy05DAOhMkAlCifOAEpwQUkgfSYRbIAAq6WZ7y+KHFoHZnO5zlWwDA6Q86mYpIA6ukuSNLlAvgSEMhoDioItzXDCjAKHYAEzlYTiWz2HlQ-DJFTIVKrbWQdG5fKw1rIAAS0AgqRZypQumCB3WUAA1nAcboQHgZqyGHBnlzBI740qVXBkNheOEgcHReLdnRHMIo5kJd9LpkEImvDB0G9QNnwFzgNJkNQABT+CCBa12B1WZ31lCJEfuty0b2+iD+vK0HNiACU7Cn4JWiX5vkIQA

gitonthescene commented 4 years ago

Hi,

I'm not sure if this issue is active, but I produced some examples of the possible variations. It seems that the only version which fails is when using an interface with a literal for the value. I've read the doc on depth subtyping, but as ubershmekel indicates above, we seem to be incurring an overly restrictive type for the literal which requires the (overly?) verbose version below.

Could we make the literal in this case automatically take on the type of the union instead in this case?

It's not clear to me why this doesn't cause a problem when using types instead of interfaces.

(If nothing else, hopefully this documents the issue better for then next person running into this.)

Thanks, -Doug

// @flow interface Foo { type: string | string[]; }

type Foo2 = { type: string | string[]; }

const test1 : Foo = { type: 'environment' }; const test2 : Foo = { type: ('environment':$PropertyType<Foo, 'type'>) }; const test3 : Foo2 = { type: 'environment' }; const test4 : Foo2 = { type: ('environment':$PropertyType<Foo, 'type'>) };

https://flow.org/try/#0PTAEAEDMBsHsHcBQBLAdgFwKYCdIEMBjTUAMVllAG9FRbR0BPAB0wC5QBndbNAc1AA+nbnwDaAXQDciAL6JEjFqXIAmUAF4qNOorbCeqfkK4HeE6XMQhQAQjugA8qmgN6AC2QdQAI2yY8ANZedjZWwASwqFz0mFwAjKDsZBSalPTMegDkmKgAbsjYkQC2OeiZoDLSEVHoMVxqSeQaVOks7AAU2XkFxaWZrAAkAAqFLNiMACoZADzJADSgmbqZAHwAlBVVkdFYXADMicqwaqmtWTn5haglGOWViNU7segALIfJJy26HV2XvbeDEawMaTGbzRbLdabeRAA