[m-rev.] for post-commit review: document requirement for non-abstract instances in interfaces

[Julien Fischer]

Hi Zoltan,

On Thu, 3 Feb 2022, Zoltan Somogyi wrote:

> For review by Julien.
>
> Julien, you added the code to the compiler to enforce this requirement
> in April 2005. The log message does not give the reason for the requirement.
> The m-rev thread on that diff says that the requirement is in the reference manual,
> but I have found no trace of that. If you happen to remember the reason
> for this design decision, please modify reference_manual.texi to mention it.

That's almost 17 years ago!  (There was almost certainly some in-office
discussion regarding this that isn't captured by the mailing list.) I
*think* the main reason for that design decision was that allowing
non-abstract instance declarations in module interfaces effectively
allows clauses (via inline method definitions) in module interfaces.
I think that may have been causing some problems at the time and the
easiest way to resolve these problems was to disallow non-abstract
instances in interfaces.  This is ok because ...

> Something that may bear on this is that .intN files have traditionally not
> allowed any non-abstract instance declarations. However, this is not a valid
> reason for banning them from .m files, since the code that decides what
> should be put into .intN files has traditionally made any non-abstract
> instance declarations into abstract declarations. The code that did this,
> which has now been pretty completely rewritten, did not say whether
> it considered non-abstract instance decls in interfaces to be an error
> *in the source file*, or not.


... allowing non-abstract instances in module interfaces doesn't tell
the compiler anything additional anyway.  (The only advantage to
non-abstract instances in interfaces is that they are a little more
succint.)

> Require non-abstract instances in interfaces.
> 
> doc/reference_manual.texi:
>     As above. The compiler enforced this requirement since April 2005,
>     but the manual was silent on the matter.
> 
> compiler/split_parse_tree_src.m:
>     When we detect an abstract instance declaration in the interface section,
>     extend the error message with a verbose-only mention of the usual fix.
> 
> tests/invalid_nodepend/Mercury.options:
>     Turn on verbose errors for this test case, which tests this error message.
> 
> tests/invalid_nodepend/instance_bug.err_exp:
>     Expect the new verbose component.
> 
> 
> diff --git a/compiler/split_parse_tree_src.m b/compiler/split_parse_tree_src.m
> index e374d9c78..7d0ebaad1 100644
> --- a/compiler/split_parse_tree_src.m
> +++ b/compiler/split_parse_tree_src.m
> @@ -1048,10 +1048,18 @@ report_error_implementation_in_interface(ModuleName, Context, !Specs) :-
>      list(error_spec)::in, list(error_spec)::out) is det.
>
>  report_non_abstract_instance_in_interface(Context, !Specs) :-
> -    Pieces = [words("Error: non-abstract instance declaration"),
> +    AlwaysPieces = [words("Error: non-abstract instance declaration"),
>          words("in module interface."), nl],
> -    Spec = simplest_spec($pred, severity_error, phase_parse_tree_to_hlds,
> -        Context, Pieces),
> +    VerbosePieces = [words("The usual fix for this problem"),

Why "usual" fix?  If the intention is that the instance be exported,
it's the only fix. Maybe it would better to start that with something
like:

      If you are trying to export this instance, move this instance
      declaration to the implementation section ...

> +        words("is to move this instance declaration"),
> +        words("to the implementation section,"),
> +        words("replacing it in the interface section"),
> +        words("with its abstract version, which contains"),
> +        words("just the class name and the instance type vector."), nl],

In the case where there are type class constraints on the instance declaration,
they also need to be replicated in the abstract declaration.

That's fine otherwise.

Julien.