[m-rev.] for review: fix Mantus bug #44

[Julien Fischer]

On Fri, 8 Feb 2008, Zoltan Somogyi wrote:

> Fix two problems that together caused bug Mantis bug #44.
>
> The first bug was that unify_gen.m wasn't checking whether a variable it was
> adding to a closure was of dummy type or not.
>
> The second bug was that the code for recognizing whether a type is dummy or not
> recognized only two cases: builtin dummy types such as io.state, and types
> with one function symbol of arity zero. In this program, there is a notag
> wrapper around a dummy type. Since the representation of a notag type is
> always the same as the type it wraps, this notag type should be recognized
> as a dummy type too.
>
> compiler/unify_gen.m:
> 	Fix the first bug by adding the required checks.
>
> compiler/code_info.m:
> 	Add a utility predicate to factor out some now common code in
> 	unify_gen.m.
>
> (The modifications to all the following files were to fix the second bug.)
>
> compiler/hlds_data.m:
> compiler/prog_type.m:
> 	Change the type_category type (in prog_type.m) and the enum_or_dummy
> 	type (in hlds_data.m) to separate out the representation of notag types
> 	from other du types. This allows the fix for the second bug, and
> 	incidentally allows some parts of the compiler to avoid the same tests
> 	over and over.
>
> 	To ensure that all places in the compiler that could need special
> 	handling for notag types get them, rename those types to
> 	type_ctor_category (since it does *not* take argument types into
> 	account) and du_type_kind respectively.
>
> 	Since the type_ctor_category type needs to be modified anyway,
> 	change it to allow code that manipulates values of the type to
> 	factor out common code fragments.
>
> 	Rename some predicates, and turn some into functions where this helps
> 	to make code (either here or in clients) more robust.
>
> compiler/make_tags.m:
> 	When creating a HLDS representation for a du type, record whether
> 	it is a notag type (we already recorded whether it is enum or dummy).
>
> compiler/type_util.m:
> 	Fix the predicate that tests for dummy types by recognizing the third
> 	way a type can be a dummy type.
>
> 	Don't test for dummyness of the argument when deciding whether
> 	a type could be a notag types; just record it as a notag type,
> 	and let later lookup code use the new fixed algorithm to do the right
> 	thing.
>
> 	Add a type for recording the is_dummy_type/is_not_dummy_type
> 	distinction.
>
> 	Rename some predicates, and turn some into functions where this helps
> 	to make code (either here or in clients) more robust.
>
> 	Add an XXX about possible redundant code.
>
> compiler/llds.m:
> 	Use the new type instead of booleans in some places.
>
>
> 	Make a few analyses more precise by using the new detail in the
> 	type_ctor_category type to make less conservative assumptions about
> 	du types that are either notag or dummy.
>
> 	In ctgc.selector.m, ctgc.util.m, make_tags.m, mlds_to_java.m
> 	and special_pred.m, add XXXs about possible bugs.

For mlds_to_java.m, most of the XXXs concern foreign enumerations which
haven't been implemented for that backend yet.  I've mentioned below what
should be done assuming that the Java backend still uses the current
data representation when/if foreign enumerations are implemented for it.

...


> Index: compiler/mlds_to_java.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/compiler/mlds_to_java.m,v
> retrieving revision 1.101
> diff -u -b -r1.101 mlds_to_java.m
> --- compiler/mlds_to_java.m	31 Dec 2007 10:03:50 -0000	1.101
> +++ compiler/mlds_to_java.m	8 Feb 2008 07:57:14 -0000
> @@ -182,7 +182,8 @@
>
> type_is_enum(Type) :-
>     Type = mercury_type(_, Builtin, _),
> -    Builtin = type_cat_enum.
> +    % XXX Should this succeed also for cat_enum_foreign?
> +    Builtin = ctor_cat_enum(cat_enum_mercury).

Yes, it should succeed for cat_enum_foreign.

>     % Succeeds iff this type is something that the Java backend will represent
>     % as an object i.e. something created using the new operator.

..

