From bab3cfe935ce59cfa4017b69cebc836e9617f22d Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Mon, 18 Sep 2023 21:50:31 +0200 Subject: [PATCH 1/3] try making top level unit error message clearer --- .../expected/partial_app.res.expected | 8 +++++- .../top_level_fn_call_not_unit.res.expected | 16 ++++++++++++ .../top_level_value_not_unit.res.expected | 14 +++++++++++ .../fixtures/top_level_fn_call_not_unit.res | 3 +++ .../fixtures/top_level_value_not_unit.res | 1 + jscomp/ext/warnings.ml | 25 ++++++++++++++++--- jscomp/ext/warnings.mli | 4 ++- jscomp/ml/typecore.ml | 16 +++++++++--- 8 files changed, 78 insertions(+), 9 deletions(-) create mode 100644 jscomp/build_tests/super_errors/expected/top_level_fn_call_not_unit.res.expected create mode 100644 jscomp/build_tests/super_errors/expected/top_level_value_not_unit.res.expected create mode 100644 jscomp/build_tests/super_errors/fixtures/top_level_fn_call_not_unit.res create mode 100644 jscomp/build_tests/super_errors/fixtures/top_level_value_not_unit.res diff --git a/jscomp/build_tests/super_errors/expected/partial_app.res.expected b/jscomp/build_tests/super_errors/expected/partial_app.res.expected index 857ffbc5f5..647ef9111f 100644 --- a/jscomp/build_tests/super_errors/expected/partial_app.res.expected +++ b/jscomp/build_tests/super_errors/expected/partial_app.res.expected @@ -7,4 +7,10 @@ 5 │ f(1, 2) 6 │ - Toplevel expression is expected to have unit type. \ No newline at end of file + This function call is at the top level and is expected to return `unit`. But, it's returning `int => int`. + +In ReScript, anything at the top level must evaluate to `unit`. You can fix this by assigning the expression to a value, or piping it into the `ignore` function. + +Possible solutions: +- Assigning to a value that is then ignored: `let _ = yourFunctionCall()` +- Piping into the built-in ignore function to ignore the result: `yourFunctionCall()->ignore` \ No newline at end of file diff --git a/jscomp/build_tests/super_errors/expected/top_level_fn_call_not_unit.res.expected b/jscomp/build_tests/super_errors/expected/top_level_fn_call_not_unit.res.expected new file mode 100644 index 0000000000..c6669459a6 --- /dev/null +++ b/jscomp/build_tests/super_errors/expected/top_level_fn_call_not_unit.res.expected @@ -0,0 +1,16 @@ + + Warning number 109 (configured as error) + /.../fixtures/top_level_fn_call_not_unit.res:3:1-18 + + 1 │ let returnsSomething = () => 123 + 2 │ + 3 │ returnsSomething() + 4 │ + + This function call is at the top level and is expected to return `unit`. But, it's returning `int`. + +In ReScript, anything at the top level must evaluate to `unit`. You can fix this by assigning the expression to a value, or piping it into the `ignore` function. + +Possible solutions: +- Assigning to a value that is then ignored: `let _ = yourFunctionCall()` +- Piping into the built-in ignore function to ignore the result: `yourFunctionCall()->ignore` \ No newline at end of file diff --git a/jscomp/build_tests/super_errors/expected/top_level_value_not_unit.res.expected b/jscomp/build_tests/super_errors/expected/top_level_value_not_unit.res.expected new file mode 100644 index 0000000000..a4d06b536d --- /dev/null +++ b/jscomp/build_tests/super_errors/expected/top_level_value_not_unit.res.expected @@ -0,0 +1,14 @@ + + Warning number 109 (configured as error) + /.../fixtures/top_level_value_not_unit.res:1:1-4 + + 1 │ 1234 + 2 │ + + This is at the top level and is expected to return `unit`. But, it's returning `int`. + +In ReScript, anything at the top level must evaluate to `unit`. You can fix this by assigning the expression to a value, or piping it into the `ignore` function. + +Possible solutions: +- Assigning to a value that is then ignored: `let _ = yourExpression` +- Piping into the built-in ignore function to ignore the result: `yourExpression->ignore` \ No newline at end of file diff --git a/jscomp/build_tests/super_errors/fixtures/top_level_fn_call_not_unit.res b/jscomp/build_tests/super_errors/fixtures/top_level_fn_call_not_unit.res new file mode 100644 index 0000000000..25fae76b0d --- /dev/null +++ b/jscomp/build_tests/super_errors/fixtures/top_level_fn_call_not_unit.res @@ -0,0 +1,3 @@ +let returnsSomething = () => 123 + +returnsSomething() diff --git a/jscomp/build_tests/super_errors/fixtures/top_level_value_not_unit.res b/jscomp/build_tests/super_errors/fixtures/top_level_value_not_unit.res new file mode 100644 index 0000000000..81c545efeb --- /dev/null +++ b/jscomp/build_tests/super_errors/fixtures/top_level_value_not_unit.res @@ -0,0 +1 @@ +1234 diff --git a/jscomp/ext/warnings.ml b/jscomp/ext/warnings.ml index 35f8e42ee8..f438ffdcda 100644 --- a/jscomp/ext/warnings.ml +++ b/jscomp/ext/warnings.ml @@ -26,6 +26,8 @@ type loc = { loc_ghost : bool; } +type topLevelUnitHelp = FunctionCall | Other + type t = | Comment_start (* 1 *) | Comment_not_end (* 2 *) @@ -83,7 +85,7 @@ type t = | Bs_unimplemented_primitive of string (* 106 *) | Bs_integer_literal_overflow (* 107 *) | Bs_uninterpreted_delimiters of string (* 108 *) - | Bs_toplevel_expression_unit (* 109 *) + | Bs_toplevel_expression_unit of (string * topLevelUnitHelp) option (* 109 *) (* If you remove a warning, leave a hole in the numbering. NEVER change the numbers of existing warnings. @@ -148,7 +150,7 @@ let number = function | Bs_unimplemented_primitive _ -> 106 | Bs_integer_literal_overflow -> 107 | Bs_uninterpreted_delimiters _ -> 108 - | Bs_toplevel_expression_unit -> 109 + | Bs_toplevel_expression_unit _ -> 109 let last_warning_number = 110 @@ -490,8 +492,23 @@ let message = function | Bs_integer_literal_overflow -> "Integer literal exceeds the range of representable integers of type int" | Bs_uninterpreted_delimiters s -> "Uninterpreted delimiters " ^ s - | Bs_toplevel_expression_unit -> - "Toplevel expression is expected to have unit type." + | Bs_toplevel_expression_unit help -> + Printf.sprintf "This%sis at the top level and is expected to return `unit`. But, it's returning %s.\n\nIn ReScript, anything at the top level must evaluate to `unit`. You can fix this by assigning the expression to a value, or piping it into the `ignore` function.%s" + (match help with + | Some (_, FunctionCall) -> " function call " + | _ -> " ") + + (match help with + | Some (returnType, _) -> Printf.sprintf "`%s`" returnType + | None -> "something that is not `unit`") + + (match help with + | Some (_, helpTyp) -> + let helpText = (match helpTyp with + | FunctionCall -> "yourFunctionCall()" + | Other -> "yourExpression") in + Printf.sprintf "\n\nPossible solutions:\n- Assigning to a value that is then ignored: `let _ = %s`\n- Piping into the built-in ignore function to ignore the result: `%s->ignore`" helpText helpText + | _ -> "") let sub_locs = function | Deprecated (_, def, use) -> diff --git a/jscomp/ext/warnings.mli b/jscomp/ext/warnings.mli index 4e36bc00e5..d5d8445c20 100644 --- a/jscomp/ext/warnings.mli +++ b/jscomp/ext/warnings.mli @@ -19,6 +19,8 @@ type loc = { loc_ghost : bool; } +type topLevelUnitHelp = FunctionCall | Other + type t = | Comment_start (* 1 *) | Comment_not_end (* 2 *) @@ -76,7 +78,7 @@ type t = | Bs_unimplemented_primitive of string (* 106 *) | Bs_integer_literal_overflow (* 107 *) | Bs_uninterpreted_delimiters of string (* 108 *) - | Bs_toplevel_expression_unit (* 109 *) + | Bs_toplevel_expression_unit of (string * topLevelUnitHelp) option (* 109 *) val parse_options : bool -> string -> unit diff --git a/jscomp/ml/typecore.ml b/jscomp/ml/typecore.ml index 3573e7819a..2c1e471ce7 100644 --- a/jscomp/ml/typecore.ml +++ b/jscomp/ml/typecore.ml @@ -3743,11 +3743,21 @@ let type_expression env sexp = Typetexp.reset_type_variables(); begin_def(); let exp = type_exp env sexp in - if Warnings.is_active Bs_toplevel_expression_unit then + if Warnings.is_active (Bs_toplevel_expression_unit None) then (try unify env exp.exp_type (instance_def Predef.type_unit) with - | Unify _ - | Tags _ -> Location.prerr_warning sexp.pexp_loc Bs_toplevel_expression_unit); + | Unify _ -> + let buffer = Buffer.create 10 in + let formatter = Format.formatter_of_buffer buffer in + Printtyp.type_expr formatter exp.exp_type; + Format.pp_print_flush formatter (); + let returnType = Buffer.contents buffer in + Location.prerr_warning sexp.pexp_loc (Bs_toplevel_expression_unit ( + match sexp.pexp_desc with + | Pexp_apply _ -> Some (returnType, FunctionCall) + | _ -> Some (returnType, Other) + )) + | Tags _ -> Location.prerr_warning sexp.pexp_loc (Bs_toplevel_expression_unit None)); end_def(); if not (is_nonexpansive exp) then generalize_expansive env exp.exp_type; generalize exp.exp_type; From dd2b3f76b3d1cd12c3cabe9d9c27635e6295b205 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Tue, 19 Sep 2023 08:16:14 +0200 Subject: [PATCH 2/3] indent message --- .../super_errors/expected/partial_app.res.expected | 8 ++++---- .../expected/top_level_fn_call_not_unit.res.expected | 8 ++++---- .../expected/top_level_value_not_unit.res.expected | 8 ++++---- jscomp/ext/warnings.ml | 4 ++-- 4 files changed, 14 insertions(+), 14 deletions(-) diff --git a/jscomp/build_tests/super_errors/expected/partial_app.res.expected b/jscomp/build_tests/super_errors/expected/partial_app.res.expected index 647ef9111f..99971bc90b 100644 --- a/jscomp/build_tests/super_errors/expected/partial_app.res.expected +++ b/jscomp/build_tests/super_errors/expected/partial_app.res.expected @@ -9,8 +9,8 @@ This function call is at the top level and is expected to return `unit`. But, it's returning `int => int`. -In ReScript, anything at the top level must evaluate to `unit`. You can fix this by assigning the expression to a value, or piping it into the `ignore` function. + In ReScript, anything at the top level must evaluate to `unit`. You can fix this by assigning the expression to a value, or piping it into the `ignore` function. -Possible solutions: -- Assigning to a value that is then ignored: `let _ = yourFunctionCall()` -- Piping into the built-in ignore function to ignore the result: `yourFunctionCall()->ignore` \ No newline at end of file + Possible solutions: + - Assigning to a value that is then ignored: `let _ = yourFunctionCall()` + - Piping into the built-in ignore function to ignore the result: `yourFunctionCall()->ignore` \ No newline at end of file diff --git a/jscomp/build_tests/super_errors/expected/top_level_fn_call_not_unit.res.expected b/jscomp/build_tests/super_errors/expected/top_level_fn_call_not_unit.res.expected index c6669459a6..5480aeda58 100644 --- a/jscomp/build_tests/super_errors/expected/top_level_fn_call_not_unit.res.expected +++ b/jscomp/build_tests/super_errors/expected/top_level_fn_call_not_unit.res.expected @@ -9,8 +9,8 @@ This function call is at the top level and is expected to return `unit`. But, it's returning `int`. -In ReScript, anything at the top level must evaluate to `unit`. You can fix this by assigning the expression to a value, or piping it into the `ignore` function. + In ReScript, anything at the top level must evaluate to `unit`. You can fix this by assigning the expression to a value, or piping it into the `ignore` function. -Possible solutions: -- Assigning to a value that is then ignored: `let _ = yourFunctionCall()` -- Piping into the built-in ignore function to ignore the result: `yourFunctionCall()->ignore` \ No newline at end of file + Possible solutions: + - Assigning to a value that is then ignored: `let _ = yourFunctionCall()` + - Piping into the built-in ignore function to ignore the result: `yourFunctionCall()->ignore` \ No newline at end of file diff --git a/jscomp/build_tests/super_errors/expected/top_level_value_not_unit.res.expected b/jscomp/build_tests/super_errors/expected/top_level_value_not_unit.res.expected index a4d06b536d..449338322a 100644 --- a/jscomp/build_tests/super_errors/expected/top_level_value_not_unit.res.expected +++ b/jscomp/build_tests/super_errors/expected/top_level_value_not_unit.res.expected @@ -7,8 +7,8 @@ This is at the top level and is expected to return `unit`. But, it's returning `int`. -In ReScript, anything at the top level must evaluate to `unit`. You can fix this by assigning the expression to a value, or piping it into the `ignore` function. + In ReScript, anything at the top level must evaluate to `unit`. You can fix this by assigning the expression to a value, or piping it into the `ignore` function. -Possible solutions: -- Assigning to a value that is then ignored: `let _ = yourExpression` -- Piping into the built-in ignore function to ignore the result: `yourExpression->ignore` \ No newline at end of file + Possible solutions: + - Assigning to a value that is then ignored: `let _ = yourExpression` + - Piping into the built-in ignore function to ignore the result: `yourExpression->ignore` \ No newline at end of file diff --git a/jscomp/ext/warnings.ml b/jscomp/ext/warnings.ml index f438ffdcda..64807763eb 100644 --- a/jscomp/ext/warnings.ml +++ b/jscomp/ext/warnings.ml @@ -493,7 +493,7 @@ let message = function "Integer literal exceeds the range of representable integers of type int" | Bs_uninterpreted_delimiters s -> "Uninterpreted delimiters " ^ s | Bs_toplevel_expression_unit help -> - Printf.sprintf "This%sis at the top level and is expected to return `unit`. But, it's returning %s.\n\nIn ReScript, anything at the top level must evaluate to `unit`. You can fix this by assigning the expression to a value, or piping it into the `ignore` function.%s" + Printf.sprintf "This%sis at the top level and is expected to return `unit`. But, it's returning %s.\n\n In ReScript, anything at the top level must evaluate to `unit`. You can fix this by assigning the expression to a value, or piping it into the `ignore` function.%s" (match help with | Some (_, FunctionCall) -> " function call " | _ -> " ") @@ -507,7 +507,7 @@ let message = function let helpText = (match helpTyp with | FunctionCall -> "yourFunctionCall()" | Other -> "yourExpression") in - Printf.sprintf "\n\nPossible solutions:\n- Assigning to a value that is then ignored: `let _ = %s`\n- Piping into the built-in ignore function to ignore the result: `%s->ignore`" helpText helpText + Printf.sprintf "\n\n Possible solutions:\n - Assigning to a value that is then ignored: `let _ = %s`\n - Piping into the built-in ignore function to ignore the result: `%s->ignore`" helpText helpText | _ -> "") let sub_locs = function From 7829798678b610d8c609b3f18d2d1406e0293734 Mon Sep 17 00:00:00 2001 From: Gabriel Nordeborn Date: Tue, 19 Sep 2023 09:28:42 +0200 Subject: [PATCH 3/3] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 51d816a9fa..8a88a8c76c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,7 @@ #### :nail_care: Polish - A little performance improvement for JSX V4 runtime helper by removing one object allocation for components with key prop. https://github.com/rescript-lang/rescript-compiler/pull/6376 +- The error message for "toplevel expressions should evaluate to unit" has been revamped and improved. https://github.com/rescript-lang/rescript-compiler/pull/6407 # 11.0.0-rc.3