[m-rev.] for review: Accept code depending on shadowed ancestor imports.
Zoltan Somogyi
zoltan.somogyi at runbox.com
Fri Aug 28 06:30:42 AEST 2026
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.
> 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?
Zoltan.
More information about the reviews
mailing list