[m-rev.] diff: Delete unused imports.

[Peter Wang]

On Wed, 30 Mar 2022 06:07:17 +1100 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> 
> 2022-03-29 12:22 GMT+11:00 "Peter Wang" <novalazy at gmail.com>:
> > (I assume there is a bug in the unused imports detection.)
> 
> Indeed there was. The attached diff, which I am asking you to review,
> fixes two of them. I haven't had time yet to test whether it fixes the issue
> you reported, or whether that is a separate bug. Though it could also be
> a known limitation rather than a bug, for anyone who cares about
> the distinction :-)
> 
> Zoltan.

> Stop ancestor imports "shadowing" unused local imports.
> 
> compiler/make_hlds_passes.m:
>     We used to add all modules imported by an ancestor of the current module
>     to the set of used modules. Once upon a time, this was meant to stop
>     the compiler generating misleading warnings about imports being unused
>     when the import wasn't even done by the current module. However, since
>     we introduced structures representations of import- and use_module

structured

>     declarations and taught unused_imports.m to use them, that has not been
>     an issue. However, a bad side-effect remained, which was that if
>     a module A imported a module B but did not use it, or it imported
>     module B in its interface but did not use it its interface, then

in its interface

>     any warning we could generate about that import being unused was
>     suppressed by any import of module B in any of module A's ancestors.
>     (The "shadowing" mentioned above.)
> 
>     Fix the problem by adding modules imported by ancestors of the
>     current module NOT to the set of used modules, but to a new field
>     in the module_info.
> 
> compiler/hlds_module.m:
>     Add this new field. As it happens, it is not needed right now,
>     but it may be needed later.
> 
>     Update some documentation.
> 
>     Note an only-tangentially-related problem.
> 
> compiler/unused_imports.m:
>     Fix a bug that was hiding behind the shadowing, which was that whether
>     the text of the warning message we generated for an unused local import-
>     or use_module declaration could be affected by the presence of an
>     import- or use_module declaration in an ancestor module.
> 
>     Improve debugging infrastructure.
> 
>     Make a predicate name more descriptive.

> diff --git a/compiler/hlds_module.m b/compiler/hlds_module.m
> index e98b3cb74..8c154398f 100644
> --- a/compiler/hlds_module.m
> +++ b/compiler/hlds_module.m
> @@ -863,25 +867,40 @@
>                  % the same file.
>                  mri_atomics_per_context         :: map(prog_context, counter),
>  
> -                % The names of all the directly imported modules
> -                % (used during type checking, and by the MLDS back-end).
> -                % XXX CLEANUP The above is COMPLETELY WRONG.
> +                % The add_item_avails predicate in make_hlds_passes.m fills
> +                % this field with information about all the nonabstract
> +                % import_module and use_module declarations in the module
> +                % being compiled, and in the .in0 interface files of its
> +                % ancestors.

.int0

What do you mean by "nonabstract" import_module and use_module
declarations?

> +                %
> +                % Each entry in the avail_module_map will specify
> +                %
> +                % - whether the module is imported or used in the interface
> +                %   of either the module or its ancestors, and
> +                %
> +                % - whether the module is imported (as opposed to used)
> +                %   in either the module or its ancestors.
> +                %
> +                % Each entry will also contain a list of avail_modules
> +                % *for import/use_module declarations in this module only*;
> +                % there won't be any entries for imports/uses in ancestors.
>                  %

> @@ -1666,6 +1689,13 @@ module_add_avail_module_name(ModuleName, NewSection, NewImportOrUse,
>      AvailMap0 = !.MI ^ mi_rare_info ^ mri_avail_module_map,
>      ( if map.search(AvailMap0, ModuleName, OldEntry) then
>          OldEntry = avail_module_entry(OldSection, OldImportOrUse, OldAvails),
> +        % XXX: If one of the entries (new or old) as a use_module in the

is a

> +        % interface sectin, while the other is an import_module in the

section

> +        % implementation section, the result *ought* to be something like
> +        % the int_use_imp_import alternative of the section_import_or_use type,
> +        % BUT the design of avail_module_entry has no way to express that.
> +        % The code of combine_old_new_avail_attrs returns "import_module in the
> +        % interface section" in such cases, which is almost certainly a bug.
>          combine_old_new_avail_attrs(OldSection, NewSection,
>              OldImportOrUse, NewImportOrUse, Section, ImportOrUse),
>          Avails = NewAvails ++ OldAvails,


> diff --git a/compiler/make_hlds_passes.m b/compiler/make_hlds_passes.m
> index 3665d746d..1cb43a81b 100644
> --- a/compiler/make_hlds_passes.m
> +++ b/compiler/make_hlds_passes.m
> @@ -659,8 +666,7 @@ add_item_avail(ItemMercuryStatus, Avail, !ModuleInfo) :-
>                  % (for Section = ms_implementation), we should be able
>                  % to generate a warning for that when we compile the ancestor.
>                  % XXX We do not currently do this.
> -                module_info_add_module_to_public_used_modules(ModuleName,
> -                    !ModuleInfo)
> +                set.insert(ModuleName, !AncestorAvailModules)
>              ;
>                  ( ImportLocn = import_locn_interface
>                  ; ImportLocn = import_locn_implementation

I think you should update/delete the comment above that change that says:

                % We therefore record it as "used" to avoid reporting
                % a warning that may be incorrect.

> diff --git a/compiler/unused_imports.m b/compiler/unused_imports.m
> index a886ef60c..c09d70e44 100644
> --- a/compiler/unused_imports.m
> +++ b/compiler/unused_imports.m
> @@ -173,17 +184,26 @@ get_avail_modules_anywhere_interface([ModuleEntry | ModuleEntries],
>  maybe_warn_about_avail(TopModuleName,
>          UnusedAnywhereImports, UnusedInterfaceImports,
>          ModuleName, AvailEntry, !Specs) :-
> -    AvailEntry = avail_module_entry(Section, ImportOrUse, Avails),
> +    AvailEntry = avail_module_entry(_Section, _ImportOrUse, Avails),
>      list.sort(compare_avails, Avails, SortedAvails),
>      (
>          SortedAvails = []
>      ;
>          SortedAvails = [HeadAvail | _],
> -        HeadAvail = avail_module(_, _, HeadContext),
> +        % NOTE: We *must* get Section and ImportOrUse from one of the elements
> +        % of Avail, and *not* from AvailEntry. This is because the first two

Avail -> HeadAvail

> +        % fields of AvailsEntry are affected by imports and/or uses in

AvailsEntry -> AvailEntry

> +        % ancestors of the current module, while every element of Avails
> +        % is derived from items in the current module. If a module is
> +        % imported in the interface section of an ancestor but is imported
> +        % in the implementation section of this module, we want the error
> +        % message to treat the import as being the implementation section.
> +        % The same reasoning applies to ImportOrUse.
> +        HeadAvail = avail_module(Section, ImportOrUse, HeadContext),
>          maybe_generate_redundant_avail_warnings(ModuleName, SortedAvails,
>              [], !Specs),
>          ( if set.member(ModuleName, UnusedAnywhereImports) then
> -            AnywhereSpec = generate_unused_warning(TopModuleName,
> +            AnywhereSpec = generate_unused_import_warning(TopModuleName,
>                  ModuleName, ImportOrUse, HeadContext, aoi_anywhere),
>              !:Specs = [AnywhereSpec | !.Specs],
>              AnywhereWarning = yes

That seems fine otherwise.

Peter