[m-rev.] for review: replace item_blocks with parse trees during pre-HLDS passes

[Julien Fischer]

Hi Zoltan,

On Fri, 13 Mar 2020, Zoltan Somogyi wrote:

> On Fri, 13 Mar 2020 01:38:07 +1100 (AEDT), Julien Fischer <jfischer at opturion.com> wrote:
>> On Tue, 10 Mar 2020, Zoltan Somogyi wrote:
>>
>>> For review by Julien, since he kindly volunteered.
>>> Thank you in advance. Note that some of the XXXs
>>> in the log message are questions for you. I would
>>> also appreciate it if, besides reviewing the diff, you
>>> also tried it out on the programs you are working on.
>>
>> I have done so and have not encountered any problems.  One thing I did
>> encounter due to the changes in warnings for unused modules is:
>>
>>      search.m:043:   warning: module `fd' is imported in the interface, but it is
>>      search.m:043:   not used in the interface.
>>      search.m:091: Warning: this `:- import_module' declaration for module `fd' in
>>      search.m:091:   the implementation section is redundant, given the
>>      search.m:091:   `:- import_module' declaration for the same module in the
>>      search.m:091:   interface section.
>>      search.m:043:   The previous declaration is here.
>>
>> We could argubly omit the second warning in cases like the above.
>
> Agreed, but the problem is that those two warnings are generated by two different
> modules, and the second one printed is actually generated first, so we can't use
> a simple "have we warned about the interface import being unused" test to
> avoid generating the warning about the implementation import being shadowed.
> The only way to handle this would be to delay the generation of the warnings about
> shadowing until *after* module qualification discovers that an interface import
> is not needed, which is not a simple change. I will look into whether it is possible
> to do that at reasonable cost after I commit this diff, but my guess is "no",
> given that any improvement in usability would be minor.

Agreed, I wouldn't do anything about it unless it's trivial to fix.
(The updated version of the compiler found quite a lot of redundant
imports in the code I tested it on; the above only occurred once or
twice.)

...

>>>     the case where A's interface contains nested submodule A.B's interface, and
>>>     A's implementation contains nested submodule A.B's implementation.
>>>     Other than that, has been split_parse_tree_src.m any duplicate inclusions
>>>     of submodules, whether via nesting or via include_module declarations.
>>
>> What is that last sentence trying to say?
>
> Yep, that paragraph was sent unfinished. Have a look to see whether the updated
> version in the attached updated log file is readable.

Looks fine.

...

>>> NEWS:
>>>     Mention that we now generate warnings for unused import_module and
>>>     use_module declarations in the interface even if the module has
>>>     submodules.
>>>
>>>     XXX Should we report this as a breaking change?
>>
>> Yes.
>
> OK, please have a look at the attached DIFF.NEWS.


> diff --git a/NEWS b/NEWS
> index 46a88fdfa..448094d92 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -11,6 +11,18 @@ Changes that may break compatibility
>    simply by importing `one_or_more` as well as (or in rare cases, instead of)
>    the `list` module.
> 
> +* If module A contains a `:- import_module` or a `:- use_module` declaration
> +  for module B but does not refer to anything defined in module B, we used
> +  to generate a warning for the declaration only if module A had no submodules,
> +  because it is possible that A's submodules refer to entities defined in B.
> +  We now generate a warning for unused `:- import_module` and `:- use_module`
> +  declaration in this case as well, which will stop the program from compiling
> +  if `--halt-at-warn` is set. The fix is simple: replace the declaration
> +  being warned about in the parent module with declarations in the child
> +  modules that need it. In the fairly frequent case that not all child modules
> +  need module B, this avoids the need to recompile those child modules
> +  when the interface of module B changes.
> +
>  Changes to the Mercury standard library
>  ---------------------------------------
> 
> @@ -174,13 +186,33 @@ Changes to the Mercury standard library
>  Changes to the Mercury compiler
>  -------------------------------
> 
> +### Changes to the treatment of unused imports
> +
> +* The compiler now generates warnings for import_module and use_module

Mark up import_module etc in the same way as in the previous entry, e.g.
`:- import_module`

> +  declarations in the interface of a module even if that module has
> +  submodules. Previously, it generated such warnings only if the module
> +  had no submodules.
> +
>  ### Changes to code model options
>
>  * `--trail-segments`
> 
> -   Grades that support trailing now always use trail segments. This means
> -   that the `--trail-segments` option now has no effect, and it is therefore
> -   deprecated.
> +  Grades that support trailing now always use trail segments. This means
> +  that the `--trail-segments` option now has no effect, and it is therefore
> +  deprecated.
> +
> +### New warning options
> +
> +* `--print-errors-warnings-when-generating-interface`
> +  Until now, the compiler did not try to detect problems with a module

You need a blank line between the item heading and body there.

> +  when generating its interface files. Now it does. To preserve compability,
> +  by default it still ignores any problems it finds then, but if this option
> +  is enabled, and if it does in fact find any problems, the compiler will
> +  report them, and if they are severe enough, it will not generate the
> +  interface file at all. In the usual use case where the compiler is asked
> +  to generate the interface file as part of rebuilding the whole executable,
> +  this behavior has the advantage that the rebuild, which would fail anyway,
> +  fails sooner.

Julien.