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

No way to refine types #6560

Open Diggsey opened 6 years ago

Diggsey commented 6 years ago

Using the spread operator has been previously recommended to use instead of intersection, because intersection doesn't behave in the way most users expect. However, the spread operator also doesn't work for refining types:

type X = {a?: string, b?: string}
type Y = {a: string, ...X}

const x: X = {}
if (x.a) {
  const y: Y = x
}

This is a very common case that seems to be impossible to represent with flow. Using exact objects instead doesn't appear to help.

apsavin commented 6 years ago

Could you please provide a link to flow.org/try with a repro?

Diggsey commented 6 years ago

https://flow.org/try/#0C4TwDgpgBAGlC8UDeBDA-ALigZ2AJwEsA7AcwBooAjTHfYkgXwChRIoBNBZFLXQ0igDphMZkwDGAeyK4oADyxxESZgQBmUABRzBKAJTImUKFJnAoILJ0RymDIA

jcready commented 6 years ago

This is because both x and y would share a reference to the same object and since all of the properties of the type X are writable it would allow you to do this:

const x: X = { a: 'string' }
if (x.a) {
  const y: Y = x
}
x.a = undefined
// oh no, `y.a` is no longer a string

However, if you spread x then everything works as expected:

const x: X = { a: 'string' }
if (x.a) {
  const y: Y = { ...x }
}
x.a = undefined
// `y.a` is still a string

P.S. the check of if (x.a) { won't pass for an empty string. A better way to refine this would be by using if (typeof x.a === 'string') {

Diggsey commented 6 years ago

@jcready That would make sense, except that the example still fails with $ReadOnly:

type X = {a?: string, b?: string}
type Y = {a: string, ...X}

const x: $ReadOnly<X> = {}
if (x.a) {
  const y: $ReadOnly<Y> = x
}

Re: the string check - yeah, this was just a short example: it just means the check is slightly too conservative, so it shouldn't affect the typing errors.

It would also be useful to be able to express a type which is nullable when read, but not when written. ie. Once you've observed a value there, you can rely on it staying there, without making the field readonly.

Diggsey commented 6 years ago

@jcready It's also inconsistent with how flow treats nullable fields:

type X = {a: ?string, b?: string}
type Y = {a: string, ...X}

const x: X = {a: 'hi'}
if (x.a) {
  const y: Y = x
  x.a = undefined
  console.log(y.a)
}

This suffers the exact same problem, and yet is accepted by flow.

goodmind commented 5 years ago

/cc @jbrown215 this is the most labeled issue ever

villesau commented 5 years ago

Even when moving a definition after spread & making everything exact & read-only it still fails:

https://flow.org/try/#0C4TwDgpgBAGlC8UDeAfKBqAhgfgFxQGdgAnASwDsBzAGgwCM9CSLKoUBfAKFEigE0EyNADpRMWlnxEyVDp04BjAPbkiUAB744iJFEz4A5MQjGDULqQBmUABTrhmAJTJOUKMtXAoIfAMTrOdiA

jbrown215 commented 5 years ago

This has nothing to do with spreads. You can just copy the properties into the object directly to make the repro.

This also just isn't how Flow does refinements. That refinement on x.a refines x.a, not x itself. See here for a smaller repro

jcready commented 5 years ago

This also just isn't how Flow does refinements. That refinement on x.a refines x.a, not x itself.

Why shouldn't Flow be able to do refinements that way? In your example:

type A = {+p?: string};
type B = {+p: string};

declare var a: A;

if (a.p != null) {
  (a.p: string); // Refined
  (a: B); // error. a isn't refined to a new type, only a.p is
}

How is it unsound to mark a as being type B?

jbrown215 commented 5 years ago

Both behaviors are sound** (edited, previously erroneously said unsound). This is just an instance of where Flow is incomplete. That's why we made disjoint unions-- to explicitly support cases where you need to distinguish between branches of object types.

goodmind commented 5 years ago

@jbrown215 You probably meant example like this

type X = { +a?: string }
type Y = { +a: string }

const x: X | Y = { a: 'rere' }
if (x.a) {
  const y: Y = x // error
}

link

jbrown215 commented 5 years ago

I didn't, but it does further illustrate my point. Flow has strictly more information in that code sample and still does not refine the object type.

villesau commented 5 years ago

If one would want to fix such issue, where to start digging & debugging?

jbrown215 commented 5 years ago

I think this is a fundamental change to our refinement model, which is pretty limited.

jcready commented 5 years ago

This change would be a huge improvement.

jbrown215 commented 5 years ago

Certainly, but maybe not feasible to implement in a way that is performant.