> -:- pred output_mercury_user_type(mer_type::in, type_category::in,
> +:- pred output_mercury_user_type(mer_type::in, type_ctor_category::in,
>     io::di, io::uo) is det.
>
> -output_mercury_user_type(Type, TypeCategory, !IO) :-
> +output_mercury_user_type(Type, CtorCat, !IO) :-
>     ( type_to_ctor_and_args(Type, TypeCtor, _ArgsTypes) ->
>         ml_gen_type_name(TypeCtor, ClassName, ClassArity),
> -        ( TypeCategory = type_cat_enum ->
> +        % XXX Should the test succeed for cat_enum_foreign?
> +        ( CtorCat = ctor_cat_enum(cat_enum_mercury) ->

Yes, it should succeed for cat_enum_foreign.

>             MLDS_Type = mlds_class_type(ClassName, ClassArity, mlds_enum)
>         ;
>             MLDS_Type = mlds_class_type(ClassName, ClassArity, mlds_class)




> @@ -2005,8 +2007,8 @@
>         IsArray = is_array
>     ; Type = mlds_mercury_array_type(_) ->
>         IsArray = is_array
> -    ; Type = mercury_type(_, TypeCategory, _) ->
> -        IsArray = type_category_is_array(TypeCategory)
> +    ; Type = mercury_type(_, CtorCat, _) ->
> +        IsArray = type_category_is_array(CtorCat)
>     ; Type = mlds_rtti_type(RttiIdMaybeElement) ->
>         rtti_id_maybe_element_java_type(RttiIdMaybeElement,
>             _JavaTypeName, IsArray)
> @@ -2016,39 +2018,51 @@
>
>     % Return is_array if the corresponding Java type is an array type.
>     %
> -:- func type_category_is_array(type_category) = is_array.
> +:- func type_category_is_array(type_ctor_category) = is_array.
>
> -type_category_is_array(type_cat_int) = not_array.
> -type_category_is_array(type_cat_char) = not_array.
> -type_category_is_array(type_cat_string) = not_array.
> -type_category_is_array(type_cat_float) = not_array.
> -type_category_is_array(type_cat_higher_order) = is_array.
> -type_category_is_array(type_cat_tuple) = is_array.
> -type_category_is_array(type_cat_enum) = not_array.
> -type_category_is_array(type_cat_foreign_enum) = not_array.
> -type_category_is_array(type_cat_dummy) = not_array.
> -type_category_is_array(type_cat_variable) = not_array.
> -type_category_is_array(type_cat_type_info) = not_array.
> -type_category_is_array(type_cat_type_ctor_info) = not_array.
> -type_category_is_array(type_cat_typeclass_info) = is_array.
> -type_category_is_array(type_cat_base_typeclass_info) = is_array.
> -type_category_is_array(type_cat_void) = not_array.
> -type_category_is_array(type_cat_user_ctor) = not_array.
> +type_category_is_array(CtorCat) = IsArray :-
> +    (
> +        % XXX I am not sure about ctor_cat_variable.
> +        ( CtorCat = ctor_cat_builtin(_)
> +        ; CtorCat = ctor_cat_enum(_)
> +        ; CtorCat = ctor_cat_builtin_dummy
> +        ; CtorCat = ctor_cat_variable
> +        ; CtorCat = ctor_cat_system(cat_system_type_info)
> +        ; CtorCat = ctor_cat_system(cat_system_type_ctor_info)
> +        ; CtorCat = ctor_cat_void
> +        ; CtorCat = ctor_cat_user(_)
> +        ),
> +        IsArray = not_array
> +    ;
> +        ( CtorCat = ctor_cat_higher_order
> +        ; CtorCat = ctor_cat_tuple
> +        ; CtorCat = ctor_cat_system(cat_system_typeclass_info)
> +        ; CtorCat = ctor_cat_system(cat_system_base_typeclass_info)
> +        ),
> +        IsArray = is_array
> +    ).

What are not sure about with ctor_cat_variable?  It should be `not_array',
since the corresponding Java type, java.lang.Object, is not an array.

...

> Index: compiler/special_pred.m
> ===================================================================
> RCS file: /home/mercury/mercury1/repository/mercury/compiler/special_pred.m,v
> retrieving revision 1.70
> diff -u -b -r1.70 special_pred.m
> --- compiler/special_pred.m	31 Oct 2007 03:58:30 -0000	1.70
> +++ compiler/special_pred.m	8 Feb 2008 08:01:39 -0000
> @@ -187,13 +187,14 @@
> special_pred_description(spec_pred_init,    "initialisation predicate").
>
> special_pred_is_generated_lazily(ModuleInfo, TypeCtor) :-
> -    TypeCategory = classify_type_ctor(ModuleInfo, TypeCtor),
> +    CtorCat = classify_type_ctor(ModuleInfo, TypeCtor),
>     (
> -        TypeCategory = type_cat_tuple
> +        CtorCat = ctor_cat_tuple
>     ;
> -        ( TypeCategory = type_cat_user_ctor
> -        ; TypeCategory = type_cat_enum
> -        ; is_introduced_type_info_type_category(TypeCategory) = yes
> +        % XXX Should this succeed for cat_enum_foreign as well?

Yes, it should succeed for foreign enumerations (and below as well).
There should be no difference in the way special preds are handled
for foreign enumerations and the way they are handled for normal
enumerations.

> +        ( CtorCat = ctor_cat_user(_)
> +        ; CtorCat = ctor_cat_enum(cat_enum_mercury)
> +        ; is_introduced_type_info_type_category(CtorCat) = yes
>         ),
>         module_info_get_type_table(ModuleInfo, Types),
>         map.search(Types, TypeCtor, TypeDefn),
> @@ -211,13 +212,14 @@
>     Body \= hlds_solver_type(_, _),
>     Body \= hlds_abstract_type(solver_type),
>
> -    TypeCategory = classify_type_ctor(ModuleInfo, TypeCtor),
> +    CtorCat = classify_type_ctor(ModuleInfo, TypeCtor),
>     (
> -        TypeCategory = type_cat_tuple
> +        CtorCat = ctor_cat_tuple
>     ;
> -        ( TypeCategory = type_cat_user_ctor
> -        ; TypeCategory = type_cat_enum
> -        ; is_introduced_type_info_type_category(TypeCategory) = yes
> +        % XXX Should this succeed for cat_enum_foreign as well?
> +        ( CtorCat = ctor_cat_user(_)
> +        ; CtorCat = ctor_cat_enum(cat_enum_mercury)
> +        ; is_introduced_type_info_type_category(CtorCat) = yes
>         ),
>         special_pred_is_generated_lazily_2(ModuleInfo, TypeCtor, Body, Status)
>     ).

The rest looks fine, although I didn't really look at the CTGC stuff since Peter
is more suited to reviewing that.

Julien.
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions:          mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------