[m-rev.] for post-commit review: better diagnostic for missing higher order insts

[Julien Fischer]

On Wed, 19 Jul 2023, Peter Wang wrote:

>> diff --git a/tests/invalid/no_ho_inst.err_exp b/tests/invalid/no_ho_inst.err_exp
>> index e69de29bb..7cba3af08 100644
>> --- a/tests/invalid/no_ho_inst.err_exp
>> +++ b/tests/invalid/no_ho_inst.err_exp
>> @@ -0,0 +1,22 @@
>> +no_ho_inst.m:044: In clause for `run_loop(in, in, out, di, uo)':
>> +no_ho_inst.m:044:   in argument 1 (i.e. the predicate term) of higher-order
>> +no_ho_inst.m:044:   predicate call:
>> +no_ho_inst.m:044:   mode error: context requires a predicate of arity 4, and
>> +no_ho_inst.m:044:   the type of AppHandler does match that expectation, but to
>> +no_ho_inst.m:044:   check the correctness of the call, the compiler also needs
>> +no_ho_inst.m:044:   to know the modes of the arguments and the determinism of
>> +no_ho_inst.m:044:   the predicate that AppHandler represents, and AppHandler's
>> +no_ho_inst.m:044:   inst does not contain that information.
>
> My attempt:
>
>    In clause for `run_loop(in, in, out, di, uo)':
>      in argument 1 (i.e. the predicate term) of higher-order
>      predicate call:
>      mode error: the context requires a predicate of arity 4.

Rather than the "the context", could we instead say "this argument"?

>      The type of AppHandler matches that expectation, but the inst of
>      AppHandler at this point is `ground', which lacks the higher-order
>      inst information required to make the call.
>
> in which I think the improvements are:
>
>  - removes an interpretation where the reader might think the mode
>    error is due to a compiler limitation
>
>  - keeps the part which says that it's (probably) not a type error.
>    That seems like a good idea.
>
>  - prints out the problematic inst
>
>  - tells the user that "higher-order inst information" is required to
>    make a higher-order call, using those exact words so they can be
>    searched for in the reference manual
>
>  - removes the line about the "usual fix" which is vague
>    (although that might have been intended as a heading to the next part)

It does rather imply there's some other, more unusual, fix.

>> +no_ho_inst.m:044:   The usual fix for this error is to add this information.
>> +no_ho_inst.m:044:   Given a higher order type such as

I would prefix the example with:

      For example, given a higher-order type ...

just to make it absolutely clear that what's being described is an
example and does not refer to the user's code.

>> +no_ho_inst.m:044:     :- type callback_t == (pred(world, world, io, io).
>> +no_ho_inst.m:044:   you would define a corresponding inst, such as
>> +no_ho_inst.m:044:     :- inst callback_i == (pred(in, out, di, uo) is det).
>> +no_ho_inst.m:044:   This inst specifies the modes of the arguments and the
>> +no_ho_inst.m:044:   determinism of a predicate. You can then tell the compiler
>> +no_ho_inst.m:044:   that a value of type callback_t has inst callback_i by
>> +no_ho_inst.m:044:   specifying either the mode `in(callback_i)' (when taking a
>> +no_ho_inst.m:044:   value of type callback_t as input) or the mode
>> +no_ho_inst.m:044:   `out(callback_i)' (when returning a value of type
>> +no_ho_inst.m:044:   callback_t as output).
>> +For more information, recompile with `-E'.
>
> I personally prefer not to see things like this in error messages.
> Once you learn what's going on, it's tiring to be told the same long
> piece of (non-specific) advice over and over.
>
> A common situation where the error would arise is when someone tries to
> take a higher-order term out of a container and tries to call it, then
> the advice would not apply.
>
> You could hide the advice behind the -E option.

I agree with Peter on that last point.

Julien.