[m-rev.] for post-commit review: improve --warn-too-private-instances

[Julien Fischer]

On Tue, 22 Jul 2025 at 22:57, Zoltan Somogyi <zoltan.somogyi at runbox.com> wrote:

> Improve --warn-too-private-instances.
>
> This implements some improvements to its idea from 2022 mar 19 on m-rev.
>
> compiler/check_typeclass.m:
>     Consider an instance justifiably kept private if a constraint
>     on the instance refers to a private type class or type constructor.
>
>     Extend the test for what is considered "private" to also include
>     classes and type constructs that are defined in private submodules
>     of the current module.
>
>     Extend the text of the warning to explain the reasoning behind it.
>
> compiler/options.m:
>     Extend the documentation of the option to explain the reasoning
>     behind it.
>
> tests/invalid/impure_method_impl.err_exp:
> tests/invalid/inconsistent_instances.err_exp:
> tests/invalid/method_impl.err_exp:
> tests/invalid/mpj_3.err_exp:
> tests/invalid/mpj_4.err_exp:
> tests/invalid/range_restrict.err_exp:
> tests/warnings/unused_interface_import.err_exp:
>     Expect the new text for the warning.
>
> tests/warnings/help_text.err_exp:
>     Expect the new help text.
>
> diff --git a/compiler/check_typeclass.m b/compiler/check_typeclass.m
> index aa4d2dc6b..b7df62e7c 100644
> --- a/compiler/check_typeclass.m
> +++ b/compiler/check_typeclass.m
> @@ -709,21 +732,124 @@ acc_type_ctors_in_type(Type, !TypeCtorSet) :-
>          acc_type_ctors_in_type(SubType, !TypeCtorSet)
>      ).
>
> -:- pred type_ctor_is_private(type_table::in, type_ctor::in) is semidet.
> +:- pred type_ctor_is_private(module_info::in, type_ctor::in) is semidet.
>
> -type_ctor_is_private(TypeTable, TypeCtor) :-
> +type_ctor_is_private(ModuleInfo, TypeCtor) :-
> +    module_info_get_type_table(ModuleInfo, TypeTable),
>      lookup_type_ctor_defn(TypeTable, TypeCtor, TypeDefn),
>      get_type_defn_status(TypeDefn, TypeStatus),
> -    type_status_defined_in_this_module(TypeStatus) = yes,
> -    type_status_is_exported(TypeStatus) = no.
> +    DefinedHere = type_status_defined_in_this_module(TypeStatus),
> +    % A type_ctor is private if either ...
> +    (
> +        DefinedHere = yes,
> +        % ... it is defined in this module, and not exported, ...
> +        type_status_is_exported(TypeStatus) = no
> +    ;
> +        DefinedHere = no,
> +        % ... or it is defined in a submodule nested inside this module,
> +        % and that submodule is not visible from outside this module.
> +        TypeCtor = type_ctor(TypeCtorSymName, _Arity),
> +        TypeCtorSymName = qualified(TypeCtorModuleName, _),
> +        module_name_is_private_submodule(ModuleInfo, TypeCtorModuleName)
> +    ).
>
> -:- pred report_unnecessarily_private_instance(class_id::in, set(type_ctor)::in,
> +:- pred class_is_private(module_info::in, class_id::in) is semidet.
> +
> +class_is_private(ModuleInfo, ClassId) :-
> +    module_info_get_class_table(ModuleInfo, ClassTable),
> +    map.lookup(ClassTable, ClassId, ClassDefn),
> +    ClassStatus = ClassDefn ^ classdefn_status,
> +    DefinedHere = typeclass_status_defined_in_this_module(ClassStatus),
> +    % A type_ctor is private if either ...

s/type_ctor/class

> +    (
> +        DefinedHere = yes,
> +        % ... it is defined in this module, and not exported, ...
> +        typeclass_status_is_exported(ClassStatus) = no
> +    ;
> +        DefinedHere = no,
> +        % ... or it is defined in a submodule nested inside this module,
> +        % and that submodule is not visible from outside this module.
> +        ClassId = class_id(ClassSymName, _Arity),
> +        ClassSymName = qualified(ClassModuleName, _),
> +        module_name_is_private_submodule(ModuleInfo, ClassModuleName)
> +    ).
> +
> +    % module_name_is_private_submodule(ModuleInfo, QueryModuleName):
> +    %
> +    % Is QueryModuleName the name of a privaye submodule of the module

s/privaye/private/

> +    % represented by ModuleInfo?
> +    %
> +:- pred module_name_is_private_submodule(module_info::in, module_name::in)
> +    is semidet.
> +
> +module_name_is_private_submodule(ModuleInfo, QueryModuleName) :-
> +    module_info_get_name(ModuleInfo, ModuleName),
> +    % XXX Should the code that tests whether QueryModuleName is a submodule
> +    % of ModuleName be in mdbcomp/sym_name.m? is_same_module_or_submodule
> +    % is already there, but it does not return LeftOverComponents.
> +    ModuleNameComponents = sym_name_to_list(ModuleName),
> +    QueryModuleNameComponents = sym_name_to_list(QueryModuleName),
> +    remove_prefix(ModuleNameComponents, QueryModuleNameComponents,
> +        LeftOverComponents),
> +    LeftOverComponents = [HeadSubComponent | _TailSubComponents],
> +    HeadSubModuleName = qualified(ModuleName, HeadSubComponent),
> +    % What we want to do here is
> +    % - to fail if any component in [HeadSubComponent | TailSubComponents]
> +    %   is in the interface section of ita parent, and conversely
> +    % - to succeed only if all of them are in the implementation section.
> +    % Unfortunately, while we can use IncludeMap to tell whether
> +    % a *direct* submodule of ModuleName is visible outside ModuleName,
> +    % we do not have the include_module_maps of ModuleName's submodules,
> +    % and so we cannot tell whether an *indirect* submodule of ModuleName
> +    % that is included in the interface section of ModuleName,
> +    % is visible outside ModuleName. We have to guess. We always guess
> +    % that it such indirect submodules are visible outside ModuleName,

Delete "it".

> +    % because we prefer to miss generating a valid warning in this
> +    % rare circumstance over generating an invalid warning.
> +    module_info_get_include_module_map(ModuleInfo, IncludeMap),
> +    map.lookup(IncludeMap, HeadSubModuleName, HeadSubIncludeInfo),
> +    HeadSubIncludeInfo = include_module_info(HeadSubSection, _),
> +    HeadSubSection = ms_implementation.
> +
> +    % XXX Should this be library/list.m?

I think so.

> +    %
> +:- pred remove_prefix(list(T)::in, list(T)::in, list(T)::out) is semidet.
> +
> +remove_prefix([], ListB, LeftOverB) :-
> +    LeftOverB = ListB.
> +remove_prefix([HeadA | TailA], [HeadB | TailB], LeftOverB) :-
> +    HeadA = HeadB,
> +    remove_prefix(TailA, TailB, LeftOverB).

The rest looks fine.

Julien.