[m-rev.] for-post-commit review: add an XXX about possible improvement

[Zoltan Somogyi]

On 2024-05-28 16:59 +10:00 AEST, "Peter Wang" <novalazy at gmail.com> wrote:
> On Tue, 28 May 2024 03:36:47 +1000 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
>> For opinions by anyone.
>> 
>> Zoltan.
> 
>> diff --git a/compiler/check_typeclass.m b/compiler/check_typeclass.m
>> index aa9c6a845..aa6fc5bfe 100644
>> --- a/compiler/check_typeclass.m
>> +++ b/compiler/check_typeclass.m
>> @@ -2149,6 +2149,14 @@ report_eqv_type_in_abstract_exported_instance(ClassId, InstanceDefn, ArgNum,
>>          [words("is an")] ++
>>          color_as_incorrect([words("abstract exported equivalence type.")]) ++
>>          [nl],
>> +        % XXX Should we add this explanatory text? If so, should it be
>> +        % in a verbose-only component? Should we add it to reference manual?
>> +        % [words("Mercury does not allow this, because allowing it"),
>> +        % words("would mean that the code inside the module"),
>> +        % words("(which can see the equivalence)"),
>> +        % words("and the code outside the module"),
>> +        % words("(which cannot)"),
>> +        % words("would disagree about the identity of the type.")]
>>      report_bad_type_in_instance(ClassId, InstanceDefn, EndPieces,
>>          abstract_exported_eqv, !Specs).
> 
> I think it should be described in the manual under "Abstract instance
> declarations".

There is already a sentence in the manual stating the restriction.
I was thinking adding the text above just after that sentence,
to give the *reason* for the restriction. The sentence is in 11.2
"Instance declarations". "Abstract instance declarations", 11.4,
is about the instance declarations being abstract, not about the
*types* in them being abstract.
 
> I'm still unsure what problem the restriction avoids.

I *think* it that the answer has to with the detection of duplicate
instances at link time. I seem to recollect that the compiler constructed
names of the instance methods include the names of the types
in the instance's arg vector, and that these are intentionally given
global scope in the generated C code. This means that having unrelated
instance declarations for the same class with the same arg vector
in different modules will generate a link error. This detection mechanism
would miss duplicate instances if the two instances differed by having
one use type t1 and the other t2 where t1 is equivalent to t2.

Another answer is type specialization. Without it, a call to an instance
method in a module that imports this instance will get the id of the
procedure to call from a typeclass info. But if mmc knows the concrete
types to which all the type var parameters of the typeclass are bound
at a given call site, it can call the instance methods directly. For that,
it needs to construct their name, and constructing a name in the call
that differs from the name on the definition of the instance method
in the other module won't work.

I don't propose to write any of that in the manual; knowing that
the implementation needs different modules to agree on the name
should be enough for users.

- the compiler derives from the the instance
> (Or tries to avoid: as check_typeclass.m mentions, the type equivalence
> could be hidden in a different module from the instance declaration.)

You are right, this mechanism will fail in that situation. However,
I seem to remember that when this was discussed in the Mercury room
a long, long time ago, the idea was that detecting some instances
of a problem was better than detecting none.

Zoltan.