From ab9bf44c725efd2ed6d7e1e957c5566b6eb6f688 Mon Sep 17 00:00:00 2001 From: Caleb Meredith Date: Tue, 29 Aug 2017 17:00:51 -0700 Subject: [PATCH] Implement the type for the compose() function natively Summary: `compose()` is a utility used a fair deal in the React community. Especially for projects that make heavy use of HOCs. The `compose()` function is exported by [Redux](http://redux.js.org/docs/api/compose.html), [Recompose](https://github.com/acdlite/recompose/blob/master/docs/API.md#compose), Lodash (as [`flow`](https://lodash.com/docs/4.17.4#flow) and [`flowRight`](https://lodash.com/docs/4.17.4#flowRight)), and [Ramda](http://ramdajs.com/docs/#compose). All popular libraries in the JavaScript community. Currently compose is typed with [multiple predicates](https://github.com/flowtype/flow-typed/blob/aff2bf770eeb3791aba0f231616a1872d0ffbcf5/definitions/npm/redux_v3.x.x/flow_v0.33.x-/redux_v3.x.x.js#L55-L104) or an [intersection](https://github.com/flowtype/flow-typed/blob/aff2bf770eeb3791aba0f231616a1872d0ffbcf5/definitions/npm/recompose_v0.24.x/flow_v0.47.x-v0.52.x/recompose_v0.24.x.js#L71-L127). This approach has multiple downsides. You may only support composing up to `n` functions, you cannot use spreads on arrays of unknown arity, and when using an intersection it is easy [to fall into a bad error message](https://github.com/istarkov/flow-compose-error). (The intersection error message may be a bug and should probably be improved anyway at some point, but I think the case for implementing `compose()` natively is stronger then just improving the error message.) [istarkov](https://github.com/istarkov) also promised me a blog post if I implemented the type for compose natively. Reviewed By: avikchaudhuri Differential Revision: D5666855 fbshipit-source-id: 2d94c47df1d722933e21b0a693b0547958fe9efc --- src/typing/codegen.ml | 2 + src/typing/debug_js.ml | 4 ++ src/typing/flow_js.ml | 121 +++++++++++++++++++++++++++----- src/typing/type.ml | 3 + src/typing/type_annotation.ml | 5 ++ src/typing/type_normalizer.ml | 1 + tests/compose/.flowconfig | 2 + tests/compose/basic.js | 18 +++++ tests/compose/compose.exp | 122 +++++++++++++++++++++++++++++++++ tests/compose/lib/recompose.js | 3 + tests/compose/recompose.js | 25 +++++++ tests/compose/spread.js | 29 ++++++++ 12 files changed, 316 insertions(+), 19 deletions(-) create mode 100644 tests/compose/.flowconfig create mode 100644 tests/compose/basic.js create mode 100644 tests/compose/compose.exp create mode 100644 tests/compose/lib/recompose.js create mode 100644 tests/compose/recompose.js create mode 100644 tests/compose/spread.js diff --git a/src/typing/codegen.ml b/src/typing/codegen.ml index 5fcb77d8f86..6d517e657d7 100644 --- a/src/typing/codegen.ml +++ b/src/typing/codegen.ml @@ -197,6 +197,8 @@ let rec gen_type t env = Type.( | CustomFunT (_, ObjectAssign) -> add_str "Object$Assign" env | CustomFunT (_, ObjectGetPrototypeOf) -> add_str "Object$GetPrototypeOf" env | CustomFunT (_, ObjectSetPrototypeOf) -> add_str "Object$SetPrototypeOf" env + | CustomFunT (_, Compose false) -> add_str "$Compose" env + | CustomFunT (_, Compose true) -> add_str "$ComposeReverse" env | CustomFunT (_, ReactPropType (React.PropType.Primitive (_, t))) -> add_str "React$PropType$Primitive<" env |> gen_type t diff --git a/src/typing/debug_js.ml b/src/typing/debug_js.ml index 261345a691c..7673f6b8d19 100644 --- a/src/typing/debug_js.ml +++ b/src/typing/debug_js.ml @@ -363,6 +363,8 @@ and _json_of_custom_fun_kind kind = Hh_json.JSON_String (match kind with | ObjectAssign -> "Object.assign" | ObjectGetPrototypeOf -> "Object.getPrototypeOf" | ObjectSetPrototypeOf -> "Object.setPrototypeOf" + | Compose false -> "Compose" + | Compose true -> "ComposeReverse" | ReactPropType _ -> "ReactPropsCheckType" | ReactCreateClass -> "React.createClass" | ReactCreateElement -> "React.createElement" @@ -1571,6 +1573,8 @@ and dump_t_ (depth, tvars) cx t = | ObjectAssign -> "ObjectAssign" | ObjectGetPrototypeOf -> "ObjectGetPrototypeOf" | ObjectSetPrototypeOf -> "ObjectSetPrototypeOf" + | Compose false -> "Compose" + | Compose true -> "ComposeReverse" | ReactPropType p -> spf "ReactPropType (%s)" (react_prop_type p) | ReactCreateElement -> "ReactCreateElement" | ReactCloneElement -> "ReactCloneElement" diff --git a/src/typing/flow_js.ml b/src/typing/flow_js.ml index 8b840546ac8..245384c774b 100644 --- a/src/typing/flow_js.ml +++ b/src/typing/flow_js.ml @@ -3831,24 +3831,6 @@ let rec __flow cx ((l: Type.t), (u: Type.use_t)) trace = React.CreateClass (React.CreateClass.Spec [], knot, call_tout))); Ops.set ops - (* Custom handling for React.createElement() *) - | CustomFunT (_, ReactCreateElement), - CallT (reason_op, { call_args_tlist = args; call_tout = tout; _ }) -> - resolve_call_list cx ~trace reason_op args ( - ResolveSpreadsToCustomFunCall (mk_id (), ReactCreateElement, tout)) - - (* Custom handling for React.cloneElement() *) - | CustomFunT (_, ReactCloneElement), - CallT (reason_op, { call_args_tlist = args; call_tout = tout; _ }) -> - resolve_call_list cx ~trace reason_op args ( - ResolveSpreadsToCustomFunCall (mk_id (), ReactCloneElement, tout)) - - (* Custom handling for React.createFactory() *) - | CustomFunT (_, ReactElementFactory component), - CallT (reason_op, { call_args_tlist = args; call_tout = tout; _ }) -> - resolve_call_list cx ~trace reason_op args ( - ResolveSpreadsToCustomFunCall (mk_id (), ReactElementFactory component, tout)) - | _, ReactKitT (reason_op, tool) -> react_kit cx trace reason_op l tool @@ -3886,10 +3868,19 @@ let rec __flow cx ((l: Type.t), (u: Type.use_t)) trace = ) call_args_tlist; rec_flow_t cx trace (VoidT.why reason_op, call_tout); + | CustomFunT (_, ( + Compose _ + | ReactCreateElement + | ReactCloneElement + | ReactElementFactory _ + as kind)), + CallT (reason_op, { call_args_tlist = args; call_tout = tout; _ }) -> + resolve_call_list cx ~trace reason_op args ( + ResolveSpreadsToCustomFunCall (mk_id (), kind, tout)) + | CustomFunT (reason, _), _ when function_like_op u -> rec_flow cx trace (DefT (reason, AnyFunT), u) - (*********************************************) (* object types deconstruct into their parts *) (*********************************************) @@ -9951,6 +9942,17 @@ and react_kit = ~filter_maybe and custom_fun_call cx trace reason_op kind args spread_arg tout = match kind with + | Compose reverse -> + let tin = mk_tvar cx reason_op in + let tvar = mk_tvar cx reason_op in + run_compose cx trace reverse args spread_arg tin tvar; + let funt = FunT ( + dummy_static reason_op, + dummy_prototype, + mk_functiontype [tin] ~rest_param:None ~def_reason:reason_op tvar + ) in + rec_flow_t cx trace (DefT (reason_op, funt), tout) + | ReactCreateElement -> (match args with (* React.createElement(component) *) | component::[] -> @@ -10043,6 +10045,87 @@ and custom_fun_call cx trace reason_op kind args spread_arg tout = match kind wi | DebugPrint -> failwith "implemented elsewhere" +(* Creates the appropriate constraints for the compose() function and its + * reversed variant. *) +and run_compose cx trace reverse fns spread_fn tin tout = + match reverse, fns, spread_fn with + (* Call the tail functions in our array first and call our head function + * last after that. *) + | false, fn::fns, _ -> + let reason = reason_of_t fn in + let tvar = mk_tvar_where cx reason (fun tvar -> + run_compose cx trace reverse fns spread_fn tin tvar) in + rec_flow cx trace (fn, + CallT (reason, mk_functioncalltype [Arg tvar] tout)) + + (* If the compose function is reversed then we want to call the tail + * functions in our array after we call the head function. *) + | true, fn::fns, _ -> + let reason = reason_of_t fn in + let tvar = mk_tvar_where cx reason (fun tvar -> + rec_flow cx trace (fn, + CallT (reason, mk_functioncalltype [Arg tin] tvar))) in + run_compose cx trace reverse fns spread_fn tvar tout + + (* If there are no functions and no spread function then we are an identity + * function. *) + | _, [], None -> + rec_flow_t cx trace (tin, tout) + + (* Correctly implementing spreads of unknown arity for the compose function + * is a little tricky. Let's look at a couple of cases. + * + * const fn = (x: number): string => x.toString(); + * declare var fns: Array; + * const x = 42; + * compose(...fns)(x); + * + * This would be invalid. We could have 0 or 1 fn in our fns array, but 2 fn + * would be wrong because string is incompatible with number. It breaks down + * as such: + * + * 1. x = 42 + * 2. fn(x) = '42' + * 3. fn(fn(x)) is an error because '42' is not a number. + * + * To get an error in this case we would only need to call the spread + * argument twice. Now let's look at a case where things get recursive: + * + * type Fn = (O) => $PropertyType; + * declare var fns: Array; + * const x = { p: { p: 42 } }; + * compose(...fns)(x); + * + * 1. x = { p: { p: 42 } } + * 2. fn(x) = { p: 42 } + * 3. fn(fn(x)) = 42 + * 4. fn(fn(fn(x))) throws an error because the p property is not in 42. + * + * Here we would need to call fn 3 times before getting an error. Now + * consider: + * + * type Fn = (O) => $PropertyType; + * declare var fns: Array; + * type X = { p: X }; + * declare var x: X; + * compose(...fns)(x); + * + * This is valid. + * + * To implement spreads in compose functions we first add a constraint based + * on tin and tout assuming that the spread is empty. Then we emit recursive + * constraints: + * + * spread_fn(tin) ~> tout + * spread_fn(tout) ~> tin + * + * The implementation of Flow should be able to terminate these recursive + * constraints. If it doesn't then we have a bug. *) + | _, [], Some spread_fn -> + run_compose cx trace reverse [] None tin tout; + run_compose cx trace reverse [spread_fn] None tin tout; + run_compose cx trace reverse [spread_fn] None tout tin + and object_spread = let open ObjectSpread in diff --git a/src/typing/type.ml b/src/typing/type.ml index 819e7071642..276406c53b8 100644 --- a/src/typing/type.ml +++ b/src/typing/type.ml @@ -896,6 +896,9 @@ module rec TypeTerm : sig | ObjectGetPrototypeOf | ObjectSetPrototypeOf + (* common community functions *) + | Compose of bool + (* 3rd party libs *) | ReactPropType of React.PropType.t | ReactCreateClass diff --git a/src/typing/type_annotation.ml b/src/typing/type_annotation.ml index 7dfe252839f..6f2c503677c 100644 --- a/src/typing/type_annotation.ml +++ b/src/typing/type_annotation.ml @@ -438,6 +438,11 @@ let rec convert cx tparams_map = Ast.Type.(function | "Object$SetPrototypeOf" -> mk_custom_fun cx loc typeParameters ObjectSetPrototypeOf + | "$Compose" -> + mk_custom_fun cx loc typeParameters (Compose false) + | "$ComposeReverse" -> + mk_custom_fun cx loc typeParameters (Compose true) + | "React$PropType$Primitive" -> check_type_param_arity cx loc typeParameters 1 (fun () -> let t = convert_type_params () |> List.hd in diff --git a/src/typing/type_normalizer.ml b/src/typing/type_normalizer.ml index 66a7c447306..c72bb4eff6d 100644 --- a/src/typing/type_normalizer.ml +++ b/src/typing/type_normalizer.ml @@ -471,6 +471,7 @@ let rec normalize_type_impl cx ids t = match t with | FunProtoT _ | ExtendsT (_, _, _, _) + | CustomFunT (_, Compose _) -> (** TODO **) failwith (spf "Unsupported type in normalize_type_impl: %s" (string_of_ctor t)) diff --git a/tests/compose/.flowconfig b/tests/compose/.flowconfig new file mode 100644 index 00000000000..7fec1930785 --- /dev/null +++ b/tests/compose/.flowconfig @@ -0,0 +1,2 @@ +[libs] +lib/ diff --git a/tests/compose/basic.js b/tests/compose/basic.js new file mode 100644 index 00000000000..c64750f1bae --- /dev/null +++ b/tests/compose/basic.js @@ -0,0 +1,18 @@ +// @flow + +declare var compose: $Compose; +declare var composeReverse: $ComposeReverse; + +(compose(n => n.toString())(42): empty); // Error: string ~> empty + +(composeReverse(n => n.toString())(42): empty); // Error: string ~> empty + +(compose( + n => n * 5, // Error: string cannot be multiplied. + n => n.toString(), +)(42): empty); // Error: number ~> empty + +(composeReverse( + n => n * 5, // OK + n => n.toString(), +)(42): empty); // Error: string ~> empty diff --git a/tests/compose/compose.exp b/tests/compose/compose.exp new file mode 100644 index 00000000000..ddccee0ab9e --- /dev/null +++ b/tests/compose/compose.exp @@ -0,0 +1,122 @@ +Error: basic.js:6 + 6: (compose(n => n.toString())(42): empty); // Error: string ~> empty + ^ number. Could not resolve name + +Error: basic.js:8 + 8: (composeReverse(n => n.toString())(42): empty); // Error: string ~> empty + ^ number. Could not resolve name + +Error: basic.js:10 + v------- + 10: (compose( + 11: n => n * 5, // Error: string cannot be multiplied. + 12: n => n.toString(), + 13: )(42): empty); // Error: number ~> empty + ----^ number. This type is incompatible with + 13: )(42): empty); // Error: number ~> empty + ^^^^^ empty + +Error: basic.js:12 + 12: n => n.toString(), + ^ number. Could not resolve name + +Error: basic.js:17 + 17: n => n.toString(), + ^ number. Could not resolve name + +Error: recompose.js:23 + 23: c: Math.round(props.p), // Error: string ~> number + ^^^^ identifier `Math`. Could not resolve name + +Error: spread.js:8 + v------- + 8: (compose( + 9: ...fns1, + 10: )(42): empty); // Error: number ~> empty + ----^ number. This type is incompatible with + 10: )(42): empty); // Error: number ~> empty + ^^^^^ empty + +Error: spread.js:12 + v------- + 12: (compose( + 13: ...fns1, // Error: string ~> number + 14: )('foo'): empty); // Error: string ~> empty and number ~> empty + -------^ number. This type is incompatible with + 14: )('foo'): empty); // Error: string ~> empty and number ~> empty + ^^^^^ empty + +Error: spread.js:12 + v------- + 12: (compose( + 13: ...fns1, // Error: string ~> number + 14: )('foo'): empty); // Error: string ~> empty and number ~> empty + -------^ string. This type is incompatible with + 14: )('foo'): empty); // Error: string ~> empty and number ~> empty + ^^^^^ empty + +Error: spread.js:13 + 13: ...fns1, // Error: string ~> number + ^^^^ string. This type is incompatible with the expected param type of + 4: declare var fns1: Array<(number) => number>; + ^^^^^^ number + +Error: spread.js:16 + v------- + 16: (compose( + 17: ...fns2, // Error: string ~> number + 18: )(42): empty); // Error: number ~> empty and string ~> empty + ----^ number. This type is incompatible with + 18: )(42): empty); // Error: number ~> empty and string ~> empty + ^^^^^ empty + +Error: spread.js:16 + v------- + 16: (compose( + 17: ...fns2, // Error: string ~> number + 18: )(42): empty); // Error: number ~> empty and string ~> empty + ----^ string. This type is incompatible with + 18: )(42): empty); // Error: number ~> empty and string ~> empty + ^^^^^ empty + +Error: spread.js:17 + 17: ...fns2, // Error: string ~> number + ^^^^ array + 17: ...fns2, // Error: string ~> number + ^^^^ string. This type is incompatible with the expected param type of + 5: declare var fns2: Array<(number) => string>; + ^^^^^^ number + +Error: spread.js:21 + v------- + 21: (compose( + 22: ...fns3, // Error: Cannot get p on number + 23: )(x1): empty); // Error: number ~> empty and object ~> empty + ----^ number. This type is incompatible with + 23: )(x1): empty); // Error: number ~> empty and object ~> empty + ^^^^^ empty + +Error: spread.js:21 + v------- + 21: (compose( + 22: ...fns3, // Error: Cannot get p on number + 23: )(x1): empty); // Error: number ~> empty and object ~> empty + ----^ object literal. This type is incompatible with + 23: )(x1): empty); // Error: number ~> empty and object ~> empty + ^^^^^ empty + +Error: spread.js:22 + 22: ...fns3, // Error: Cannot get p on number + ^^^^ number. Could not resolve name + +Error: spread.js:27 + v-------- + 27: ((compose( + 28: ...fns3, + 29: ))(x2): empty); // Error: object ~> empty + -----^ object type. This type is incompatible with + 29: ))(x2): empty); // Error: object ~> empty + ^^^^^ empty + + +Found 17 errors diff --git a/tests/compose/lib/recompose.js b/tests/compose/lib/recompose.js new file mode 100644 index 00000000000..4b8a235711d --- /dev/null +++ b/tests/compose/lib/recompose.js @@ -0,0 +1,3 @@ +declare module 'recompose' { + declare export var compose: $Compose; +} diff --git a/tests/compose/recompose.js b/tests/compose/recompose.js new file mode 100644 index 00000000000..f6b919a2ab1 --- /dev/null +++ b/tests/compose/recompose.js @@ -0,0 +1,25 @@ +/** + * @flow + * + * This test was taken from: + * https://github.com/istarkov/flow-compose-error + */ + +import { compose } from 'recompose'; + +// shared code between bad/good Compose +type Comp = (a: A) => void; +type HOC = (a: Comp) => Comp; + +function myEnhancer(mapper: B => A): HOC { + return (comp: Comp) => (props: B) => comp(mapper(props)); +} + +const enhancer: HOC<*, { p: number, e: string }> = compose( + myEnhancer(props => ({ + p: `${props.p * 3}`, + })), + myEnhancer(props => ({ + c: Math.round(props.p), // Error: string ~> number + })) +); diff --git a/tests/compose/spread.js b/tests/compose/spread.js new file mode 100644 index 00000000000..fa0be8869e5 --- /dev/null +++ b/tests/compose/spread.js @@ -0,0 +1,29 @@ +// @flow + +declare var compose: $Compose; +declare var fns1: Array<(number) => number>; +declare var fns2: Array<(number) => string>; +declare var fns3: Array<(O) => $PropertyType>; + +(compose( + ...fns1, +)(42): empty); // Error: number ~> empty + +(compose( + ...fns1, // Error: string ~> number +)('foo'): empty); // Error: string ~> empty and number ~> empty + +(compose( + ...fns2, // Error: string ~> number +)(42): empty); // Error: number ~> empty and string ~> empty + +const x1 = { p: { p: { p: { p: 42 } } } }; +(compose( + ...fns3, // Error: Cannot get p on number +)(x1): empty); // Error: number ~> empty and object ~> empty + +type X2 = { p: X2 }; +declare var x2: X2; +((compose( + ...fns3, +))(x2): empty); // Error: object ~> empty