Don't suggest a semicolon when the type is not unit

[Julow]

This was suggested by @Octachron in #11679 (review)

This hint message is printed when a function receives too many arguments. It doesn't make sense when the return type of the function is not unit:

Line 2, characters 2-11:
2 |   (+) 1 2 3
      ^^^^^^^^^
Error: The function '(+)' has type int -> int -> int
       It is applied to too many arguments
Line 2, characters 8-10:
2 |   (+) 1 2 3
            ^^
  Hint: Did you forget a ';'?

This removes the message when it is not applicable.

The return type is computed again in the printing function by traversing the arrows to avoid passing more values and modifying the type checker.

[Octachron]

There is a small missing call to expand_head to handle the cases where the -> arrow is hidden behind a constructor:

type t = int -> int -> unit 
let f (x:t) = x 0 1 2

[Julow]

Thanks, fixed.

I noticed that the main message could be improved too:

Line 2, characters 21-28:
2 | let f (x:int -> t) = x 0 1 2
                         ^^^^^^^
Error: The function 'x' has type int -> t
       It is applied to too many arguments
Line 2, characters 25-27:
2 | let f (x:int -> t) = x 0 1 2
                             ^^
  Hint: Did you forget a ';'?
Line 2, characters 27-28:
2 | let f (x:int -> t) = x 0 1 2
                               ^
  This extra argument is not expected.

I added a commit to fully expand arrows before the printing.

[Octachron]

I am not sure if I agree with this change: we are erasing information providing by the user in form of type abbreviations, that might indicate that the root issue was earlier. For instance, I could have a combinator library, let's say for naive random variables:

type 'a rand = int -> 'a
let zero_or_one n = n mod 2
let add (x:'a rand) (y:'a rand): 'a rand = fun n -> x n + y n

Then, if I write

let t = add zero_or_one zero_or_one zero_or_one zero_or_one

the previous error message:

| let t = add zero_or_one zero_or_one zero_or_one zero_or_one
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Error: The function 'add' has type int rand -> int rand -> int rand
       It is applied to too many arguments
File "test.ml", line 4, characters 48-59:
4 | let t = add zero_or_one zero_or_one zero_or_one zero_or_one
                                                    ^^^^^^^^^^^
  This extra argument is not expected.
File "test.ml", line 4, characters 8-27:

carries the extra-information that add morally only takes two arguments.

[Julow]

I agree with this point.
The printed type shows two arguments but the problem is due to a fourth argument. The "type a is not included in type b" message repeats with increasing amount of expansion, could we do the same here ?

Error: The function 'add' has type int rand -> int rand -> int rand
       It is applied to 4 arguments but expect 3:
         int rand -> int rand -> int -> int

I'll drop this commit, make this PR ready and eventually open a new one with more ideas.

[Octachron]

The location of my comment was probably confusing but I would propose to move the expansion call just before the pattern match:

match get_desc (expand_head env rhs) with

rather than before the recursive call to res_type_is_unit to avoid any question of the form "was the type expanded enough before pattern matching"?

On the very nitpicking side, the responsibility of computing the return type of the function could be left to report_too_many_arg_error by transferring the current env as an argument, but the current location also makes sense.

[Julow]

I rewrote it with a different approach that doesn't have this problem.

[Julow]

I removed this assumption. It will be useful if this code is ever copy-pasted or if the code around is changed.

I like that it's not in report_too_many_arg_error as it shows what information is needed and doesn't make assumptions.

[Octachron]

In its current state, this PR is already a clear improvement: we remove a misleading hint with a handful of lines of code.

Thanks !

And I concur with the idea of keeping this PR simple (so I can merge it on Monday), while exploring further improvements in subsequent PRs.

[gasche]

The return type is computed again in the printing function by traversing the arrows to avoid passing more values and modifying the type checker.

I'm not fond of this approach.

Looking at the place in typecore where the Apply_non_function error is raised, it looks like for an application of the form t u, either t has an arrow type and type-checking proceed, or t has another type and we raise the error. Could we just include the type of t in the error payload, and print the hint when that type is unit?

  let type_unknown_arg (ty_fun, typed_args) (lbl, sarg) =
    let (ty_arg, ty_res) =
      let ty_fun = expand_head env ty_fun in
      match get_desc ty_fun with
      | Tvar _ ->
          let t1 = newvar () and t2 = newvar () in
          (* ... *)
          unify env ty_fun (newty (Tarrow(lbl,t1,t2,commu_var ())));
          (t1, t2)
      | Tarrow (l,t1,t2,_) when l = lbl
        || !Clflags.classic && lbl = Nolabel && not (is_optional l) ->
          (t1, t2)
      | td ->
          let ty_fun = match td with Tarrow _ -> newty td | _ -> ty_fun in
          (* ... *)
              raise(Error(funct.exp_loc, env, Apply_non_function {
                  funct;
                  func_ty = expand_head env funct.exp_type;
                  previous_arg_loc;
                  extra_arg_loc = sarg.pexp_loc; }))

My impression is that including td as a new record field (nonfunc_ty?) would suffice.

[Octachron]

Good point, having the first non-functional return type available in Apply_non_function would avoid the need to track the precondition that the function type is not recursive (and some duplicated computation).

[gasche]

@Julow are you planning to improve your PR as suggested? (Instead of suggesting new format features, hmmm... :-)

[Julow]

It's on my list! Sorry for the slow reply.

[Octachron]

The new code is indeed simpler this since we don't rely on the precondition that recursive functions types never raise the App_non_error function. I will squash and merge this afternoon.