[m-dev.] diff: tvar renaming bug fix

[Simon Taylor]

Hi,

I'd like to see another diff after you've fixed these (could you send
me the full file for polymorphism.m again as well?).

Simon.

> Estimated hours taken: 20
> 
> Fix a bug in the renaming of tvars in type class constraint proofs, and
> generally clean up the handling of variable renaming in polymorphism.m.
> 
 

> Index: compiler/hlds_data.m
> ===================================================================
> RCS file: /home/staff/zs/imp/mercury/compiler/hlds_data.m,v
> retrieving revision 1.25
> diff -u -t -r1.25 hlds_data.m
> --- hlds_data.m	1998/07/08 20:56:10	1.25
> +++ hlds_data.m	1998/09/10 06:54:25
> @@ -751,9 +751,20 @@
>  
>          % `Proof' of why a constraint is redundant
>  :- type constraint_proof                        
> -                        % Apply the following instance rule, the second 
> -                        % argument being the number of the instance decl.
> -        --->    apply_instance(hlds_instance_defn, int)
> +                        % Apply the instance decl with the given number.
> +                        % Note that we don't store the actual 
> +                        % hlds_instance_defn for two reasons:
> +                        % - That would require storing a renamed version of
> +                        %   the constraint_proofs for *every* use of an instance
> +                        %   declaration. This would't even get GCed for a long
> +                        %   time because it would be stored in the pred_info.
> +                        % - The superclass proofs stored in the
> +                        %   hlds_instance_defn would need to store
> +                        %   all the constraint_proofs for all its ancestors.
> +                        %   This would require the class relation to be
> +                        %   topologically sorted before checking the instance
> +                        %   declarations.
> +        --->    apply_instance(int)
>  
>                          % The constraint is redundant because of the following
>                          % class's superclass declaration

Line wrapping.

  
> +        % XXX the MaybePredProcId args can be deleted now that we aren't going
> +        % to do specialisation here.
> +polymorphism__make_typeclass_info_var(Constraint, TypeSubst, ExistQVars,
> +                Context, _MaybePredProcId0, MaybePredProcId,
>                  ExtraGoals0, ExtraGoals, ConstrainedTVars0, ConstrainedTVars, 
I think you should do this rather than leave an XXX.

> +                        apply_subst_to_constraint_list(RenameSubst,
> +                                InstanceConstraints0, InstanceConstraints1),
> +                        apply_rec_subst_to_constraint_list(InstanceSubst,
> +                                InstanceConstraints1, InstanceConstraints),
> +                        apply_subst_to_constraint_proofs(RenameSubst,
> +                                SuperClassProofs0, SuperClassProofs1),
> +                        apply_rec_subst_to_constraint_proofs(InstanceSubst,
> +                                SuperClassProofs1, SuperClassProofs),
> +
> +                        poly_info_set_typevarset(NewTVarset, Info0, Info1),
> +
> +                                % Make the type_infos for the types
> +                                % that are constrained by this. These
> +                                % are packaged in the typeclass_info
> +                        polymorphism__make_type_info_vars(
> +                                ConstrainedTypes, ExistQVars, Context, 
> +                                InstanceExtraTypeInfoVars, TypeInfoGoals,
> +                                Info1, Info2),
> +
> +                                % Make the typeclass_infos for the
> +                                % constraints from the context of the
> +                                % instance decl.
> +                        polymorphism__make_typeclass_info_vars_2(
> +                                InstanceConstraints, TypeSubst, 
> +                                ExistQVars, Context, no, _,
> +                                [], InstanceExtraTypeClassInfoVars, 
> +                                ExtraGoals0, ExtraGoals1, 
> +                                [], _,
> +                                Info2, Info3),

I think the use of InstanceSubst and TypeSubst in this section isn't quite
right (although it probably will work). At the top of this predicate, the
first thing this predicate does is apply TypeSubst recursively to Constraint.
That would suggest that instead of applying InstanceSubst to the constraint
list and passing TypeSubst to polymorphism__make_typeclass_info_vars_2, this
should not apply InstanceSubst to the constraints but instead pass
InstanceSubst instead of TypeSubst. Actually, since the unsubstituted
constraint (Constraint) is no longer used for anything, it might be better
to substitute the constraints before they are passed in to here from
polymorphism__process_call and not pass a type substitution at all to
polymorphism__make_typeclass_info_var.

> +
> +                        polymorphism__construct_typeclass_info(
> +                                InstanceExtraTypeInfoVars, 
> +                                InstanceExtraTypeClassInfoVars, 
> +                                ClassId, ConstrainedTypes,
> +                                SuperClassProofs, ExistQVars, Var, NewGoals, 
> +                                Info3, Info4),
> +
> +                        poly_info_set_typevarset(TypeVarSet, Info4, Info),
> +

Should the typevarset really be reset to its old value here (I think
you've substituted away all the new type variables, but a comment would
be nice). Also perhaps it would be better if you didn't rename TypeVarSet0
to TypeVarSet, so that it would be clear that this is the old version.

> @@ -1807,26 +1764,18 @@
>                          MaybePredProcId = no,
>  
>                                  % Then work out where to extract it from
> -                        SubClassConstraint0 = 
> -                                constraint(SubClassName, SubClassTypes0),
> -                        term__apply_substitution_to_list(SubClassTypes0, Subst,
> -                                SubClassTypes1),
> -                        % we need to maintain the invariant that types in
> -                        % class constraints do not contain any information
> -                        % in their term__context fields
> -                        strip_term_contexts(SubClassTypes1, SubClassTypes),

I assume this is done somwhere else.

> @@ -2047,13 +1980,19 @@
>          poly_info_get_proofs(Info0, Proofs),
>  
>          poly_info_get_varset(Info0, VarSet0),
> -        ClassDefn = hlds_class_defn(SuperClasses, ClassVars, _, ClassVarSet, _),
> -        map__from_corresponding_lists(ClassVars, InstanceTypes, TypeSubst),
> +        ClassDefn = hlds_class_defn(SuperClasses0, ClassVars0, 
> +                _, ClassVarSet, _),
>          varset__merge_subst(VarSet0, ClassVarSet, VarSet1, Subst),
>          poly_info_set_varset(VarSet1, Info0, Info1),

I'm a bit worried about this -- the ClassVarSet contains type
variables doesn't it. Should this be being merged into the typevarset
rather than the varset?

Simon.