From 9c45ed6e54275ea735e75addcf12a50bf7fe2d9f Mon Sep 17 00:00:00 2001 From: Jordan Brown Date: Tue, 2 Apr 2024 03:49:05 -0700 Subject: [PATCH] Preserve Opaque Types when evaluating ReactDRO destructor Summary: The ReactDRO destructor adds a flag to all object types that make that object deeply-read-only for local analysis. This is unique among destructors in that it isn't really *transforming* the type as it is *marking* the type. Typically, we handle OpaqueTs in destructors by using their underlying type or otherwise falling back on behavior in flow_js.ml's larger pattern match when the underlying type is not exposable. For destructors that transform this makes sense: there's no sense in keeping around an opaque type because the opaque type is undergoing a transformation that would make it fundamentally different than its original definition. There's no sense in making that type opaque. But for ReactDRO, we don't actually want to lose the opaque type, we just want to mark the underlying_t and super_t with the ReactDRO marker. It would make sense to preserve the opacity, and a DRO OpaqueT should be compatible with its non-DRO equivalent, since ReactDRO is just for local analysis. This diff solves the issue by preserving opacity when evaluating ReactDRO on an opaque type. We have to go one extra step of forcing the evaluation of ReactDRO applied to the underlying_t and super_t because our refinement machinery expects to be able to inspect the underlying_t and super_t of opaque types. Without the change to eagerly evaluate we get ~50 errors matching the conditional.js test case added. Changelog: [fix] Fixed a behavior where component syntax would transform opaque types into their underlying types. Reviewed By: SamChou19815 Differential Revision: D55575945 fbshipit-source-id: 64a6ca0febf717f6aa4e047e20a8a052f520abf1 --- src/typing/flow_js.ml | 115 +++++++++++++----- src/typing/type_subst.ml | 4 + src/typing/type_subst.mli | 3 + .../.flowconfig | 4 + .../conditional.js | 7 ++ .../non-local.js | 9 ++ .../opaque.js | 11 ++ ...types_and_component_syntax_destructors.exp | 19 +++ 8 files changed, 139 insertions(+), 33 deletions(-) create mode 100644 tests/opaque_types_and_component_syntax_destructors/.flowconfig create mode 100644 tests/opaque_types_and_component_syntax_destructors/conditional.js create mode 100644 tests/opaque_types_and_component_syntax_destructors/non-local.js create mode 100644 tests/opaque_types_and_component_syntax_destructors/opaque.js create mode 100644 tests/opaque_types_and_component_syntax_destructors/opaque_types_and_component_syntax_destructors.exp diff --git a/src/typing/flow_js.ml b/src/typing/flow_js.ml index a2ce170f125..84ad001e1be 100644 --- a/src/typing/flow_js.ml +++ b/src/typing/flow_js.ml @@ -7477,6 +7477,19 @@ struct let void = VoidT.make reason in destruct_union ?f reason [t; void] upper in + let destruct_and_preserve_opaque_t r ({ underlying_t; super_t; _ } as opaquetype) = + let eval_t t = + let tvar = Tvar.mk_no_wrap cx reason in + (* We have to eagerly evaluate these destructors when possible because + * various other systems, like type_filter, expect OpaqueT underlying_t and + * super_t to be inspectable *) + eagerly_eval_destructor_if_resolved cx ~trace use_op reason t d tvar + in + let underlying_t = Base.Option.map ~f:eval_t underlying_t in + let super_t = Base.Option.map ~f:eval_t super_t in + let opaque_t = OpaqueT (r, { opaquetype with underlying_t; super_t }) in + rec_flow_t cx trace ~use_op (opaque_t, OpenT tout) + in let should_destruct_union () = match d with | ConditionalType { distributive_tparam_name; _ } -> Option.is_some distributive_tparam_name @@ -7490,9 +7503,11 @@ struct | _ -> true) | _ -> true in - (match t with - | GenericT - { bound = OpaqueT (_, { underlying_t = Some t; _ }); reason = r; id; name; no_infer } + (match (t, d) with + | ( GenericT + { bound = OpaqueT (_, { underlying_t = Some t; _ }); reason = r; id; name; no_infer }, + _ + ) when ALoc.source (loc_of_reason r) = ALoc.source (def_loc_of_reason r) -> eval_destructor cx @@ -7502,21 +7517,25 @@ struct (GenericT { bound = t; reason = r; id; name; no_infer }) d tout - | OpaqueT (r, { underlying_t = Some t; _ }) + | (OpaqueT (r, opaquetype), ReactDRO _) -> destruct_and_preserve_opaque_t r opaquetype + | (OpaqueT (r, { underlying_t = Some t; _ }), _) when ALoc.source (loc_of_reason r) = ALoc.source (def_loc_of_reason r) -> eval_destructor cx ~trace use_op reason t d tout (* Specialize TypeAppTs before evaluating them so that we can handle special cases. Like the union case below. mk_typeapp_instance will return an AnnotT which will be fully resolved using the AnnotT case above. *) - | GenericT - { - bound = - TypeAppT { reason = _; use_op = use_op_tapp; type_; targs; from_value; use_desc = _ }; - reason = reason_tapp; - id; - name; - no_infer; - } -> + | ( GenericT + { + bound = + TypeAppT + { reason = _; use_op = use_op_tapp; type_; targs; from_value; use_desc = _ }; + reason = reason_tapp; + id; + name; + no_infer; + }, + _ + ) -> let destructor = TypeDestructorT (use_op, reason, d) in let t = mk_typeapp_instance_annot @@ -7538,8 +7557,10 @@ struct destructor, UseT (use_op, OpenT tout) ) - | TypeAppT - { reason = reason_tapp; use_op = use_op_tapp; type_; targs; from_value; use_desc = _ } -> + | ( TypeAppT + { reason = reason_tapp; use_op = use_op_tapp; type_; targs; from_value; use_desc = _ }, + _ + ) -> let destructor = TypeDestructorT (use_op, reason, d) in let t = mk_typeapp_instance_annot @@ -7559,39 +7580,47 @@ struct Instead, we preserve the union by pushing down the destructor onto the branches of the unions. *) - | UnionT (r, rep) when should_destruct_union () -> + | (UnionT (r, rep), _) when should_destruct_union () -> destruct_union r (UnionRep.members rep) (UseT (unknown_use, OpenT tout)) - | GenericT { reason; bound = UnionT (_, rep); id; name; no_infer } + | (GenericT { reason; bound = UnionT (_, rep); id; name; no_infer }, _) when should_destruct_union () -> destruct_union ~f:(fun bound -> GenericT { reason = reason_of_t bound; bound; id; name; no_infer }) reason (UnionRep.members rep) (UseT (use_op, OpenT tout)) - | MaybeT (r, t) when should_destruct_union () -> + | (MaybeT (r, t), _) when should_destruct_union () -> destruct_maybe r t (UseT (unknown_use, OpenT tout)) - | GenericT { reason; bound = MaybeT (_, t); id; name; no_infer } when should_destruct_union () - -> + | (GenericT { reason; bound = MaybeT (_, t); id; name; no_infer }, _) + when should_destruct_union () -> destruct_maybe ~f:(fun bound -> GenericT { reason = reason_of_t bound; bound; id; name; no_infer }) reason t (UseT (use_op, OpenT tout)) - | OptionalT { reason = r; type_ = t; use_desc = _ } when should_destruct_union () -> + | (OptionalT { reason = r; type_ = t; use_desc = _ }, _) when should_destruct_union () -> destruct_optional r t (UseT (unknown_use, OpenT tout)) - | GenericT - { reason; bound = OptionalT { reason = _; type_ = t; use_desc = _ }; id; name; no_infer } + | ( GenericT + { + reason; + bound = OptionalT { reason = _; type_ = t; use_desc = _ }; + id; + name; + no_infer; + }, + _ + ) when should_destruct_union () -> destruct_optional ~f:(fun bound -> GenericT { reason = reason_of_t bound; bound; id; name; no_infer }) reason t (UseT (use_op, OpenT tout)) - | AnnotT (r, t, use_desc) -> + | (AnnotT (r, t, use_desc), _) -> let t = reposition_reason ~trace cx r ~use_desc t in let destructor = TypeDestructorT (use_op, reason, d) in rec_flow_t cx trace ~use_op:unknown_use (Cache.Eval.id cx t destructor, OpenT tout) - | GenericT { bound = AnnotT (_, t, use_desc); reason = r; name; id; no_infer } -> + | (GenericT { bound = AnnotT (_, t, use_desc); reason = r; name; id; no_infer }, _) -> let t = reposition_reason ~trace cx r ~use_desc t in let destructor = TypeDestructorT (use_op, reason, d) in rec_flow_t @@ -7787,6 +7816,28 @@ struct ) )) + and eagerly_eval_destructor_if_resolved cx ~trace use_op reason t d tvar = + eval_destructor cx ~trace use_op reason t d (reason, tvar); + let result = OpenT (reason, tvar) in + if + (not (Subst_name.Set.is_empty (Type_subst.free_var_finder cx t))) + || (not (Subst_name.Set.is_empty (Type_subst.free_var_finder_in_destructor cx d))) + || Tvar_resolver.has_unresolved_tvars cx t + || Tvar_resolver.has_unresolved_tvars_in_destructors cx d + then + result + else ( + Tvar_resolver.resolve cx result; + let t = singleton_concrete_type_for_inspection cx reason result in + match t with + | OpenT (_, id) -> + let (_, constraints) = Context.find_constraints cx id in + (match constraints with + | FullyResolved t -> Context.force_fully_resolved_tvar cx t + | _ -> t) + | t -> t + ) + and mk_possibly_evaluated_destructor cx use_op reason t d id = let eval_t = EvalT (t, TypeDestructorT (use_op, reason, d), id) in if Subst_name.Set.is_empty (Type_subst.free_var_finder cx eval_t) then @@ -10740,6 +10791,12 @@ struct and possible_concrete_types_for_inspection cx t = possible_concrete_types (fun ident -> ConcretizeForInspection ident) cx t + and singleton_concrete_type_for_inspection cx reason t = + match possible_concrete_types_for_inspection cx reason t with + | [] -> EmptyT.make reason + | [t] -> t + | t1 :: t2 :: ts -> UnionT (reason, UnionRep.make t1 t2 ts) + and add_specialized_callee_method_action cx trace l = function | CallM { specialized_callee; _ } | ChainM { specialized_callee; _ } -> @@ -10765,14 +10822,6 @@ module rec FlowJs : Flow_common.S = struct let react_get_config = React.get_config - (* Returns a list of concrete types after breaking up unions, maybe types, etc *) - - let singleton_concrete_type_for_inspection cx reason t = - match possible_concrete_types_for_inspection cx reason t with - | [] -> EmptyT.make reason - | [t] -> t - | t1 :: t2 :: ts -> UnionT (reason, UnionRep.make t1 t2 ts) - let possible_concrete_types_for_imports_exports = possible_concrete_types (fun ident -> ConcretizeForImportsExports ident) diff --git a/src/typing/type_subst.ml b/src/typing/type_subst.ml index d0820142095..cf941d733a8 100644 --- a/src/typing/type_subst.ml +++ b/src/typing/type_subst.ml @@ -93,6 +93,10 @@ let free_var_finder cx ?(bound = Subst_name.Set.empty) t = let { free; _ } = visitor#type_ cx Polarity.Positive { free = Subst_name.Set.empty; bound } t in free +let free_var_finder_in_destructor cx ?(bound = Subst_name.Set.empty) d = + let { free; _ } = visitor#destructor cx { free = Subst_name.Set.empty; bound } d in + free + (** Substitute bound type variables with associated types in a type. **) let new_name name fvs = diff --git a/src/typing/type_subst.mli b/src/typing/type_subst.mli index 67e5c1118c2..0ca016714a3 100644 --- a/src/typing/type_subst.mli +++ b/src/typing/type_subst.mli @@ -7,6 +7,9 @@ val free_var_finder : Context.t -> ?bound:Subst_name.Set.t -> Type.t -> Subst_name.Set.t +val free_var_finder_in_destructor : + Context.t -> ?bound:Subst_name.Set.t -> Type.destructor -> Subst_name.Set.t + val new_name : Subst_name.t -> Subst_name.Set.t -> Subst_name.t val subst : diff --git a/tests/opaque_types_and_component_syntax_destructors/.flowconfig b/tests/opaque_types_and_component_syntax_destructors/.flowconfig new file mode 100644 index 00000000000..87564423f1c --- /dev/null +++ b/tests/opaque_types_and_component_syntax_destructors/.flowconfig @@ -0,0 +1,4 @@ +[options] +component_syntax=true +no_flowlib=false +all=true diff --git a/tests/opaque_types_and_component_syntax_destructors/conditional.js b/tests/opaque_types_and_component_syntax_destructors/conditional.js new file mode 100644 index 00000000000..352576e389a --- /dev/null +++ b/tests/opaque_types_and_component_syntax_destructors/conditional.js @@ -0,0 +1,7 @@ +component Foo() { + return null; +} +component Bar(x: React.Element) { + const y: number = x && 3; // OK + return null; +} diff --git a/tests/opaque_types_and_component_syntax_destructors/non-local.js b/tests/opaque_types_and_component_syntax_destructors/non-local.js new file mode 100644 index 00000000000..114e87c39a5 --- /dev/null +++ b/tests/opaque_types_and_component_syntax_destructors/non-local.js @@ -0,0 +1,9 @@ +import {Context} from './opaque'; +import {useContext} from 'react'; +import * as React from 'react'; + +component Foo() { + const context = useContext(Context); + context as number; // ERROR + return null; +} diff --git a/tests/opaque_types_and_component_syntax_destructors/opaque.js b/tests/opaque_types_and_component_syntax_destructors/opaque.js new file mode 100644 index 00000000000..255d2e41081 --- /dev/null +++ b/tests/opaque_types_and_component_syntax_destructors/opaque.js @@ -0,0 +1,11 @@ +import {createContext, useContext} from 'react' + +export opaque type Tag = number; + +export const Context: React$Context = createContext(3); + +component Foo() { + const context = useContext(Context); + context as number; // OK + return null; +} diff --git a/tests/opaque_types_and_component_syntax_destructors/opaque_types_and_component_syntax_destructors.exp b/tests/opaque_types_and_component_syntax_destructors/opaque_types_and_component_syntax_destructors.exp new file mode 100644 index 00000000000..61f6258eca9 --- /dev/null +++ b/tests/opaque_types_and_component_syntax_destructors/opaque_types_and_component_syntax_destructors.exp @@ -0,0 +1,19 @@ +Error ------------------------------------------------------------------------------------------------- non-local.js:7:3 + +Cannot cast `context` to number because `Tag` [1] is incompatible with number [2]. [incompatible-cast] + + non-local.js:7:3 + 7| context as number; // ERROR + ^^^^^^^ + +References: + opaque.js:5:37 + 5| export const Context: React$Context = createContext(3); + ^^^ [1] + non-local.js:7:14 + 7| context as number; // ERROR + ^^^^^^ [2] + + + +Found 1 error