Improve the type clash error message by Julow · Pull Request #12182 · ocaml/ocaml

Julow · 2023-04-13T16:57:03Z

Related: #12152

The second commit re-use the mechanism implemented in #11679 for type clash errors:

Error: The expression '43' has type int
       but an expression was expected of type int32

The third commit change the denomination for some expressions instead of the generic "expression".
For example:

The constant '42' has type ...

This field has type ...

This is similar to the previous PR (#11679):

The function 'foo' has type ...

This could be extended to most expressions (eg. object, unnamed function, tuple, match expression, etc..).

Julow · 2023-04-13T16:58:06Z

This is request for comment. I don't mind dropping the third commit or giving up the entire idea.

gasche · 2023-04-13T20:59:30Z

Interesting, thanks!

I think that I disagree with some of the category names. For example "The method x#m" reads wrong to me, x#m is a method call, m is the method. I suspect that I don't like "record field" for the same reason, but I wasn't able to find an example in the testsuite to make an opinion. Could you ensure that all special cases are covered by at least one test?

Julow · 2023-04-14T10:22:01Z

A more correct denomination might be "method call", "record field access". I don't think length is a problem at this point.

Perhaps the message could print "The method 'm' has type" and remove the x# part, that could not be nominal in some cases ?

Julow · 2023-04-14T10:22:43Z

+Line 3, characters 18-22:
+3 | let _ = print_int Cons
+                      ^^^^
+Error: This expression has type v but an expression was expected of type int


There's a bug here.

Julow · 2023-04-14T10:23:25Z

+- #12182: Improve the type clash error message.
+  For example, this message:
+    This expression has type ...
+  is changed into:
+    The constant '42' has type ...
+  (Jules Aguillon, review by Gabriel Scherer)


The change entry is perhaps too long ?

Fine with me. I like descriptive change entries.

Octachron · 2023-04-26T14:25:37Z

I believe that using the method 'm' or the field 'f' doesn't work because both don't have a type on their own. Consider for instance:

type ('a,'b,'c) t = { x:'a; y:'b; z:'c }
let f r = let a = r.z (1 + r.x) ("y" ^ r.y) in [None; r.z]

It would be confusing to state that

Error: The field 'z' has type int -> string -> 'a
       but an expression was expected of type 'b option

when there is no such constraints on the field z in general.

gasche · 2024-04-08T06:18:39Z

Do we want to revisit this or close it?

Octachron · 2024-04-08T07:54:44Z

Overall, I like the change; and if @Julow has the time I would be happy to revisit it.

Julow · 2024-04-08T08:28:13Z

Hi, I would be happy to revisit it. I also don't mind if this is closed even after I put some time into it.

Julow · 2024-04-08T08:29:47Z

I believe that using the method 'm' or the field 'f' doesn't work because both don't have a type on their own. Consider for instance:

To anchor the error message in the context, we could say "the method 'x#m'" or "the field 'r.f'" ?

Octachron · 2024-04-08T08:36:57Z

Possibly, but then we have the issue that the field r.f reads a bit strange when the field is f.
Another option would be to restructure the error message from

"%a has type" "The field f"

to

"In the record projection* `r.f`, the field `f` has type" (* projection => access? *)

(which also has the advantage of being localisable)

Julow · 2024-04-11T15:22:44Z

With the last commits, a denomination is used for multi-keyword expressions like for, while, if, match.
It's a useful confirmation that the error is about the containing expression rather than a nested one, for which the printed code is hard to read when spanning several lines. There's no example for some of these but this would become useful if other messages use the new mechanism, notably Apply_non_function and Not_a_function.

What do you think of applying this change to the other messages starting in "This expression" ?

I worry that adding an intro to the message makes reading harder as it's of irregular length. What do you think of these:
Simpler and still localisable: (currently implemented in this branch)

The field access "r.f" has type ...

More correct:

The field "f" has type ...
in the context of the field access "r.f"

Perhaps more uses of the context could be found ?

Octachron · 2024-04-11T15:25:16Z

+Line 1, characters 14-36:
+1 | let _ : int = while false do () done
+                  ^^^^^^^^^^^^^^^^^^^^^^
+Error: This while expression has type "unit"


We want inline code quote here is since while is not part of the grammatical structure of the sentence.

Implemented!

Octachron · 2024-04-12T15:26:12Z

Using field access and method call is fine with me. I think it is better to keep this PR simple, we can think later if we want to add more context.

P.S: Note that those messages are not localizable, the issue is that composing error messages by fragments like %a has type makes the hypothesis that the subject and the rest of the sentence can be translated independently. This may be not be the case if for instance the verb conjugation depends on the subject gender.

Octachron · 2024-05-14T11:42:27Z

+(** [sexp_for_hint] is used by error messages to report literals in their
    original formatting *)
-let unify_pat ?sdesc_for_hint env pat expected_ty =
+let unify_pat ?sexp_for_hint env pat expected_ty =


This PR is still only using the description for hints in the pattern case. I propose to keep the code unchanged.

Octachron · 2024-05-14T11:44:10Z

+(** [sexp_for_hint] is used by error messages to report literals in their
    original formatting *)
-let unify_exp ?sdesc_for_hint env exp expected_ty =
+let unify_exp ?sexp_for_hint env exp expected_ty =


The sexp_for_hint is used for more than hints now. I think it would be better to rename it to sexp and make it non-optional. That will fix the missing constructors case.

Thanks, this fixed many cases indeed.

Octachron · 2024-05-14T11:47:59Z

+    | Pexp_constant { pconst_desc = Pconst_string (s, _, None); _ } ->
+        (* Avoid adding quotes around a quoted string constant but make sure to
+           use consistent inline code style. *)
+        Format.pp_print_string ppf (String.escaped s)


This is only an issue with dumb terminal and in the testsuite, isn't it? Moreover using String.escaped does not mesh well with non-ascii contents. I would propose to skip this step.

You are right, making the strings nominal is problematic. I removed it.

Octachron · 2024-05-14T11:51:47Z


 let report_expr_type_clash_hints exp diff =
  match exp with
-  | Some (Pexp_constant const) -> report_literal_type_constraint const diff


match exp.pexp_desc with ?

Octachron · 2024-05-14T11:59:06Z

+        let d = match denom with Some d -> d | None -> "expression" in
+        fprintf ppf "%s" d
+      in
+      report_general_this_exp denom ppf


It seems to me that the two report_this_exp function could be merged without loss of readability.

Do you mean change report_this_pexp to take an option pexp ?

Yes, I was thinking of making the choice of denomination in one place:

let report_this_pexp_opt denom ppf exp = let denom ppf = match denom, exp with | Some d, _ -> fprintf ppf "%s" d | None, Some exp -> pp_pexp_denom ppf exp | None, None -> fprintf ppf "expression" in let nexp = Option.bind exp Pprintast.Doc.(nominal_exp_doc longident) in match nexp with | Some nexp -> fprintf ppf "The %t %a" denom pp_doc nexp | _ -> fprintf ppf "This %t" denom

Looking at the call sites, we could also make the sentence more readable by adding the has type suffix.

You removed the wrapping of nexp with Style.as_inline_code in your snippet. Is that intended ?

No, that was a mistake.

Thanks, I've changed the code.

Octachron · 2024-05-21T11:30:14Z

There is non-trivial conflict with #13169 (because I didn't convert most of the AST printers which are generally used to print immediately). Assuming that #13169 is merged first, @Julow don't hesitate to ping for fixing the conflict.

Julow

Rebased and ready for an other review. Thanks for waiting!

Julow · 2024-07-12T16:51:19Z

                    ^^^^
-Error: This expression has type "bool" but an expression was expected of type
-         "int"
+Error: The constructor "\#true" has type "bool"


This could be improved.

This looks like an instance of #13263.

I can have a look at this issue later.

Julow · 2024-07-12T16:51:39Z


 let report_expr_type_clash_hints exp diff =
  match exp with
-  | Some (Pexp_constant const) -> report_literal_type_constraint const diff


Julow · 2024-07-12T16:52:08Z

+        let d = match denom with Some d -> d | None -> "expression" in
+        fprintf ppf "%s" d
+      in
+      report_general_this_exp denom ppf


Do you mean change report_this_pexp to take an option pexp ?

Julow · 2024-07-12T16:53:11Z

+(** [sexp_for_hint] is used by error messages to report literals in their
    original formatting *)
-let unify_pat ?sdesc_for_hint env pat expected_ty =
+let unify_pat ?sexp_for_hint env pat expected_ty =


Julow · 2024-07-12T16:53:45Z

+(** [sexp_for_hint] is used by error messages to report literals in their
    original formatting *)
-let unify_exp ?sdesc_for_hint env exp expected_ty =
+let unify_exp ?sexp_for_hint env exp expected_ty =


Thanks, this fixed many cases indeed.

Julow · 2024-07-12T16:54:22Z

+    | Pexp_constant { pconst_desc = Pconst_string (s, _, None); _ } ->
+        (* Avoid adding quotes around a quoted string constant but make sure to
+           use consistent inline code style. *)
+        Format.pp_print_string ppf (String.escaped s)


You are right, making the strings nominal is problematic. I removed it.

Octachron · 2024-07-15T12:46:52Z

+     - Do not contain spaces when printed.
+     - Is a constant that is short enough.
+  *)
+  let nominal_exp_doc lid t =


