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

[Julien Fischer]

On Tue, 30 Apr 2024, Zoltan Somogyi wrote:

> Use color in mode error diagnostics.m.

...

> diff --git a/tests/invalid/bug117.err_exp b/tests/invalid/bug117.err_exp
> index 37b288004..597c4a741 100644
> --- a/tests/invalid/bug117.err_exp
> +++ b/tests/invalid/bug117.err_exp
> @@ -1,10 +1,10 @@
>  bug117.m:028: In clause for `extract(in) = out(list.non_empty_list)':
>  bug117.m:028:   mode error: the function result did not get sufficiently
>  bug117.m:028:   instantiated.
> -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:       ).
> +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.)

...

> 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).

...

> 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.

...

> 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.

>  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?

The diff looks fine otherwise.

Julien.