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

[Peter Wang]

On Wed, 19 Jul 2023 20:28:12 +1000 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> 
> On 2023-07-19 04:56 +02:00 CEST, "Peter Wang" <novalazy at gmail.com> wrote:
> >> +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.
> >       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.
> 
> I like most aspects of that, except one.
> 
> > in which I think the improvements are:
> > 
> >   - removes an interpretation where the reader might think the mode
> >     error is due to a compiler limitation
> 
> Actually, it IS due to a compiler limitation. The limitation is that
> even if you include the higher order inst info in the type, the
> compiler won't pay attention to it. But I agree that there is no point
> in emphasizing that fact.

As far as I'm aware, the language only allows higher order inst info in
the argument type of a du type definition. I don't think it's completely
obvious to allow higher order inst info elsewhere, so I wouldn't call
it a compiler limitation but a question of language design. Anyway.

> 
> >   - keeps the part which says that it's (probably) not a type error.
> >     That seems like a good idea.
> > 
> >   - prints out the problematic inst
> 
> Agree with both.
> 
> >   - 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
> 
> I agree that giving that search terms is useful. But your text also *requires*
> a search term for novices, because it does not say itself what "higher order
> inst information" *is*: the modes of the args, and the determinism.
> I would strongly prefer to provide that info in this error message.

Ok.

> 
> >   - removes the line about the "usual fix" which is vague
> >     (although that might have been intended as a heading to the next part)
> 
> It was intended to be such a heading. It seems a blank line
> would have helped here.
> 
> >> +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
> >> +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.
> 
> Yes, we *could* make everything from the "usual fix" reference onward
> a verbose-only part of the message. The reason I did not do so is that
> the very people who need the info in that part are the people who may not
> pay attention to the "recompile with -E" at the bottom of the output,
> partly because it may not follow the error message to which it applies
> *immediately*.
> 

Fair enough.

> > 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.
> 
> The four possible sources of a higher order term are:
> 
> - an input arg in the clause head
> - an output arg in a call in the clause body
> - a lambda expression (possibly implicit) in a construct unification
>   the clause body
> - a deconstruct unification in the clause body.
> 
> The third is not a problem, since in that case, the higher order inst
> is present by construction. The "usual fix" message covers the first two.
> The fourth is, in my experience, much rarer than the other three,
> precisely because there *is* no good way to preserve the higher order
> inst info of whatever closure was put into the data structure. There is
> a way, consisting of always specifying an inst for the whole data structure
> that includes the higher order inst of the higher order value, but
> I wouldn't call it "good". In that case, the good solution would be
> to include the higher order value's inst in its type, but this needs the compiler
> to be able to use that info.

I consider the solution of keeping the higher order value within a term
of a bespoke du type "pretty good". We don't have to mention it in the
error message though.

Peter