From 8b1ca403b044329ba2c3e03eb08eede221f529f4 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Thu, 22 May 2025 11:15:08 +0200 Subject: [PATCH 1/9] improve array <-> tuple error message by looking at what the user actually wrote --- compiler/bsc/rescript_compiler_main.ml | 20 ++++++++ compiler/ml/error_message_utils.ml | 48 ++++++++++++++++++- compiler/ml/typecore.ml | 14 +++--- compiler/ml/typecore.mli | 2 +- ...array_literal_passed_to_tuple.res.expected | 14 ++++++ .../array_var_passed_to_tuple.res.expected | 13 +++++ .../array_literal_passed_to_tuple.res | 5 ++ .../fixtures/array_var_passed_to_tuple.res | 7 +++ 8 files changed, 114 insertions(+), 9 deletions(-) create mode 100644 tests/build_tests/super_errors/expected/array_literal_passed_to_tuple.res.expected create mode 100644 tests/build_tests/super_errors/expected/array_var_passed_to_tuple.res.expected create mode 100644 tests/build_tests/super_errors/fixtures/array_literal_passed_to_tuple.res create mode 100644 tests/build_tests/super_errors/fixtures/array_var_passed_to_tuple.res diff --git a/compiler/bsc/rescript_compiler_main.ml b/compiler/bsc/rescript_compiler_main.ml index f9113c40b6..a7d9190b60 100644 --- a/compiler/bsc/rescript_compiler_main.ml +++ b/compiler/bsc/rescript_compiler_main.ml @@ -12,6 +12,26 @@ let absname = ref false +external to_comment : Res_comment.t -> Error_message_utils.comment = "%identity" +external from_comment : Error_message_utils.comment -> Res_comment.t + = "%identity" + +let () = + Error_message_utils.parse_source := + fun source -> + let res = + Res_driver.parse_implementation_from_source ~for_printer:false + ~display_filename:"" ~source + in + (res.parsetree, res.comments |> List.map to_comment) + +let () = + Error_message_utils.reprint_source := + fun parsetree comments -> + Res_printer.print_implementation parsetree + ~comments:(comments |> List.map from_comment) + ~width:80 + let set_abs_input_name sourcefile = let sourcefile = if !absname && Filename.is_relative sourcefile then diff --git a/compiler/ml/error_message_utils.ml b/compiler/ml/error_message_utils.ml index 31d8cafc3b..d652456350 100644 --- a/compiler/ml/error_message_utils.ml +++ b/compiler/ml/error_message_utils.ml @@ -1,6 +1,12 @@ type extract_concrete_typedecl = Env.t -> Types.type_expr -> Path.t * Path.t * Types.type_declaration +type comment +let parse_source : (string -> Parsetree.structure * comment list) ref = + ref (fun _ -> ([], [])) +let reprint_source : (Parsetree.structure -> comment list -> string) ref = + ref (fun _ _ -> "") + type type_clash_statement = FunctionCall type type_clash_context = | SetRecordField @@ -62,7 +68,14 @@ let is_record_type ~extract_concrete_typedecl ~env ty = | _ -> false with _ -> false -let print_extra_type_clash_help ~extract_concrete_typedecl ~env ppf +let extract_location_string ~src (loc : Location.t) = + let start_pos = loc.loc_start in + let end_pos = loc.loc_end in + let start_offset = start_pos.pos_cnum in + let end_offset = end_pos.pos_cnum in + String.sub src start_offset (end_offset - start_offset) + +let print_extra_type_clash_help ~extract_concrete_typedecl ~env loc ppf (bottom_aliases : (Types.type_expr * Types.type_expr) option) type_clash_context = match (type_clash_context, bottom_aliases) with @@ -185,6 +198,39 @@ let print_extra_type_clash_help ~extract_concrete_typedecl ~env ppf | _, Some ({Types.desc = Tconstr (p1, _, _)}, _) when Path.same p1 Predef.path_promise -> fprintf ppf "\n\n - Did you mean to await this promise before using it?\n" + | _, Some ({Types.desc = Tconstr (p1, _, _)}, {Types.desc = Ttuple _}) + when Path.same p1 Predef.path_array -> + let src = Ext_io.load_file loc.Location.loc_start.pos_fname in + let sub_src = extract_location_string ~src loc in + let parsed, comments = !parse_source sub_src in + let suggested_rewrite = + match parsed with + | [ + ({ + Parsetree.pstr_desc = + Pstr_eval (({pexp_desc = Pexp_array items} as exp), l); + } as str_item); + ] -> + Some + (!reprint_source + [ + { + str_item with + pstr_desc = + Pstr_eval ({exp with pexp_desc = Pexp_tuple items}, l); + }; + ] + comments) + | _ -> None + in + fprintf ppf + "\n\n - Fix this by passing a tuple instead of an array%s@{%s@}\n" + (match suggested_rewrite with + | Some _ -> ", like: " + | None -> "") + (match suggested_rewrite with + | Some rewrite -> rewrite + | None -> "") | _ -> () let type_clash_context_from_function sexp sfunct = diff --git a/compiler/ml/typecore.ml b/compiler/ml/typecore.ml index 7fb9472b9f..de48f4b2c7 100644 --- a/compiler/ml/typecore.ml +++ b/compiler/ml/typecore.ml @@ -728,7 +728,7 @@ let rec collect_missing_arguments env type1 type2 = | None -> None) | _ -> None -let print_expr_type_clash ?type_clash_context env trace ppf = +let print_expr_type_clash ?type_clash_context env loc trace ppf = (* this is the most frequent error. We should do whatever we can to provide specific guidance to this generic error before giving up *) let bottom_aliases_result = bottom_aliases trace in @@ -785,7 +785,7 @@ let print_expr_type_clash ?type_clash_context env trace ppf = (function | ppf -> error_type_text ppf type_clash_context) (function ppf -> error_expected_type_text ppf type_clash_context); - print_extra_type_clash_help ~extract_concrete_typedecl ~env ppf + print_extra_type_clash_help ~extract_concrete_typedecl ~env loc ppf bottom_aliases_result type_clash_context; show_extra_help ppf env trace @@ -4173,7 +4173,7 @@ let type_expr ppf typ = Printtyp.reset_and_mark_loops typ; Printtyp.type_expr ppf typ -let report_error env ppf error = +let report_error env loc ppf error = match error with | Polymorphic_label lid -> fprintf ppf "@[The record field %a is polymorphic.@ %s@]" longident lid @@ -4237,7 +4237,7 @@ let report_error env ppf error = | Expr_type_clash (trace, type_clash_context) -> (* modified *) fprintf ppf "@["; - print_expr_type_clash ?type_clash_context env trace ppf; + print_expr_type_clash ?type_clash_context env loc trace ppf; fprintf ppf "@]" | Apply_non_function typ -> ( (* modified *) @@ -4542,13 +4542,13 @@ let report_error env ppf error = fprintf ppf "Direct field access on a dict is not supported. Use Dict.get instead." -let report_error env ppf err = - Printtyp.wrap_printing_env env (fun () -> report_error env ppf err) +let report_error env loc ppf err = + Printtyp.wrap_printing_env env (fun () -> report_error env loc ppf err) let () = Location.register_error_of_exn (function | Error (loc, env, err) -> - Some (Location.error_of_printer loc (report_error env) err) + Some (Location.error_of_printer loc (report_error env loc) err) | Error_forward err -> Some err | _ -> None) diff --git a/compiler/ml/typecore.mli b/compiler/ml/typecore.mli index c11f557a2e..fef96830c2 100644 --- a/compiler/ml/typecore.mli +++ b/compiler/ml/typecore.mli @@ -108,7 +108,7 @@ type error = exception Error of Location.t * Env.t * error exception Error_forward of Location.error -val report_error : Env.t -> formatter -> error -> unit +val report_error : Env.t -> Location.t -> formatter -> error -> unit (* Deprecated. Use Location.{error_of_exn, report_error}. *) (* Forward declaration, to be filled in by Typemod.type_module *) diff --git a/tests/build_tests/super_errors/expected/array_literal_passed_to_tuple.res.expected b/tests/build_tests/super_errors/expected/array_literal_passed_to_tuple.res.expected new file mode 100644 index 0000000000..5fe60abda3 --- /dev/null +++ b/tests/build_tests/super_errors/expected/array_literal_passed_to_tuple.res.expected @@ -0,0 +1,14 @@ + + We've found a bug for you! + /.../fixtures/array_literal_passed_to_tuple.res:5:17-34 + + 3 │ } + 4 │ + 5 │ let x = doStuff(["hello", "world"]) + 6 │ + + This has type: array<'a> + But it's expected to have type: (string, string) + + - Fix this by passing a tuple instead of an array, like: ("hello", "world") + \ No newline at end of file diff --git a/tests/build_tests/super_errors/expected/array_var_passed_to_tuple.res.expected b/tests/build_tests/super_errors/expected/array_var_passed_to_tuple.res.expected new file mode 100644 index 0000000000..e75a50f376 --- /dev/null +++ b/tests/build_tests/super_errors/expected/array_var_passed_to_tuple.res.expected @@ -0,0 +1,13 @@ + + We've found a bug for you! + /.../fixtures/array_var_passed_to_tuple.res:7:17-18 + + 5 │ let xx = ["hello", "world"] + 6 │ + 7 │ let x = doStuff(xx) + 8 │ + + This has type: array + But this function argument is expecting: (string, string) + + - Fix this by passing a tuple instead of an array \ No newline at end of file diff --git a/tests/build_tests/super_errors/fixtures/array_literal_passed_to_tuple.res b/tests/build_tests/super_errors/fixtures/array_literal_passed_to_tuple.res new file mode 100644 index 0000000000..4b203b92ce --- /dev/null +++ b/tests/build_tests/super_errors/fixtures/array_literal_passed_to_tuple.res @@ -0,0 +1,5 @@ +let doStuff = ((one, two)) => { + one ++ two +} + +let x = doStuff(["hello", "world"]) diff --git a/tests/build_tests/super_errors/fixtures/array_var_passed_to_tuple.res b/tests/build_tests/super_errors/fixtures/array_var_passed_to_tuple.res new file mode 100644 index 0000000000..7fee1bbc8b --- /dev/null +++ b/tests/build_tests/super_errors/fixtures/array_var_passed_to_tuple.res @@ -0,0 +1,7 @@ +let doStuff = ((one, two)) => { + one ++ two +} + +let xx = ["hello", "world"] + +let x = doStuff(xx) From 6a28ea9a112ec26b9f5aeb517c58eb0162327f3b Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Thu, 22 May 2025 12:56:35 +0200 Subject: [PATCH 2/9] refactor --- compiler/bsc/rescript_compiler_main.ml | 19 ++++-- compiler/ml/error_message_utils.ml | 88 ++++++++++++++++---------- 2 files changed, 67 insertions(+), 40 deletions(-) diff --git a/compiler/bsc/rescript_compiler_main.ml b/compiler/bsc/rescript_compiler_main.ml index a7d9190b60..6b59a25360 100644 --- a/compiler/bsc/rescript_compiler_main.ml +++ b/compiler/bsc/rescript_compiler_main.ml @@ -12,24 +12,29 @@ let absname = ref false -external to_comment : Res_comment.t -> Error_message_utils.comment = "%identity" -external from_comment : Error_message_utils.comment -> Res_comment.t - = "%identity" +(* TODO: Maybe there's a better place to do this init. *) +module Error_message_utils_support = struct + external to_comment : Res_comment.t -> Error_message_utils.Parser.comment + = "%identity" + external from_comment : Error_message_utils.Parser.comment -> Res_comment.t + = "%identity" +end let () = - Error_message_utils.parse_source := + Error_message_utils.Parser.parse_source := fun source -> let res = Res_driver.parse_implementation_from_source ~for_printer:false ~display_filename:"" ~source in - (res.parsetree, res.comments |> List.map to_comment) + ( res.parsetree, + res.comments |> List.map Error_message_utils_support.to_comment ) let () = - Error_message_utils.reprint_source := + Error_message_utils.Parser.reprint_source := fun parsetree comments -> Res_printer.print_implementation parsetree - ~comments:(comments |> List.map from_comment) + ~comments:(comments |> List.map Error_message_utils_support.from_comment) ~width:80 let set_abs_input_name sourcefile = diff --git a/compiler/ml/error_message_utils.ml b/compiler/ml/error_message_utils.ml index d652456350..4304d8c22d 100644 --- a/compiler/ml/error_message_utils.ml +++ b/compiler/ml/error_message_utils.ml @@ -1,11 +1,56 @@ type extract_concrete_typedecl = Env.t -> Types.type_expr -> Path.t * Path.t * Types.type_declaration -type comment -let parse_source : (string -> Parsetree.structure * comment list) ref = - ref (fun _ -> ([], [])) -let reprint_source : (Parsetree.structure -> comment list -> string) ref = - ref (fun _ _ -> "") +module Parser : sig + type comment + + val parse_source : (string -> Parsetree.structure * comment list) ref + + val reprint_source : (Parsetree.structure -> comment list -> string) ref + + val parse_expr_at_loc : + Warnings.loc -> (Parsetree.expression * comment list) option + + val reprint_expr_at_loc : + ?mapper:(Parsetree.expression -> Parsetree.expression option) -> + Warnings.loc -> + string option +end = struct + type comment + + let parse_source : (string -> Parsetree.structure * comment list) ref = + ref (fun _ -> ([], [])) + + let reprint_source : (Parsetree.structure -> comment list -> string) ref = + ref (fun _ _ -> "") + + let extract_location_string ~src (loc : Location.t) = + let start_pos = loc.loc_start in + let end_pos = loc.loc_end in + let start_offset = start_pos.pos_cnum in + let end_offset = end_pos.pos_cnum in + String.sub src start_offset (end_offset - start_offset) + + let parse_expr_at_loc loc = + (* TODO: Maybe cache later on *) + let src = Ext_io.load_file loc.Location.loc_start.pos_fname in + let sub_src = extract_location_string ~src loc in + let parsed, comments = !parse_source sub_src in + match parsed with + | [{Parsetree.pstr_desc = Pstr_eval (exp, _)}] -> Some (exp, comments) + | _ -> None + + let wrap_in_structure exp = + [{Parsetree.pstr_desc = Pstr_eval (exp, []); pstr_loc = Location.none}] + + let reprint_expr_at_loc ?(mapper = fun _ -> None) loc = + match parse_expr_at_loc loc with + | Some (exp, comments) -> ( + match mapper exp with + | Some exp -> Some (!reprint_source (wrap_in_structure exp) comments) + | None -> None) + | None -> None +end type type_clash_statement = FunctionCall type type_clash_context = @@ -68,13 +113,6 @@ let is_record_type ~extract_concrete_typedecl ~env ty = | _ -> false with _ -> false -let extract_location_string ~src (loc : Location.t) = - let start_pos = loc.loc_start in - let end_pos = loc.loc_end in - let start_offset = start_pos.pos_cnum in - let end_offset = end_pos.pos_cnum in - String.sub src start_offset (end_offset - start_offset) - let print_extra_type_clash_help ~extract_concrete_typedecl ~env loc ppf (bottom_aliases : (Types.type_expr * Types.type_expr) option) type_clash_context = @@ -200,28 +238,12 @@ let print_extra_type_clash_help ~extract_concrete_typedecl ~env loc ppf fprintf ppf "\n\n - Did you mean to await this promise before using it?\n" | _, Some ({Types.desc = Tconstr (p1, _, _)}, {Types.desc = Ttuple _}) when Path.same p1 Predef.path_array -> - let src = Ext_io.load_file loc.Location.loc_start.pos_fname in - let sub_src = extract_location_string ~src loc in - let parsed, comments = !parse_source sub_src in let suggested_rewrite = - match parsed with - | [ - ({ - Parsetree.pstr_desc = - Pstr_eval (({pexp_desc = Pexp_array items} as exp), l); - } as str_item); - ] -> - Some - (!reprint_source - [ - { - str_item with - pstr_desc = - Pstr_eval ({exp with pexp_desc = Pexp_tuple items}, l); - }; - ] - comments) - | _ -> None + Parser.reprint_expr_at_loc loc ~mapper:(fun exp -> + match exp.Parsetree.pexp_desc with + | Pexp_array items -> + Some {exp with Parsetree.pexp_desc = Pexp_tuple items} + | _ -> None) in fprintf ppf "\n\n - Fix this by passing a tuple instead of an array%s@{%s@}\n" From a8378e6a35396e745d4533a0bfd2908b2385de63 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Thu, 22 May 2025 13:09:45 +0200 Subject: [PATCH 3/9] example of detecting and providing rewrite for ReScript object -> record --- compiler/ml/error_message_utils.ml | 35 +++++++++++++++---- ..._passed_when_record_expected.res.expected} | 12 ++++--- ...t_literal_passed_when_record_expected.res} | 0 3 files changed, 36 insertions(+), 11 deletions(-) rename tests/build_tests/super_errors/expected/{object_passed_when_record_expected.res.expected => object_literal_passed_when_record_expected.res.expected} (51%) rename tests/build_tests/super_errors/fixtures/{object_passed_when_record_expected.res => object_literal_passed_when_record_expected.res} (100%) diff --git a/compiler/ml/error_message_utils.ml b/compiler/ml/error_message_utils.ml index 4304d8c22d..5c9097a3f7 100644 --- a/compiler/ml/error_message_utils.ml +++ b/compiler/ml/error_message_utils.ml @@ -227,12 +227,35 @@ let print_extra_type_clash_help ~extract_concrete_typedecl ~env loc ppf | _, Some ({desc = Tobject _}, ({Types.desc = Tconstr _} as t1)) when is_record_type ~extract_concrete_typedecl ~env t1 -> fprintf ppf - "\n\n\ - \ You're passing a @{ReScript object@} where a @{record@} \ - is expected. \n\n\ - \ - Did you mean to pass a record instead of an object? Objects are \ - written with quoted keys, and records with unquoted keys. Remove the \ - quotes from the object keys to pass it as a record instead of object. \n\n" + "@,\ + @,\ + You're passing a @{ReScript object@} where a @{record@} is \ + expected. Objects are written with quoted keys, and records with \ + unquoted keys."; + + let suggested_rewrite = + Parser.reprint_expr_at_loc loc ~mapper:(fun exp -> + match exp.Parsetree.pexp_desc with + | Pexp_extension + ( {txt = "obj"}, + PStr + [ + { + pstr_desc = + Pstr_eval (({pexp_desc = Pexp_record _} as record), _); + }; + ] ) -> + Some record + | _ -> None) + in + fprintf ppf + "@,\ + @,\ + Possible solutions: @,\ + - Rewrite the object to a record, like: @{%s@}@," + (match suggested_rewrite with + | Some rewrite -> rewrite + | None -> "") | _, Some ({Types.desc = Tconstr (p1, _, _)}, _) when Path.same p1 Predef.path_promise -> fprintf ppf "\n\n - Did you mean to await this promise before using it?\n" diff --git a/tests/build_tests/super_errors/expected/object_passed_when_record_expected.res.expected b/tests/build_tests/super_errors/expected/object_literal_passed_when_record_expected.res.expected similarity index 51% rename from tests/build_tests/super_errors/expected/object_passed_when_record_expected.res.expected rename to tests/build_tests/super_errors/expected/object_literal_passed_when_record_expected.res.expected index be09067202..b69288a686 100644 --- a/tests/build_tests/super_errors/expected/object_passed_when_record_expected.res.expected +++ b/tests/build_tests/super_errors/expected/object_literal_passed_when_record_expected.res.expected @@ -1,6 +1,6 @@ We've found a bug for you! - /.../fixtures/object_passed_when_record_expected.res:4:14-26 + /.../fixtures/object_literal_passed_when_record_expected.res:4:14-26 2 │ type xx = array 3 │ @@ -9,7 +9,9 @@ This has type: {"one": bool} But it's expected to have type: x - - You're passing a ReScript object where a record is expected. - - - Did you mean to pass a record instead of an object? Objects are written with quoted keys, and records with unquoted keys. Remove the quotes from the object keys to pass it as a record instead of object. \ No newline at end of file + + You're passing a ReScript object where a record is expected. Objects are written with quoted keys, and records with unquoted keys. + + Possible solutions: + - Rewrite the object to a record, like: {one: true} + \ No newline at end of file diff --git a/tests/build_tests/super_errors/fixtures/object_passed_when_record_expected.res b/tests/build_tests/super_errors/fixtures/object_literal_passed_when_record_expected.res similarity index 100% rename from tests/build_tests/super_errors/fixtures/object_passed_when_record_expected.res rename to tests/build_tests/super_errors/fixtures/object_literal_passed_when_record_expected.res From fa7e4836aebda204c5a0d983e7dd4728175f2c65 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Thu, 22 May 2025 13:24:05 +0200 Subject: [PATCH 4/9] remove Belt prefix --- compiler/ml/error_message_utils.ml | 6 +++--- .../super_errors/expected/math_operator_float.res.expected | 2 +- .../super_errors/expected/primitives7.res.expected | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/compiler/ml/error_message_utils.ml b/compiler/ml/error_message_utils.ml index 5c9097a3f7..c6e8663514 100644 --- a/compiler/ml/error_message_utils.ml +++ b/compiler/ml/error_message_utils.ml @@ -155,8 +155,8 @@ let print_extra_type_clash_help ~extract_concrete_typedecl ~env loc ppf you cannot %s a float and an int without converting between the two.\n\n\ \ Possible solutions:\n\ \ - Ensure all values in this calculation has the type @{%s@}. \ - You can convert between floats and ints via \ - @{Belt.Float.toInt@} and @{Belt.Int.fromFloat@}." + You can convert between floats and ints via @{Float.toInt@} and \ + @{Int.fromFloat@}." operator_text (if for_float then "float" else "int")); match (is_constant, bottom_aliases) with @@ -204,7 +204,7 @@ let print_extra_type_clash_help ~extract_concrete_typedecl ~env loc ppf "\n\n\ \ Possible solutions:\n\ \ - Unwrap the option to its underlying value using \ - `yourValue->Belt.Option.getWithDefault(someDefaultValue)`" + `yourValue->Option.getOr(someDefaultValue)`" | Some ComparisonOperator, _ -> fprintf ppf "\n\n You can only compare things of the same type." | Some ArrayValue, _ -> diff --git a/tests/build_tests/super_errors/expected/math_operator_float.res.expected b/tests/build_tests/super_errors/expected/math_operator_float.res.expected index 640e57a500..28b4a7e7ec 100644 --- a/tests/build_tests/super_errors/expected/math_operator_float.res.expected +++ b/tests/build_tests/super_errors/expected/math_operator_float.res.expected @@ -13,7 +13,7 @@ Floats and ints have their own mathematical operators. This means you cannot add a float and an int without converting between the two. Possible solutions: - - Ensure all values in this calculation has the type float. You can convert between floats and ints via Belt.Float.toInt and Belt.Int.fromFloat. + - Ensure all values in this calculation has the type float. You can convert between floats and ints via Float.toInt and Int.fromFloat. - Change the operator to +, which works on int You can convert int to float with Int.toFloat. diff --git a/tests/build_tests/super_errors/expected/primitives7.res.expected b/tests/build_tests/super_errors/expected/primitives7.res.expected index a6fe42b223..6f5cd351a3 100644 --- a/tests/build_tests/super_errors/expected/primitives7.res.expected +++ b/tests/build_tests/super_errors/expected/primitives7.res.expected @@ -13,7 +13,7 @@ Floats and ints have their own mathematical operators. This means you cannot add a float and an int without converting between the two. Possible solutions: - - Ensure all values in this calculation has the type float. You can convert between floats and ints via Belt.Float.toInt and Belt.Int.fromFloat. + - Ensure all values in this calculation has the type float. You can convert between floats and ints via Float.toInt and Int.fromFloat. - Change the operator to +, which works on int You can convert int to float with Int.toFloat. From 991dfb616f70193f77c5aa98f3fc70b64b847696 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Thu, 22 May 2025 14:37:07 +0200 Subject: [PATCH 5/9] add dedicated JSX element type mismatch errors --- compiler/bsc/rescript_compiler_main.ml | 38 +++++++------ compiler/ml/error_message_utils.ml | 53 +++++++++++++++++++ ...x_type_mismatch_array_element.res.expected | 13 +++++ .../jsx_type_mismatch_array_raw.res.expected | 13 +++++ .../jsx_type_mismatch_float.res.expected | 13 +++++ .../jsx_type_mismatch_int.res.expected | 13 +++++ .../jsx_type_mismatch_string.res.expected | 13 +++++ .../jsx_type_mismatch_array_element.res | 19 +++++++ .../fixtures/jsx_type_mismatch_array_raw.res | 17 ++++++ .../fixtures/jsx_type_mismatch_float.res | 17 ++++++ .../fixtures/jsx_type_mismatch_int.res | 17 ++++++ .../fixtures/jsx_type_mismatch_string.res | 17 ++++++ 12 files changed, 226 insertions(+), 17 deletions(-) create mode 100644 tests/build_tests/super_errors/expected/jsx_type_mismatch_array_element.res.expected create mode 100644 tests/build_tests/super_errors/expected/jsx_type_mismatch_array_raw.res.expected create mode 100644 tests/build_tests/super_errors/expected/jsx_type_mismatch_float.res.expected create mode 100644 tests/build_tests/super_errors/expected/jsx_type_mismatch_int.res.expected create mode 100644 tests/build_tests/super_errors/expected/jsx_type_mismatch_string.res.expected create mode 100644 tests/build_tests/super_errors/fixtures/jsx_type_mismatch_array_element.res create mode 100644 tests/build_tests/super_errors/fixtures/jsx_type_mismatch_array_raw.res create mode 100644 tests/build_tests/super_errors/fixtures/jsx_type_mismatch_float.res create mode 100644 tests/build_tests/super_errors/fixtures/jsx_type_mismatch_int.res create mode 100644 tests/build_tests/super_errors/fixtures/jsx_type_mismatch_string.res diff --git a/compiler/bsc/rescript_compiler_main.ml b/compiler/bsc/rescript_compiler_main.ml index 6b59a25360..52f4b85651 100644 --- a/compiler/bsc/rescript_compiler_main.ml +++ b/compiler/bsc/rescript_compiler_main.ml @@ -12,30 +12,33 @@ let absname = ref false -(* TODO: Maybe there's a better place to do this init. *) module Error_message_utils_support = struct external to_comment : Res_comment.t -> Error_message_utils.Parser.comment = "%identity" external from_comment : Error_message_utils.Parser.comment -> Res_comment.t = "%identity" -end -let () = - Error_message_utils.Parser.parse_source := - fun source -> - let res = - Res_driver.parse_implementation_from_source ~for_printer:false - ~display_filename:"" ~source - in - ( res.parsetree, - res.comments |> List.map Error_message_utils_support.to_comment ) + let setup () = + (Error_message_utils.Parser.parse_source := + fun source -> + let res = + Res_driver.parse_implementation_from_source ~for_printer:false + ~display_filename:"" ~source + in + (res.parsetree, res.comments |> List.map to_comment)); -let () = - Error_message_utils.Parser.reprint_source := - fun parsetree comments -> - Res_printer.print_implementation parsetree - ~comments:(comments |> List.map Error_message_utils_support.from_comment) - ~width:80 + (Error_message_utils.Parser.reprint_source := + fun parsetree comments -> + Res_printer.print_implementation parsetree + ~comments:(comments |> List.map from_comment) + ~width:80); + + Error_message_utils.configured_jsx_module := + Some + (match !Js_config.jsx_module with + | React -> "React" + | Generic {module_name} -> module_name) +end let set_abs_input_name sourcefile = let sourcefile = @@ -66,6 +69,7 @@ let process_file sourcefile ?kind ppf = properly *) setup_outcome_printer (); + Error_message_utils_support.setup (); let kind = match kind with | None -> diff --git a/compiler/ml/error_message_utils.ml b/compiler/ml/error_message_utils.ml index c6e8663514..e16d635aaf 100644 --- a/compiler/ml/error_message_utils.ml +++ b/compiler/ml/error_message_utils.ml @@ -1,6 +1,13 @@ type extract_concrete_typedecl = Env.t -> Types.type_expr -> Path.t * Path.t * Types.type_declaration +let configured_jsx_module : string option ref = ref None + +let with_configured_jsx_module s = + match !configured_jsx_module with + | None -> s + | Some module_name -> module_name ^ "." ^ s + module Parser : sig type comment @@ -276,6 +283,52 @@ let print_extra_type_clash_help ~extract_concrete_typedecl ~env loc ppf (match suggested_rewrite with | Some rewrite -> rewrite | None -> "") + | ( _, + Some + ( {desc = Tconstr (p, type_params, _)}, + {desc = Tconstr (Pdot (Pident {name = "Jsx"}, "element", _), _, _)} ) + ) -> ( + (* Looking for a JSX element but got something else *) + let is_jsx_element ty = + match Ctype.expand_head env ty with + | {desc = Tconstr (Pdot (Pident {name = "Jsx"}, "element", _), _, _)} -> + true + | _ -> false + in + + let print_jsx_msg ?(extra = "") name target_fn = + fprintf ppf + "@,\ + @,\ + In JSX, all content must be JSX elements. You can convert %s to a JSX \ + element with @{%s@}%s.@," + name target_fn extra + in + + match type_params with + | _ when Path.same p Predef.path_int -> + print_jsx_msg "int" (with_configured_jsx_module "int") + | _ when Path.same p Predef.path_string -> + print_jsx_msg "string" (with_configured_jsx_module "string") + | _ when Path.same p Predef.path_float -> + print_jsx_msg "float" (with_configured_jsx_module "float") + | [tp] when Path.same p Predef.path_array && is_jsx_element tp -> + print_jsx_msg + ~extra: + (" (for example by using a pipe: ->" + ^ with_configured_jsx_module "array" + ^ ".") + "array" + (with_configured_jsx_module "array") + | [_] when Path.same p Predef.path_array -> + fprintf ppf + "@,\ + @,\ + You need to convert each item in this array to a JSX element first, \ + then use @{%s@} to convert the array of JSX elements into a \ + single JSX element.@," + (with_configured_jsx_module "array") + | _ -> ()) | _ -> () let type_clash_context_from_function sexp sfunct = diff --git a/tests/build_tests/super_errors/expected/jsx_type_mismatch_array_element.res.expected b/tests/build_tests/super_errors/expected/jsx_type_mismatch_array_element.res.expected new file mode 100644 index 0000000000..1cf8426d78 --- /dev/null +++ b/tests/build_tests/super_errors/expected/jsx_type_mismatch_array_element.res.expected @@ -0,0 +1,13 @@ + + We've found a bug for you! + /.../fixtures/jsx_type_mismatch_array_element.res:19:13-30 + + 17 │ } + 18 │ + 19 │ let x = <> {[React.string("")]} + 20 │ + + This has type: array<'a> + But it's expected to have type: React.element (defined as Jsx.element) + + You need to convert each item in this array to a JSX element first, then use React.array to convert the array of JSX elements into a single JSX element. \ No newline at end of file diff --git a/tests/build_tests/super_errors/expected/jsx_type_mismatch_array_raw.res.expected b/tests/build_tests/super_errors/expected/jsx_type_mismatch_array_raw.res.expected new file mode 100644 index 0000000000..1eb1d4870d --- /dev/null +++ b/tests/build_tests/super_errors/expected/jsx_type_mismatch_array_raw.res.expected @@ -0,0 +1,13 @@ + + We've found a bug for you! + /.../fixtures/jsx_type_mismatch_array_raw.res:17:13-16 + + 15 │ } + 16 │ + 17 │ let x = <> {[""]} + 18 │ + + This has type: array<'a> + But it's expected to have type: React.element (defined as Jsx.element) + + You need to convert each item in this array to a JSX element first, then use React.array to convert the array of JSX elements into a single JSX element. \ No newline at end of file diff --git a/tests/build_tests/super_errors/expected/jsx_type_mismatch_float.res.expected b/tests/build_tests/super_errors/expected/jsx_type_mismatch_float.res.expected new file mode 100644 index 0000000000..af4279bd52 --- /dev/null +++ b/tests/build_tests/super_errors/expected/jsx_type_mismatch_float.res.expected @@ -0,0 +1,13 @@ + + We've found a bug for you! + /.../fixtures/jsx_type_mismatch_float.res:17:13-14 + + 15 │ } + 16 │ + 17 │ let x = <> {1.} + 18 │ + + This has type: float + But it's expected to have type: React.element (defined as Jsx.element) + + In JSX, all content must be JSX elements. You can convert float to a JSX element with React.float. \ No newline at end of file diff --git a/tests/build_tests/super_errors/expected/jsx_type_mismatch_int.res.expected b/tests/build_tests/super_errors/expected/jsx_type_mismatch_int.res.expected new file mode 100644 index 0000000000..5a43157d03 --- /dev/null +++ b/tests/build_tests/super_errors/expected/jsx_type_mismatch_int.res.expected @@ -0,0 +1,13 @@ + + We've found a bug for you! + /.../fixtures/jsx_type_mismatch_int.res:17:13 + + 15 │ } + 16 │ + 17 │ let x = <> {1} + 18 │ + + This has type: int + But it's expected to have type: React.element (defined as Jsx.element) + + In JSX, all content must be JSX elements. You can convert int to a JSX element with React.int. \ No newline at end of file diff --git a/tests/build_tests/super_errors/expected/jsx_type_mismatch_string.res.expected b/tests/build_tests/super_errors/expected/jsx_type_mismatch_string.res.expected new file mode 100644 index 0000000000..10d1d64dc7 --- /dev/null +++ b/tests/build_tests/super_errors/expected/jsx_type_mismatch_string.res.expected @@ -0,0 +1,13 @@ + + We've found a bug for you! + /.../fixtures/jsx_type_mismatch_string.res:17:13-14 + + 15 │ } + 16 │ + 17 │ let x = <> {""} + 18 │ + + This has type: string + But it's expected to have type: React.element (defined as Jsx.element) + + In JSX, all content must be JSX elements. You can convert string to a JSX element with React.string. \ No newline at end of file diff --git a/tests/build_tests/super_errors/fixtures/jsx_type_mismatch_array_element.res b/tests/build_tests/super_errors/fixtures/jsx_type_mismatch_array_element.res new file mode 100644 index 0000000000..cebe870397 --- /dev/null +++ b/tests/build_tests/super_errors/fixtures/jsx_type_mismatch_array_element.res @@ -0,0 +1,19 @@ +@@config({ + flags: ["-bs-jsx", "4"], +}) + +module React = { + type element = Jsx.element + type componentLike<'props, 'return> = 'props => 'return + type component<'props> = Jsx.component<'props> + + @module("react/jsx-runtime") + external jsx: (component<'props>, 'props) => element = "jsx" + + type fragmentProps = {children?: element} + @module("react/jsx-runtime") external jsxFragment: component = "Fragment" + + external string: string => element = "%identity" +} + +let x = <> {[React.string("")]} diff --git a/tests/build_tests/super_errors/fixtures/jsx_type_mismatch_array_raw.res b/tests/build_tests/super_errors/fixtures/jsx_type_mismatch_array_raw.res new file mode 100644 index 0000000000..3a3c531b50 --- /dev/null +++ b/tests/build_tests/super_errors/fixtures/jsx_type_mismatch_array_raw.res @@ -0,0 +1,17 @@ +@@config({ + flags: ["-bs-jsx", "4"], +}) + +module React = { + type element = Jsx.element + type componentLike<'props, 'return> = 'props => 'return + type component<'props> = Jsx.component<'props> + + @module("react/jsx-runtime") + external jsx: (component<'props>, 'props) => element = "jsx" + + type fragmentProps = {children?: element} + @module("react/jsx-runtime") external jsxFragment: component = "Fragment" +} + +let x = <> {[""]} diff --git a/tests/build_tests/super_errors/fixtures/jsx_type_mismatch_float.res b/tests/build_tests/super_errors/fixtures/jsx_type_mismatch_float.res new file mode 100644 index 0000000000..208eacac39 --- /dev/null +++ b/tests/build_tests/super_errors/fixtures/jsx_type_mismatch_float.res @@ -0,0 +1,17 @@ +@@config({ + flags: ["-bs-jsx", "4"], +}) + +module React = { + type element = Jsx.element + type componentLike<'props, 'return> = 'props => 'return + type component<'props> = Jsx.component<'props> + + @module("react/jsx-runtime") + external jsx: (component<'props>, 'props) => element = "jsx" + + type fragmentProps = {children?: element} + @module("react/jsx-runtime") external jsxFragment: component = "Fragment" +} + +let x = <> {1.} diff --git a/tests/build_tests/super_errors/fixtures/jsx_type_mismatch_int.res b/tests/build_tests/super_errors/fixtures/jsx_type_mismatch_int.res new file mode 100644 index 0000000000..6836a4865d --- /dev/null +++ b/tests/build_tests/super_errors/fixtures/jsx_type_mismatch_int.res @@ -0,0 +1,17 @@ +@@config({ + flags: ["-bs-jsx", "4"], +}) + +module React = { + type element = Jsx.element + type componentLike<'props, 'return> = 'props => 'return + type component<'props> = Jsx.component<'props> + + @module("react/jsx-runtime") + external jsx: (component<'props>, 'props) => element = "jsx" + + type fragmentProps = {children?: element} + @module("react/jsx-runtime") external jsxFragment: component = "Fragment" +} + +let x = <> {1} diff --git a/tests/build_tests/super_errors/fixtures/jsx_type_mismatch_string.res b/tests/build_tests/super_errors/fixtures/jsx_type_mismatch_string.res new file mode 100644 index 0000000000..55ec5786e6 --- /dev/null +++ b/tests/build_tests/super_errors/fixtures/jsx_type_mismatch_string.res @@ -0,0 +1,17 @@ +@@config({ + flags: ["-bs-jsx", "4"], +}) + +module React = { + type element = Jsx.element + type componentLike<'props, 'return> = 'props => 'return + type component<'props> = Jsx.component<'props> + + @module("react/jsx-runtime") + external jsx: (component<'props>, 'props) => element = "jsx" + + type fragmentProps = {children?: element} + @module("react/jsx-runtime") external jsxFragment: component = "Fragment" +} + +let x = <> {""} From 9f03508f47bf89bbf58658cdc44d5f10232f76f2 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Thu, 22 May 2025 14:46:12 +0200 Subject: [PATCH 6/9] note that you need to unwrap options in JSX --- compiler/ml/error_message_utils.ml | 14 +++++++++++++- .../jsx_type_mismatch_option.res.expected | 14 ++++++++++++++ .../fixtures/jsx_type_mismatch_option.res | 17 +++++++++++++++++ 3 files changed, 44 insertions(+), 1 deletion(-) create mode 100644 tests/build_tests/super_errors/expected/jsx_type_mismatch_option.res.expected create mode 100644 tests/build_tests/super_errors/fixtures/jsx_type_mismatch_option.res diff --git a/compiler/ml/error_message_utils.ml b/compiler/ml/error_message_utils.ml index e16d635aaf..890675a391 100644 --- a/compiler/ml/error_message_utils.ml +++ b/compiler/ml/error_message_utils.ml @@ -259,7 +259,10 @@ let print_extra_type_clash_help ~extract_concrete_typedecl ~env loc ppf "@,\ @,\ Possible solutions: @,\ - - Rewrite the object to a record, like: @{%s@}@," + - Rewrite the object to a record%s@{%s@}@," + (match suggested_rewrite with + | Some _ -> ", like: " + | None -> "") (match suggested_rewrite with | Some rewrite -> rewrite | None -> "") @@ -312,6 +315,15 @@ let print_extra_type_clash_help ~extract_concrete_typedecl ~env loc ppf print_jsx_msg "string" (with_configured_jsx_module "string") | _ when Path.same p Predef.path_float -> print_jsx_msg "float" (with_configured_jsx_module "float") + | [_] when Path.same p Predef.path_option -> + fprintf ppf + "@,\ + @,\ + You need to unwrap this option to its underlying value first, then \ + turn that value into a JSX element.@,\ + For @{None@}, you can use @{%s@} to output nothing into \ + JSX.@," + (with_configured_jsx_module "null") | [tp] when Path.same p Predef.path_array && is_jsx_element tp -> print_jsx_msg ~extra: diff --git a/tests/build_tests/super_errors/expected/jsx_type_mismatch_option.res.expected b/tests/build_tests/super_errors/expected/jsx_type_mismatch_option.res.expected new file mode 100644 index 0000000000..fee1ae03c6 --- /dev/null +++ b/tests/build_tests/super_errors/expected/jsx_type_mismatch_option.res.expected @@ -0,0 +1,14 @@ + + We've found a bug for you! + /.../fixtures/jsx_type_mismatch_option.res:17:13-16 + + 15 │ } + 16 │ + 17 │ let x = <> {None} + 18 │ + + This has type: option<'a> + But it's expected to have type: React.element (defined as Jsx.element) + + You need to unwrap this option to its underlying value first, then turn that value into a JSX element. + For None, you can use React.null to output nothing into JSX. \ No newline at end of file diff --git a/tests/build_tests/super_errors/fixtures/jsx_type_mismatch_option.res b/tests/build_tests/super_errors/fixtures/jsx_type_mismatch_option.res new file mode 100644 index 0000000000..334d11978c --- /dev/null +++ b/tests/build_tests/super_errors/fixtures/jsx_type_mismatch_option.res @@ -0,0 +1,17 @@ +@@config({ + flags: ["-bs-jsx", "4"], +}) + +module React = { + type element = Jsx.element + type componentLike<'props, 'return> = 'props => 'return + type component<'props> = Jsx.component<'props> + + @module("react/jsx-runtime") + external jsx: (component<'props>, 'props) => element = "jsx" + + type fragmentProps = {children?: element} + @module("react/jsx-runtime") external jsxFragment: component = "Fragment" +} + +let x = <> {None} From 4f302256d1af674579096ad87272cea340e08324 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Thu, 22 May 2025 16:43:04 +0200 Subject: [PATCH 7/9] fix --- compiler/ml/error_message_utils.ml | 2 +- .../super_errors/expected/math_operator_float.res.expected | 2 +- .../build_tests/super_errors/expected/primitives7.res.expected | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/compiler/ml/error_message_utils.ml b/compiler/ml/error_message_utils.ml index 890675a391..b8eac99181 100644 --- a/compiler/ml/error_message_utils.ml +++ b/compiler/ml/error_message_utils.ml @@ -161,7 +161,7 @@ let print_extra_type_clash_help ~extract_concrete_typedecl ~env loc ppf \ Floats and ints have their own mathematical operators. This means \ you cannot %s a float and an int without converting between the two.\n\n\ \ Possible solutions:\n\ - \ - Ensure all values in this calculation has the type @{%s@}. \ + \ - Ensure all values in this calculation have the type @{%s@}. \ You can convert between floats and ints via @{Float.toInt@} and \ @{Int.fromFloat@}." operator_text diff --git a/tests/build_tests/super_errors/expected/math_operator_float.res.expected b/tests/build_tests/super_errors/expected/math_operator_float.res.expected index 28b4a7e7ec..fe5df9c95c 100644 --- a/tests/build_tests/super_errors/expected/math_operator_float.res.expected +++ b/tests/build_tests/super_errors/expected/math_operator_float.res.expected @@ -13,7 +13,7 @@ Floats and ints have their own mathematical operators. This means you cannot add a float and an int without converting between the two. Possible solutions: - - Ensure all values in this calculation has the type float. You can convert between floats and ints via Float.toInt and Int.fromFloat. + - Ensure all values in this calculation have the type float. You can convert between floats and ints via Float.toInt and Int.fromFloat. - Change the operator to +, which works on int You can convert int to float with Int.toFloat. diff --git a/tests/build_tests/super_errors/expected/primitives7.res.expected b/tests/build_tests/super_errors/expected/primitives7.res.expected index 6f5cd351a3..bfe3cce7c0 100644 --- a/tests/build_tests/super_errors/expected/primitives7.res.expected +++ b/tests/build_tests/super_errors/expected/primitives7.res.expected @@ -13,7 +13,7 @@ Floats and ints have their own mathematical operators. This means you cannot add a float and an int without converting between the two. Possible solutions: - - Ensure all values in this calculation has the type float. You can convert between floats and ints via Float.toInt and Int.fromFloat. + - Ensure all values in this calculation have the type float. You can convert between floats and ints via Float.toInt and Int.fromFloat. - Change the operator to +, which works on int You can convert int to float with Int.toFloat. From aa1856a0e3675cafe8cc2495b18da0ec9c6e8668 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Thu, 22 May 2025 18:05:45 +0200 Subject: [PATCH 8/9] add simple error message indicating dict syntax --- compiler/ml/error_message_utils.ml | 4 ++++ .../super_errors/expected/dict_helper.res.expected | 13 +++++++++++++ .../super_errors/fixtures/dict_helper.res | 3 +++ 3 files changed, 20 insertions(+) create mode 100644 tests/build_tests/super_errors/expected/dict_helper.res.expected create mode 100644 tests/build_tests/super_errors/fixtures/dict_helper.res diff --git a/compiler/ml/error_message_utils.ml b/compiler/ml/error_message_utils.ml index b8eac99181..ebc138284f 100644 --- a/compiler/ml/error_message_utils.ml +++ b/compiler/ml/error_message_utils.ml @@ -223,6 +223,10 @@ let print_extra_type_clash_help ~extract_concrete_typedecl ~env loc ppf \ - Use a tuple, if your array is of fixed length. Tuples can mix types \ freely, and compiles to a JavaScript array. Example of a tuple: `let \ myTuple = (10, \"hello\", 15.5, true)" + | _, Some (_, {desc = Tconstr (p2, _, _)}) when Path.same Predef.path_dict p2 + -> + fprintf ppf + "@,@,Dicts are written like: @{dict{\"a\": 1, \"b\": 2}@}@," | _, Some ({Types.desc = Tconstr (_p1, _, _)}, {desc = Tconstr (p2, _, _)}) when Path.same Predef.path_unit p2 -> fprintf ppf diff --git a/tests/build_tests/super_errors/expected/dict_helper.res.expected b/tests/build_tests/super_errors/expected/dict_helper.res.expected new file mode 100644 index 0000000000..b2e35e89b7 --- /dev/null +++ b/tests/build_tests/super_errors/expected/dict_helper.res.expected @@ -0,0 +1,13 @@ + + We've found a bug for you! + /.../fixtures/dict_helper.res:3:11-23 + + 1 │ external takesDict: dict => unit = "takesDict" + 2 │ + 3 │ takesDict({"test": "1"}) + 4 │ + + This has type: {"test": string} + But it's expected to have type: dict + + Dicts are written like: dict{"a": 1, "b": 2} \ No newline at end of file diff --git a/tests/build_tests/super_errors/fixtures/dict_helper.res b/tests/build_tests/super_errors/fixtures/dict_helper.res new file mode 100644 index 0000000000..85e2ba4fea --- /dev/null +++ b/tests/build_tests/super_errors/fixtures/dict_helper.res @@ -0,0 +1,3 @@ +external takesDict: dict => unit = "takesDict" + +takesDict({"test": "1"}) From 2f2dcb74a1b687837c2a3939575fa60f8b978604 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Thu, 22 May 2025 18:16:05 +0200 Subject: [PATCH 9/9] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a5998097a..b44e383e83 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ - Suggest awaiting promise before using it when types mismatch. https://github.com/rescript-lang/rescript/pull/7498 - Complete from `RegExp` stdlib module for regexes. https://github.com/rescript-lang/rescript/pull/7425 - Allow oneliner formatting when including module with single type alias. https://github.com/rescript-lang/rescript/pull/7502 +- Improve error messages for JSX type mismatches, passing objects where record is expected, passing array literal where tuple is expected, and more. https://github.com/rescript-lang/rescript/pull/7500 # 12.0.0-alpha.13