facebook / flow

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

Flow can't distinguish shapes of union types when testing the shapes of properties within the subtypes #4328

Open dclowd9901 opened 7 years ago

dclowd9901 commented 7 years ago

You may see this bug in action at https://flow.org/try/#0C4TwDgpgBAYg9nKBeKBvAUFKwAWBLAOwHMAuKAcl0KPIBp0BfAbnXVElgQBtk1MoARtTLkhxOoxZtw0AEIBDAE68MWCAQCuAWxHrtErADd5XDRDLw49Zq3Zz5ALxX89Oiq7pQA9F6gBNCABnWigASSgAawI4AHcoPGByQOwcaED5LQgQgQ1geLzAnDgNLgATKEDgPC4eAWh5AS5oYERgCBqofixunt6fKAAzRTgtFOgwYchFUCgAY1TZiP5jU3NOOC5rKX7QgnitMCbMgmB5KrgCVgGNAlnzvYGCAAowJQyyBWUAHygFBwBKMgAfksfCweAGUBeby0ADoVmZYVRiMgkChKPhxP8wd1FBBgBpFHtXooMvCTGYWFgGPx+HiCUSoJoaiwGEA

Code is here:

type Foo = {
  thing: 'thing',
};

type Fool = {
  bing: 'bing',
};

type Bar = {
  enum: 'enum',
  value: Foo,
};

type Baz = {
  enum: 'enum', // Yes, I know it's the same, but it should still be able to tell 
                // from the property check
  value: Fool,
};

// In implementation

function fn(param: Bar | Baz): ?Foo {
  if (param.value.thing === 'thing') {
    return param.value;
  }

  return null;
}

Error:

