[m-rev.] for review: prepare for two new warnings on instances

[Julien Fischer]

Hi Zoltan,

On Sat, 19 Mar 2022, Zoltan Somogyi wrote:

> Prepare to add new warnings about instances.
> 
> Prepare for these new options, --warn-unnecessarily-private-instance and
> --warn-inconsistent-instance-order, by
> 
> - moving some checks on instance declarations to a point in time
>   when the set of instance declarations in the module is complete, and
> 
> - adding a new field to instance declarations to record a context needed
>   for the second of the above warnings.
> 
> compiler/add_class.m
> compiler/check_typeclass.m
>     Move some checks on instances from add_class.m, where they have to
>     work with incomplete data, to check_typeclass.m, where they have access
>     to all instance declarations. The tests moved are
>
>     - the test whether the abstract and concrete forms of an instance
>       specify the same constraints or not,
>     - the test for duplicate concrete instances, and
>     - the test for overlapping instances.
>
>     The new code can be, and is, much more systematic in looking for
>     inconsistencies between instance declarations. For example,
>     it distinguishes between duplicate and overlapping instance declarations,
>     whereas the old code did not. This allows it to generate more precise
>     messages.
>
>     We do in fact generate a new warning message, for duplicate abstract
>     instance declarations.
>
>     Crucially, the new code can also gather the information needed
>     for --warn-inconsistent-instance-order, although the gather info

s/gather/gathered/

>     is then throw away. This will change soon.

...


> diff --git a/compiler/check_typeclass.m b/compiler/check_typeclass.m
> index cc5da35d9..926a4027c 100644
> --- a/compiler/check_typeclass.m
> +++ b/compiler/check_typeclass.m

...

> +:- pred check_for_missing_concrete_instances_in_class_and_vector(class_id::in,
> +    pair(type_vector, type_vector_instances)::in, hlds_instance_defn::out,
>      list(error_spec)::in, list(error_spec)::out) is det.
> 
> -check_for_corresponding_instances_2(Concretes, ClassId, AbstractInstance,
> -        !Specs) :-
> -    AbstractTypes = AbstractInstance ^ instdefn_types,
> -    ( if multi_map.search(Concretes, ClassId, ConcreteInstances) then
> -        ( if
> -            list.member(ConcreteInstance, ConcreteInstances),
> -            ConcreteTypes = ConcreteInstance ^ instdefn_types,
> -            ConcreteTypes = AbstractTypes
> -        then
> -            MissingConcreteError = no
> -        else
> -            % There were concrete instances for ClassId in the implementation
> -            % but none of them matches the abstract instance we have.
> -            MissingConcreteError = yes
> -        )
> +check_for_missing_concrete_instances_in_class_and_vector(ClassId,
> +        _TypeVector - VectorInstances, PickedInstance, !Specs) :-
> +    % XXX We should use _TypeVector to implement the check
> +    % now done by check_instance_declaration_types_for_instance.
> +    %
> +    % XXX We should also use it to implement --warn-unnecessarily-private-
> +    % instance, which (as proposed by Julien on 2022 mar 8) would warn about
> +    % non-exported instances whose type vectors contain only imported
> +    % type constructors.

See below.

> diff --git a/compiler/options.m b/compiler/options.m
> index ceb058379..77b23ce26 100644
> --- a/compiler/options.m
> +++ b/compiler/options.m
> @@ -193,6 +193,8 @@
>      ;       warn_missing_opt_files
>      ;       warn_missing_trans_opt_files
>      ;       warn_missing_trans_opt_deps
> +    ;       warn_unnecessarily_private_instance
> +    ;       warn_inconsistent_instance_order
>      ;       warn_inconsistent_pred_order_clauses
>      ;       warn_inconsistent_pred_order_foreign_procs
>      ;       warn_non_contiguous_decls
> @@ -1238,6 +1240,8 @@ optdef(oc_warn, warn_nothing_exported,                  bool(yes)).
>  optdef(oc_warn, warn_unused_args,                       bool(no)).
>  optdef(oc_warn, warn_interface_imports,                 bool(yes)).
>  optdef(oc_warn, warn_interface_imports_in_parents,      bool(no)).
> +optdef(oc_warn, warn_unnecessarily_private_instance,    bool(no)).
> +optdef(oc_warn, warn_inconsistent_instance_order,       bool(no)).
>  optdef(oc_warn, warn_inconsistent_pred_order_clauses,   bool(no)).
>  optdef(oc_warn, warn_inconsistent_pred_order_foreign_procs, bool(no)).
>  optdef(oc_warn, warn_non_contiguous_decls,              bool(yes)).
> @@ -2135,6 +2139,10 @@ long_option("warn-unused-args",         warn_unused_args).
>  long_option("warn-interface-imports",   warn_interface_imports).
>  long_option("warn-interface-imports-in-parents",
>                                          warn_interface_imports_in_parents).
> +long_option("warn-unnecessarily-private-instance",
> +                                        warn_unnecessarily_private_instance).
> +long_option("warn-inconsistent-instance-order",
> +                                        warn_inconsistent_instance_order).
>  long_option("warn-inconsistent-pred-order",
>                      warn_inconsistent_pred_order_clauses).
>  long_option("warn-inconsistent-pred-order-clauses",
> @@ -4268,6 +4276,19 @@ options_help_warning(Stream, !IO) :-
>          "\tto allow `.trans_opt' files to be read when creating other",
>          "\t`.trans_opt' files has been lost. The information can be",
>          "\trecreated by running `mmake <mainmodule>.depend'",
> +long_option("warn-unnecessarily-private-instance",
> +long_option("warn-inconsistent-instance-order",
> +        "--warn-unnecessarily-private-instance",
> +        "\tGenerate a warning if an instance declaration is not exported",
> +        "\teven though none of the type constructors mentioned in the",
> +        % XXX Should this be like this (what Julien proposed)
> +        "\tmember types are defined in the current module.",
> +        % XXX or like this (my preference)?
> +        "\tmember types are local to the current module.",
> +        "--warn-inconsistent-instance-order",
> +        "\tGenerate a warning if the order of concrete instance declarations",
> +        "\tdoes not match the order of the corresponding abstract",
> +        "\tdeclarations.",

What I said on the mailing list was "non-local" types, which is a bit
ambiguous; I probably should have said was "non-private".
You should get the warning if a module has a non-exported type class instance
whose type vector consists only of (a) imported types** or (b) types exported
in the interface of the _current_ module.

Should we get the warning if there are any class constraints on the
instance that refer to private type classes?

** Types imported from private submodules are not non-private so should not
trigger the warning.

The diff looks fine.

Julien.