[m-rev.] for review: colour in error messages

[Zoltan Somogyi]

On 2024-04-24 22:14 +10:00 AEST, "Julien Fischer" <jfischer at opturion.com> wrote:
> * I think it should be possible to set the default via an option to
>   the configure script -- being able to do so is potentially useful for
>   packagers.

I didn't think of that, since I have not worked in that space, but that's fine.

> * When colours are enabled, the compiler should respect the NO_COLOR
>   environment variable (<https://no-color.org/>) and disable them
>   if that environment variable is set appropriately.

Agreed.

> * Many programs that write coloured output, only do so when writing
>   to a terminal and disable coloured output when the relevant output
>   stream is redirected to a file. On Unix-like systems, this is done
>   via a call to isatty() (or _isatty() on Windows). (I don't know how
>   such a thing can easily be supported when the compiler is built in
>   the Java or C# grades.)

I am of the strong opinion that this would be the wrong way to go
for Mercury. What should govern whether color is enabled is the *final*
destination of the output, not its *initial* destination. With mmc --make,
which puts all error output into .err files, and then copies (parts of) those
files to stdout, which may be a tty, this would make colour error messages
unobtainable. With mmake also, we also get the compiler to put diagnostics
into .err files that we then view on a tty.

I think --no-enable-color-diagnostics and NO_COLOR should be sufficient
controls for users. If experience proves this assertion to be false, we could
use *another* set-and-forget env var, named maybe MMC_DESTINATION_AGNOSTIC_COLOR,
that, when set, causes the color-vs-no-color choice to be unaffected by the
destination of error output, but in whose absence we disable color when
writing to non-ttys.

People will have to do some such setup work anyway. For example, I had a lot of
trouble with the initial output, which came out garbled, even though I followed the
details on the wikipedia page exactly. It turned out that this was caused by
the fact that (a) I was piping the compiler output through less, and (b) less
FILTERS OUT escape sequences, unless you set an option asking it to leave
them (kids) alone.

> * The terminal's background color will affect this.

Yes, it would, which is why users should have a way to set the foreground color
to something that will work with their background.

>> What should be the name of the option that controls it?
>> Something like --enable-colo{u}r?
> 
> --enable-colo{u}r-messages or --enable-colo{u}r-diagnostics?

Both of those work for me.

>> 2. Programmers who enable colour should be able to control
>> the actual colours used. What should the option names be?
>> --set-correct-colo{u}r-to 45?
> 
> I suggest naming these options according to some sort of scheme,
> e.g. somehting like:
> 
>    --set-colo{u}r-FOO
> 
> where FOO is the feature whose colour is to be set.

That works.

>> 3. Should we support something beyond 8-bit color (such as that 45)?
>> I have no idea how widespread the support for 24 bit rgb is in terminals
>> (as opposed to actual display devices).
>>
>> If so, what should the option names be?
> 
> I would not worry about it initially.

I am not worrying.

>> 5. At the moment, the diff supports two colours, intended for correct
>> and incorrect code respectively. In error messsages of the form
>> "expected x, got y", the intention is that x would be in the correct colour,
>> and the y in the incorrect colour.
>>
>> Should we have a separate logical colour for "this may be caused by"
>> messages?
> 
> I think so.  More generally, we probably need to go thorugh the expected
> outputs in tests/invalid and tests/warnings and work out where colour
> could be useful.

Agreed, but actually I would first prefer to go through typecheck_errors.m
and see where I can eliminate unnecessary differences first. That module
evolved somewhat haphazardly, so there are mechanisms to be helpful
to the programmer that are implemented only for one kind of error, but not
other, closely related kinds.

>> If so, what should be the default actual colour?
> 
> I'm not sure.

My guess is yellow, as blue is too close to black and orange too close to red,
allowing for at least momentary confusion.

>> 6. The bike-shed question: it is clear that option names should have
>> two versions, one with "color" and one with "colour", but which spelling
>> we should use internally? The american version is slightly less likely to cause
>> a line to be too long, but graph_colour.m and set_of_var.m use brit spelling.
> 
> Based on a grep of the compiler source code (and a review of your diff),
> I think that's already been "decided": colour.

Actually, when I wrote the code, I used "color" throughout, because that is what came
naturally. I only changed to brit spelling at the end, to match the rest of the compiler.
The switch is trivial, a simple global search and replace, so I think this "decision" is
*far* from set it stone.

>> +        SCUnit = sc_colour_end,
>> +        LineEndReset1 = line_end_reset_colour,
>> +        expect(unify(NextSpace0, " "), $pred, "NextSpace0 != 1 for colour end"),
>> +        merge_adjacent_colour_changes(SCUnit, SCUnits0, SCUnits, !ColourStack),
>> +        % We put the space between the previous actual word and the next one
>> +        % *after* the colour change. (Actually, the recursive call takes
>> +        % cate of that, *if* there are any words on the rest of the line.)
> 
> s/cate/care/

Fixed.

>> @@ -1445,11 +1718,54 @@ find_matching_rp([HeadLine0 | TailLines0], !MidLinesCord, !MidLinesLen,
>>          )
>>      ).
>> > +%---------------------------------------------------------------------------%
>> +%
>> +% Colour management.
>> +%
>> +
>> +:- type colour
>> +    --->    colour_8bit(int)
> 
> Why not a uint8?

Done.

>> +    % The terminal control codes we use here are described by
>> +    % https://en.wikipedia.org/wiki/ANSI_escape_code#Colors.
>> +    %
>> +:- func colour_to_string(colour) = string.
>> +
>> +colour_to_string(colour_8bit(ColourNum)) = Str :-
>> +    string.format("\033\[38;5;%dm", [i(ColourNum)], Str).
>> +colour_to_string(colour_reset) = Str :-
>> +    Str =  "\033\[39;49m".
> 
> Perhaps we should support the \e escape that several Prolog implementations
> provide?

That would be a good idea, but that is separate from this.

> A this is not enabled by default, I'm happy for you to go ahead and commit it.

Done, and thanks.

Zoltan.