From c49243c17253ee2ee50a47a038d5e10eeef818ea Mon Sep 17 00:00:00 2001 From: Cheng Lou Date: Mon, 12 Mar 2018 18:16:36 -0700 Subject: [PATCH 1/2] [Super errors] Make the second worst ReasonReact error a bit shorter The "state escape scope" one. This one shouldn't catch any false positives, so we can safely remove the original type clash error. Also, we only detected state escaping scope, not that it's actually related to ReasonReact.componentSpec. So every state escape scope error ended up giving a ReasonReact error correction tip. This is fixed now. Sorry about that. --- jscomp/super_errors/super_reason_react.ml | 51 +++++++++++++++++----- jscomp/super_errors/super_reason_react.mli | 5 ++- jscomp/super_errors/super_typecore.ml | 27 ++++++------ 3 files changed, 58 insertions(+), 25 deletions(-) diff --git a/jscomp/super_errors/super_reason_react.ml b/jscomp/super_errors/super_reason_react.ml index a7cc910a8d..4fd1aad23e 100644 --- a/jscomp/super_errors/super_reason_react.ml +++ b/jscomp/super_errors/super_reason_react.ml @@ -8,32 +8,32 @@ let rec drill_through_tlink_and_tsubst t = match t.desc with | Tlink t | Tsubst t -> drill_through_tlink_and_tsubst t - | t -> t + | _ -> t let is_weak_type_after_drilling t = match drill_through_tlink_and_tsubst t with - | Tvar _ -> true + | {desc = Tvar _} -> true | _ -> false let component_spec_weak_type_variables t = match drill_through_tlink_and_tsubst t with (* ReasonReact <=0.3.4 *) - | Tconstr ( + | {desc = Tconstr ( Pdot ((Pident {name = "ReasonReact"}), "componentSpec", _), [state; _initial_state; retained_props; _initial_retained_props; action], _ - ) -> + )} -> ( state |> is_weak_type_after_drilling, retained_props |> is_weak_type_after_drilling, action |> is_weak_type_after_drilling ) (* future ReasonReact version with retainedProps removed *) - | Tconstr ( + | {desc = Tconstr ( Pdot ((Pident {name = "ReasonReact"}), "componentSpec", _), [state; _initial_state; action], _ - ) -> + )} -> ( state |> is_weak_type_after_drilling, false, @@ -66,6 +66,7 @@ let rec get_to_bottom_of_aliases f = function end | _ -> false + let state_escape_scope = get_to_bottom_of_aliases (function (* https://github.com/BuckleScript/ocaml/blob/ddf5a739cc0978dab5e553443825791ba7b0cef9/typing/printtyp.ml?#L1348 *) (* so apparently that's the logic for detecting "the constructor out of scope" error *) @@ -74,14 +75,42 @@ let state_escape_scope = get_to_bottom_of_aliases (function | _ -> false ) +let trace_both_component_spec = get_to_bottom_of_aliases (fun (t1, t2) -> + match (t1, t2) with + | ({desc = Tconstr ( + (Pdot ((Pident {name = "ReasonReact"}), "componentSpec", _)), + ([state1; _; _; _; action1] | [state1; _; action1]), + _ + )}, + {desc = Tconstr ( + (Pdot ((Pident {name = "ReasonReact"}), "componentSpec", _)), + ([state2; _; _; _; action2] | [state2; _; action2]), + _ + )}) + -> true + | _ -> false +) + let is_array_wanted_reactElement = get_to_bottom_of_aliases (function - | ({desc = Tconstr (path1, _, _)}, {desc = Tconstr (path2, _, _)}) - when Path.last path1 = "array" && Path.last path2 = "reactElement" -> true + | ({desc = Tconstr (path1, _, _)}, + {desc = Tconstr ( + (Pdot ((Pident {name = "ReasonReact"}), "reactElement", _)), + _, + _ + )}) when Path.last path1 = "array"-> true | _ -> false ) -let is_componentSpec_wanted_reactElement = get_to_bottom_of_aliases (function - | ({desc = Tconstr (path1, _, _)}, {desc = Tconstr (path2, _, _)}) - when Path.last path1 = "componentSpec" && Path.last path2 = "reactElement" -> true +let is_component_spec_wanted_reactElement = get_to_bottom_of_aliases (function + | ({desc = Tconstr ( + (Pdot ((Pident {name = "ReasonReact"}), "componentSpec", _)), + ([state1; _; _; _; action1] | [state1; _; action1]), + _ + )}, + {desc = Tconstr ( + (Pdot ((Pident {name = "ReasonReact"}), "reactElement", _)), + _, + _ + )}) -> true | _ -> false ) diff --git a/jscomp/super_errors/super_reason_react.mli b/jscomp/super_errors/super_reason_react.mli index 353b29dd68..37af4a8ba5 100644 --- a/jscomp/super_errors/super_reason_react.mli +++ b/jscomp/super_errors/super_reason_react.mli @@ -7,8 +7,11 @@ val component_spec_weak_type_variables_in_module_type: Types.module_type -> (boo val state_escape_scope: (Types.type_expr * Types.type_expr) list -> bool (** Used by super_typecore when we detect the message "The type constructor state would escape its scope" *) +val trace_both_component_spec: (Types.type_expr * Types.type_expr) list -> bool +(** Used by super_typecore when we detect the message "The type constructor state would escape its scope" *) + val is_array_wanted_reactElement: (Types.type_expr * Types.type_expr) list -> bool (** Used by super_typecore when we detect the message "This has type array but expected reactElement" *) -val is_componentSpec_wanted_reactElement: (Types.type_expr * Types.type_expr) list -> bool +val is_component_spec_wanted_reactElement: (Types.type_expr * Types.type_expr) list -> bool (** Used by super_typecore when we detect the message "This has type componentSpec but expected reactElement" *) diff --git a/jscomp/super_errors/super_typecore.ml b/jscomp/super_errors/super_typecore.ml index b71f3cf11a..3bf89e10c2 100644 --- a/jscomp/super_errors/super_typecore.ml +++ b/jscomp/super_errors/super_typecore.ml @@ -104,25 +104,26 @@ let check_bs_arity_mismatch ppf trace = let print_expr_type_clash env trace ppf = (* this is the most frequent error. Do whatever we can to provide specific guidance to this generic error before giving up *) - if Super_reason_react.state_escape_scope trace then + if Super_reason_react.state_escape_scope trace && Super_reason_react.trace_both_component_spec trace then fprintf ppf "@[\ @[@{Is this a ReasonReact reducerComponent or component with retained props?@}@ \ - If so, is the type for state, retained props or action declared _after_ the component declaration?@ \ - Moving these types above the component declaration should resolve this!@]@,@,\ - @[@{Here's the original error message@}@]@,\ + If so, is the type for state, retained props or action declared _after_@ the component declaration?@ \ + @{Moving these types above the component declaration@} should resolve this!@]\ @]" - else if Super_reason_react.is_array_wanted_reactElement trace then + (* This one above shouldn't catch any false positives, so we can safely not display the original type clash error. *) + else begin + if Super_reason_react.is_array_wanted_reactElement trace then fprintf ppf "@[\ @[@{Did you pass an array as a ReasonReact DOM (lower-case) component's children?@}@ If not, disregard this.@ \ If so, please use `ReasonReact.createDomElement`:@ https://reasonml.github.io/reason-react/docs/en/children.html@]@,@,\ @[@{Here's the original error message@}@]@,\ @]" - else if Super_reason_react.is_componentSpec_wanted_reactElement trace then - fprintf ppf "@[\ - @[@{Did you want to create a ReasonReact element without using JSX?@}@ If not, disregard this.@ \ - If so, don't forget to wrap this value in `ReasonReact.element` yourself:@ https://reasonml.github.io/reason-react/docs/en/jsx.html#capitalized@]@,@,\ - @[@{Here's the original error message@}@]@,\ - @]"; + else if Super_reason_react.is_component_spec_wanted_reactElement trace then + fprintf ppf "@[\ + @[@{Did you want to create a ReasonReact element without using JSX?@}@ If not, disregard this.@ \ + If so, don't forget to wrap this value in `ReasonReact.element` yourself:@ https://reasonml.github.io/reason-react/docs/en/jsx.html#capitalized@]@,@,\ + @[@{Here's the original error message@}@]@,\ + @]"; begin let bottom_aliases_result = bottom_aliases trace in let missing_arguments = match bottom_aliases_result with @@ -137,7 +138,7 @@ let print_expr_type_clash env trace ppf = else fprintf ppf "@[(~%s: %a)@]" label type_expr argtype ) in - begin match missing_arguments with + match missing_arguments with | Some [singleArgument] -> (* btw, you can't say "final arguments". Intermediate labeled arguments might be the ones missing *) @@ -170,7 +171,7 @@ let print_expr_type_clash env trace ppf = (function ppf -> fprintf ppf "But somewhere wanted:"); show_extra_help ppf env trace; - end; + end end (* taken from https://github.com/BuckleScript/ocaml/blob/d4144647d1bf9bc7dc3aadc24c25a7efa3a67915/typing/typecore.ml#L3769 *) (* modified branches are commented *) From ab89b39a519bfe82a42b9f81b9ba58b5acc21128 Mon Sep 17 00:00:00 2001 From: Cheng Lou Date: Mon, 12 Mar 2018 20:21:37 -0700 Subject: [PATCH 2/2] [Super errors] Clean up trace walking logic --- jscomp/super_errors/super_reason_react.ml | 37 +++++++++++++++------- jscomp/super_errors/super_reason_react.mli | 4 +-- jscomp/super_errors/super_typecore.ml | 4 +-- 3 files changed, 29 insertions(+), 16 deletions(-) diff --git a/jscomp/super_errors/super_reason_react.ml b/jscomp/super_errors/super_reason_react.ml index 4fd1aad23e..599c22ad23 100644 --- a/jscomp/super_errors/super_reason_react.ml +++ b/jscomp/super_errors/super_reason_react.ml @@ -57,17 +57,31 @@ let component_spec_weak_type_variables_in_module_type (mty : Types.module_type) ) | _ -> [] -(* recursively drill down the types (first item is the type alias, if any. Second is the content of the alias) *) -let rec get_to_bottom_of_aliases f = function - | (_alias1, type1) :: (_alias2, type2) :: rest -> - begin match get_to_bottom_of_aliases f rest with - | false -> f (type1, type2) - | true -> true - end +(* `trace` is a funny data structure. It's an always even list of tuples. This error: + this is foo (aliased as array(int)), wanted bar (aliased as array(string)) + the incompatible part: int vs string + gives the following `trace` data structure: + [ + (foo, array(int)), + (bar, array(string)), + (_, int), + (_, string) + ] + *) +(* recursively walk the trace from right to left, calling f and checking if f matches part of the trace *) +let check_each_trace_chunk_bottom_up f = fun t -> + let t_flipped = List.rev t in + let rec check f = function + (* we flipped the trace, so instead of [t1, t2, t3, t4, ...] it's [t4, t3, ...] *) + | (_alias2, type2) :: (_alias1, type1) :: rest -> + if f (type1, type2) then true + else check f rest | _ -> false + in + check f t_flipped -let state_escape_scope = get_to_bottom_of_aliases (function +let state_escape_scope = check_each_trace_chunk_bottom_up (function (* https://github.com/BuckleScript/ocaml/blob/ddf5a739cc0978dab5e553443825791ba7b0cef9/typing/printtyp.ml?#L1348 *) (* so apparently that's the logic for detecting "the constructor out of scope" error *) | ({desc = Tconstr (p, _, _)}, {desc = Tvar _; level}) @@ -75,8 +89,7 @@ let state_escape_scope = get_to_bottom_of_aliases (function | _ -> false ) -let trace_both_component_spec = get_to_bottom_of_aliases (fun (t1, t2) -> - match (t1, t2) with +let trace_both_component_spec = check_each_trace_chunk_bottom_up (function | ({desc = Tconstr ( (Pdot ((Pident {name = "ReasonReact"}), "componentSpec", _)), ([state1; _; _; _; action1] | [state1; _; action1]), @@ -91,7 +104,7 @@ let trace_both_component_spec = get_to_bottom_of_aliases (fun (t1, t2) -> | _ -> false ) -let is_array_wanted_reactElement = get_to_bottom_of_aliases (function +let is_array_wanted_react_element = check_each_trace_chunk_bottom_up (function | ({desc = Tconstr (path1, _, _)}, {desc = Tconstr ( (Pdot ((Pident {name = "ReasonReact"}), "reactElement", _)), @@ -101,7 +114,7 @@ let is_array_wanted_reactElement = get_to_bottom_of_aliases (function | _ -> false ) -let is_component_spec_wanted_reactElement = get_to_bottom_of_aliases (function +let is_component_spec_wanted_react_element = check_each_trace_chunk_bottom_up (function | ({desc = Tconstr ( (Pdot ((Pident {name = "ReasonReact"}), "componentSpec", _)), ([state1; _; _; _; action1] | [state1; _; action1]), diff --git a/jscomp/super_errors/super_reason_react.mli b/jscomp/super_errors/super_reason_react.mli index 37af4a8ba5..297669e8c7 100644 --- a/jscomp/super_errors/super_reason_react.mli +++ b/jscomp/super_errors/super_reason_react.mli @@ -10,8 +10,8 @@ val state_escape_scope: (Types.type_expr * Types.type_expr) list -> bool val trace_both_component_spec: (Types.type_expr * Types.type_expr) list -> bool (** Used by super_typecore when we detect the message "The type constructor state would escape its scope" *) -val is_array_wanted_reactElement: (Types.type_expr * Types.type_expr) list -> bool +val is_array_wanted_react_element: (Types.type_expr * Types.type_expr) list -> bool (** Used by super_typecore when we detect the message "This has type array but expected reactElement" *) -val is_component_spec_wanted_reactElement: (Types.type_expr * Types.type_expr) list -> bool +val is_component_spec_wanted_react_element: (Types.type_expr * Types.type_expr) list -> bool (** Used by super_typecore when we detect the message "This has type componentSpec but expected reactElement" *) diff --git a/jscomp/super_errors/super_typecore.ml b/jscomp/super_errors/super_typecore.ml index 3bf89e10c2..a9f65d8401 100644 --- a/jscomp/super_errors/super_typecore.ml +++ b/jscomp/super_errors/super_typecore.ml @@ -112,13 +112,13 @@ let print_expr_type_clash env trace ppf = @]" (* This one above shouldn't catch any false positives, so we can safely not display the original type clash error. *) else begin - if Super_reason_react.is_array_wanted_reactElement trace then + if Super_reason_react.is_array_wanted_react_element trace then fprintf ppf "@[\ @[@{Did you pass an array as a ReasonReact DOM (lower-case) component's children?@}@ If not, disregard this.@ \ If so, please use `ReasonReact.createDomElement`:@ https://reasonml.github.io/reason-react/docs/en/children.html@]@,@,\ @[@{Here's the original error message@}@]@,\ @]" - else if Super_reason_react.is_component_spec_wanted_reactElement trace then + else if Super_reason_react.is_component_spec_wanted_react_element trace then fprintf ppf "@[\ @[@{Did you want to create a ReasonReact element without using JSX?@}@ If not, disregard this.@ \ If so, don't forget to wrap this value in `ReasonReact.element` yourself:@ https://reasonml.github.io/reason-react/docs/en/jsx.html#capitalized@]@,@,\