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

width subtyping with nonexact known properties fails in idiomatic javascript #6146

Open villesau opened 6 years ago

villesau commented 6 years ago

See in try flow

Documentation mentions width-subtyping, caveats and how to use exact types to be able to be more precise: https://flow.org/en/docs/lang/width-subtyping/

However, there are cases where all the possible types are known even when the ojbects are not exact.

As you can see, both object properties defines property selector. This means that the property can be either true or false and based on that props has at least bunch of properties. If it's true, we know that property a will be number, not string etc.

Anyhow this is not working in flow which is major defect since the code pasted in try flow is pretty idiomatic way of using Javascript, and as far as I know, there can't be uncertainty of defined types which means it should be completely possible for Flow to type check the above code properly.

PS. the above example does not work with exact types either.

apsavin commented 6 years ago

Here the fixed example.

Issues with destructuring of unions are alredy reported: https://github.com/facebook/flow/issues?utf8=%E2%9C%93&q=is%3Aissue+is%3Aopen+union+destructuring

villesau commented 6 years ago

@apsavin that's not logically the same. In your example, selector is passed into sub components when in the original example it's parsed away from the props. I can live with that, but it's suboptimal as I need to change the code in order to make it work with Flow.

E: Interesting examples: This works but passes also selector: example

But when spreading the object, it stops working: example

apsavin commented 6 years ago

it's suboptimal as I need to change the code in order to make it work with Flow.

Agree, I hope it will be fixed with the ability to use destructuring of unions.

villesau commented 5 years ago

This is actually very broken with exact types too: Try Flow

There might be duplicate of this issue though.

villesau commented 5 years ago

Here are bunch of related issues or duplicates:

https://github.com/facebook/flow/issues/6805 https://github.com/facebook/flow/issues/6594 https://github.com/facebook/flow/issues/6408 https://github.com/facebook/flow/issues/5745 https://github.com/facebook/flow/issues/4772 https://github.com/facebook/flow/issues/3932 https://github.com/facebook/flow/issues/5461

Could e.g @mrkev or @jbrown215 comment on wether this could be prioritized? Looks like there are quite a many developers struggling with this.

jbrown215 commented 5 years ago

cc @samwgoldman, who has a lot of changes to object types in the works.

samwgoldman commented 5 years ago

There are a few issues here, one is a know bug with destructuring unions of objects. Here is a working version:

import * as React from 'react'

type Sub1Props = {| a: number, b: number |};
type Sub2Props = {| a: string, c: string |};

const Sub1 = (props: Sub1Props) => <div />;
const Sub2 = (props: Sub2Props) => <div />;

const Com = (props: {| selector: true, ...Sub1Props |} | {| selector: false, ...Sub2Props |}) => {
  if (props.selector) {
    let {selector, ...rest} = props;
    return <Sub1 {...rest} />;
  } else {
    let {selector, ...rest} = props;
    return <Sub2 {...rest} />;
  }
}

Once you destructure selector and props into separate variables, you can't refine props by doing a check on selector—the relation between these variables is not tracked by Flow. Instead, you need to keep the union together, discriminate between the arms of the union as I've done above, then you can destructure.

villesau commented 5 years ago

Thanks a lot! Definitely sounds very reasonable, but it's also surprising if you don't know exactly how Flow internals works. That's probably the reason for the confusion that's caused so many bug reports.

shanshanzhu commented 5 years ago

@samwgoldman The solution is not ideal, especially if we wanna use flowtype to represent the proto3 oneof field. Is there any plan to improve it? https://developers.google.com/protocol-buffers/docs/reference/proto3-spec#oneof_and_oneof_field