I have the impression that we don't need to delay the dependency on longident anymore?
Also since the function is in the Doc submodule, the _doc suffix seems redundant.

Octachron

The PR looks good to me: directly quoting the conflicting part of the source code should help readers to locate the source of the type error.

This should be a noticeable improvement, which only requires a small and localized change in Typecore.

Octachron · 2024-07-17T13:31:20Z

@Julow , do you wish to clean the history in order to preserve the individual message commits? Otherwise, I would squash the PR and merge the message commits.

Julow · 2024-07-17T15:00:19Z

Thanks :) Yes, I can clean up the history.

This function can be used to improve the type clash error message, which uses the parsetree instead of the typedtree to avoid loosing some information from the source code (eg. how constants are printed). This function is used for the "this function ..." message, which can be easily changed to use untyped expression.

Re-use the mechanism implemented for the "This function" message in type clash errors to print the original expression if possible. Type error messages now look like this: Error: The expression '43' has type int but an expression was expected of type int32 The argument 'unify_exp ~sexp' is made non-optional as this isn't too intrusive and ensures the new error is implemented in more cases.

Clarify type clash error messages by printing the kind of the reported expression. Currently implemented for expression that are likely 'nominal' but not necessarily. Example: The constant "42" has type ... This field has type ... This is similar to the `Apply_non_function` error: The function "foo" has type ... This function has type ...

