[m-rev.] for review: Fix some bugs with constrained polymorphic modes.

[Julien Fischer]

Hi Peter,

On Thu, 4 Sep 2014, Peter Wang wrote:

> 1. An inst variable from the head of the clause could be bound in the body.
> The mode error in the call `P(X)' was not detected because the inst
> variable I could be bound to ground.
>
> 	:- pred p(pred(T), T).
> 	:- mode p(pred(in(I =< ground)) is det, in) is det.
>
> 	p(P, X) :- P(X).
>
> 2. In David Overton's thesis, a get_subst function produces an inst
> substitution from the callee's initial insts to the arguments' initial
> insts, and the substitution is applied to all insts from the callee.
> In the implementation we actually build the substitution while matching
> the arguments' insts with the callee's insts, but we lost some precision
> by not applying the incremental substitutions built as we match a list
> of insts.
>
> 3. We rename apart the callee's inst variables from our own, but did not
> keep the merged inst varset.  When a renamed inst variable appears in a
> mode error it would be printed without its original name.
> XXX could this avert more serious problems?
>
> 4. inst_merge_3 was missing a case.
>
> compiler/inst_util.m:
> 	Handle in inst_merge_3 the case
> 	    InstA \= constrained_inst_var(_, _),
> 	    InstB = constrained_inst_var(_, _).
>
> 	Move InstA = not_reached case to inst_merge_2 to maintain
> 	inst_merge(not_reached, InstB @ constrained_inst_var(_, _)) = InstB
>
> compiler/mode_util.m:
> compiler/modecheck_call.m:
> 	Keep the merged inst varsets after renaming.
>
> compiler/mode_info.m:
> 	Add "head inst vars" in mode_info structure.
>
> compiler/modecheck_util.m:
> 	Add get_constrained_inst_vars to extract constrained inst vars from a
> 	list of mode.
>
> 	Make modecheck_var_has_inst_list_* fail if the computed substitution
> 	would change the constraints of any head inst variables.
>
> compiler/modes.m:
> 	Initialise mode_info with head inst variables.
>
> compiler/pd_info.m:
> compiler/pd_util.m:
> 	Conform to change in mode_info_init (not specifically tested).
>
> compiler/prog_mode.m:
> 	Export inst_apply_substitution.
>
> 	Make rename_apart_inst_vars return the merged inst varset.
>
> 	Fix comments.
>
> tests/invalid/Mmakefile:
> tests/invalid/constrained_poly_insts2.err_exp:
> tests/invalid/constrained_poly_insts2.m:
> tests/valid/Mmakefile:
> tests/valid/constrained_poly_multi.m:
> 	Add test cases.

...

> diff --git a/compiler/modecheck_util.m b/compiler/modecheck_util.m
> index 963016b..863be92 100644
> --- a/compiler/modecheck_util.m
> +++ b/compiler/modecheck_util.m

...

> @@ -185,16 +192,17 @@
> :- import_module check_hlds.inst_match.
> :- import_module check_hlds.inst_util.
> :- import_module check_hlds.mode_errors.
> +:- import_module check_hlds.mode_util.
> :- import_module check_hlds.modecheck_goal.
> :- import_module check_hlds.modecheck_unify.
> :- import_module check_hlds.polymorphism.
> :- import_module check_hlds.type_util.
> -:- import_module hlds.hlds_module.
> :- import_module hlds.pred_table.
> :- import_module hlds.special_pred.
> :- import_module mdbcomp.
> :- import_module mdbcomp.prim_data.
> :- import_module mdbcomp.sym_name.
> +:- import_module parse_tree.prog_mode.
> :- import_module parse_tree.prog_type.
> :- import_module parse_tree.set_of_var.
>
> @@ -205,9 +213,13 @@
> :- import_module pair.
> :- import_module require.
> :- import_module set.
> +:- import_module set_tree234.
> :- import_module term.
> +:- import_module unit.
> :- import_module varset.
>
> +:- type expansions == set_tree234(inst_name).

Move this type closer to where it actually used.  Also I suggest naming it
inst_expansions rather than just expansions.

The rest of the change looks okay to me although you may want to get a second
opinion from someone who is more familiar with the mode analyser than me.

Cheers,
Julien.