Improve unclear error and warning messages

CHANGELOG.md

@@ -46,6 +46,7 @@
 - Refactor analysis to decouple I/O from core logic. https://github.com/rescript-lang/rescript/pull/8426
 - Deprecate `Stdlib_Error` and `Stdlib_Exn` modules in favor of `JsError/JsExn`. https://github.com/rescript-lang/rescript/pull/8404
 - Remove vendored `Json` library and use `yojson` and `lsp` library for analysis. https://github.com/rescript-lang/rescript/pull/8436
+- Improve clarity of various error and warning messages. https://github.com/rescript-lang/rescript/pull/8460
 
 #### :house: Internal
 

compiler/ext/warnings.ml

@@ -373,7 +373,7 @@ let message = function
     Printf.sprintf "this open statement shadows the %s %s (which is later used)"
       kind s
   | Attribute_payload (a, s) ->
-    Printf.sprintf "illegal payload for attribute '%s'.\n%s" a s
+    Printf.sprintf "illegal payload for attribute @%s.\n%s" a s
   | No_cmi_file (name, None) ->
     "no cmi file was found in path for module " ^ name
   | No_cmi_file (name, Some msg) ->
@@ -383,26 +383,26 @@ let message = function
     Printf.sprintf
       "Code should not depend on the actual values of\n\
        this constructor's arguments. They are only for information\n\
-       and may change in future versions. (See manual section 8.5)"
+       and may change in future versions."
   | Unreachable_case ->
     "this match case is unreachable.\n\
      Consider replacing it with a refutation case '<pat> -> .'"
   | Misplaced_attribute attr_name ->
-    Printf.sprintf "the %S attribute cannot appear in this context" attr_name
+    Printf.sprintf "the @%s attribute cannot appear in this context" attr_name
   | Duplicated_attribute attr_name ->
-    Printf.sprintf "the %S attribute is used more than once on this expression"
+    Printf.sprintf "the @%s attribute is used more than once on this expression"
       attr_name
   | Ambiguous_pattern vars ->
     let msg =
       let vars = List.sort String.compare vars in
       match vars with
       | [] -> assert false
       | [x] -> "variable " ^ x
-      | _ :: _ -> "variables " ^ String.concat "," vars
+      | _ :: _ -> "variables " ^ String.concat ", " vars
     in
     Printf.sprintf
       "Ambiguous or-pattern variables under guard;\n\
-       %s may match different arguments. (See manual section 8.5)"
+       %s may match different arguments."
       msg
   | Unused_module s -> "unused module " ^ s ^ "."
   | Constraint_on_gadt ->

compiler/frontend/ast_attributes.ml

@@ -161,21 +161,31 @@ let process_derive_type (attrs : t) : derive_attr * t =
 
 (* duplicated attributes not allowed *)
 let iter_process_bs_string_int_unwrap_uncurry (attrs : t) =
-  let st = ref `Nothing in
-  let assign v (({loc; _}, _) as attr : attr) =
-    if !st = `Nothing then (
-      Used_attributes.mark_used_attribute attr;
-      st := v)
-    else Bs_syntaxerr.err loc Conflict_attributes
+  let attr_name = function
+    | `String -> "string"
+    | `Int -> "int"
+    | `Ignore -> "ignore"
+    | `Unwrap -> "unwrap"
   in
-  Ext_list.iter attrs (fun (({txt; loc = _}, _) as attr) ->
-      match txt with
-      | "string" -> assign `String attr
-      | "int" -> assign `Int attr
-      | "ignore" -> assign `Ignore attr
-      | "unwrap" -> assign `Unwrap attr
-      | _ -> ());
-  !st
+  (* Collect all of @string/@int/@ignore/@unwrap so the conflict error can
+     report every attribute involved, not just the first pair. *)
+  let found : ([`String | `Int | `Ignore | `Unwrap] * _) list =
+    Ext_list.filter_map attrs (fun (({txt; _}, _) as attr) ->
+        match txt with
+        | "string" -> Some (`String, attr)
+        | "int" -> Some (`Int, attr)
+        | "ignore" -> Some (`Ignore, attr)
+        | "unwrap" -> Some (`Unwrap, attr)
+        | _ -> None)
+  in
+  match found with
+  | [] -> `Nothing
+  | [(v, attr)] ->
+    Used_attributes.mark_used_attribute attr;
+    (v :> [`Nothing | `String | `Int | `Ignore | `Unwrap])
+  | (_, ({loc; _}, _)) :: _ as conflicting ->
+    Bs_syntaxerr.err loc
+      (Conflict_attributes (List.map (fun (v, _) -> attr_name v) conflicting))
 
 let iter_process_bs_string_as (attrs : t) : string option =
   let st = ref None in

