[m-rev.] for review: Accept code depending on shadowed ancestor imports.

[Peter Wang]

On Thu, 27 Aug 2026 22:30:42 +0200 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> 
> 
> On Wed, 26 Aug 2026 14:25:44 +0200 (CEST), "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> > > +        Permission = not_permitted,
> > > +        (
> > > +            OtherPermission = not_permitted,
> > > +            !:IntMismatches = [ModuleName | !.IntMismatches]
> > > +        ;
> > > +            ( OtherPermission = permitted
> > > +            ; OtherPermission = permitted_with_warning
> > > +            ; OtherPermission = permitted_with_warning_shadowed
> > > +            ),
> > > +            !:QualMismatches = [ModuleName | !.QualMismatches]
> > > +        )
> > >      ).
> > 
> > I can't untangle the mess diff makes of this change; I will review it post-commit.
> 
> The quoted last part of that updated predicate is hard to understand.
> 
> One issue is that some invariants are not explicitly documented.
> I believe all of these should hold:
> 
> - in perm_in_INT_qual_unqual, the qual field should be at least as permissive
>   as the unqual field;
> - the same should be true for perm_in_IMP_qual_unqual;
> - the qual field in perm_in_IMP_qual_unqual should be at least as permissive
>   as the qual field in perm_in_INT_qual_unqual;
> - the same should true for their unqual fields;
> - at least one of the four fields (int/imp, qual/unqual) should permit *something*,
>   because otherwise there should be no module_permission at all
>   for the entity in question.
> 
> If you agree that these are indeed invariants, then they should be documented.
> 
> It is the third, fourth and fifth of these invariants that together imply that
> Permission and OtherPermission (which would be better named OtherQualPermission)
> both being not_permitted imply that  Permission and OtherPermission are both
> from a perm_in_INT, and at least one half of perm_in_IMP must be more permissive.
> Which is why classifying the reference as being permitted in the implementation
> but not in the interface is correct. There should be a comment here explaining this.
> 

Thanks, I've added your invariants in a comment. I believe I only used
the first two when writing the code, but it doesn't hurt to write them
all down.

I've made add_matching_and_nearmiss_modules_int more understandable now.

> > I will review the changes to the tests post-commit as well.
> 
> They look good, though I did not check the messages against the source files.
> 
> One thing I noticed: two of the .err_exp files are zero size. Why? Shouldn't
> a test that generates no diagnostics be in a non-invalid test directory?

Strictly speaking, yes, but they are part of a set of test cases that
differ only in the location of the import_module or use_module declarations.
Furthermore, those tests are run using custom make rules,
which I don't want to duplicate in another directory.

We could make those tests produce diagnostics for some arbitrary reason,
but that seems pointless.

Peter