Add Hint when a module is used in place of a module type

[xvw]

Attempt to fix #6633
The patch add an Hint if a module is used in place of a module type. For example:

module Equal = struct
  module type Equal = sig ... end
end
module Foo = functor (E : Equal) -> struct end;;

Will add to the error:

Line 5, characters 26-31:
5 | module Foo = functor (E : Equal) -> struct end;;
                              ^^^^^
Error: Unbound module type Equal

The following Hint:

Hint: There is a module called [Equal] but it is not a module type

If there is namesake the hint is not displayed.

[gasche]

Thanks! This looks good overall, but see comments (in particular on the wording of the new warning).

We would have the option to hint: "Did you mean (module type of Equal)". Do we want to do this? I'm not sure this is a good idea, but I sort of like making this feature more discoverable through hints.

[gasche]

The warning says "there is a module called [Foo]", I would (1) drop the brackets and (2) use "named" instead of "called": "there is a module named Foo".

[gasche]

I don't find the find_namesake_module auxiliary function very helpful, in part because "namesake" is a term I am not so familiar with. You could just call find_module_by_name directly. (You can use match ... with exception Not_found -> ... | _ -> ... if it leads to nicer code than try .. with ...)

[gasche]

Do we want this new hint before, or after, the spellchecking hint (if any)? (I'm not sure if one order is objectively better than the other.)

[Octachron]

I am not sure if I like the hint: we are special casing the (module, module type) case (the pair (class, class_type) is probably as confusing as this one), in one direction, without integration with the spellchecking hint.

Moreover, I don't expect to be ever right in code that I write, because having a module and module type named the same is just an accident and reporting this accident doesn't bring much information to me.

This is thus a beginner-oriented hint. However, the hint message doesn't give any hint on how to solve the error, because this is fundamentally a programmer mental model error. At best, the hint may explain that module and module types are not the same thing.

I would thus suggest to at least amend the hint to:

• only trigger if there is no spellchecking candidates (the spellchecking hint is effective)
• rephrase the hint to state that module and modules types are not the same thing.

There is a module named Foo, but modules are not module types

(which somehow exacerbates my issue with the hint: why are we reporting that an unrelated entity is also named Foo?)

Am I missing some case where the hint can help a beginner to fix its code?
The original report looks too me like it is more a misunderstanding on how ml file works.

[gasche]

we are special casing the (module, module type) case (the pair (class, class_type) is probably as confusing as this one)

It would be fine to add other situations where similar confusions occur with class types, here or in another PR. Do you have an example of class-vs-class-type confusion that would benefit from a hint?

in one direction

We should ask the people who were confused, but my impression is that it is not so surprising that people would expect to use a module as a module type: modules have an obvious module type they correspond to, and using for example fun (X : M) to mean "an argument X with the same interface as M" sounds like something someone would do. On the other hand, I don't see how someone could possibly expect to use a module type in place of a module.

After having written the paragraph above, I do see a situation where that oculd happen: for type-only modules which are isomorphic to signatures. If someones defines a type-only module and then tries to use it as a signature, we could give a hint.

without integration with the spellchecking hint.

Yes, this is a good point. It may be necessary to refactor the spellchecking code a bit to return the information of whether a hint was given or not.

There is a module named Foo, but modules are not module types

I like the proposed rephrasing. We could possibly refer to the manual for more explanations?

[Octachron]

For the reverse error, beginners try to use module types as interfaces:

module type S = sig type t val show: t -> string end
let f (x: S.t ) = ()

often enough that I have a ready-made answer for that situation.

For class types, the problem can only appear when using class types as classes:

class type ct = object method m: int end
class c = object inherit ct end

We could refer to the manual chapter on modules as a hint that there may be deeper issues at work.

For the no-runtime component subcase, it is true that in this situation, we have a more direct hint that makes sense: Foo is a type-only module. Was it supposed to be a module type?.

[gasche]

Your answer is very useful, but a bit lacking in actionability: what do you propose that @xvw should do to address your concern? A proposal for a scope (which hint(s) to implement for a PR that you would like?) and maybe slightly more specific on the wording (what do you think would be actual good hints to give?) would be even more useful.

[xvw]

Thanks both of you for your suggestions and insight. I've sligthly remanied the PR with some opiniated choices (but I'm open to update anything, of course).
In order to keep the PR small, I did not go down to the rabbit hole trying to patch spellchecking. So I take this (potentially temporary) decision:

• If we have module instead of module type (and vice-versa) or class type instead of class, I show the Hint.
• If there is no correspondance in the image (module <=> module type, class => type class) I show the spellchecking.

[Octachron]

I think the prioritization of the same name, different category hint over the spellchecking is fine. Moreover, we could tweak it later if needed.
I am less fond of the explanation part of the hints: in the right context, they are an improvement; but we are not guarantee to be in the right context:

module type A
module M = A

yields

Hint: A is a module type. Modules types are specifications, they do not define types nor modules

whereas there is no type nor modules at play.

It would be possible to keep enough context in order to specialize the message when it makes sense. However, I would propose to stop at the generic hint message first and refine it in a later PR.

[xvw]

Thanks @Octachron for your feedback. The hint is now more concise (and avoid misleading information).

[gasche]

The new implementation looks good to me. I think this is good to merge if @Octachron agrees and also the CI.

Note: we could have had the symmetric support in the Unbound_cltype case.

[Octachron]

It would be impossible to trigger the hint since class declarations automatically define a class type with the same name as the class.

[gasche]

Makes sense, thanks. Do you also approve merging?

@xvw you should write a Changes entry (see CONTRIBUTING.md for details, or just imitate existing entries).

[Octachron]

Yes, I think the added symmetry (where it makes sense) and interaction with the spellchecker hint makes the new hint useful enough.

[xvw]

Thanks. I'll write the Change Entry (and try to have the CI green). Thanks both of you for your help and your insight. Just to be sure, module types or modules types?

[gasche]

module types.