compiler/frontend/ast_external_process.ml

@@ -623,7 +623,7 @@ let external_desc_of_non_obj (loc : Location.t) (st : external_desc)
         "Ill defined attribute %@set_index (arity of 3)"
   | {set_index = true} ->
     Bs_syntaxerr.err loc
-      (Conflict_ffi_attribute "Attribute found that conflicts with %@set_index")
+      (Conflict_ffi_attribute "Attribute found that conflicts with @set_index")
   | {
    get_index = true;
    val_name = None;
@@ -649,7 +649,7 @@ let external_desc_of_non_obj (loc : Location.t) (st : external_desc)
         arg_type_specs_length
   | {get_index = true} ->
     Bs_syntaxerr.err loc
-      (Conflict_ffi_attribute "Attribute found that conflicts with %@get_index")
+      (Conflict_ffi_attribute "Attribute found that conflicts with @get_index")
   | {
    module_as_val = Some external_module_name;
    get_index = false;
@@ -750,7 +750,7 @@ let external_desc_of_non_obj (loc : Location.t) (st : external_desc)
     else Js_call {splice; name; external_module_name; scopes; tagged_template}
   | {call_name = Some _} ->
     Bs_syntaxerr.err loc
-      (Conflict_ffi_attribute "Attribute found that conflicts with %@val")
+      (Conflict_ffi_attribute "Attribute found that conflicts with @val")
   | {
    val_name = Some {name; source = _};
    external_module_name;
@@ -777,7 +777,7 @@ let external_desc_of_non_obj (loc : Location.t) (st : external_desc)
     Js_var {name; external_module_name; scopes}
   | {val_name = Some _} ->
     Bs_syntaxerr.err loc
-      (Conflict_ffi_attribute "Attribute found that conflicts with %@val")
+      (Conflict_ffi_attribute "Attribute found that conflicts with @val")
   | {
    splice;
    scopes;
@@ -856,7 +856,7 @@ let external_desc_of_non_obj (loc : Location.t) (st : external_desc)
     Js_new {name; external_module_name; splice; scopes}
   | {new_name = Some _} ->
     Bs_syntaxerr.err loc
-      (Conflict_ffi_attribute "Attribute found that conflicts with %@new")
+      (Conflict_ffi_attribute "Attribute found that conflicts with @new")
   | {
    set_name = Some {name; source = _};
    val_name = None;

compiler/frontend/bs_syntaxerr.ml

@@ -27,7 +27,7 @@ type untagged_variant = OnlyOneUnknown | AtMostOneObject | AtMostOneArray
 type error =
   | Unsupported_predicates
   | Duplicated_bs_deriving
-  | Conflict_attributes
+  | Conflict_attributes of string list
   | Expect_int_literal
   | Expect_string_literal
   | Expect_int_or_string_or_json_literal
@@ -58,28 +58,36 @@ let pp_error fmt err =
     | Optional_in_uncurried_bs_attribute ->
       "Uncurried function doesn't support optional arguments yet"
     | Expect_opt_in_bs_return_to_opt ->
-      "%@return directive *_to_opt expect return type to be \n\
-       syntax wise `_ option` for safety"
-    | Not_supported_directive_in_bs_return -> "Not supported return directive"
+      "This @return directive requires the external's return type to be an \
+       option, e.g. `option<int>`."
+    | Not_supported_directive_in_bs_return ->
+      "Unsupported @return directive. Supported directives are `null_to_opt`, \
+       `null_undefined_to_opt` (or `nullable`), and `identity`."
     | Illegal_attribute -> "Illegal attributes"
-    | Unsupported_predicates -> "unsupported predicates"
-    | Duplicated_bs_deriving -> "duplicate @deriving attribute"
-    | Conflict_attributes -> "conflicting attributes "
-    | Expect_string_literal -> "expect string literal "
-    | Expect_int_literal -> "expect int literal "
+    | Unsupported_predicates -> "Unsupported predicates"
+    | Duplicated_bs_deriving ->
+      "Duplicate @deriving attribute; a type can only have one."
+    | Conflict_attributes names ->
+      Printf.sprintf
+        "Conflicting attributes: %s cannot be combined; use only one."
+        (String.concat ", " (List.map (fun n -> "@" ^ n) names))
+    | Expect_string_literal -> "Expected a string literal."
+    | Expect_int_literal ->
+      "The @as payload on an @int variant must be an integer literal, e.g. \
+       @as(42)."
     | Expect_int_or_string_or_json_literal ->
-      "expect int, string literal or json literal {json|text here|json} "
+      "The @as payload must be an integer, string, or JSON literal."
     | Invalid_underscore_type_in_external ->
       "_ is not allowed in combination with external optional type"
-    | Invalid_bs_string_type -> "Not a valid type for %@string"
-    | Invalid_bs_int_type -> "Not a valid type for %@int"
+    | Invalid_bs_string_type -> "Not a valid type for @string"
+    | Invalid_bs_int_type -> "Not a valid type for @int"
     | Invalid_bs_unwrap_type ->
-      "Not a valid type for %@unwrap. Type must be an inline variant (closed), \
+      "Not a valid type for @unwrap. Type must be an inline variant (closed), \
        and\n\
        each constructor must have an argument."
     | Conflict_ffi_attribute str -> "Conflicting attributes: " ^ str
     | Bs_this_simple_pattern ->
-      "%@this expect its pattern variable to be simple form"
+      "@this expects its first parameter to be a simple variable (or `_`)."
     | Experimental_feature_not_enabled feature ->
       Printf.sprintf
         "Experimental feature not enabled: %s. Enable it by setting \"%s\" to \

compiler/frontend/bs_syntaxerr.mli

@@ -27,7 +27,7 @@ type untagged_variant = OnlyOneUnknown | AtMostOneObject | AtMostOneArray
 type error =
   | Unsupported_predicates
   | Duplicated_bs_deriving
-  | Conflict_attributes
+  | Conflict_attributes of string list
   | Expect_int_literal
   | Expect_string_literal
   | Expect_int_or_string_or_json_literal

compiler/ml/ast_untagged_variants.ml

@@ -73,9 +73,10 @@ let report_error ppf =
   function
   | InvalidVariantAsAnnotation ->
     fprintf ppf
-      "A variant case annotation @as(...) must be a string or integer, \
-       boolean, null, undefined"
-  | Duplicated_bs_as -> fprintf ppf "duplicate @as "
+      "A variant case annotation @as(...) must be a string, integer, boolean, \
+       null, or undefined."
+  | Duplicated_bs_as ->
+    fprintf ppf "Duplicate @as annotation; only one @as is allowed here."
   | InvalidVariantTagAnnotation ->
     fprintf ppf "A variant tag annotation @tag(...) must be a string"
   | InvalidUntaggedVariantDefinition untagged_variant ->

compiler/ml/rec_check.ml

@@ -472,7 +472,7 @@ let check_recursive_bindings valbinds =
 let report_error ppf = function
   | Illegal_letrec_expr ->
     Format.fprintf ppf
-      "This kind of expression is not allowed as right-hand side of `let rec'"
+      "This kind of expression is not allowed as right-hand side of `let rec`"
 
 let () =
   Location.register_error_of_exn (function

compiler/ml/translattribute.ml

@@ -79,10 +79,10 @@ let rec add_inline_attribute (expr : Lambda.lambda) loc attributes =
   | Lprim (((Pjs_fn_make _ | Pjs_fn_make_unit) as p), [e], l), _ ->
     Lambda.Lprim (p, [add_inline_attribute e loc attributes], l)
   | expr, Always_inline ->
-    Location.prerr_warning loc (Warnings.Misplaced_attribute "inline1");
+    Location.prerr_warning loc (Warnings.Misplaced_attribute "inline");
     expr
   | expr, Never_inline ->
-    Location.prerr_warning loc (Warnings.Misplaced_attribute "inline2");
+    Location.prerr_warning loc (Warnings.Misplaced_attribute "inline");
     expr
 
 (* Get the [@inlined] attribute payload (or default if not present).

compiler/ml/translmod.ml

@@ -486,7 +486,9 @@ let transl_implementation module_name (str, cc) =
 
 let report_error ppf = function
   | Fragile_pattern_in_toplevel ->
-    Format.fprintf ppf "@[Such fragile pattern not allowed in the toplevel@]"
+    Format.fprintf ppf
+      "@[This pattern can fail to match, so it is not allowed in a top-level \
+       `let` binding.@]"
 
 let () =
   Location.register_error_of_exn (function

compiler/ml/typecore.ml

@@ -4612,7 +4612,7 @@ let report_error env loc ppf error =
             (Ident.name id))
       (function ppf -> fprintf ppf "but on the right-hand side it has type")
   | Multiply_bound_variable name ->
-    fprintf ppf "Variable %s is bound several times in this matching" name
+    fprintf ppf "Variable %s is bound several times in this pattern" name
   | Orpat_vars (id, valid_idents) ->
     fprintf ppf "Variable %s must occur on both sides of this | pattern"
       (Ident.name id);
@@ -4663,7 +4663,7 @@ let report_error env loc ppf error =
     | _ ->
       fprintf ppf
         "@[<v>@[<2>This can't be called, it's not a function.@]@,\
-         The function has type: %a@]"
+         It has type: %a@]"
         type_expr typ)
   | Apply_wrong_label (l, ty) ->
     let print_message ppf = function
@@ -4782,16 +4782,21 @@ let report_error env loc ppf error =
   | Not_a_packed_module ty ->
     fprintf ppf "This expression is packed module, but the expected type is@ %a"
       type_expr ty
-  | Unexpected_existential -> fprintf ppf "Unexpected existential"
+  | Unexpected_existential ->
+    fprintf ppf
+      "This pattern introduces an existential type from a GADT constructor, \
+       which is not allowed here. Match on it inside a switch instead."
   | Unqualified_gadt_pattern (tpath, name) ->
-    fprintf ppf "@[The GADT constructor %s of type %a@ %s.@]" name Printtyp.path
-      tpath "must be qualified in this pattern"
+    fprintf ppf
+      "@[The GADT constructor %s of type %a@ must be written with its module \
+       prefix in this pattern.@]"
+      name Printtyp.path tpath
   | Invalid_interval ->
     fprintf ppf "@[Only character intervals are supported in patterns.@]"
   | Invalid_for_loop_index ->
     fprintf ppf "@[Invalid for-loop index: only variables and _ are allowed.@]"
   | No_value_clauses ->
-    fprintf ppf "None of the patterns in this 'match' expression match values."
+    fprintf ppf "None of the patterns in this switch match values."
   | Exception_pattern_below_toplevel ->
     fprintf ppf
       "@[Exception patterns must be at the top level of a match case.@]"
@@ -4823,7 +4828,7 @@ let report_error env loc ppf error =
   | Unknown_literal (n, m) ->
     fprintf ppf "Unknown modifier '%c' for literal %s%c" m n m
   | Illegal_letrec_pat ->
-    fprintf ppf "Only variables are allowed as left-hand side of `let rec'"
+    fprintf ppf "Only variables are allowed as left-hand side of `let rec`"
   | Empty_record_literal ->
     fprintf ppf
       "Empty record literal {} should be type annotated or used in a record \
@@ -4974,7 +4979,7 @@ let report_error env loc ppf error =
       name type_expr typ
   | Type_params_not_supported lid ->
     fprintf ppf
-      "The type %a@ has type parameters, but type parameters is not supported \
+      "The type %a@ has type parameters, but type parameters are not supported \
        here."
       longident lid
   | Field_access_on_dict_type ->

compiler/ml/typemod.ml

@@ -1885,7 +1885,8 @@ let report_error ppf = function
     fprintf ppf "This expression is not a packed module. It has type@ %a"
       type_expr ty
   | Incomplete_packed_module ty ->
-    fprintf ppf "The type of this packed module contains variables:@ %a"
+    fprintf ppf
+      "The type of this first-class module still contains type variables:@ %a"
       type_expr ty
   | Scoping_pack (lid, ty) ->
     fprintf ppf "The type %a in this module cannot be exported.@ " longident lid;

compiler/ml/typetexp.ml

@@ -786,18 +786,19 @@ let report_error env ppf = function
   | Unbound_type_constructor_2 p ->
     fprintf ppf "The type constructor@ %a@ is not yet completely defined" path p
   | Type_arity_mismatch (lid, expected, provided) ->
+    let plural n = if n = 1 then "" else "s" in
     if expected == 0 then
       fprintf ppf
-        "@[The type %a is not generic so expects no arguments,@ but is here \
-         applied to %i argument(s).@ Have you tried removing the angular \
-         brackets `<` and `>` and the@ arguments within them and just writing \
-         `%a` instead? @]"
-        longident lid provided longident lid
+        "@[The type `%a` is not generic so expects no arguments,@ but is here \
+         given %i argument%s.@ Have you tried removing the angular brackets \
+         `<` and `>` and the@ arguments within them and just writing `%a` \
+         instead? @]"
+        longident lid provided (plural provided) longident lid
     else
       fprintf ppf
-        "@[The type constructor %a@ expects %i argument(s),@ but is here \
-         applied to %i argument(s)@]"
-        longident lid expected provided
+        "@[The type constructor `%a`@ expects %i type argument%s,@ but is \
+         given %i@]"
+        longident lid expected (plural expected) provided
   | Type_mismatch trace ->
     Printtyp.report_unification_error ppf Env.empty trace
       (function
@@ -828,7 +829,10 @@ let report_error env ppf = function
       Misc.did_you_mean ppf (fun () -> ["`" ^ s])
     | _ -> ())
   | Invalid_variable_name name ->
-    fprintf ppf "The type variable name %s is not allowed in programs" name
+    fprintf ppf
+      "The type variable name %s is not allowed; type variable names cannot \
+       start with an underscore."
+      name
   | Cannot_quantify (name, v) ->
     fprintf ppf
       "@[<hov>The universal type variable '%s cannot be generalized:@ %s.@]"

compiler/syntax/src/res_core.ml

@@ -6688,7 +6688,7 @@ and parse_newline_or_semicolon_structure p =
     else
       Parser.err ~start_pos:p.prev_end_pos ~end_pos:p.end_pos p
         (Diagnostics.message
-           "consecutive statements on a line must be separated by ';' or a \
+           "Consecutive statements on a line must be separated by ';' or a \
             newline")
   | _ -> ()
 

tests/analysis_tests/tests/not_compiled/expected/Diagnostics.res.txt

@@ -1,6 +1,6 @@
 [
   {
-    "message": "consecutive statements on a line must be separated by ';' or a newline",
+    "message": "Consecutive statements on a line must be separated by ';' or a newline",
     "range": {
       "end": { "character": 6, "line": 2 },
       "start": { "character": 4, "line": 2 }

tests/build_tests/super_errors/expected/UntaggedDuplicatedBsAs.res.expected

@@ -6,4 +6,4 @@
   2 │ type t = | @as("x") @as("y") A | B
   3 │ 
 
-  duplicate @as
+  Duplicate @as annotation; only one @as is allowed here.

tests/build_tests/super_errors/expected/UntaggedInvalidVariantAsAnnotation.res.expected

@@ -5,4 +5,4 @@
   1 │ type t = | @as(foo) A | B
   2 │ 
 
-  A variant case annotation @as(...) must be a string or integer, boolean, null, undefined
+  A variant case annotation @as(...) must be a string, integer, boolean, null, or undefined.