[m-rev.] for review: don't have the parser return dummy trees

[Zoltan Somogyi]

2022-04-30 13:54 GMT+10:00 "Julien Fischer" <jfischer at opturion.com>:
>> @@ -315,11 +326,70 @@ read_dependencies(Globals, Search, ModuleName, ExpectationContexts,
>>      % XXX If SrcSpecs contains error messages, the parse tree may not be
>>      % complete, and the rest of this predicate may work on incorrect data.
> 
> Why does that comment refer to SrcSpecs?  Both the existing code and this
> modification don't contain that variable (assuming SrcSpecs is meant to refer
> to a variable)? Does it mean Specs there?

"git log -p" tells me it refers to a variable that got incorporated into SrcReadModuleErrors,
which this diff incorporates into HaveReadModuleSrc. I have updated the comment.

>> +        % - It includes modules which are imported but whose sources
>> +        %   are not in the current directory among the dependencies
>> +        %   of the main module in the main module's .dv file.
>> +        %
>> +        %   I see three main classes of such modules. The two obvious classes
>> +        %   are Mercury library modules and a project's own library modules.
> 
> 
> Due to source file mappings a project's own source files may not reside in
> the current directory, although I think that distinction is not important here.

Agreed. I have generalized the description here to cover that case as well.
 
>> +        %   A third class is nested submodules which are not found because
>> +        %   they are looked up as if they were separate submodules. This is
>> +        %   exemplified by the tests/invalid_make_int/sub_c test case,
>> +        %   where sub_c.m imports sub_a.sub_1, but sub_a.sub_1.m does not
>> +        %   exist, because sub_1 is a nested submodule inside sub_a.m.
>> +        %
>> +        %   It is arguable whether including the first two categories
>> +        %   among the main module's dependencies in the .dv file
>> +        %   is a good or not. With the right view path, they can detect
>> +        %   when the main module needs to be rebuilt due to changes in them,
>> +        %   but specifying that view path to --generate-dependencies, and
>> +        %   having the compiler actually use that view path, would be
>> +        %   a better idea, Including the third is probably a bad idea
> 
> Are you assuming that the second class, the project's own library modules, have
> been installed (i.e. as the result of mmake or mmc --make's install target).
> It is not unknown for libraries to be used without having been installed.

No, I am not making that assumption, and I don't think the comment does either.

>> +        %   from all respects, though (a) distinguishing that category
>> +        %   from the other two is nontrivial, and (b) fixing that bad idea
>> +        %   would require changing the expected output of the sub_c test case.
> 
> Why is (b) an issue?  Would the new error be less meaningful?

(b) and (c) are fundamentally different. In case (b), you would like there
to be a dependence, on the named module, so that this module is recompiled
if the named module is changed. You (or at least I) would prefer that
this be a dependence on the *actual location* of the named module's
automatically generated files, but getting it via a mmake view path will do,
and (in many cases) not having a dependence would be liveable as well.
(Many build systems do not handle cross-directory dependencies well.)
In case (c), the actual location of those automatically generated files
will be the current directory, so that issue does not arise. The problem is that
you do need those dependencies on those automatically generated files, but
those "automatically generated" files will never be generated unless the
dependency discovery process reaches (in the case of this example)
the sub_a module, because without reading sub_a.m, mmc can never
discover that sub_a.sub_1 is stored in sub_a.m, NOT in sub_a.sub_1.m,
and that therefore building sub_a.sub_1's autogen files requires operating
on sub_a.m.

The two issues are in a sense orthogonal. In principle, if the imported
module is a nested submodule of a module that is stored in a different
directory, BOTH issues can arise at the same time.
 
>> +        %
>> +        %   The commented out code below is a possible replacement of this
>> +        %   switch and the call following it. We could use it if decide
>> +        %   not to put dummy parse_tree_srcs into the deps_map. Switching
>> +        %   to it bootchecks, though only with a different expected output
>> +        %   for the sub_c test case.
>> +        %
>> +        % - Strangely, even the totally empty parse_tree_src we construct
>> +        %   will end up having dependencies, because we implicitly import
>> +        %   both the public and private builtin modules into all modules,
>> +        %   even if they contain nothing that could refer to either builtin
>> +        %   module :-( However, these dependencies should not drag into
>> +        %   the deps_map any module that other, nondummy modules' dependencies
>> +        %   wouldn't drag in anyway.
> 
> I am puzzled about what the existing behaviour is here: are we writing out
> .dv files in the presence of errors?

Yes. Having "mmc --generate-dependencies" refuse to work just because e.g.
you misspelt a type name in a source file would be a major usability problem.
The question is: what kinds of errors are major enough that when mmc
--generate-dependencies reports that it can't do its job because of them,
people intuitively respond "oh, that's fair".

Basically, the proposed replacement code makes the judgement that
if a module (in this case sub_c.m) imports a module (sub_a.sub_1)
that is physically in the current directory but mmc has no way of finding it there
(it is not in a file that would be the canonical filename for that module name,
it is not in Mercury.options, and you don't even import the module, sub_a,
within which it is nested), then that deserves an error message.

Note that the third point just above would go away if we automagically
imported every ancestor of every imported module, which is something
we have discussed before, though without working through the necessary details.

> That looks fine otherwise.

Thanks for the review. I followed all your suggestions.

Zoltan.