[m-dev.] For review: Make Mercury cope with impure code (part 1/2)

[Fergus Henderson]

Peter Schachte, you wrote:
> Make Mercury cope with impure code
[...]
> compiler/purity.m
> 	New compiler pass for purity checking.

Missing `:' in log message.

> compiler/hlds_goal.m:
> 	Add `impure' and `semipure' to the goal_feature enum

Missing `.' at end of log message.

> compiler/.cvsignore
> profiler/.cvsignore
> tests/hard_coded/.cvsignore
> library/.cvsignore:
> tests/valid/.cvsignore
> 	Ignore *.opt *.optdate *.date3 *.int3

I think a better solution is to put those (y)our personal
~/.cvsignore files.

Otherwise you also need to ignore *.date, *.int and *.int2.

>  hlds_out__write_goal_2(switch(Var, CanFail, CasesList, _), ModuleInfo, VarSet,
>  		AppendVarnums, Indent, Follow, TypeQual) -->
>  	hlds_out__write_indent(Indent),
> -	io__write_string("( % "),
> +	io__write_string("(			% "),
>  	hlds_out__write_can_fail(CanFail),
>  	io__write_string(" switch on `"),
>  	mercury_output_var(Var, VarSet, AppendVarnums),

Was that change deliberate?  If so, what's the rationale?

> -:- import_module hlds_data, hlds_goal, hlds_module, llds, prog_data, instmap.
> +:- import_module hlds_data, hlds_goal, hlds_module, llds, prog_data, instmap,
> +	purity.

If an `import_module' declaration doesn't fit on a line,
could you please use multiple `import_module' declarations,
rather than wrapping?  This makes it easier to grep for
e.g. `import_module.*foo'.

> +++ make_hlds.m	1997/11/14 00:59:06
[...]
>  		{ string__append_list(
> +			% XXX It would be terser (and so less likely to
> +			%     overrun the line), if this were instead:
> +			%	  ["`", PragmaName, "' pragma"],
>  			["`:- pragma ", PragmaName, "' declaration"],
>  			Description) },

So why put an XXX, rather than just changing the message?

An XXX is supposed to indicate something wrong.
If the fix is easy, better to fix it rather than documenting it.
If you're not sure which is better, add a comment

> +transform_goal_2(purity(Goal0, Purity), _, VarSet0, Subst, Goal,
> +		VarSet, Info0, Info) -->
> +	transform_goal(Goal0, VarSet0, Subst, Goal1, VarSet,
> +			 Info0, Info),
> +	{ mark_goal_with_purity(Goal1, Purity, Goal) }.
> +
> +
> +:- pred mark_goal_with_purity(hlds_goal, purity, hlds_goal).
> +:- mode mark_goal_with_purity(in, in, out) is det.
> +
> +mark_goal_with_purity(Goal0-GoalInfo0, Purity, Goal-GoalInfo) :-
> +	(   Goal0 = conj(Goals0) ->
> +		Goal = conj(Goals),
> +		GoalInfo = GoalInfo0,
> +		mark_calls_with_purity(Goals0, Purity, Goals)
> +	;
> +		Goal = Goal0,
> +		add_goal_info_purity_feature(GoalInfo0, Purity, GoalInfo)
> +	).
> +
> +
> +:- pred mark_calls_with_purity(list(hlds_goal), purity, list(hlds_goal)).
> +:- mode mark_calls_with_purity(in, in, out) is det.
> +
> +mark_calls_with_purity([], _, []).
> +mark_calls_with_purity([Goal0-GoalInfo0|Goals0], Purity,
> +		[Goal-GoalInfo|Goals]) :-
> +	(   Goal0 = call(_,_,_,_,_,_) ->
> +		Goal = Goal0,
> +		add_goal_info_purity_feature(GoalInfo0, Purity, GoalInfo)
> +	;   Goal0 = conj(Goals1) ->
> +		GoalInfo = GoalInfo0,
> +		Goal = conj(Goals2),
> +		mark_calls_with_purity(Goals1, Purity, Goals2)
> +	;
> +		Goal = Goal0,
> +		GoalInfo = GoalInfo0
> +	),
> +	mark_calls_with_purity(Goals0, Purity, Goals).

I understand what this code does, but I don't quite understand why.
Could you please document it a bit?

Also, please use "Foo - Bar" rather than "Foo-Bar"
and "[Foo | Bar]" rather than "[Foo|Bar]".

> +		maybe_write_string(Verbose,
> +			"% Program is purity-correct.\n")

Didn't you misspell "politically"? ;-)

> +goedel_output_goal_2(purity(Goal,_), VarSet, Indent) -->
> +	% XXX we just ignore purity indicators altogether -- is this right?
> +	goedel_output_goal(Goal, VarSet, Indent).

It would be better to output the purity indicators as comments.