22: function fn(param: Bar | Baz): ?Foo {
                                    ^ property `thing`. Property not found in
24:     return param.value;
               ^ object type

Hard to explain, but in essence it seems as though when Flow needs to check a property of a subtype of a union in order to determine the subtype, it fails.

bendman commented 7 years ago

The minimal case I could think of that produces this issue is this:

type Cup = {| handle: string |};
type Saucer = {| contents: string |};

type Dish = Cup | Saucer;

function fn(param: Dish): ?Cup {
  if (param.handle != null) {
    return param;
  }
  return;
}
dclowd9901 commented 7 years ago

Thanks @bendman

rsolomon commented 7 years ago

The original issue is capturing what appears to be a real bug, but I think the example is catching an error for a different reason. The situation itself is somewhat convoluted and involves any attempt at refining a disjoint union that is nested within a property of another refined disjoint union. Here is a relatively simple example.

The two examples given so far are failing not due to this bug, but because they are attempting to refine a disjoint union using a property name that is not guaranteed to be available on all union branches. This scenario is outlined in the documentation: "Disjoint unions require you to use a single property to distinguish each object type. You cannot distinguish two different objects by different properties."

ariesshrimp commented 7 years ago

@rsolomon what about this workaround from the documentation? It comes right after the section you refer to. It claims that exact object types circumvent this constraint by forcing a property conflict between the disjointed types:

However, to get around this you could use exact object types.


// @flow
type Success = {| success: true, value: boolean |};
type Failed  = {| error: true, message: string |};

type Response = Success | Failed;

function handleResponse(response: Response) { if (response.success) { // note that (response.success != null) produces the type error here, why? var value: boolean = response.value; } else { var message: string = response.message; } }


> With exact object types, we cannot have additional properties, so the objects conflict with one another and we are able to distinguish which is which.

How is this semantically different from @bendman's case?
ariesshrimp commented 7 years ago

@rsolomon sorry, re-reading this thread, i think what we're all saying is:

// @flow
type Success = {| success: true |};
type Failed  = {| error: true |};
type Response = Success | Failed;

function handleResponse(response: Response) {
  if (response.success) // this
  if (response.success != null) // should behave the same as this
  if (response.success != undefined) // should behave the same as this
  if (String(response.success)) // should behave the same as this
  // etc
}
rsolomon commented 7 years ago

Fair enough, I should have kept reading! I think we are maybe capturing 2 different issues here:

  1. What you are talking about, regarding refinement of a union based on a negative check.

  2. Inability to refine a top level disjoint union when attempting to refine based on a property that is also a disjoint union.

bendman commented 7 years ago

I'm wondering if there's consensus on whether this should be fixed. It would appear that this reveals many related issues, in that (almost?) any check other than comparing an always true prop using the exact format if (object.prop) will throw an error.

Some examples are below, although there are many more. I haven't yet looked into the cause of the issues, but I would expect all of these to work:

type Success = {| trueCheck: true, falseCheck: false, boolTypeCheck: boolean |};
type Failed  = {| error: true |};
type Response = Success | Failed;

// Working

function checkForTruthyTrue(response: Response): ?Success {
  if (response.trueCheck) return response;
}

// Failing

function checkForEqualTrue(response: Response): ?Success {
  if (response.trueCheck == true) return response;
}

function checkForExactlyTrue(response: Response): ?Success {
  if (response.trueCheck === true) return response;
}

function checkForExactlyFalse(response: Response): ?Success {
  if (response.falseCheck === false) return response;
}

function checkForType(response: Response): ?Success {
  if (typeof response.boolTypeCheck === 'boolean') return response;
}

One note is that some of this could get into equality oddities, but strict types help minimize those cases.

ariesshrimp commented 7 years ago

yeah, is there another open issue about this? i'm shocked if this is the first mention. this kind of checking is the most natural way to handle disparate inputs in js

bendman commented 7 years ago

For the record, the TypeScript version works, but you must explicitly state your types in the conditional, because it doesn't infer anything.

interface Success { trueCheck: true; falseCheck: false; boolTypeCheck: boolean; }
interface Failed { error: true; falseCheck: true; boolTypeCheck: string }
type Resp = Success | Failed;

// Working
function checkForTruthyTrue(response: Resp): Success | undefined {
    if ((<Success>response).trueCheck) return <Success>response;
}

function checkForEqualTrue(response: Resp): Success | undefined {
    if ((<Success>response).trueCheck == true) return <Success>response;
}

function checkForExactlyTrue(response: Resp): Success | undefined {
  if ((<Success>response).trueCheck === true) return <Success>response;
}

function checkForExactlyFalse(response: Resp): Success | undefined {
  if ((<Success>response).falseCheck === false) return <Success>response;
}

function checkForType(response: Resp): Success | undefined {
  if (typeof (<Success>response).boolTypeCheck === 'boolean') return <Success>response;
}
ariesshrimp commented 7 years ago

i think the relevant test code is here:

https://github.com/facebook/flow/blob/3ea841038418f680b0985f8a6084e507b04a484f/tests/exact/prop_test.js#L19

ariesshrimp commented 7 years ago

i started poking around the hack codebase for these prop checks, but it's a pretty complex system. i think starting on this would require some guidance from a 🐫

bendman commented 7 years ago

So, to clarify things, doing a check for if (object.property) will refine the scope when Exact object types are used. This seems to serve the same purpose as TypeScript's (<Type>variable) assertions.

@dclowd9901's original issue can be solved in a few equivalent ways:

type Foo = {
  thing: 'thing',
};

type Fool = {|
  bing: 'bing',
|};

type Bar = {
  enum: 'enum',
  value: Foo,
};

type Baz = {
  enum: 'enum',
  // You could also forego the pipes (`|`) in Fool above by using `$Exact<Fool>` here
  value: Fool,
};

// In implementation

function fn(param: Bar | Baz): ?Foo {
  // Note the initial check for `param.value.thing`, refining the type
  if (param.value.thing && param.value.thing === 'thing') {
    return param.value;
  }

  return null;
}

This works in this case (and in the negative) but it wouldn't work for some cases, like typeof checks.

chasingmaxwell commented 5 years ago

Maybe it belongs in another issue, but I'm seeing that when the distinguishing property is nested, there are errors, but only when you assert the type of the top-level object in the union. Referencing it's child properties is fine. Here's a simple example:

// @flow
type Success = { nested: { success: true, value: boolean } };
type Failed  = { nested: { success: false, error: string } };

type Response = Success | Failed;

function handleResponse(response: Response) {
  if (response.nested && response.nested.success) {
    var value: boolean = response.nested.value; // Works!
  } else {
    var error: string = response.nested.error; // Works!
    acceptFailedNested(response.nested); // Works!
    acceptFailed(response); // Fails :(
  }
}

function acceptFailedNested(failedNested: { success: false, error: string }) {
  console.log(failedNested.error);
}

function acceptFailed(failure: Failed) {
  console.log(failure.nested.error);
}

Try it here

I also tried @bendman's suggestion of using exact types. That did not seem to make any difference.