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

[Zoltan Somogyi]

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.

>     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.

> --- 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.

> --- 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.

> --- 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 :-( 

> --- 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.

> @@ -529,53 +532,44 @@ add_matching_and_nearmiss_modules_int(InInt, FullyModuleQualified,
>      Permissions = module_permissions(PermInInt, PermInImp),
>      (
>          InInt = mq_used_in_interface,
> -        (
> -            PermInInt = may_not_use_in_int,
> -            !:IntMismatches = [ModuleName | !.IntMismatches]
> -        ;
> -            (
> -                PermInInt = may_use_in_int(NeedQual),
> -                MaybeWarn = do_not_warn
> -            ;
> -                PermInInt = may_use_in_int_warn(NeedQual),
> -                MaybeWarn = warn_if_matching
> -            ),
> -            add_matching_and_nearmiss_modules_qual(FullyModuleQualified,
> -                NeedQual, ModuleName, MaybeWarn, !Matches, !QualMismatches)
> -        )
> +        PermInInt = perm_in_int(PermQual, PermUnqual)
>      ;
>          InInt = mq_not_used_in_interface,
> -        PermInImp = may_use_in_imp(NeedQual),
> -        add_matching_and_nearmiss_modules_qual(FullyModuleQualified,
> -            NeedQual, ModuleName, do_not_warn, !Matches, !QualMismatches)
> -    ).
> -
> -:- type maybe_warn_if_matching
> -    --->    do_not_warn
> -    ;       warn_if_matching.
> -
> -:- pred add_matching_and_nearmiss_modules_qual(bool::in, need_qualifier::in,
> -    module_name::in, maybe_warn_if_matching::in,
> -    list(module_match_type)::in, list(module_match_type)::out,
> -    list(module_name)::in, list(module_name)::out) is det.
> -
> -add_matching_and_nearmiss_modules_qual(FullyModuleQualified, NeedQual,
> -        ModuleName, MaybeWarn, !Matches, !QualMismatches) :-
> -    ( if
> -        ( FullyModuleQualified = yes
> -        ; NeedQual = may_be_unqualified
> -        )
> -    then
> +        PermInImp = perm_in_imp(PermQual, PermUnqual)
> +    ),
> +    (
> +        FullyModuleQualified = yes,
> +        Permission = PermQual,
> +        OtherPermission = PermUnqual
> +    ;
> +        FullyModuleQualified = no,
> +        Permission = PermUnqual,
> +        OtherPermission = PermQual
> +    ),
> +    (
> +        Permission = permitted,
> +        !:Matches = [match(ModuleName) | !.Matches]
> +    ;
>          (
> -            MaybeWarn = do_not_warn,
> -            MatchType = match(ModuleName)
> +            Permission = permitted_with_warning,
> +            WarnType = no_warn_shadowed_ancestor_import
>          ;
> -            MaybeWarn = warn_if_matching,
> -            MatchType = match_with_warning(ModuleName)
> +            Permission = permitted_with_warning_shadowed,
> +            WarnType = also_warn_shadowed_ancestor_import
>          ),
> -        !:Matches = [MatchType | !.Matches]
> -    else
> -        !:QualMismatches = [ModuleName | !.QualMismatches]
> +        !:Matches = [match_with_warning(ModuleName, WarnType) | !.Matches]
> +    ;
> +        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.
I will review the changes to the tests post-commit as well.

The diff is fine; thank you. Commit away.

Zoltan.