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

[Peter Wang]

On Wed, 26 Aug 2026 14:25:44 +0200 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> 
> 
> On Wed, 26 Aug 2026 15:58:41 +1000, Peter Wang <novalazy at gmail.com> wrote:
> > compiler/module_qual.collect_mq_info.m:
> >     When collecting permissions for entities from .int or .int files,
> 
> The suffix 3 is missing from one of the ".int"s.

Fixed.

> >     if the interface was also read due to a shadowed ancestor import,
> >     let that grant visibility to entities that would not be visible
> >     otherwise, with a warning attached to the permission.
> 
> ... to USES of that permission.

Fixed.

> > --- a/compiler/build_eqv_maps.m
> > +++ b/compiler/build_eqv_maps.m
> > @@ -122,7 +122,8 @@ build_eqv_maps_in_ancestor_int_spec(AncestorIntSpec,
> >  
> >  build_eqv_maps_in_direct_int1_spec(DirectIntSpec,
> >          !TypeEqvMap, !InstEqvMap) :-
> > -    DirectIntSpec = direct_int1(ParseTreeInt1, ReadWhy1),
> > +    DirectIntSpec = direct_int1(ParseTreeInt1, ReadWhy1,
> > +        _MaybeShadowedReadWhy1),
> >      build_eqv_maps_in_parse_tree_int1(ReadWhy1, ParseTreeInt1,
> >          !TypeEqvMap, !InstEqvMap).
> 
> About ignoring the second ReadWhy: what is the correctness argument
> for this being safe to do? Please add a comment describing it.
> 
> The same question also arises for building eqv_maps in other parse trees
> just below.

I have added comments or code where I think it was warranted.

> > --- a/compiler/d_file_deps.m
> > +++ b/compiler/d_file_deps.m
> > @@ -543,7 +543,8 @@ gather_fim_specs_in_ancestor_int_spec(AncestorIntSpec, !FIMSpecs) :-
> >      set(fim_spec)::in, set(fim_spec)::out) is det.
> >  
> >  gather_fim_specs_in_direct_int1_spec(DirectInt1Spec, !FIMSpecs) :-
> > -    DirectInt1Spec = direct_int1(ParseTreeInt1, _ReadWhy1),
> > +    DirectInt1Spec = direct_int1(ParseTreeInt1, _ReadWhy1,
> > +        _MaybeShadowedReadWhy1),
> >      gather_fim_specs_in_parse_tree_int1(ParseTreeInt1, !FIMSpecs).
> 
> And here.

gather_fim_specs_in_parse_tree_int1 doesn't care why the .int file
was read, so adding a secondary reason wouldn't make a difference.

> > --- a/compiler/module_qual.collect_mq_info.m
> > +++ b/compiler/module_qual.collect_mq_info.m
> > @@ -221,10 +243,10 @@ collect_mq_info_in_parse_tree_int0(ReadWhy0, ParseTreeInt0, !Info) :-
> >          % the ancestor imported that mq_id in its INTERFACE or not.
> >          % Since we don't know where that import was, this is a
> >          % conservative approximation.
> > -        IntPermInInt = may_use_in_int(may_be_unqualified),
> > -        IntPermInImp = may_use_in_imp(may_be_unqualified),
> > -        ImpPermInInt = may_use_in_int(may_be_unqualified),
> > -        ImpPermInImp = may_use_in_imp(may_be_unqualified),
> > +        IntPermInInt = perm_in_int(permitted, permitted),
> > +        IntPermInImp = perm_in_imp(permitted, permitted),
> > +        ImpPermInInt = perm_in_int(permitted, permitted),
> > +        ImpPermInImp = perm_in_imp(permitted, permitted),
> 
> I think adding "_qual_unqual" as a suffix to perm_in_{int,imp} would
> make the code easier to understand. Even if the reade remembers
> that the two args for different qualifications, it is easy to forget
> which is first :-( 
> 

Done.

> > --- a/compiler/module_qual.id_set.m
> > +++ b/compiler/module_qual.id_set.m
> > +update_permission(OldPermission, NewPermission) = Result :-
> > +    (
> > +        OldPermission = not_permitted,
> > +        Result = NewPermission
> > +    ;
> > +        OldPermission = permitted,
> > +        Result = permitted
> > +    ;
> > +        ( OldPermission = permitted_with_warning
> > +        ; OldPermission = permitted_with_warning_shadowed
> > +        ),
> > +        % permitted_with_warning and permitted_with_warning_shadowed will be
> > +        % used during a transition period. We do not expect to reach this
> > +        % point.
> > +        Result = OldPermission
> >      ).
> 
> The reason why we don't expect to reach that switch arm is because we add
> the permissions that include warnings *after* the ones that do not include them.
> 

Added.

> 
> I can't untangle the mess diff makes of this change; I will review it post-commit.
> I will review the changes to the tests post-commit as well.
> 
> The diff is fine; thank you. Commit away.

Thanks for the review.

Peter