Skip to content

Commit

Permalink
[flow][ref-as-prop] Allow arbitrary ref prop in component type annota…
Browse files Browse the repository at this point in the history
…tion

Summary:
In this diff, we make further progress towards full ref-as-prop support. We will now allow the ref prop in the component type annotation to have arbitrary types, while the ref prop for component declarations are still validated to be a subtype of `React.RefSetter`.

This is important to allow the following code to not error:

```
declare function HOC<R>(C: component(ref?: R)): component(ref?: R);
declare component ComponentNoRefProp();
HOC(ComponentNoRefProp) // return type is component(ref?: empty)
```

Previously, the code above is often in the form of

```
declare function HOC<I>(C: component(ref?: React.RefSetter<I>)): component(ref?: React.RefSetter<I>);
declare component ComponentNoRefProp();
HOC(ComponentNoRefProp) // return type is component(ref?: React.RefSetter<void>)
```

which is a leftover of the world filled with `React.AbstractComponent`. We want to get rid of these `React.RefSetter<void>` which really should be `empty` to indicate the absence of the ref prop.

Changelog: [fix] In component type annotation, the ref prop can now have any type.

Reviewed By: jbrown215

Differential Revision: D70508002

fbshipit-source-id: f9d3e9db7503a913e1c56305a6a1c47d9825bfcf
  • Loading branch information
SamChou19815 authored and facebook-github-bot committed Mar 3, 2025
1 parent f122db3 commit e944391
Show file tree
Hide file tree
Showing 11 changed files with 37 additions and 128 deletions.
31 changes: 17 additions & 14 deletions src/typing/component_params.ml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ module type S = sig

val config_and_instance :
Context.t ->
in_annotation:bool ->
config_reason:Reason.reason ->
instance_reason:Reason.reason ->
Types.t ->
Expand Down Expand Up @@ -72,7 +73,8 @@ module Make

let add_rest r x = { x with rest = Some r }

let config_and_instance cx ~config_reason ~instance_reason { params_rev; rest; reconstruct = _ } =
let config_and_instance
cx ~in_annotation ~config_reason ~instance_reason { params_rev; rest; reconstruct = _ } =
let (pmap, ref_prop) =
List.fold_left
(fun (acc, ref_prop) p ->
Expand Down Expand Up @@ -126,20 +128,21 @@ module Make
C.read_react cx key_loc;
let open Type in
let () =
let open Reason in
let reason_op = mk_reason RReactRef key_loc in
let u =
Flow_js.get_builtin_react_typeapp
if not in_annotation then
let open Reason in
let reason_op = mk_reason RReactRef key_loc in
let u =
Flow_js.get_builtin_react_typeapp
cx
reason_op
Flow_intermediate_error_types.ReactModuleForReactRefSetterType
[AnyT.error reason_op]
in
Context.add_post_inference_subtyping_check
cx
reason_op
Flow_intermediate_error_types.ReactModuleForReactRefSetterType
[AnyT.error reason_op]
in
Context.add_post_inference_subtyping_check
cx
ref_prop
(Op (DeclareComponentRef { op = reason_op }))
u
ref_prop
(Op (DeclareComponentRef { op = reason_op }))
u
in
ComponentInstanceAvailableAsRefSetterProp ref_prop
in
Expand Down
6 changes: 4 additions & 2 deletions src/typing/component_sig.ml
Original file line number Diff line number Diff line change
Expand Up @@ -122,11 +122,13 @@ module Make
let body_ast = B.eval cx reason_cmp renders_t body in
(params_ast, body_ast)

let component_type cx _component_loc x =
let component_type cx ~in_annotation x =
let { T.reason; tparams; cparams; renders_t; id_opt; _ } = x in
let config_reason = update_desc_reason (fun desc -> RPropsOfComponent desc) reason in
let instance_reason = update_desc_reason (fun desc -> RInstanceOfComponent desc) reason in
let (config, instance) = F.config_and_instance cx ~config_reason ~instance_reason cparams in
let (config, instance) =
F.config_and_instance cx ~in_annotation ~config_reason ~instance_reason cparams
in
let component_kind =
match id_opt with
| None -> Structural
Expand Down
2 changes: 1 addition & 1 deletion src/typing/component_sig_intf.ml
Original file line number Diff line number Diff line change
Expand Up @@ -33,5 +33,5 @@ module type S = sig

val toplevels : Context.t -> t -> component_params_tast * (ALoc.t * Type.t) BodyConfig.body

val component_type : Context.t -> ALoc.t -> t -> Type.t
val component_type : Context.t -> in_annotation:bool -> t -> Type.t
end
2 changes: 1 addition & 1 deletion src/typing/env_resolution.ml
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,7 @@ let resolve_annotated_component cx scope_kind reason tparams_map component_loc c
in
let cache = Context.node_cache cx in
Node_cache.set_component_sig cache sig_loc sig_data;
Statement.Component_declaration_sig.component_type cx component_loc component_sig
Statement.Component_declaration_sig.component_type cx ~in_annotation:false component_sig
end

let rec binding_has_annot = function
Expand Down
5 changes: 3 additions & 2 deletions src/typing/type_annotation.ml
Original file line number Diff line number Diff line change
Expand Up @@ -3143,8 +3143,9 @@ module Make (Statement : Statement_sig.S) : Type_annotation_sig.S = struct
id_opt;
}
in
let loc = loc_of_reason reason in
let t = Component_type_sig.component_type env.cx loc sig_ in
let t =
Component_type_sig.component_type env.cx ~in_annotation:(Base.Option.is_none id_opt) sig_
in
(t, tparam_asts, params_ast, renders_ast)

