Improve the error message about too many arguments#11679
Improve the error message about too many arguments#11679Octachron merged 4 commits intoocaml:trunkfrom
Conversation
parsing/location.ml
Outdated
| Format.fprintf ppf "%s | %s@," line_nb line; | ||
| Format.fprintf ppf "%*s " (String.length line_nb) ""; | ||
| String.iteri (fun i c -> | ||
| for i = 0 to rightmost.pos_cnum do |
There was a problem hiding this comment.
This change is weird.
I think your intent is to support locations that are outside the source line, "one past" the end. You should say so in a comment, because the code is confusing if we don't know this.
Note that the code computing lines above appears to allow "one past" positions, but I wonder if this was intentional or in fact a programming mistake (did the authors realize that their definition of end_pos allow this? I think they believed that there were only including valid line indices in the range). Could you add a comment there as well?
There was a problem hiding this comment.
Agree it's complicated ! Added a comment.
I think the code above works because the end of line has an indice (there's a pos_cnum that correspond to it), so only one line can be picked if the location spans it (it won't pick the start of the next line).
I ended up rewriting it in #11678 (with more comments)
There was a problem hiding this comment.
Wait, rightmost.pos_cnum counts the number of character since the start of the file. It is far too large, the right size is rightmost.pos_cnum - line_cnum_start - 1.
There was a problem hiding this comment.
Right! That was a big oversight, thanks.
typing/typecore.ml
Outdated
| in | ||
| parent expr "#" meth' | ||
| | Texp_instvar (_, _, name) -> Some name.txt | ||
| | _ -> None |
There was a problem hiding this comment.
I think you can do this with much less code. The problem you have decided to solve is to be able to show the function to the user in the error message, but only if "it can be named", which basically amounts to "it is a short/atomic construct". Is the expression "short" enough that I want to include it in the error message? This is the question.
But you are mixing this the actual printing logic, which is needlessly duplicated. It may also very well be wrong / imperfect; for example, do you handle the case of infix operators properly?
My suggestion: turn this into a is_short function that returns a boolean, move it to typedtree.ml, and call it from the error message printer (is funct short?). For the printing you could call Untypeast.untype_expression and then Pprintast.expression. (Or maybe you have access to the parsetree directly at error-raising point and don't need Untypeast.)
There was a problem hiding this comment.
I wasn't proud of this code! I've implemented it like you suggested and it's much better. I've included more cases and written down why I choose them.
There was a problem hiding this comment.
(I don't currently see the new version in the PR.)
There was a problem hiding this comment.
Sorry, I just pushed it now.
| { loc_start; | ||
| loc_end = { loc_start with pos_cnum = loc_start.pos_cnum + 1 }; | ||
| loc_ghost = false } | ||
| in |
There was a problem hiding this comment.
There is a corner case here: there is not necessarily a white space after the last valid argument. For instance
let f x y z = x + y + z
let t = f 0 1
(+)2yields
File "test.ml", line 3, characters 7-8:
3 | (+)2
^
Hint: Did you forget a ';'?
File "test.ml", line 3, characters 7-8:
3 | (+)2
^
This argument is not expected.
One option that might work would be to widen the highlighting interval to include the last character of the previous arg:
3 | (+)2
^^
Hint: Did you forget a ';'?
to make it clearer that we are proposing to insert ; in the middle of the interval.
There was a problem hiding this comment.
Good idea. It's nice when there's a space too, it shows that the semicolon has to be inserted "after the argument".
typing/typecore.ml
Outdated
| in | ||
| let warned = ref false in | ||
| let rec type_args args ty_fun ty_fun0 sargs = | ||
| let rec type_args args ty_fun ty_fun0 sargs prev_arg_loc = |
There was a problem hiding this comment.
Personal nitpicking: as prev_arg_loc is a metadata parameter of the functions whereas sargs is the decreasing argument in the recursion, I would rather group it with the args argument and keep sargs as the last argument of the function.
There was a problem hiding this comment.
I paired the location with the argument, which makes the code clearer. However, I'm not sure it decreases the diff size.
typing/typecore.ml
Outdated
| Some (fun () -> option_none env (instance ty) Location.none) | ||
| in | ||
| let remaining_sargs, arg = | ||
| let remaining_sargs, arg, arg_loc = |
There was a problem hiding this comment.
Personal nitpicking 2: the arg argument requires some computation whereas arg_loc is straightforward to compute. It may make sense to commute those two elements of the tuple so you can decrease the diff size.
There was a problem hiding this comment.
Paired with the arg now. The location needs to be passed because arg is wrapped in a thunk doing side effects.
Octachron
left a comment
There was a problem hiding this comment.
Overall, I like the new information available in the error message. It is a really good idea to point to the supernumerary arguments: it is extremely helpful for beginners and still helpful for the experimented developer.
The hint for ; could be refined to only appear when adding ; is correct in the strict mode but this is the existing behavior and improvements are better left to another PR.
Even if I have still have many small remarks, the code is in good shape and could be merged, once the missing hint tag is added.
cf90cd3 to
bcb05bb
Compare
|
Ready for review again. I rebased and cleaned the history.
Added to my TODO. |
| of the argument. *) | ||
| let arg_end = previous_arg_loc.loc_end in | ||
| { loc_start = cnum_offset ~-1 arg_end; | ||
| loc_end = cnum_offset ~+1 arg_end; |
There was a problem hiding this comment.
This use of unary + is a bit surprising (this is the only instance in the whole compiler)?
There was a problem hiding this comment.
That's nice, isn't it ?
4b6ce50 to
6b7689b
Compare
|
This is good to merge once the check-typo commit has been split off. |
Allow to write for example:
File "./test.ml", line 4, characters 46-47:
| f (List.map string_of_int [ 1; 2; 3; 4; 5 ])
^
Maybe you forgot a ';'.
Iterating on the string instead of the range is not useful because range
calculation have already been done.
The location of the application, upto the problematic argument, is computed and
used as the location for the main message.
Two sub-messages are added: one for the suggested location of a potential `;`
and the other for highlighting the extra argument.
Before:
File "./test.ml", line 4, characters 2-13:
4 | print_lines (List.map string_of_int [ 1; 2; 3; 4; 5 ])
^^^^^^^^^^^
Error: This function has type string list -> unit
It is applied to too many arguments; maybe you forgot a `;'.
After:
File "./test.ml", lines 4-5, characters 2-15:
4 | ..print_lines (List.map string_of_int [ 1; 2; 3; 4; 5 ])
5 | print_endline......
Error: This function has type string list -> unit
It is applied to too many arguments
File "./test.ml", line 4, characters 56-57:
| print_lines (List.map string_of_int [ 1; 2; 3; 4; 5 ])
^
Maybe you forgot a ';'.
File "./test.ml", line 5, characters 2-15:
5 | print_endline "foo"
^^^^^^^^^^^^^
This argument is not expected.
Increase clarity of the message since the location no longer point directly to the function. Introduce 'Typedtree.exp_is_nominal' for testing whether an expression can be printed as the subject of a sentence with 'Pprintast'.
Extend the location to the right to cover the end of the previous argument as well. This renders well when there's no space between the two arguments and shows better that the semicolon has to be inserted (between two position).
|
Merged, thanks a lot for the great improvement to the compiler error messages! |
The "applied to too many arguments" error message confuses me every time I see even though it often has a trivial to fix cause.
The message highlights the function but not the whole application, which can be useful to understand which argument is problematic and where I should add the suggested
;.This PR change the location of the main message to span the whole application and adds two sub-messages:
;, for which the location extends outside the range of the line.;or if I should remove an argument.The multi-line highlighting is not easy to read, I have a suggestion in #11678
Before:
After: