[m-rev.] for review: Improve coerce mode error messages.

[Peter Wang]

On Sat, 15 May 2021 00:06:07 +1000 Julien Fischer <jfischer at opturion.com> wrote:
> 
> Hi Peter,
> 
> On Wed, 12 May 2021, Peter Wang wrote:
> 
> > compiler/mode_errors.m:
> >    Replace the two existing options for coerce mode errors with one
> >    that includes:
> >    - the path to the input subterm where the error was detected
> >    - the type of the input subterm
> >    - the target type that the subterm should be converted to
> >    - the reason for the error
> >
> > compiler/modecheck_coerce.m:
> >    Modify the code such to produce the information above.
> 
>      Modify the code to produce the above information.
> 

Fixed.

> > diff --git a/tests/invalid/coerce_clobbered.err_exp b/tests/invalid/coerce_clobbered.err_exp
> > index 2bc874620..fd7140fa4 100644
> > --- a/tests/invalid/coerce_clobbered.err_exp
> > +++ b/tests/invalid/coerce_clobbered.err_exp
> > @@ -1,4 +1,4 @@
> > coerce_clobbered.m:021: In clause for `bad(in(clobbered), out)':
> > coerce_clobbered.m:021:   in coerce:
> 
> I think we should replace that with either:
> 
>      in coerce expression:
> 
> or:
> 
>      in type conversion expression:
> 

Done.

> > -coerce_clobbered.m:021:   mode error: `X' has instantiatedness `clobbered',
> > -coerce_clobbered.m:021:   but it must be ground.
> > +coerce_clobbered.m:021:   mode error: the input term has instantiatedness
> > +coerce_clobbered.m:021:   `clobbered', but it must be ground.
> 
> I would be inclined to use "argument" rather than "input term"  (Or
> perhaps, "argument term".)

Done.

> (If it does change the terminology in the reference manual should be
> updated too.)
> 

Hmm, it's quite useful to have "input term/inst/type" to contrast with
"result term/inst/type". I will leave this for a separate change.

> Also, ground should be quoted there.
> 

It needs to be a ground inst, but not necessarily `ground'.

> > diff --git a/tests/invalid/coerce_instvar.err_exp b/tests/invalid/coerce_instvar.err_exp
> > index 292f9a29d..0cb0e84a7 100644
> > --- a/tests/invalid/coerce_instvar.err_exp
> > +++ b/tests/invalid/coerce_instvar.err_exp
> > @@ -1,7 +1,7 @@
> > coerce_instvar.m:043: In clause for `bad(in((I =< ground)), out((I =<
> > coerce_instvar.m:043:   ground)))':
> > coerce_instvar.m:043:   in coerce:
> > -coerce_instvar.m:043:   mode error: the input term has instantiatedness
> > -coerce_instvar.m:043:   `( I =< ground )',
> > -coerce_instvar.m:043:   and cannot be converted to the type
> > -coerce_instvar.m:043:   `coerce_instvar.citrus'.
> > +coerce_instvar.m:043:   mode error: cannot convert the input term from type
> > +coerce_instvar.m:043:   `coerce_instvar.fruit' to `coerce_instvar.citrus'
> > +coerce_instvar.m:043:   because it has instantiatedness `ground', and the
> > +coerce_instvar.m:043:   former type is not a subtype of the latter type.
> 
> I suggest using the type names here rather than "former type" and "latter type".
> (Yes, it's a bit repetitious, but it reads less awkwardly.)
> 

Done.

> > +coerce_int.m:032:   mode error: in the input term:
> > +coerce_int.m:032:   in the argument of function symbol `wrap':
> > +coerce_int.m:032:   the subterm has instantiatedness `bound(1 ; 2)', which is
> > +coerce_int.m:032:   invalid for the type `uint'.
> 
> The problem there is that the new message does not make it clear
> that 'wrap' is part of the input term's inst.  Perhaps:
> 
> 
>      mode error: in the instantiatedness of the input term:
>          ...

I have tweaked it a bit. It now says:

    coerce_int.m:032:   in coerce expression:
    coerce_int.m:032:   mode error: in the argument:
    coerce_int.m:032:   in the argument of function symbol `wrap':
    coerce_int.m:032:   the instantiatedness of the subterm `bound(1 ; 2)' is
    coerce_int.m:032:   invalid for the type of the subterm `uint'.

> 
> Mind, as noted in the code, this is one of those things that
> should really be checked for elsewhere and earlier.  For one, thing
> the actual here is either on line 28 or 29 (depending on which part
> of the mismatch is wrong.)

Right.

> > coerce_mode_error.m:064: In clause for `bad_from_list_to_least2(in, out)':
> > coerce_mode_error.m:064:   in coerce:
> > -coerce_mode_error.m:064:   mode error: the input term has instantiatedness
> > -coerce_mode_error.m:064:   `bound('[|]'(ground, ground))',
> > -coerce_mode_error.m:064:   and cannot be converted to the type
> > -coerce_mode_error.m:064:   `coerce_mode_error.least2(V_1)'.
> > +coerce_mode_error.m:064:   mode error: in the input term:
> > +coerce_mode_error.m:064:   in argument 2 of function symbol `'[|]'':
> 
>      in the second argument of the function symbol
> 

Ok, done. I do think the numbers were easier to read, at least when you
have a series of "argument N in foo" stacked up.

> Everywhere else you use:
> 
> (Use nth_fixed from error_util.)
> 
> And anywhere else you use the "argument N" form.
> 

I didn't understand what you meant by this.

> Apart from my quibbles over the wording of the error messages,
> the diff is fine.

Thanks.

Peter