[m-rev.] for review: Accept code depending on old visibility rule.

[Zoltan Somogyi]

On Tue, 18 Aug 2026 15:56:03 +1000, Peter Wang <novalazy at gmail.com> wrote:
> compiler/comp_unit_interface.m:
>     Make the new warning message be printed, but not stop interface
>     files from being written out.

This entry describes the change to error_util.m, and the diff
contains no mention of comp_unit_interface.m. You seem to have
mixed up the filename.

> --- a/compiler/module_qual.collect_mq_info.m
> +++ b/compiler/module_qual.collect_mq_info.m
> @@ -298,9 +298,9 @@ collect_mq_info_in_parse_tree_int1(ReadWhy1, ParseTreeInt1, !Info) :-
>          IntPermInInt = may_use_in_int(may_be_unqualified),
>          IntPermInImp = may_use_in_imp(may_be_unqualified)
>      ;
> -        ( ReadWhy1 = rwi1_ancestor_imp_import
> -        ; ReadWhy1 = rwi1_imp_import
> -        ),
> +        % New submodule visibility rule:
> +        % ReadWhy1 = rwi1_ancestor_imp_import
> +        ReadWhy1 = rwi1_imp_import,
>          IntPermInInt = may_not_use_in_int,
>          IntPermInImp = may_use_in_imp(may_be_unqualified)

What does the comment "New submodule visibility rule" apply to?
To the commented-out nature of rwi1_ancestor_imp_import, or
to the non-commented-out nature of rwi1_imp_import?
I think this needs a bit longer comment.

The same issue also occurs later.

> +        % Old submodule visibility rule: during the transition,
> +        % allow entities imported in an ancestor implementation section
> +        % to be visible in the interface section, but generate a warning
> +        % if that is how an entity is used.
> +        ReadWhy1 = rwi1_ancestor_imp_import,
> +        IntPermInInt = may_use_in_int_warn(may_be_unqualified),
> +        IntPermInImp = may_use_in_imp(may_be_unqualified)

This switch case should be next to the case it was carved out of.
Again, the same issue occurs again later.


> --- a/compiler/module_qual.id_set.m
> +++ b/compiler/module_qual.id_set.m
> +            ( PermInInt = may_use_in_int(NeedQual), MaybeWarn = no
> +            ; PermInInt = may_use_in_int_warn(NeedQual), MaybeWarn = yes
> +            ),
>              add_matching_and_nearmiss_modules_qual(FullyModuleQualified,
> -                NeedQual, ModuleName, !Matches, !QualMismatches)
> +                NeedQual, ModuleName, MaybeWarn, !Matches, !QualMismatches)
>          )

I would have used a bespoke type instead of bool for the new argument
of the callee here, but since this should be only temporary code, it's fine.

> --- a/compiler/module_qual.qual_errors.m
> +++ b/compiler/module_qual.qual_errors.m
> @@ -447,6 +458,25 @@ report_ambiguous_match(ErrorContext, Id, IdType,
>          verbose_only(verbose_always, VerbosePieces)]),
>      Spec = gen_spec($pred, severity_error, phase_pt2h, [Msg]).
>  
> +report_old_submodule_visibility_match(ErrorContext, Id, IdType, ModuleName,
> +        Spec) :-
> +    mq_error_context_to_pieces(ErrorContext, Context, _ShouldUnqualId,
> +        ErrorContextPieces),
> +    qual_id_kind_to_string(IdType, IdTypeStr),
> +    MainPieces = [words("In")] ++ ErrorContextPieces ++ [suffix(":"), nl,
> +        words("the"), fixed(IdTypeStr)] ++
> +        color_as_subject([wrap_qual_id(Id)]) ++
> +        [words("is only visible here due to an import of")] ++
> +        color_as_subject([wrap_module_name(ModuleName)]) ++
> +        [words("in an ancestor module's implementation section."),
> +        words("This behaviour is")] ++
> +        color_as_incorrect([words("deprecated.")]) ++
> +        [nl],
> +    Msg = simple_msg(Context, [always(MainPieces)]),
> +    Spec = gen_spec($pred,
> +        severity_warning(warn_old_submodule_visibility_rule), phase_pt2h,
> +        [Msg]).

Two things: this can be a simple spec (no need to construct Msg), and
I usually use a separate Severity = severity_warning(...) assignment,
after which the assignment to Spec fits on one line.

> --- a/compiler/options.m
> +++ b/compiler/options.m
> +optdb(oc_warn_lang_change, warn_old_submodule_visibility_rule, bool(yes),
> +    help("warn-old-submodule-visibility-rule", [
> +        w("Do not generate a warning if an entity is only visible"),
> +        w("using a deprecated submodule visibility rule.")])).

I would use a longer sentence to explain the specific change here.
The wording in the new diagnostic should work here as well,
suitably modified.

The diff is otherwise fine. Thanks for adding it.

Zoltan.