[m-rev.] for review: color schemes

[Peter Wang]

On Mon, 03 Jun 2024 17:51:45 +1000 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> > 
> > dark16
> >     subject=14          (bright cyan)
> >     correct=10          (bright green)
> >     incorrect=9         (bright red)
> >     possible_cause=11   (bright yellow - only if not default)
> 
> What does "only if not default" mean here?
> 

Never mind, it was nothing.

> >> +    else if
> >> +        string.remove_prefix("specified@", SchemeName, SettingsStr)
> >> +    then
> > 
> > Is the "specified@" prefix necessary? We can assume that builtin scheme
> > names will never contain a '=', for example.
> 
> It is indeed not necessary NOW. The reason I added it is to allow us
> to add other syntaxes for specifying color schemes more easily.
> With a prefix, we can add another syntax by adding another prefix,
> and have the prefix select which syntax to parse for. Without a prefix
> on this scheme, that wouldn't be possible, and we would need
> more ad-hoc mechanism to effectively guess which syntax the user
> intended, especially if the string they wrote had a syntax error.

Ok, the prefix could also be made optional in future as well.
It's not important.

> >> +                ColorColors = choose_number(MissingRoles, "color", "colors"),
> >> +                RoleRoles = choose_number(MissingRoles, "role", "roles"),
> >> +                Pieces = [words("Error: the value of")] ++ Source ++
> >> +                    [words("does not specify the"), words(ColorColors),
> >> +                    words("to use for the"), words(RoleRoles), words("of")] ++
> >> +                    piece_list_to_pieces("and", MissingRoles) ++
> >> +                    [suffix("."), nl],
> >> +                Specs = [no_ctxt_spec($pred, severity_error, phase_options,
> >> +                    Pieces)]
> >> +            )
> > 
> > If a role is not specified, it can just be left uncolored instead
> > instead of reporting an error.
> 
> Are you saying that because you think that is a useful capability,
> or just out of general concerns about the length of the string
> required to specify all four colors?

It's just how I expected something like this to behave.

Ignoring unassigned roles would have the benefit that if a new role is
added, when a user updates the compiler, the compiler will continue
working instead of refusing to do anything until the user assigns a
color to the new role. That would be pretty annoying.

Related to that, if the user does update their MERCURY_COLOR_SCHEME
setting, then needs to run an older version of the compiler, the older
compiler will yell about the role it doesn't know about. Also annoying.

I understand that if the compiler just ignored roles it doesn't
recognise, it would not be able to report typos. It's a tradeoff I would
take, myself.

> At the moment, if the use of color is enabled, then all four colors must
> be specified. That can be changed, but that would itself raise some questions.
> The one I am thinking of is color nesting. At the moment, none of our
> diagnostics generates piece lists that do this:
> 
> abc
> start subject color
> def
> start incorrect color
> ghi
> end incorrect color
> jkl
> end subject color
> mno
> 
> but the code of write_error_spec.m does have code to handle that,
> and that machinery works. I know, because I have created diagnostics
> with such nesting in the past, admittedly by accident, and it worked.
> 
> If the user specified a subject color but not an incorrect color,  what
> color should ghi be in the above example? Should it be the neutral
> color, making ghi match abc and mno, or should it be the subject color,
> making ghi match def and jkl?

If a role is unassigned, I think it should act as if the role does not
exist. Then ghi would remain the same color as the rest of the subject.

Consider the analogous example of syntax highlighting, where strings are
highlighted one color, and there is an option to highlight escape
sequences within strings a different color. If the user doesn't specify
a color for escape sequences, you would expect escape sequences to be
highlighted the same color as the rest of the string.

> > possible_cause is a bit long. Ideally, it would be another single word,
> > say "possible", "maybe", "suggestion", "hint"? None of those is likely
> > to be a perfect match, but one of them may be close enough.
> 
> At the moment, we use that color for two purposes, which may be
> distinct enough that maybe they should be different color names,
> which may, or may not, be assigned different shades.
> 
> One purpose is add additional info about an error, info that tells the user
> about the usual cause of the error. For this use, the word "hint" is a perfect
> description.
> 
> The other purpose occurs when we have an error that reports not that
> some part of the user's code is definitely wrong, but that two parts of
> the user's code are inconsistent with one another. In that case, one
> of those pieces of code is wrong, but we don't which one, so we
> can't color one correct and one incorrect, so we color both with possible_cause.
> For this use, the word "possible" would work, as a shorthand for "possibly incorrect".
> "inconsistent" would also work.
> 
> Should these roles be split? I am fine with either answer.

Sure, "hint" and "inconsistent" sound fine.

> I do note that we would need a fifth color for each color scheme,
> and in the 16 versions, there are not that many left to choose from.
> 

That's fine.

> The difference lies in what happens when a user adds --enable-color-diagnostics
> to an mmc command line. With your proposal, they get the default color scheme
> in mmc, which they had no hand in choosing. With Julien's proposal, they get
> the color scheme in MERCURY_COLOR_SCHEME, which they (probably) *did* choose.

I did forget about --enable-color-diagnostics.

> I don't have a strong opinion on which of MERCURY_COLOR_SCHEME and --color-scheme
> should win if both are specified. I have no objection to switching priority to the
> option, provided Julien does not object.

> I gave priority to the envvar in my draft code
> because I thought it quite unlikely that anyone would type in the long argument
> need after --color-scheme on the command line, which means that any such option
> will almost certainly come from a Mmakefile or Mercury.options file. Since such
> files are shared among several people, the envvar is more personal than that.
> I though that the personal should override the shared, not the other way around.

We are in agreement. I meant the option --color-scheme on the command
line should have precedence over the environment variable.

But, ideally, --color-scheme in a Mercury.options file ought to have
lower priority than the environment variable. I believe that Unix
programs generally take their setting from these sources, in increasing
order of priority:

  1. system wide config files
  2. local config files
  3. environment variables
  4. command line arguments

mmc --make also follows that, at least the manual says:

    All variables in ‘Mercury.options’ are treated as if they are
    assigned using ‘:=’. Variables may also be set in the environment,
    overriding settings in options files.

Anyway, I expect users will set MERCURY_COLOR_SCHEME in the shell
startup file once, and --color-scheme will roughly never be used.
So it doesn't actually matter in practice.

> > Is there any reason to keep the --set-color-* options?
> 
> We have to keep them, because that is the only way that write_error_spec.m
> has to access the chosen color scheme. However, we don't have to let users know
> about them. We don't even need them to be specifiable on the command line;
> the system should work without that. If we made them unspecifiable in that way,
> then obviously they wouldn't be an input to the color selection process;
> they would just record its output.

Ok, let's make --set-color-* unspecifiable.

BTW, I had another look at https://no-color.org
According to their informal standard,
we should disable color if NO_COLOR != "",
but then color *may* still be enabled by command line options.

Peter