[m-rev.] for review: warn about insts without matching types

[Julien Fischer]

On Tue, 4 Jul 2006, Ian MacLarty wrote:

> compiler/mercury_compile.m:
> 	Check insts at stage 12, before type and mode checking.
>
> compiler/options.m:
> 	Add an option to turn the new warning on or off.

You also need to add the new option to the user's guide.  You also need
to mention the new module in compiler/notes/compiler_design.html.

...

> Index: compiler/inst_check.m
> ===================================================================
> RCS file: compiler/inst_check.m
> diff -N compiler/inst_check.m
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ compiler/inst_check.m	4 Jul 2006 00:10:33 -0000
> @@ -0,0 +1,291 @@
> +%-----------------------------------------------------------------------------%
> +% vim: ft=mercury ts=4 sw=4 et
> +%-----------------------------------------------------------------------------%
> +% Copyright (C) 2006 The University of Melbourne.
> +% This file may only be copied under the terms of the GNU General
> +% Public License - see the file COPYING in the Mercury distribution.
> +%-----------------------------------------------------------------------------%
> +%
> +% File: inst_check.m
> +% Main author: maclarty.
> +%
> +% This module exports a predicate that checks that each user defined inst is
> +% consistant with at least one type in scope.
> +%
> +% TODO:
> +%   If we find an inst that is not consistent with any types in scope,

not consistent == inconsistent

I would say "any of the types" in preference to "any" - if you're just
skimming the comments it's too easy to confuse that with the any of any
insts.  (change the other spots in the NEWS file etc as well.)

> +%   except for one function symbol with a different arity, then we
> +%   should communicate this in the warning message.
> +%
> +%-----------------------------------------------------------------------------%
> +
> +:- module check_hlds.inst_check.
> +
> +:- interface.
> +
> +:- import_module io.
> +
> +:- import_module hlds.hlds_module.
> +
> +    % This predicate issues a warning for each user defined bound insts
> +    % that is not consistant with at least one type in scope.
> +    %
> +:- pred check_insts_have_matching_types(module_info::in,
> +    io::di, io::uo) is det.
> +

...

> +check_insts_have_matching_types(Module, !IO) :-
> +    module_info_get_inst_table(Module, InstTable),
> +    inst_table_get_user_insts(InstTable, UserInstTable),
> +    user_inst_table_get_inst_defns(UserInstTable, InstDefs),
> +    module_info_get_type_table(Module, TypeTable),
> +    TypeDefs = map.values(TypeTable),

You should exclude opt-imported type definitions here.  Otherwise we could end
up with the situation where a program will compile with
`--intermodule-optimization' enabled but won't compile otherwise.  ( You'll also
need to add a test case for that as well - or extend the existing one.)

...

> +:- type functors_to_types == multi_map(sym_name_and_arity, hlds_type_defn).
> +
> +:- type bound_inst_functor
> +    --->    name_and_arity(sym_name, int)

s/int/arity/

...

> +:- pred maybe_issue_inst_check_warning(inst_id::in, hlds_inst_defn::in,
> +    list(list(type_defn_or_builtin))::in, io::di, io::uo) is det.
> +
> +maybe_issue_inst_check_warning(InstId, InstDef, MatchingTypeLists,
> +        !IO) :-
> +    ( if
> +        MatchingTypeLists = [MatchingTypeList | _],
> +        not (some [Type] (
> +            list.member(Type, MatchingTypeList),
> +            all [TypeList] (
> +                list.member(TypeList, MatchingTypeLists)
> +            =>
> +                list.member(Type, TypeList)
> +            )
> +        ))
> +    then
> +        Context = InstDef ^ inst_context,
> +        InstId = inst_id(InstName, InstArity),
> +        InstSymNameAndArity = InstName / InstArity,
> +        report_warning(Context, 0, [words("warning: inst "),
> +            sym_name_and_arity(InstSymNameAndArity),
> +            words(" has no matching type in scope.")], !IO)

For consistency with the other warnings in the compiler: s/warning/Warning/.

Just a formatting issue: I would rewrite that as

	Warning = [
		words("Warning: inst"),
		sym_name_and_arity(InstName / InstArity),
		words("does not match any of the types in scope.")
	],
	report_warning(Context, 0, Warning, !IO).

The rationale is that it makes the warning message easier to modify.
(Also you have extraneous spaces in the words format component.)

...

> +long_option("warn-insts-without-matching-type",
> +    warn_insts_without_matching_type).
>
>  % verbosity options
>  long_option("verbose",                  verbose).
> @@ -2766,6 +2770,9 @@
>          "--no-warn-inferred-erroneous",
>          "\tDon't warn about procedures whose determinism is inferred",
>          "\terroneous but whose determinism declarations are laxer.",
> +        "--no-warn-insts-without-matching-type",
> +        "\tDon't warn about insts that are not consistent with any",
> +        "\ttypes in scope.",


	Don't warn about insts that are inconsistent with any of the
	types in scope.


> Index: tests/warnings/inst_with_no_type.m
> ===================================================================
> RCS file: tests/warnings/inst_with_no_type.m
> diff -N tests/warnings/inst_with_no_type.m
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ tests/warnings/inst_with_no_type.m	4 Jul 2006 00:10:33 -0000
> @@ -0,0 +1,64 @@
> +:- module inst_with_no_type.
> +
> +:- interface.
> +
> +:- inst chars
> +	--->	a
> +	;	b
> +	;	c.
> +
> +:- inst i1
> +	--->	t1_f1(ground, ground)
> +	;	t1_f2(ground).
> +
> +:- inst i1_no_match
> +	--->	t1_f1(ground)
> +	;	t1_f2(ground).
> +
> +:- type t1
> +	--->	t1_f1(int, int)
> +	;	t1_f2(string).
> +
> +:- inst i2(I)
> +	--->	i2_f1(ground, I)
> +	;	i2_f2(ground, ground, ground)
> +	;	i2_f3(ground)
> +	;	i2_f4.
> +
> +:- type t2(T)
> +	--->	i2_f1(character, T)
> +	;	i2_f2(T, T, int)
> +	;	i2_f3(float)
> +	;	i2_f4
> +	;	i2_f5.
> +
> +:- inst i2_no_match(I)
> +	--->	i2_f1(ground, I)
> +	;	i2_f2(ground, ground, ground, ground)
> +	;	i2_f3(ground)
> +	;	i2_f4.
> +
> +:- inst i3
> +	--->	"a"
> +	;	"b"
> +	;	"c".
> +
> +:- inst i4_no_match
> +	--->	1
> +	;	2
> +	;	"3".
> +
> +:- inst i5
> +	--->	1
> +	;	2.
> +
> +:- inst i6
> +	--->	1.1
> +	;	1.2.
> +
> +:- inst i7_no_match
> +	--->	1.0
> +	;	1.
> +
> +:- inst tuple
> +	--->	{ground, ground}.

I suggest adding some other insts to this test case as well, e.g. some
higher-order ones, any ones, (mostly-)uniques ones etc.

Also, we (currently) need to support the situation where an inst in the
interface refers to constructors in the implementation (due to the
implementation not supporting abstract insts), e.g.

	:- interface.

	:- type fruit.
	:- type citrus ---> lemon ; orange.

	:- implementation.

	:- type fruit ---> apple ; orange ; lemon.

The test case should also check that this still works.

Julien.
--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------