[m-rev.] for post-commit review: add color in mode_errors.m

[Zoltan Somogyi]

On 2024-05-01 23:43 +10:00 AEST, "Julien Fischer" <jfischer at opturion.com> wrote:
>> +bug117.m:028:   Final instantiatedness of `X' was `ground',
>> +bug117.m:028:   expected final instantiatedness was 
>> +bug117.m:028:     named inst list.non_empty_list
>> +bug117.m:028:     which expands to
>> +bug117.m:028:       bound(
>> +bug117.m:028:         '[|]'(ground, ground)
>> +bug117.m:028:       ).
> 
> I wonder if "named inst" and "which expands to" should not be colored,
> so as the actual insts stand out.  (OTOH, given the size some of those
> inst expressions grow to the in the other messages I can see why you
> did it this way.)

The thing is, text of the form "named inst ..., which expands to ..." can occur
not just at the top level, but in the middle of other insts as well.
Treating the two cases differently is possible, but it would be a lot of work, and
I don't think it is worthwhile.
 
>> diff --git a/tests/invalid/coerce_clobbered.err_exp b/tests/invalid/coerce_clobbered.err_exp
>> index c3c75f42a..1093ff05c 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 expression:
>>  coerce_clobbered.m:021:   mode error: the argument term has instantiatedness
>> -coerce_clobbered.m:021:   `clobbered', but it must have a ground inst.
>> +coerce_clobbered.m:021:   `clobbered', but it must have a ground inst.
> 
> Not related to color: it would be nice if this error message mentioned the name of the
> argument term (i.e. Y in this case).

The problem is harder than it looks, for two reasons. The more obvious one is that
in superhomogeneous form, a coerce is always from one var to another, but the var
being coerced may be an anonymous variable added by superhomogeneous.m.
That means that printing the term being coerced would require reconstructing it
from the construction unifications preceding the coerce goal. The less obvious one
is that the code of modecheck_coerce.m can detect errors for which it itself
has no variable to report about, because the error is not at the level of the top
type_ctors of the types of X and Y, but one or more levels down.

>> diff --git a/tests/invalid/no_ho_inst.err_exp b/tests/invalid/no_ho_inst.err_exp
>> index cc44364a4..b615b813e 100644
>> --- a/tests/invalid/no_ho_inst.err_exp
>> +++ b/tests/invalid/no_ho_inst.err_exp
>> @@ -1,14 +1,14 @@
>>  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. The
>> -no_ho_inst.m:044:   type of AppHandler does match that expectation. However, 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. The insts of
>> -no_ho_inst.m:044:   higher order values should contain this information, but
>> -no_ho_inst.m:044:   AppHandler's inst, which is `ground' at this point, does
>> -no_ho_inst.m:044:   not.
>> +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.
>> +no_ho_inst.m:044:   However, to check the correctness of the call, the compiler
>> +no_ho_inst.m:044:   also needs to know the modes of the arguments and the
>> +no_ho_inst.m:044:   determinism of the predicate that AppHandler represents.
>> +no_ho_inst.m:044:   The insts of higher order values should contain this
>> +no_ho_inst.m:044:   information, but AppHandler's inst, which is `ground' at
>> +no_ho_inst.m:044:   this point, does not.
>>  no_ho_inst.m:044:
>>  no_ho_inst.m:044:   The fix for this error is to add this information. For
>>  no_ho_inst.m:044:   example, given a higher order type such as
> 
> Unrelated: presumably AppHandler should quoted in that one since it's a program variable.

As a variable, it must start with an upper case letter. I think that is sufficient
to make it clear that it is variable, especially when I color its first occurrence
with cyan. Do you disagree?

>> diff --git a/tests/invalid/polymorphic_unification.err_exp b/tests/invalid/polymorphic_unification.err_exp
>> index 9bf23a870..d806f5d73 100644
>> --- a/tests/invalid/polymorphic_unification.err_exp
>> +++ b/tests/invalid/polymorphic_unification.err_exp
>> @@ -1,20 +1,20 @@
>>  polymorphic_unification.m:023: In clause for `p(in, (list.list_skel >> dead))':
>>  polymorphic_unification.m:023:   in polymorphically-typed unification:
>> -polymorphic_unification.m:023:   mode error: variable `X' has instantiatedness
>> -polymorphic_unification.m:023:     named inst list.list_skel
>> -polymorphic_unification.m:023:     which expands to
>> -polymorphic_unification.m:023:       named inst list.list_skel(free)
>> -polymorphic_unification.m:023:       which expands to
>> -polymorphic_unification.m:023:         bound(
>> -polymorphic_unification.m:023:           []
>> -polymorphic_unification.m:023:         ;
>> -polymorphic_unification.m:023:           '[|]'(
>> -polymorphic_unification.m:023:             free,
>> -polymorphic_unification.m:023:             named inst list.list_skel(free)
>> -polymorphic_unification.m:023:           )
>> -polymorphic_unification.m:023:         ),
>> -polymorphic_unification.m:023:   expected instantiatedness was `ground' or
>> -polymorphic_unification.m:023:   `any'.
>> +polymorphic_unification.m:023:   mode error: variable `X' has instantiatedness 
>> +polymorphic_unification.m:023:     named inst list.list_skel
>> +polymorphic_unification.m:023:     which expands to
>> +polymorphic_unification.m:023:       named inst list.list_skel(free)
>> +polymorphic_unification.m:023:       which expands to
>> +polymorphic_unification.m:023:         bound(
>> +polymorphic_unification.m:023:           []
>> +polymorphic_unification.m:023:         ;
>> +polymorphic_unification.m:023:           '[|]'(
>> +polymorphic_unification.m:023:             free,
>> +polymorphic_unification.m:023:             named inst list.list_skel(free)
>> +polymorphic_unification.m:023:           )
>> +polymorphic_unification.m:023:         ),
>> +polymorphic_unification.m:023:   expected instantiatedness was `ground' or
>> +polymorphic_unification.m:023:   `any'.
> 
> I think it would look better not to color "or" there.

I will look into that, but I think that would be worthwhile only if decide
to make this (don't color connectives) a rule. Should we? I am fine with
either answer. I presume you vote "yes"; Peter, what do you think?

>>  polymorphic_unification.m:023:   When unifying two variables whose type will
>>  polymorphic_unification.m:023:   not be known until runtime, the variables must
>>  polymorphic_unification.m:023:   both be ground (or have inst `any').
> 
> Should this explanation not be colored with color_cause?

That text is not intended to *point out* a possible cause of the error;
the red color in the text above gives the cause exactly.
That text is printed only with -E, and it is intended to explain
the rule that the buggy code broke. (Though in this case, it just
gives the rule without doing much explaining.) It is thus
a different kind of thing from I have used color_cause for so far.
We *could* expand color_cause's remit, so to speak, to encompass
this kind of thing as well, but in this case, I don't think it would
be helpful, since it does not give any pointers to the possible
cause of the error reported in red.

> The diff looks fine otherwise.

Thank you.

Zoltan.