let mk_super env loc c targs =
Expand Down
2 changes: 1 addition & 1 deletion tests/component_syntax/annotation.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ const optBad = <OptionalRest foo="bad" />; // ERROR
declare var Duplicate: component(x: number, ...{x: number}); // error
<Duplicate x={1} />;

declare var InlineRef: component(ref: number); // error
declare var InlineRef: component(ref: number); // ok: ref type in componenent type can be arbitrary
<InlineRef ref={1} />; // error: string and number refs are still not allowed

declare var SpreadRef: component(...p: {ref: number}); // error
Expand Down
26 changes: 1 addition & 25 deletions tests/component_syntax/component_syntax.exp
Original file line number Diff line number Diff line change
Expand Up @@ -154,30 +154,6 @@ References:
^ [2]


Error ---------------------------------------------------------------------------------------------- annotation.js:48:34

Cannot declare ref because: [incompatible-type]
- Either number [1] is incompatible with object type [2].
- Or number [1] is incompatible with function type [3].

The `ref` parameter must be a subtype of `React.RefSetter`.

annotation.js:48:34
48| declare var InlineRef: component(ref: number); // error
^^^

References:
annotation.js:48:39
48| declare var InlineRef: component(ref: number); // error
^^^^^^ [1]
<BUILTINS>/react.js:113:5
113| | { -current: T | null, ... }
^^^^^^^^^^^^^^^^^^^^^^^^^^^ [2]
<BUILTINS>/react.js:114:6
114| | ((T | null) => mixed)
^^^^^^^^^^^^^^^^^^^ [3]


Error ---------------------------------------------------------------------------------------------- annotation.js:51:40

Components do not support ref properties [1] within spreads [2] [invalid-component-prop]
Expand Down Expand Up @@ -3700,7 +3676,7 @@ Strict mode function may not have duplicate parameter names



Found 239 errors
Found 238 errors

Only showing the most relevant union/intersection branches.
To see all branches, re-run Flow with --show-all-branches
2 changes: 1 addition & 1 deletion tests/component_type/class_to_component_type.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,5 +12,5 @@ Foo as component( // error: Foo ~> string
);
Foo as component( // error again due to bad ref
bar: string,
ref: ImNotARefSetter // error: bad ref
ref: ImNotARefSetter // ok: ref prop in component type can be arbitrary
);
82 changes: 3 additions & 79 deletions tests/component_type/component_type.exp
Original file line number Diff line number Diff line change
Expand Up @@ -67,33 +67,7 @@ Cannot cast `Foo` to component because: [incompatible-cast]

References:
class_to_component_type.js:15:10
15| ref: ImNotARefSetter // error: bad ref
^^^^^^^^^^^^^^^ [1]
<BUILTINS>/react.js:113:5
113| | { -current: T | null, ... }
^^^^^^^^^^^^^^^^^^^^^^^^^^^ [2]
<BUILTINS>/react.js:114:6
114| | ((T | null) => mixed)
^^^^^^^^^^^^^^^^^^^ [3]


Error ---------------------------------------------------------------------------------- class_to_component_type.js:15:5

Cannot declare ref because: [incompatible-type]
- Either `ImNotARefSetter` [1] is not a subtype of object type [2]. Class instances are not subtypes of object types;
consider rewriting object type [2] as an interface.
- Or `ImNotARefSetter` [1] is incompatible with function type [3]. Non-callable objects are not compatible with
functions.

