Try making top level unit error message clearer (#6407)

* try making top level unit error message clearer

* indent message

* changelog

Author: zth

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
 

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.
+  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`

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`

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`

jscomp/build_tests/super_errors/fixtures/top_level_fn_call_not_unit.res

@@ -0,0 +1,3 @@
+let returnsSomething = () => 123
+
+returnsSomething()

jscomp/build_tests/super_errors/fixtures/top_level_value_not_unit.res

@@ -0,0 +1 @@
+1234

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\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 " 
+        | _ -> " ") 
+
+        (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\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
   | Deprecated (_, def, use) ->

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
 

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;