$Required utility type

[goodmind]

Fixes #5134

It doesn't touch T | void unions

[facebook-github-bot]

@dsainati1 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[dsainati1]

This looks like a good approach, but I have a couple of concerns.

I'm not entirely convinced by how this utility type behaves with respect to class and instance types. You are able to cast something of type PointC to $Required<PointB>, which seems to violate the nominality classes are supposed to have. I think because you are producing an object type in the code where you handle instances, your check ignores the class hierarchy which I don't think is what we'd want. The code as written also only handles own props. Is there a good reason we'd want this utility to work on instances in the first place? It seems like this would be much simpler if we restrict it to object types.

Your provided tests also have some discrepancies in them: you add an error comment on
(pc: $Required<PointB>); // error, (ps: $Required<{ x?: number }>); // error, and (pi: $Required<{ x?: number }>); // error but no error is reported in these places. Did you intend for there to be an error on these casts?

I'd also like to see a few tests that ensure that property variance is properly maintained by this utility.

[goodmind]

@dsainati1 it converts classes to objects, just like $ReadOnly and $Rest does.
PointC to $Required<PointB> looks like bug

How can I seal object completely from flowing?

[jbrown215]

Looking good overall, but I think you worked harder than you needed to.

If you move away from using a new use_t and instead implement $Required in object_kit (search for "and object_kit" in flow_js.ml) then you'll be able to reduce your changes quite a bit.

Polymorphism + type destructors is sometimes tricky, so please add a few tests to make sure $Required works in polymorphic functions.

[jbrown215]

Instead of adding a new use type, can you follow the way ReadOnly is implemented in object_kit and do the same? It will significantly simplify your code by handling a ton of edge cases for you.

For example, your code will error on mixed/empty, which the object_kit code can handle more elegantly.

You also won't need the IncompatibleRequiredT because object kit handles errors with types that can't be converted into an object.

You also shouldn't need to distinguish between instances and objects, since object_kit will take care of that for you.

[goodmind]

But object_kit doesn't have polarity, so it always becomes neutral

[goodmind]

Also there is issue in object_kit with statics, maybe you know how to fix it?
#7572

[goodmind]

I don't understand why contravariant properties get replaced with mixed in object_kit
https://github.com/facebook/flow/blob/master/src/typing/flow_js.ml#L10884-L10888

| None ->
      let reason = replace_reason_const (RUnknownProperty (Some x)) r in
      let t = DefT (reason, bogus_trust (), MixedT Mixed_everything) in
      t
    in

[goodmind]

I added original props (raw_props) because I needed polarity from field structure, and also doesn't wanted to get mixed on write-only properties. Not sure if this belongs here, or perhaps object_kit can be more generic

[goodmind]

This handles getters/setters in object, but not in classes, not sure why

[goodmind]

Probably should merge original props also? But they don't get used here

[goodmind]

Pick implemented with $Required

type RestUnsound<T1, T2> = $Rest<T1, $Required<T2>> // $Rest doesn't remove optional props

type Pick<T1, T2> = RestUnsound<
  T1,
  $ObjMap<RestUnsound<T1, T2>, <V>(V) => any>
>;

declare var picked: Pick<{| +a?: number, b?: string, c: string |}, {| a: any |}>;

picked.a; // void | number
picked.a = 1; // doesn't save polarity
picked.b; // error
picked.c; // error

Avoids this bug #5936
Edit: this isn't bug actually as per comment about Rest

If the object we are using to subtract has an optional property, non-own
property, or is inexact then we should add this prop to our result, but
make it optional as we cannot know for certain whether or not at runtime
the property would be subtracted.

[goodmind]

@jbrown215 friendly ping here

[jbrown215]

I can't make time for this yet. I'm spending all my time on a big hurdle in my spreads project. Once I get past that, this will be the first thing I review.

[goodmind]

The more i think of this, probably there should be more general solution, i.e. some mapper or even $ObjMap itself should be allowed to configure property type descriptor (similar to js, but not identical)

type $PropertyDescriptor<T> = {|
   optional: boolean,
   enumerable: boolean, // ???
   value: T,
   polarity: 
     | '+' // positive 
     | '-'  // negative
     | 'N'  // neutral
|}

type $ReadOnly<T> = $ObjModify<
  T, 
  <Descriptor>(Descriptor) => {|...Descriptor, polarity: '+'|}
>
type $WriteOnly<T> = $ObjModify<
   T, 
   <Descriptor>(Descriptor) => {|...Descriptor, polarity: '-'|}
>
type $ReadWrite<T>  = $ObjModify<
  T, 
  <Descriptor>(Descriptor) => {|...Descriptor, polarity: 'N'|}
>

type $Required<T> = $ObjModify<
  T, 
  <Descriptor>(Descriptor) => {|...Descriptor, optional: false|}
>

I realize that type destructors are tricky, and not all of them work well with each other, so perhaps generalization would only make it worse

[dsainati1]

So after chatting to some people on the team I think we would like to decrease the number of $ types in Flow's system, so I am going to close this since it is moving us in slightly different direction that we'd like. This can be equivalently implemented with $ObjMap in any case.

[goodmind]

@dsainati1 this can't be implemented with $ObjMap