[m-rev.] for review: prepare for two new warnings on instances

[Zoltan Somogyi]

2022-03-19 13:58 GMT+11:00 "Julien Fischer" <jfischer at opturion.com>:
>> +        "--warn-unnecessarily-private-instance",
>> +        "\tGenerate a warning if an instance declaration is not exported",
>> +        "\teven though none of the type constructors mentioned in the",
>> +        % XXX Should this be like this (what Julien proposed)
>> +        "\tmember types are defined in the current module.",
>> +        % XXX or like this (my preference)?
>> +        "\tmember types are local to the current module.",
>> +        "--warn-inconsistent-instance-order",
>> +        "\tGenerate a warning if the order of concrete instance declarations",
>> +        "\tdoes not match the order of the corresponding abstract",
>> +        "\tdeclarations.",
> 
> What I said on the mailing list was "non-local" types, which is a bit
> ambiguous; I probably should have said was "non-private".
> You should get the warning if a module has a non-exported type class instance
> whose type vector consists only of (a) imported types** or (b) types exported
> in the interface of the _current_ module.

Then I think we agree, but I don't think that description is clear.

How about

"Generate a warning if an instance declaration is not exported
even though none of the type constructors mentioned in the member types
is private to the current module."?

> Should we get the warning if there are any class constraints on the
> instance that refer to private type classes?

No.

We could go with

"Generate a warning if an instance declaration is not exported
even though none of the type constructors mentioned in the member types
is private to the current module, and none of its constraints refer
to private type classes".

Or we could switch to another approach, which clarifies the intent:

"An instance declaration has to be private if some of its member types
refer to type constructors private to the current module, or if its constraints
refer to type classes that are private to the current module. Generate
a warning if an instance declaration is not exported even though
neither of these conditions apply."

Overall, I think I prefer that last approach. We could even expand
on it, e.g. by putting "Such instances can only be relevant in the current module."
between the two sentences above, then possibly replacing the last sentence
with "Generate a warning if an instance declaration that can be relevant outside
the current module is not exported.".

> ** Types imported from private submodules are not non-private so should not
> trigger the warning.

Want to rephrase that one without the triply nested negation? :-)

Are you trying to say that we should replace "private to the current module"
with something like "private to the current module and its submodules (if any)"?

> The diff looks fine.

Thanks for the prompt review.

Zoltan.