Add denominations for expressions like `for`, `while`, `if`, `match`, etc.. that are often spanning several lines. It acts as a confirmation that the error is about the containing expression rather than a nested one as the printed code is sometimes harder to read. There's no test for some of them (`try`, `if`) but they'll become easily reachable if the denomination is used in other messages.

Julow commented Apr 14, 2023

View reviewed changes

Octachron reviewed Apr 8, 2024

View reviewed changes

Comment thread typing/typecore.ml Outdated

Julow force-pushed the print-expr-type-clash branch from b127ccc to c78123e Compare April 11, 2024 15:14

Octachron reviewed Apr 11, 2024

View reviewed changes

Julow force-pushed the print-expr-type-clash branch from 34e668d to f9dd42d Compare April 11, 2024 16:30

Octachron reviewed May 14, 2024

View reviewed changes

Comment thread typing/typecore.ml Outdated

Octachron reviewed May 14, 2024

View reviewed changes

Julow mentioned this pull request May 21, 2024

A document type for error messages #13169

Merged

Julow force-pushed the print-expr-type-clash branch from f9dd42d to d478de5 Compare July 12, 2024 16:50

Julow commented Jul 12, 2024

View reviewed changes

Octachron reviewed Jul 15, 2024

View reviewed changes

Octachron approved these changes Jul 16, 2024

View reviewed changes

Julow added 5 commits July 17, 2024 17:57

Update Changes

bc88b1b

Julow force-pushed the print-expr-type-clash branch from 9162094 to bc88b1b Compare July 17, 2024 16:27

Octachron added the merge-me label Jul 17, 2024

Octachron merged commit 9a982c0 into ocaml:trunk Jul 18, 2024

Conversation

Julow commented Apr 13, 2023

Uh oh!

Julow commented Apr 13, 2023

Uh oh!

gasche commented Apr 13, 2023

Uh oh!

Julow commented Apr 14, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Octachron commented Apr 26, 2023

Uh oh!

gasche commented Apr 8, 2024

Uh oh!

Octachron commented Apr 8, 2024

Uh oh!

Julow commented Apr 8, 2024

Uh oh!

Julow commented Apr 8, 2024

Uh oh!

Octachron commented Apr 8, 2024

Uh oh!

Uh oh!

Julow commented Apr 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Octachron commented Apr 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Octachron commented May 21, 2024

Uh oh!

Julow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!