toplevel: Show hints for the "undefined global" error

[wikku]

This PR adds hints for loading modules when hitting the "undefined global" error. The advice is basically taken from the toplevel chapter of the manual, but we can make it a bit more helpful because we can see what files are already there (in the load paths).

For example, this is the hint in the following (possibly confusing for the user) interaction:

# #show Dynlink.is_native;;
val is_native : bool
# Dynlink.is_native;;
Error: Reference to undefined global `Dynlink'
Hint: This means a module's interface is loaded, but its implementation is not.
      Found otherlibs/dynlink/dynlink.cma in the load paths. 
      Did you mean to load it with #load "dynlink.cma" 
      or by passing it as an argument to the toplevel?

The advice with the .ml file in the code might be not as useful, because it probably only fires if there is a .cmi file but no .cmo file, so I don't know whether to keep it.

I realize the code looks repetitive, but the clever ways to rewrite it I can think of don't feel right and don't save much code.

[gasche]

I like the idea and I think the messages you suggest are good. (See minor code comment.)

(A Changes entry would be nice.)

[wikku]

I'm still not sure if "This means a module's interface is loaded, but its implementation is not" is always true and if the s in Undefined_global s is a module name. Anyone can confirm?

[xavierleroy]

It's an interesting idea to add hints for errors reported by the toplevel interactive loop. However, bytecomp/symtable.ml is shared between the toplevel, ocamlc, and ocamldebug (at least), so I'm not sure the "hints" code belongs there: I'd be happier if it was in a toplevel-specific module.

What about extending the toplevel so that it reports errors not just by calling Location.report_exception, but by calling Location.report_exception, then matching on the exception to add hints?

[Octachron]

Another option would be to override the Symtable exception printer with an extended one in Toploop.

[xavierleroy]

Another option would be to override the Symtable exception printer with an extended one in Toploop.

This could work too. @wiktorkuchta : are you willing to explore these alternate approaches?

[wikku]

Moved the code to toplevel/topcommon.ml. It looks like a better place for miscellanea than toplevel/toploop.ml, which is mainly concerned with executing phrases.

However, it's still code specific to the bytecode backend (and won't be hit by the native toplevel unless it somehow raises the Symtable.Error exception from bytecomp), maybe it should go to toplevel/byte/topmain.ml or toplevel/byte/topeval.ml.
But on the other hand, we may want to keep places where exceptions printers are overridden to a minimum (and for instance I could write a follow-up PR with compiling/loading hints in the case when there is only a .ml file, both for native and bytecode).

[wikku]

This patch would give the correct hint for https://discuss.ocaml.org/t/wheres-the-unix-library/8782.

Though while testing with runtop in the source tree I noticed it would find unix.cmo before unix.cma, which is wrong, but wouldn't happen with a normal install. Maybe cmas should be searched before cmos or the hint should print all results?

[wikku]

@Octachron ping

[gasche]

This printer, given the way it is registered, claims to support all Symtable.Error cases, but in fact ignores all errors that are not Undefined_global. This is awkward and also probably wrong. I would feel safer if the loading_hint_printer was passed directly to register_error_of_exn, and would return Some only in the case that it definitely knows how to handle better than the standard printers.

[gasche]

Note: in general there is no reason for foo.cma to provide an implementation of the module Foo, and this makes this heuristic frustratingly incomplete. It is very wrong in the general case, but also correct surprisingly often (Dynlink, Unix, Graphics etc.). I would keep the code as you wrote it, but include a comment to point out that hinting at the .cma may be wrong.

[gasche]

I'm happy with the refactored implementation. Thanks!

(You could squash/fixup the commits together, to save us from the annoying github UI to do it at merge time.)