[m-dev.] for review: abstract instance declarations

[Fergus Henderson]

On 19-Feb-1999, David Glen JEFFERY <dgj at hydra.cs.mu.oz.au> wrote:
> check_typeclass.m:
> > +				"In instance declaration for `",
> > +				ClassNameString, "/", ClassArityString, "': ",
> > +				"incorrect method name(s)."],
> 
> Is this a new error message?

Yes.  Sorry, the log message did not mention that change --
it mentioned the improved error checking in make_hlds.m
but I forgot to mention the improved error checking here.
This check catches the case when you have some methods
in the instance declaration which don't correspond to
any method in the class.

> I'm not sure that "incorrect method name(s)" is
> the right thing to say.

Do you have a better suggestion?

Certainly it would be better to say what the incorrect method
names were, and report error messages for each one at the
appropriate line number, rather than reporting a single
rather uninformative message at the class definition.
However, that information is not trivial to obtain, and
in the absense of such information I couldn't think of
anything better to say than "incorrect method name(s)".

If you can think of a simple improvement I'd be happy to
do it, but if you want a really informative error message
then please implement it yourself ;-)
My change at least improves things by diagnosing these errors
(previously they weren't diagnosed at all).

> > +:- pred check_for_overlapping_instances(hlds_instance_defn,
> > +		list(hlds_instance_defn), class_id, io__state, io__state).
> > +:- mode check_for_overlapping_instances(in, in, in, di, uo) is det.
> > +
> > +check_for_overlapping_instances(NewValue, Values, ClassId) -->
> > +	{ IsOverlapping = lambda([(Context - OtherContext)::out] is nondet, (
> > +		NewValue = hlds_instance_defn(_Status, Context,
> > +				_, Types, Body, _, VarSet, _),
> > +		Body \= abstract, % XXX
> > +		list__member(OtherValue, Values),
> > +		OtherValue = hlds_instance_defn(_OtherStatus, OtherContext,
> > +				_, OtherTypes, OtherBody, _, OtherVarSet, _),
> > +		OtherBody \= abstract, % XXX
> > +		varset__merge(VarSet, OtherVarSet, OtherTypes,
> > +				_NewVarSet, NewOtherTypes),
> > +		type_list_subsumes(Types, NewOtherTypes, _)
> > +	)) },
> > +	aggregate(IsOverlapping,
> > +		report_overlapping_instance_declaration(ClassId)).
> 
> `Values' isn't a very good variable name in this context.

Yep, you're right.  I've done s/Value/InstanceDefn/g

> (It's OK at the
> point where you call check_for_overlapping_instances because at that point
> it is quite clear that the `Values' are the values from the instance map.

I think even there InstanceDefn is clearer.

> > +report_overlapping_instance_declaration(class_id(ClassName, ClassArity),
> > +		Context - OtherContext) -->
> > +	io__set_exit_status(1),
> > +	prog_out__write_context(Context),
> > +	io__write_string("Error: multiply defined (or overlapping) instance\n"),
> 
> Could it actually be overlapping, given that we restrict instance types to be
> a single functor with variable args?

Well, consider the following case:

	:- instance foo(list(T)) <= bar(T) where [...].
	:- instance foo(list(T)) <= baz(T) where [...].

This is not really a case of a single thing being multiply defined.
Each instance definition defines a _different_ set of instances.
The problem is just that those two sets may overlap, if there
is any type which is an instance of both bar/1 and baz/1.

> > +Each @samp{instance} declaration
> > +must specify a binding for every method declared in the corresponding
> > + at samp{class} declaration. 
> 
> Is "binding" the right word? The word I normally use is "implementation", but
> I'm not sure that it's quite correct. This sentence should probably also state
> that there must be *exactly* one implementation of each method. Also, the
> declaration is "typeclass", not "class". Maybe:
> 
> 	Each @samp{instance} declaration
> 	must specify exactly one implementation for every method declared in the
> 	corresponding @samp{typeclass} declaration. 

OK.

> BTW, there are plenty of places in this diff where "typeclass" is used where
> "type class" should be. Almost all of those places were just existing mistakes
> that you didn't fix. I will make a pass over the reference manual (one of
> these days...) and fix all of them.

Good, in that case I won't worry about fixing that.

--------------------------------------------------

Estimated hours taken: 0.1

compiler/make_hlds.m:
	Use more meaningful variable names,
	as suggested by dgj's review of my previous change.

Index: compiler/make_hlds.m
===================================================================
RCS file: /home/mercury1/repository/mercury/compiler/make_hlds.m,v
retrieving revision 1.282
diff -u -r1.282 make_hlds.m
--- make_hlds.m	1999/02/12 03:46:57	1.282
+++ make_hlds.m	1999/02/19 15:35:28
@@ -1790,14 +1790,14 @@
 		{ map__search(Classes, ClassId, _) }
 	->
 		{ map__init(Empty) },
-		{ NewValue = hlds_instance_defn(Status, Context,
+		{ NewInstanceDefn = hlds_instance_defn(Status, Context,
 			Constraints, Types, Body, no, VarSet, Empty) },
-		{ map__lookup(Instances0, ClassId, Values) },
-		check_for_overlapping_instances(NewValue, Values, ClassId),
-		{ map__det_update(Instances0, ClassId, [NewValue|Values], 
-			Instances) },
-		{ module_info_set_instances(Module0, Instances,
-			Module) }
+		{ map__lookup(Instances0, ClassId, InstanceDefn) },
+		check_for_overlapping_instances(NewInstanceDefn, InstanceDefns,
+			ClassId),
+		{ map__det_update(Instances0, ClassId,
+			[NewInstanceDefn | InstanceDefns], Instances) },
+		{ module_info_set_instances(Module0, Instances, Module) }
 	;
 		undefined_type_class_error(ClassName, ClassArity, Context,
 			"instance declaration"),
@@ -1808,14 +1808,15 @@
 		list(hlds_instance_defn), class_id, io__state, io__state).
 :- mode check_for_overlapping_instances(in, in, in, di, uo) is det.
 
-check_for_overlapping_instances(NewValue, Values, ClassId) -->
+check_for_overlapping_instances(NewInstanceDefn, InstanceDefns, ClassId) -->
 	{ IsOverlapping = lambda([(Context - OtherContext)::out] is nondet, (
-		NewValue = hlds_instance_defn(_Status, Context,
+		NewInstanceDefn = hlds_instance_defn(_Status, Context,
 				_, Types, Body, _, VarSet, _),
 		Body \= abstract, % XXX
-		list__member(OtherValue, Values),
-		OtherValue = hlds_instance_defn(_OtherStatus, OtherContext,
-				_, OtherTypes, OtherBody, _, OtherVarSet, _),
+		list__member(OtherInstanceDefn, InstanceDefns),
+		OtherInstanceDefn = hlds_instance_defn(_OtherStatus,
+				OtherContext, _, OtherTypes, OtherBody,
+				_, OtherVarSet, _),
 		OtherBody \= abstract, % XXX
 		varset__merge(VarSet, OtherVarSet, OtherTypes,
 				_NewVarSet, NewOtherTypes),

I'll send a diff for the changes to reference_manual.texi tomorrow.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "Binaries may die
WWW: <http://www.cs.mu.oz.au/~fjh>  |   but source code lives forever"
PGP: finger fjh at 128.250.37.3        |     -- leaked Microsoft memo.