[m-rev.] for review: Conditionally enable color diagnostics by default.

[Julien Fischer]

Hi Peter,

On Tue, 25 Jun 2024 at 11:29, Peter Wang <novalazy at gmail.com> wrote:
>
> Questions:
>
> Do we need config_default_color_diagnostics?

It was intended for those packaging Mercury packagers.
I don't think the ability to set the default at configuration time hurts.

> mmake always redirects output to .err files; by default the .err files
> will contain uncolored output. If you want colored output in those files,
> you will need to explicitly ask for it. I think this is fine, but maybe
> there are differing opinions.
>
...

> diff --git a/compiler/handle_options.m b/compiler/handle_options.m
> index 05e6baec5..0df42c288 100644
> --- a/compiler/handle_options.m
> +++ b/compiler/handle_options.m

...

> @@ -3197,11 +3188,48 @@ handle_colors(!Globals, !IO) :-
>          UseColor = EnableValue
>      ;
>          EnableIsSet = no,
> -        % If the user dod not set the enable option, use the default.
> -        UseColor = ConfigDefault
> +        % If the user did not explicitly enable or disable color, then we
> +        % enable color based on whether standard error is associated with a
> +        % terminal device.
> +        %
> +        % NOTE Since mmc --make writes diagnostics into both .err files *and*
> +        % to stderr, there is an argument to be made that the two outputs
> +        % should be independent: the output going to a terminal could contain
> +        % color escape sequences, but the output going into .err files might
> +        % not. That would be a non-trivial change as mmc --make works by
> +        % writing diagnostics into a .err file, then copying the first N lines
> +        % to stderr. (Writing to a file first also prevents interleaved output
> +        % from parallel tasks.)
> +        %
> +        % In the absence of support for producing independent outputs, and an
> +        % option to control that behaviour, we will simply produce the same
> +        % output to stderr and .err files.
> +        check_stderr_is_terminal(IsTerminal, !IO),
> +        (
> +            IsTerminal = yes,
> +            % If the user did not set the enable option, use the default.
> +            % XXX can we just make this yes?
> +            UseColor = ConfigDefault
> +        ;
> +            IsTerminal = no,
> +            UseColor = no
> +        )
>      ),
>      globals.set_option(use_color_diagnostics, bool(UseColor), !Globals).

> +:- pred check_stderr_is_terminal(bool::out, io::di, io::uo) is det.

A more accurate output here would be: yes, no, or i_dont_know.

> +:- pragma foreign_proc("C",
> +    check_stderr_is_terminal(IsTerminal::out, _IO0::di, _IO::uo),
> +    [will_not_call_mercury, promise_pure, thread_safe, may_not_duplicate],
> +"
> +#ifdef MR_HAVE_ISATTY
> +    IsTerminal = (isatty(STDERR_FILENO) == 1) ? MR_YES : MR_NO;

That probably wants to be _isatty() on Windows.

> +#else
> +    IsTerminal = MR_NO;
> +#endif
> +").

Do we not need definitions for the C# and Java backends?  We still want to be
able to build the compiler in those grades.

...

> diff --git a/doc/user_guide.texi b/doc/user_guide.texi
> index 028739e99..ee5908f49 100644
> --- a/doc/user_guide.texi
> +++ b/doc/user_guide.texi
> @@ -7173,21 +7173,15 @@ For information about how the compiler uses colors in diagnostic messages,
>  and about the syntax of color scheme specifications,
>  please see the @ref{Color schemes} section for the details.
...

>  @c @sp 1
> @@ -11852,45 +11846,40 @@ while the second is not;
>  it is in fact a defacto standard for disabling color.
>  @samp{https://no-color.org}.
>
> -The rules the compiler uses to decide
> -whether the use of color in diagnostics is enabled
> -are as follows.
> +The compiler decides
> +whether to enable the use of color in diagnostics
> +by the following rules:

I suggest: using the following rules.

Julien.