[m-rev.] for post-commit review: stop putting never-read entries in a map

[Zoltan Somogyi]

On Sat, 13 Dec 2025 14:50:10 +1100, Julien Fischer <jfischer at opturion.com> wrote:

> On Fri, 12 Dec 2025 at 09:04, Zoltan Somogyi <zoltan.somogyi at runbox.com> wrote:
> 
> > Stop putting never-read entries in a map.
> >
> > compiler/options_file.m:
> >     Stop putting entries for environment variables into the map for
> >     options variables, since the hit on the variable name in the environment
> >     variable map will cause them to always be ignored.
> >
> >     After the above change, all entries in the map for options variables
> >     actually come from options files (some of which are called configuration
> >     files), so stop recording a source kind for each entry.
> >
> >     Document the updated relationship between the two maps.
> >
> >     Document the choice the two possible ways we can handle errors in
> >     the values of environment variables that represent Mercury options.
> >
> > tests/options_file/basic_test.optfile_exp:
> >     Stop expecting the now-deleted source kind flags.
> 
> > diff --git a/compiler/options_file.m b/compiler/options_file.m
> > index e15a3de94..0bc13f9c6 100644
> > --- a/compiler/options_file.m
> > +++ b/compiler/options_file.m
> > @@ -159,7 +159,6 @@
> >  :- import_module map.
> >  :- import_module one_or_more.
> >  :- import_module pair.
> > -:- import_module require.
> >  :- import_module set.
> >  :- import_module string.
> >  :- import_module term_context.
> > @@ -171,29 +170,26 @@
> >      % (such as Mercury.config and Mercury.options).
> >  :- type env_optfile_variables
> >      --->    env_optfile_variables(
> > -                % This field is for the variable values we get from
> > -                % the small set of environment variables that
> > -                % we pay attention to.
> > +                % This field contains the set of all the environment variables
> > +                % of the current process. Most of these are not related to
> > +                % Mercury, which means we cannot pre-process them all.
> 
>    *at* all

No, I meant just "all", though it is clear from your question that
the sentence did not get across what I wanted it to get across.
I am replacing it with:


Most of these are not related to Mercury, so preprocessing all environment
variable values would yield lots of diagnostics about settings that have
nothing to do with Mercury.

> > +                % We cannot pre-process the set that are relevant Mercury
> 
>    *to* Mercury

Done.

> > +            % Checking now means that we detect and report errors in the value
> > +            % of an environment variable as early as possible after that
> > +            % envvar is set to a value we do not consider acceptable.
> > +            %
> > +            % On the other hand, if two envvars relevant to a Mercury program,
> > +            % say MCFLAGS-mod_a and MCFLAGS-mod_b, both have the same error,
> > +            % but only MCFLAGS-mod_a appears in a Mercury.options file, then
> > +            % the code here would report the error only for the first envvar,
> > +            % leaving the second to be reported by lookup_variable_words.
> > +            % That would seem to violate the law of least astonishment.
> 
> > +            % Reporting any errors here and now preserves old behavior.
> > +            % XXX The question is: *should* we preserve this old behavior?
> 
> On balance, I would be inclined to preserve the existing behaviour.

In the meantime, I thought of a possible third approach that gets
the best of both worlds, *most* of the time. The idea is to process
not all envvars, but all envvars whose names fit one of the patterns
that Mercury pays attention to.

Some patterns would be fixed strings: MCFLAGS and EXTRA_MCFLAGS
would be two examples.

Other patterns would be prefix matches: MCFLAGS- and CFLAGS-
would be examples of these.

We would need lists of both kinds of patterns, but scripts/Mmake.{vars,rules}
should give us that.

The "most of the time" qualifier is there because it is theoretically possible
for someone to defined an envvar named e.g. MCFLAGS-xyz *without* intending it
to apply to a Mercury module named xyz. However, I don't think this is likely to be
a significant problem in practice.

Opinions?

> The rest looks fine.

Thank you.

Zoltan.