The `ref` parameter must be a subtype of `React.RefSetter`.

class_to_component_type.js:15:5
15| ref: ImNotARefSetter // error: bad ref
^^^

References:
class_to_component_type.js:15:10
15| ref: ImNotARefSetter // error: bad ref
15| ref: ImNotARefSetter // ok: ref prop in component type can be arbitrary
^^^^^^^^^^^^^^^ [1]
<BUILTINS>/react.js:113:5
113| | { -current: T | null, ... }
Expand Down Expand Up @@ -668,33 +642,7 @@ Cannot cast `Foo` to component because: [incompatible-cast]

References:
function_to_component_type.js:14:10
14| ref: ImNotARefSetter // error: bad ref
^^^^^^^^^^^^^^^ [1]
<BUILTINS>/react.js:113:5
113| | { -current: T | null, ... }
^^^^^^^^^^^^^^^^^^^^^^^^^^^ [2]
<BUILTINS>/react.js:114:6
114| | ((T | null) => mixed)
^^^^^^^^^^^^^^^^^^^ [3]


Error ------------------------------------------------------------------------------- function_to_component_type.js:14:5

Cannot declare ref because: [incompatible-type]
- Either `ImNotARefSetter` [1] is not a subtype of object type [2]. Class instances are not subtypes of object types;
consider rewriting object type [2] as an interface.
- Or `ImNotARefSetter` [1] is incompatible with function type [3]. Non-callable objects are not compatible with
functions.

The `ref` parameter must be a subtype of `React.RefSetter`.

function_to_component_type.js:14:5
14| ref: ImNotARefSetter // error: bad ref
^^^

References:
function_to_component_type.js:14:10
14| ref: ImNotARefSetter // error: bad ref
14| ref: ImNotARefSetter // ok: ref prop in component type can be arbitrary
^^^^^^^^^^^^^^^ [1]
<BUILTINS>/react.js:113:5
113| | { -current: T | null, ... }
Expand Down Expand Up @@ -931,30 +879,6 @@ References:
^^^^^ [2]


Error -------------------------------------------------------------------------- ref_prop_implicit_instantiation.js:8:51

Cannot declare ref because: [incompatible-type]
- Either mixed [1] is incompatible with object type [2].
- Or mixed [1] is incompatible with function type [3].

The `ref` parameter must be a subtype of `React.RefSetter`.

ref_prop_implicit_instantiation.js:8:51
8| declare function extractRefSetter<S>(c: component(ref: S)): S; // error: bad ref
^^^

References:
ref_prop_implicit_instantiation.js:8:56
8| declare function extractRefSetter<S>(c: component(ref: S)): S; // error: bad ref
^ [1]
<BUILTINS>/react.js:113:5
113| | { -current: T | null, ... }
^^^^^^^^^^^^^^^^^^^^^^^^^^^ [2]
<BUILTINS>/react.js:114:6
114| | ((T | null) => mixed)
^^^^^^^^^^^^^^^^^^^ [3]


Error -------------------------------------------------------------------------- ref_prop_implicit_instantiation.js:11:1

Cannot cast `refSetter` to empty because function type [1] is incompatible with empty [2]. [incompatible-cast]
Expand Down Expand Up @@ -1068,7 +992,7 @@ expression. [signature-verification-failure]



Found 57 errors
Found 54 errors

Only showing the most relevant union/intersection branches.
To see all branches, re-run Flow with --show-all-branches
2 changes: 1 addition & 1 deletion tests/component_type/function_to_component_type.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,5 @@ Foo as component( // error: void ~> string
);
Foo as component( // error again due to bad ref
bar: string,
ref: ImNotARefSetter // error: bad ref
ref: ImNotARefSetter // ok: ref prop in component type can be arbitrary
);
5 changes: 4 additions & 1 deletion tests/component_type/ref_prop_implicit_instantiation.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ const instance = extractInstance(C);
instance as Promise<string>; // ok
instance as empty; // error

declare function extractRefSetter<S>(c: component(ref: S)): S; // error: bad ref
declare function extractRefSetter<S>(c: component(ref: S)): S; // ok: ref prop in component type can be arbitrary
const refSetter = extractRefSetter(C); // ok
refSetter as React.RefSetter<Promise<string>>; // ok
refSetter as empty; // error

declare component ComponentNoRef();
extractRefSetter(ComponentNoRef) as empty; // ok

0 comments on commit e944391

Please sign in to comment.