[m-rev.] for post-commit review: source_file_map.m

[Julien Fischer]

On Wed, 16 Jul 2025 at 19:24, Zoltan Somogyi <zoltan.somogyi at runbox.com> wrote:
> Improve source_file_map.m.

...

> diff --git a/compiler/file_names.m b/compiler/file_names.m
> index e18a24f60..a97d127f6 100644
> --- a/compiler/file_names.m
> +++ b/compiler/file_names.m
> @@ -132,7 +132,7 @@
>  % whose files are both grade-specific and architecture-specific.
>  %
>  % We include "pgs" in the names of extensions whose files' *existence*
> -% is grade-specific, but whole files' *contents* is not. The one such
> +% is grade-specific, but whose files' *contents* is not. The one such

are not

>  % extension is .mh. They exist only in C grades, but since they consists

Not something from this change, but s/consists/consist/

>  % of the C declarations of the Mercury predicates and functions that
>  % the program exports to C, their content will be the same in *all* C grades.

...

> diff --git a/compiler/parse_module.m b/compiler/parse_module.m
> index 6ffafd153..ffeaea18c 100644
> --- a/compiler/parse_module.m
> +++ b/compiler/parse_module.m
> @@ -100,7 +100,7 @@
>      % peek_at_file(FileStream, SourceFileName, MaybeDefaultModuleName,
>      %   MaybeModuleName, !IO):
>      %
> -    % "mmc -f", which creates the Mercury.module file that later compiler
> +    % "mmc -f", which creates the Mercury.module file that all later

Mercury.modules

>      % compiler invocations will use to map modules to the name of the file
>      % containing them, uses this predicate to peek into a file to try to read
>      % its first Mercury item.

...

> diff --git a/compiler/source_file_map.m b/compiler/source_file_map.m
> index e350123b0..fb8d14968 100644
> --- a/compiler/source_file_map.m
> +++ b/compiler/source_file_map.m

...

> @@ -274,7 +359,17 @@ lookup_source_file_module(FileName, MaybeModuleName, !IO) :-
>
>      % Bidirectional map between module names and file names.
>      %
> -:- type source_file_map == bimap(module_name, string).
> +    % The only module names in this map will be modules that are
> +    % the top module in the file that contains them. Any submodules nested
> +    % within them will NOT appear in this bimap. Without this restruction,

s/restruction/restriction/

> +    % this map could not be a bijection.
> +    %
> +    % Both the code that constructs source_file_maps for Mercury.modules files,
> +    % and the code that reads in Mercury.modules files, ensure that this map
> +    % is a bijection. They do this by treating any possible deviation from
> +    % being a bijection as an error to be reported.
> +    %
> +:- type source_file_map == bimap(module_name, file_name).
>
>  %---------------------%
>
> @@ -307,15 +402,32 @@ get_source_file_map(SourceFileMap, !IO) :-
>          (
>              ReadResult = ok(FileLines),
>              bimap.init(SourceFileMap0),
> -            parse_source_file_map(FileLines, ModulesFileName, 1, ErrorMsg,
> -                SourceFileMap0, SourceFileMap1),
> -            ( if ErrorMsg = "" then
> +            parse_source_file_map(FileLines, ModulesFileName, 1,
> +                cord.init, ErrorMsgCord, SourceFileMap0, SourceFileMap1),
> +            ErrorMsgs = cord.list(ErrorMsgCord),
> +            (
> +                ErrorMsgs = [],
>                  SourceFileMap = SourceFileMap1
> -            else
> +            ;
> +                ErrorMsgs = [_ | _],
>                  % If the file does exist but is malformed, then
> -                % we *should* generate an error, but pretending that
> -                % the file exists and is empty preserves old behavior.
> -                bimap.init(SourceFileMap)
> +                % we *should* print ErrorMsgs, but before 2025 jul 16,
> +                % we did not do so. Granted, corrupted Mercury.modules files
> +                % happen rarely, but precisely because of that, if it
> +                % does happen, users probably won't connect the strange
> +                % error messages that result from us returning an empty
> +                % SourceFileMap here to such corruption in the absence of
> +                % this kind of diagnostic.
> +                %
> +                % It would be nice if our callers told us the stream
> +                % to which this error should be reported, but for many of them,
> +                % this would require a nontrivial amount of complication
> +                % that this very rare error probably does not deserve.
> +                bimap.init(SourceFileMap),
> +                io.stderr_stream(StdErr, !IO),
> +                io.write_strings(StdErr, ErrorMsgs, !IO),
> +                io.write_string(StdErr,
> +                    "You need to rebuild Mercury.modules.\n", !IO)

Do we need to set the exit status to non-zero value at this point?

The rest looks fine.

Julien.