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

[Julien Fischer]

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

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

   *to* Mercury

> +                % either, because we don't know what that set is.
> +                % The reason is the presence of module names in
> +                % options variable name of forms such as MCFLAGS-modulename.
> +                %
> +                % We therefore process the values of environment variables
> +                % (meaning, we check whether they can be split up into words
> +                % cleanly) only when the compiler looks up a key in this map.
>                  eov_env     :: map(env_optfile_var, string),

...


> @@ -1074,60 +1070,69 @@ get_string_acc([Char | Chars0], Chars, RevString0, RevString, MaybeError) :-
>      list(error_spec)::in, list(error_spec)::out,
>      list(error_spec)::in, list(error_spec)::out, io::di, io::uo) is det.
>
> -update_variable(FileName, LineNumber, SetOrAdd, VarName, NewValue0,
> +update_variable(FileName, LineNumber, SetOrAdd, VarName, NewChars0,
>          !Variables, !ParseSpecs, !UndefSpecs, !IO) :-
>      expand_any_var_references(!.Variables, FileName, LineNumber,
> -        NewValue0, NewValue1, !ParseSpecs, !UndefSpecs, !IO),
> -    MaybeWords1 = split_into_words(NewValue1),
> +        NewChars0, NewChars1, !ParseSpecs, !UndefSpecs, !IO),
> +    MaybeWords1 = split_into_words(NewChars1),
>      (
>          MaybeWords1 = ok(Words1),
>          !.Variables = env_optfile_variables(EnvMap, OptsMap0),
> -        ( if map.search(EnvMap, VarName, EnvValue) then
> -            Value = string.to_char_list(EnvValue),
> -            MaybeWords = split_into_words(Value),
> +        ( if map.search(EnvMap, VarName, EnvStr) then
> +            % We do not need to insert the VarName/Words1 pair into OptsMap0,
> +            % because, due to the presence of VarName key in EnvMap,
> +            % it will never be consulted.
> +            %
> +            % However, the first time we get here for a given VarName
> +            % is the first time we learn that the environment variable
> +            % with that name is relevant to Mercury, and therefore we should
> +            % check whether it splits into words cleanly.
> +            %
> +            % Should we check that here and now, or should we leave it
> +            % to lookup_variable_words predicate, which is invoked when
> +            % the compiler wants to use this environment variable?
> +            % There are arguments in favor of both options.
> +            %
> +            % 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.

The rest looks fine.

Julien.