> +:- type schedule_culprit
> +	--->	goal_itself_was_impure
> +	;	goals_followed_by_impure_goal(hlds_goal)
> +	;	just_because.	% we've reached the end of a conjunction
> +				% and there were still delayed goals

I think `conj_floundered' might be a better name than `just_because'.

> +%  check whether there are any delayed goals at the point where
> +%  we are about to schedule an impure goal.  If so, that is an error.
> +:- pred check_for_impurity_error(hlds_goal, impurity_errors, impurity_errors,
> +		mode_info, mode_info).

The code for this predicate handles headvar unifications specially;
this is not reflected in the above documentation.

> @@ -1191,8 +1278,7 @@
>  	modecheck_set_var_inst(Var0, InitialInst, FinalInst,
>  				Var, ExtraGoals0, ExtraGoals1),
>  	modecheck_set_var_inst_list_2(Vars0, InitialInsts, FinalInsts,
> -				ExtraGoals1, Vars, ExtraGoals).
> -
> + 				ExtraGoals1, Vars, ExtraGoals).
>  :- pred modecheck_set_var_inst(var, inst, inst, var, extra_goals, extra_goals,
>  				mode_info, mode_info).
>  :- mode modecheck_set_var_inst(in, in, in, out, in, out,

I think you accidentally deleted a useful blank line here.

> --- prog_data.m	1997/10/09 09:39:05	1.26
> +++ prog_data.m	1997/11/14 07:17:36
>  	;	call(sym_name, list(term))
> -	;	unify(term, term).
> +	;	unify(term, term)
> +	;	purity(goal,purity).

Why is purity attached to goals, not to calls?
(This question also applies to the code in prog_io_dcg.m and prog_io_goal.m
for parsing purity declarations.)

>  		% XXX we should warn about this (if the goal wasn't `fail')
>  		%
>  		Detism = failure,
> +		\+ goal_info_is_impure(GoalInfo0),
>  		( det_info_get_fully_strict(DetInfo, no)
>  		; code_aux__goal_cannot_loop(ModuleInfo, Goal0)
>  		)
> @@ -194,6 +195,7 @@
>  		simplify_info_get_instmap(Info0, InstMap0),
>  		det_no_output_vars(NonLocalVars, InstMap0, InstMapDelta,
>  			DetInfo),
> +		\+ goal_info_is_impure(GoalInfo0),
>  		( det_info_get_fully_strict(DetInfo, no)
>  		; code_aux__goal_cannot_loop(ModuleInfo, Goal0)
>  		)

Any reason to use `\+ goal_info_is_impure' rather than
`goal_info_is_pure'?  If not, please change it.
If so, it should be documented.

> -	( simplify_do_calls(Info2) ->
> +	( simplify_do_calls(Info2),
> +	  goal_info_is_pure(GoalInfo0) ->	
>  		common__optimise_call(PredId, ProcId, Args, Goal0, GoalInfo0,
>  			Goal1, Info2, Info3)
> -	; simplify_do_warn_calls(Info0) ->
> +	; simplify_do_warn_calls(Info0),
> +	  goal_info_is_pure(GoalInfo0) ->
>  		% we need to do the pass, for the warnings, but we ignore
>  		% the optimized goal and instead use the original one
>  		common__optimise_call(PredId, ProcId, Args, Goal0, GoalInfo0,

>  simplify__goal_2(Goal0, GoalInfo, Goal, GoalInfo, Info0, Info) :-
>  	Goal0 = pragma_c_code(_, _, PredId, ProcId, Args, _, _, _),
> -	( simplify_do_calls(Info0) ->
> +	( simplify_do_calls(Info0),
> +	  goal_info_is_pure(GoalInfo) ->	
>  		common__optimise_call(PredId, ProcId, Args, Goal0,
>  			GoalInfo, Goal, Info0, Info)
>  	;

That doesn't fit our layout guidelines.
The `;' and `->' should either be on the same line or
both alone on their own lines.

> +++ compiler_design.html	1997/11/14 00:33:06
> @@ -211,6 +211,15 @@
>  	  that are used in a variety of different places within the compiler
>  	</ul>
>  
> +<dt> purity analysis
> +	
> +	<dd>
> +	purity.m is responsible for purity checking, as well as
> +	defining the <CODE>purity</CODE> type and a few public
> +	operations on it.  It also completes the handling of predicate
> +	and function overloading for cases which typecheck.m is unable
> +	to handle.
> +
>  <dt> mode analysis
>  
>  	<dd>

I think you also need to delete or modify the following paragraph
describing mode analysis:

|	It also converts function calls into predicate calls, and
|	does the final step of figuring out which pred_id to use
|	for a call to an overloaded predicate.


Apart from the things noted above, that looks fine.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>   |  "I have always known that the pursuit
WWW: <http://www.cs.mu.oz.au/~fjh>   |  of excellence is a lethal habit"
PGP: finger fjh at 128.250.37.3         |     -- the last words of T. S. Garp.