Improve unclear error and warning messages (rescript-lang#8460)

* Improve non-function application error wording

Saying "The function has type" right after "it's not a function" was
contradictory. Use "It has type" instead.

* Improve @return(*_to_opt) error message

The old message was grammatically broken ("expect return type to be syntax
wise `_ option` for safety") and rendered `%@return` literally. State the
requirement plainly and show the option syntax.

* Improve @int variant @as payload error message

"expect int literal" was terse and contextless. Name the attribute and
show the expected form.

* Improve conflicting-attributes error message

"conflicting attributes" did not say which attributes conflict. List the
mutually-exclusive set (@string, @int, @ignore, @unwrap).

* Render @ correctly in FFI attribute error messages

These messages used "%@" where they meant "@" (e.g. "%@string",
"%@unwrap", "conflicts with %@Val"). pp_print_string emits it literally, so
users saw "%@". Use "@" so the attribute name reads correctly.

* Improve @this simple-pattern error message

Fix the "%@this" rendering and the broken grammar ("expect its pattern
variable to be simple form"). State what is required and why.

* Tidy the "expected a string literal" error message

Capitalize and use a full sentence instead of "expect string literal "
(trailing space). Kept generic since it is shared by @as payloads and
@@directive.

* Tidy the @as int/string/json literal error message

Replace "expect int, string literal or json literal {json|text here|json}"
with a plain sentence.

* Tighten @this and @return wording for accuracy

Verified against the raise sites:

- @this (ast_uncurry_gen.ml): the self parameter must satisfy
  is_single_variable_pattern_conservative, i.e. a plain variable or `_`. A
  constant pattern also fails and `_` is allowed, so "not a destructured
  pattern" was imprecise. State the requirement positively.
- @return (ast_external_process.ml): `nullable` and `null_undefined_to_opt`
  trigger this too, not just `*_to_opt` directives, so describe the option
  requirement generically rather than naming the directive shape.

* Fix subject-verb agreement in type-params-not-supported error

* Fix list grammar in @as variant annotation error

* Use ReScript-style quoting for `let rec` in letrec errors

Replace the OCaml-style `let rec' quoting with `let rec`.

* Drop OCaml-manual references from warning messages

Warnings 52 and 57 pointed users to "manual section 8.5" of the OCaml
manual, which is irrelevant to ReScript. Also add a space after the comma
when listing ambiguous or-pattern variables.

* Improve duplicate @as annotation error wording

* Improve duplicate @deriving attribute error wording

* Improve unsupported @return directive error message

List the supported directives instead of the terse "Not supported return
directive".

* Make duplicate @as message context-neutral

Duplicated_bs_as also fires for record fields (e.g. inline records) and
polymorphic-variant tags, not just variant cases, so the message should not
say "a variant case".

* Report the real attribute name in misplaced @inline warning

The misplaced-@inline warning hardcoded "inline1"/"inline2" as the
attribute name, leaking an internal distinction; the user wrote @inline. Use
the real attribute name.

* Use @-prefixed attribute names in attribute warnings

Warnings 47/53/54 referred to attributes as "inline" / 'inline'; ReScript
messages elsewhere use the @-prefixed form (@as, @deriving, ...). Use @inline
etc. for consistency.

* Name the conflicting attributes in the conflict error

Conflict_attributes now carries the list of attributes involved, so the
message reports all of them (e.g. "@string, @int, @unwrap") instead of the
full set of every possible attribute.

* Use 'switch' instead of OCaml ''match' expression' in no-value-clauses error

* Say 'pattern' instead of OCaml 'matching' in multiply-bound-variable error

* Clarify the fragile-pattern-in-toplevel error wording

* State the rule in the invalid type-variable-name error

"is not allowed in programs" was vague; type variable names cannot start
with an underscore.

* Fix pluralization and phrasing in type-arity-mismatch error

"expects 1 argument(s), but is here applied to 2 argument(s)" -> proper
singular/plural and "is given".

* Clarify packed-module / GADT-pattern / existential error messages

- first-class module instead of "packed module"
- explain that a cross-module GADT constructor needs its module prefix
- explain the existential-in-pattern error and point to switch

* Backtick the type-constructor name in type-arity-mismatch error

Quote the name like the other polished messages (and the same message's
zero-argument branch already does).

* Add changelog entry for error/warning message improvements

* Capitalize two lowercase error messages for consistency

Errors are sentence-case; "unsupported predicates" and the consecutive-
statements syntax error started lowercase.

* Cover new error-message branches flagged by codecov

- Add a fixture for a non-generic type applied to arguments (covers the
  zero-arity branch of the type-arity-mismatch message, and the plural case).
- Add an @ignore/@unwrap conflict fixture (covers those attr_name arms).
- Drop the unreachable `Nothing` arm from attr_name by typing the collected
  list to the four real tags and coercing the single-tag result.

Author: JonoPrest

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