[m-dev.] for review: constrain interface assertions to using interface symbols

[Fergus Henderson]

On 11-Nov-1999, Peter Ross <petdr at cs.mu.OZ.AU> wrote:
> Assertion declarations in the interface must not contain any symbols from
> modules imported only in the implementation.  This constraint requires
> that each imported symbol be annotated with whether it is imported from
> the interface or implementation.
>
> compiler/prog_data.m:
>     Update the pseudo declaration `:- imported' to include which section
>     (either implementation or interface) the items following it where
>     imported from.

s/where/were/

> compiler/module_qual.m:
>     Any unqualified symbol in an assertion might come from *any*
>     of the imported modules, so turn off the warning about modules which
>     are imported in the interface but not used.

I suggest you make it clear in the log message exactly when
you are disabling the warning:
	- if an unqualified symbol appears in an assertion?
	- if the interface contains an assertion?
	- all the time?

> compiler/accumulator.m:
>     Use where a symbol is imported from when deciding whether an
>     assertion in the interface is valid.

Did you mean `assertion.m' rather than `accumulator.m' here?
I don't see why accumulator.m should have to change as a result
of this change.

> Index: hlds_pred.m
> ===================================================================
> RCS file: /home/staff/zs/imp/mercury/compiler/hlds_pred.m,v
> retrieving revision 1.66
> diff -u -r1.66 hlds_pred.m
> --- hlds_pred.m	1999/10/15 03:44:49	1.66
> +++ hlds_pred.m	1999/10/27 04:48:20
> @@ -974,8 +975,8 @@
>  pred_info_mark_as_external(PredInfo0, PredInfo) :-
>  	PredInfo0 = predicate(A, B, C, D, E, F, G, H, I, _, K, L, M, N, O, P,
>  		Q, R, S, T, U, V),
> -	PredInfo  = predicate(A, B, C, D, E, F, G, H, I, imported, K, L, M, 
> -		N, O, P, Q, R, S, T, U, V).
> +	PredInfo  = predicate(A, B, C, D, E, F, G, H, I, imported(interface),
> +		K, L, M, N, O, P, Q, R, S, T, U, V).

Hmm, is that correct?
Shouldn't you check whether the old status was
`local' or `exported'?  If it was `local', then
the new status should be `imported(implementation)',
not `imported(interface)'.

I suggest you add the following test case to tests/invalid

	% this is invalid because the interface refers
	% to stuff defined only in the implementation
	:- module tricky_assert1.
	:- interface.
	:- assertion tricky_assert1__local.

	:- implementation.
	:- pred tricky_assert1__local is semidet.
	:- external(tricky_assert1__local/0).

and the following one to tests/valid:

	:- module tricky_assert2.
	:- interface.
	:- pred tricky_assert2__local is semidet.
	:- assertion tricky_assert2__local.

	:- implementation.
	:- external(tricky_assert2__local/0).

The current compiler correctly rejects tricky_assert1 but incorrectly
also rejects tricky_assert2.  I think with your changes as posted, the
compiler will correctly accept tricky_assert2 but will incorrectly also
accept tricky_assert1.

> +collect_mq_info_2(assertion(_, _), Info0, Info) :-
> +	% Any unqualified symbol in the assertion might come from *any*
> +	% of the imported modules.
> +	% There's no way for us to tell which ones.
> +	% So we conservatively assume that it uses all of them.
> +	% XXX a better solution would be to only make this assumption if
> +	% the assertion contains an unqualified symbol, however since
> +	% the structure isn't in superhomogenous form yet processing the
> +	% goal is complicated.
> +	set__init(UnusedInterfaceModules),
> +	mq_info_set_unused_interface_modules(Info0, UnusedInterfaceModules,
> +		Info).

Well, not _that_ complicated; you do have to traverse the `goal' type
(requiring one ~30-line predicate), but once you get to a `call'
or `unify', it's fairly easy: 

	:- pred goal_contains_unqual_sym(goal::in) is semidet.
	... lots of clauses with recursive calls, then ...
	goal_contains_unqual_sym(call(SymName, Args)) :-
		sym_name_and_args_contains_unqual_sym(SymName, Args).
	goal_contains_unqual_sym(unify(LHS, RHS)) :-
		( term_contains_unqual_sym(LHS)
		; term_contains_unqual_sym(LHS)
		).

	term_contains_unqual_sym(Term) :-
		sym_name_and_args(Term, SymName, Args),
		sym_name_and_args_contains_unqual_sym(SymName, Args).

	sym_name_and_args_contains_unqual_sym(SymName, Args) :-
		(
			SymName = unqualified(_)
		;
			list__member(Arg, Args),
			term_contains_unqual_sym(Arg)
		).

Go on, it should only take you another 5-15 minutes to finish that off ;-)

>  :- pred warn_if_duplicate_use_import_decls(module_name, list(module_name),
>  		list(module_name), list(module_name), list(module_name), 
> +		list(module_name), list(module_name), list(module_name), 
> +		list(module_name), io__state, io__state).
> +:- mode warn_if_duplicate_use_import_decls(in, in, out, in, out, in, out,
> +		in, out, di, uo) is det.
> +
> +% This predicate ensures that all every import_module declaration is
> +% checked against every use_module declaration.
> +% warn_if_duplicate_use_import_decls/7 is called to generate the actual
> +% warnings.
> +
> +warn_if_duplicate_use_import_decls(ModuleName,
> +		IntImportedModules0, IntImportedModules, 
> +		IntUsedModules0, IntUsedModules, 
> +		ImpImportedModules0, ImpImportedModules, 
> +		ImpUsedModules0, ImpUsedModules) -->
> +
> +	warn_if_duplicate_use_import_decls(ModuleName,
> +		IntImportedModules0, IntImportedModules1,
> +		IntUsedModules0, IntUsedModules1),
> +	warn_if_duplicate_use_import_decls(ModuleName,
> +		IntImportedModules1, IntImportedModules,
> +		ImpUsedModules0, ImpUsedModules1),
> +
> +	warn_if_duplicate_use_import_decls(ModuleName,
> +		ImpImportedModules0, ImpImportedModules1,
> +		IntUsedModules1, IntUsedModules),
> +	warn_if_duplicate_use_import_decls(ModuleName,
> +		ImpImportedModules1, ImpImportedModules,
> +		ImpUsedModules1, ImpUsedModules).
> +

Actually I think it makes perfect sense to have `:- use_module foo'.
in the interface and `:- import_module foo.' in the implementation.
The other three cases don't make sense, but that one does.
The compiler should not warn about it.
So I think you should delete that third call to
`warn_if_duplicate_use_import_decls',
and modify the documentation for this pred accordingly.

Apart from that, this change looks good, but I'd like to see another
round of reviewing for it.

Cheers,
	Fergus.

-- 
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.
--------------------------------------------------------------------------
mercury-developers mailing list
Post messages to:       mercury-developers at cs.mu.oz.au
Administrative Queries: owner-mercury-developers at cs.mu.oz.au
Subscriptions:          mercury-developers-request at cs.mu.oz.au
--------------------------------------------------------------------------