From 9edc9b355d71822f7a28a91cd185b2828f69d752 Mon Sep 17 00:00:00 2001 From: Cheng Lou Date: Wed, 28 Feb 2018 05:48:50 -0800 Subject: [PATCH 1/2] [Super errors] Better missing param msg Continuation of #2548 Mini usability testing result out. Apparently the previous message was too confusing, so I'm adding back the fallback type clash msg. Additionally, I'm now using `Ctype.matches`, which considers `option('a)` and `option(string)` as equal. This, along with the original code in #2548, fixes https://github.com/reasonml-community/error-message-improvement/issues/14 But since this produces false positives "missing parameters" msg for such code below: ```reason let a: int => float => option(int) = (a) => Some(""); ``` Where `option(int)` matches `option('a)` inferred from `Some("")`, I've also tweaked the msg to indicate that we're _guessing_ that it _might_ be because of missing parameters (but that we're not sure). The fallback type clash msg gives us leeway to guess. cc @cristianoc --- jscomp/super_errors/super_typecore.ml | 52 ++++++++++++++++----------- 1 file changed, 32 insertions(+), 20 deletions(-) diff --git a/jscomp/super_errors/super_typecore.ml b/jscomp/super_errors/super_typecore.ml index 28f5dfbf93..3dc37128cb 100644 --- a/jscomp/super_errors/super_typecore.ml +++ b/jscomp/super_errors/super_typecore.ml @@ -58,9 +58,10 @@ let show_extra_help ppf env trace = begin | _ -> (); end -(* given type1 is foo => bar => baz and type 2 is bar => baz, return Some(foo) *) +(* given type1 is foo => bar => baz(qux) and type 2 is bar => baz(qux), return Some(foo) *) let rec collect_missing_arguments env type1 type2 = match type1 with - | {desc=Tarrow (label, argtype, typ, _)} when Ctype.equal env true [typ] [type2] -> + (* why do we use Ctype.matches here? Please see https://github.com/BuckleScript/bucklescript/pull/2554 *) + | {desc=Tarrow (label, argtype, typ, _)} when Ctype.matches env typ type2 -> Some [(label, argtype)] | {desc=Tarrow (label, argtype, typ, _)} -> begin match collect_missing_arguments env typ type2 with @@ -153,48 +154,59 @@ let report_error env ppf = function @[@{Here's the original error message@}@]@,\ @]"; begin - let bottom_aliases_result = bottom_aliases trace in + let bottom_aliases_result = bottom_aliases trace in let missing_arguments = match bottom_aliases_result with | Some (actual, expected) -> collect_missing_arguments env actual expected | None -> assert false in - let print_arguments = + let print_arguments = Format.pp_print_list ~pp_sep:(fun ppf _ -> fprintf ppf ",@ ") (fun ppf (label, argtype) -> if label = "" then fprintf ppf "@[%a@]" type_expr argtype else fprintf ppf "@[(~%s: %a)@]" label type_expr argtype ) - in + in begin match missing_arguments with | Some [singleArgument] -> - fprintf ppf "@[@{This call is missing a final argument@} of type@ %a@]" + (* btw, you can't say "final arguments". Intermediate labeled + arguments might be the ones missing *) + fprintf ppf "@[@{This call is missing an argument@} of type@ %a@]" print_arguments [singleArgument] | Some arguments -> - fprintf ppf "@[@{This call is missing final arguments@} of type:@ %a@]" + fprintf ppf "@[@{This call is missing arguments@} of type:@ %a@]" print_arguments arguments | None -> let missing_parameters = match bottom_aliases_result with | Some (actual, expected) -> collect_missing_arguments env expected actual | None -> assert false in + + fprintf ppf "@["; + begin match missing_parameters with | Some [singleParameter] -> - fprintf ppf "@[This value seems to @{need to be wrapped in a function@ that@ takes@ an@ extra@ argument@}@ of@ type@ %a@]" - print_arguments [singleParameter] + fprintf ppf "@[This value might need to be @{wrapped in a function@ that@ takes@ an@ extra@ parameter@}@ of@ type@ %a@]@,@," + print_arguments [singleParameter]; + fprintf ppf "@[@{Here's the original error message@}@]@," | Some arguments -> - fprintf ppf "@[This value seems to @{need to be wrapped in a function that takes extra@ arguments@}@ of@ type:@ @[%a@]@]" - print_arguments arguments - | None -> - (* final fallback: show the generic type mismatch error *) - check_bs_arity_mismatch ppf trace; - super_report_unification_error ppf env trace - (function ppf -> - fprintf ppf "This has type:") - (function ppf -> - fprintf ppf "But somewhere wanted:"); - show_extra_help ppf env trace; + fprintf ppf "@[This value seems to @{need to be wrapped in a function that takes extra@ arguments@}@ of@ type:@ @[%a@]@]@,@," + print_arguments arguments; + fprintf ppf "@[@{Here's the original error message@}@]@," + | None -> () end; + + (* final fallback: show the generic type mismatch error *) + check_bs_arity_mismatch ppf trace; + super_report_unification_error ppf env trace + (function ppf -> + fprintf ppf "This has type:") + (function ppf -> + fprintf ppf "But somewhere wanted:"); + show_extra_help ppf env trace; + + fprintf ppf "@]" + end; end | Apply_non_function typ -> From fec02d0b566b0b64c39c07b161d57008a8b057de Mon Sep 17 00:00:00 2001 From: Cheng Lou Date: Wed, 28 Feb 2018 15:39:34 -0800 Subject: [PATCH 2/2] Clean up code --- jscomp/super_errors/super_typecore.ml | 139 +++++++++++++------------- 1 file changed, 68 insertions(+), 71 deletions(-) diff --git a/jscomp/super_errors/super_typecore.ml b/jscomp/super_errors/super_typecore.ml index 3dc37128cb..06d10abdd2 100644 --- a/jscomp/super_errors/super_typecore.ml +++ b/jscomp/super_errors/super_typecore.ml @@ -101,6 +101,71 @@ let check_bs_arity_mismatch ppf trace = false in ignore (traverse 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 + 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@}@]@,\ + @]" + else if Super_reason_react.is_array_wanted_reactElement trace then + fprintf ppf "@[\ + @[@{Are you passing 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@}@]@,\ + @]"; + begin + let bottom_aliases_result = bottom_aliases trace in + let missing_arguments = match bottom_aliases_result with + | Some (actual, expected) -> collect_missing_arguments env actual expected + | None -> assert false + in + let print_arguments = + Format.pp_print_list + ~pp_sep:(fun ppf _ -> fprintf ppf ",@ ") + (fun ppf (label, argtype) -> + if label = "" then fprintf ppf "@[%a@]" type_expr argtype + else fprintf ppf "@[(~%s: %a)@]" label type_expr argtype + ) + in + begin match missing_arguments with + | Some [singleArgument] -> + (* btw, you can't say "final arguments". Intermediate labeled + arguments might be the ones missing *) + fprintf ppf "@[@{This call is missing an argument@} of type@ %a@]" + print_arguments [singleArgument] + | Some arguments -> + fprintf ppf "@[@{This call is missing arguments@} of type:@ %a@]" + print_arguments arguments + | None -> + let missing_parameters = match bottom_aliases_result with + | Some (actual, expected) -> collect_missing_arguments env expected actual + | None -> assert false + in + begin match missing_parameters with + | Some [singleParameter] -> + fprintf ppf "@[This value might need to be @{wrapped in a function@ that@ takes@ an@ extra@ parameter@}@ of@ type@ %a@]@,@," + print_arguments [singleParameter]; + fprintf ppf "@[@{Here's the original error message@}@]@," + | Some arguments -> + fprintf ppf "@[This value seems to @{need to be wrapped in a function that takes extra@ arguments@}@ of@ type:@ @[%a@]@]@,@," + print_arguments arguments; + fprintf ppf "@[@{Here's the original error message@}@]@," + | None -> () + end; + (* final fallback: show the generic type mismatch error *) + check_bs_arity_mismatch ppf trace; + super_report_unification_error ppf env trace + (function ppf -> + fprintf ppf "This has type:") + (function ppf -> + fprintf ppf "But somewhere wanted:"); + show_extra_help ppf env trace; + end; + end (* taken from https://github.com/BuckleScript/ocaml/blob/d4144647d1bf9bc7dc3aadc24c25a7efa3a67915/typing/typecore.ml#L3769 *) (* modified branches are commented *) let report_error env ppf = function @@ -138,77 +203,9 @@ let report_error env ppf = function (Ident.name id) | Expr_type_clash trace -> (* modified *) - (* 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 - 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@}@]@,\ - @]" - else if Super_reason_react.is_array_wanted_reactElement trace then - fprintf ppf "@[\ - @[@{Are you passing 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@}@]@,\ - @]"; - begin - let bottom_aliases_result = bottom_aliases trace in - let missing_arguments = match bottom_aliases_result with - | Some (actual, expected) -> collect_missing_arguments env actual expected - | None -> assert false - in - let print_arguments = - Format.pp_print_list - ~pp_sep:(fun ppf _ -> fprintf ppf ",@ ") - (fun ppf (label, argtype) -> - if label = "" then fprintf ppf "@[%a@]" type_expr argtype - else fprintf ppf "@[(~%s: %a)@]" label type_expr argtype - ) - in - begin match missing_arguments with - | Some [singleArgument] -> - (* btw, you can't say "final arguments". Intermediate labeled - arguments might be the ones missing *) - fprintf ppf "@[@{This call is missing an argument@} of type@ %a@]" - print_arguments [singleArgument] - | Some arguments -> - fprintf ppf "@[@{This call is missing arguments@} of type:@ %a@]" - print_arguments arguments - | None -> - let missing_parameters = match bottom_aliases_result with - | Some (actual, expected) -> collect_missing_arguments env expected actual - | None -> assert false - in - - fprintf ppf "@["; - - begin match missing_parameters with - | Some [singleParameter] -> - fprintf ppf "@[This value might need to be @{wrapped in a function@ that@ takes@ an@ extra@ parameter@}@ of@ type@ %a@]@,@," - print_arguments [singleParameter]; - fprintf ppf "@[@{Here's the original error message@}@]@," - | Some arguments -> - fprintf ppf "@[This value seems to @{need to be wrapped in a function that takes extra@ arguments@}@ of@ type:@ @[%a@]@]@,@," - print_arguments arguments; - fprintf ppf "@[@{Here's the original error message@}@]@," - | None -> () - end; - - (* final fallback: show the generic type mismatch error *) - check_bs_arity_mismatch ppf trace; - super_report_unification_error ppf env trace - (function ppf -> - fprintf ppf "This has type:") - (function ppf -> - fprintf ppf "But somewhere wanted:"); - show_extra_help ppf env trace; - - fprintf ppf "@]" - - end; - end + fprintf ppf "@["; + print_expr_type_clash env trace ppf; + fprintf ppf "@]" | Apply_non_function typ -> (* modified *) reset_and_mark_loops typ;