This PR also removes the "hooks" (Misc.MakeHooks et al) machinery which is not directly related to the plugin mechanism. Is this intended?

@nojb I thought the majority of the "hooks" code---exactly those parts I deleted---were introduced along with the plugins mechanism. (I deduced that by looking at some of the documentation, which described their use in conjunction with plugins, if I remember correctly.) I did leave some "hooks" in place, in particular in the toplevel code.

@nojb I thought the majority of the "hooks" code---exactly those parts I deleted---were introduced along with the plugins mechanism. (I deduced that by looking at some of the documentation, which described their use in conjunction with plugins, if I remember correctly.) I did leave some "hooks" in place, in particular in the toplevel code.

Yes, but the hook mechanism could still be used when building standalone compiler drivers. I don't have any use case in mind, so am not arguing to keep them. But maybe worth asking about it in caml-devel.

I do have a light use case for hooks: I have at least one toy projects, https://github.com/Octachron/more_ocaml_warnings, which use the Typemod's hook in order to add a warning for the beginner's mistake like module M: sig type t end = ....

Going from a compiler plugin to a statistically-linked driver does not change much in term of usability (it may even improve it), but removing hooks would make harder to maintain such toy projects.

This is certainly not a very strong point for keeping those hooks. However, since the maintenance cost of such hooks is far lighter than the plugin mechanism, it might make sense to keep it?

• I think the general notion of hooks is useful, as they allow to implement language/compiler extensions by creating custom drivers, without having to fork the compiler entirely. I'm in favor of keeping existing hooks (and possibly adding more, if the need arises).

• I initially thought that removing support for dynamic plugins in the compiler would be a no-brainer, since users could always create their own custom driver that would link with Dynlink to provide the same feature. But now I remember that it's probably not possible, since Dynlink would clash with internal data structures from the compiler; is that right? Static linking of "plugins" in custom drivers is a possibility, but this might be much less convenient that drivers with support for dynamic plugins. Wouldn't it be possible to find a way to allow a program to link with both compilerlibs and dynlink, even if the "official" compiler doesn't link with dynlink itself?

@alainfrisch #2268 will provide what you want, I think, although I'm still working out some tricky details with @xclerc .

Regarding hooks, it doesn't seem to me that these are worth keeping. I think the main reason is as follows: they just exist in a small collection of rather arbitrary locations. As such it seems to me that someone's future development work changing the behaviour of the compiler will, with some reasonable probability, not coincide with a place where one of these hooks exist---and it seems messy to add hooks everywhere. The whole thing seems like a poor-man's version of aspect-oriented programming (in which case it must be quite poor).

It also isn't difficult to rebuild the compiler libraries if you want to implement functionality that isn't provided by any hook. This might also, perhaps, encourage people to contribute such functionality upstream.

@xavierleroy and @damiendoligez : Could you please offer an opinion?

FTR: my use of a plugin was indeed just lazyness. For my use case it would have been better, for various other reasons, to write my own driver.

Personally my plugin didn't use any fancy "hook" mechanism. It just updated some references already present in the typechecker; but I assume that the hook mechanism is just a nicer interface, which covers a different set of references than the one I happened to need?
In any case, it was useful, and so I'd tend to agree with Alain. It is possible to do with them, but it becomes tedious.

On hooks: Some of them are quite essential for alternate toplevels (like jsoo's toplevel or sketch.ml). In particular the hook to redefine .cmi loading. Maintaining a fork of the typechecker just for these is not reasonable.

The .cmi loading hook and the hooks in the toplevel have not been removed by this patch.

There is a debugger test that fails on Windows.

I've pushed a single changeset containing everything so far, since there was some trouble rebasing.

CI green! The only objection was about not removing the toplevel hooks and indeed they are not. Unless someone else objects, this looks good to merge.

Has it been through precheck? Precheck #211

One small spanner to throw into the works - is it definitely the case that the -plugin command line flag should be deleted? It would seem better to update the text and display either a warning that it's ignored or a proper error message that the functionality was only available until 4.08.0?

I think deleting the flag is fine, given the data we collected about the (non-)use of plugins.

That's not the point, though - the issue is how we deal with changes in the CLI - there are other flags for which this has happened and it should be consistentm

Unrelated to that, otherlibs/dynlink/nodynlink.ml should be deleted as part of this GPR.

@dra27 Given that the work has been done to remove the flag now, and with the lack of use of plugins, I really think it is fine as it is. There is a clear Changes entry saying what has been removed. We're just making work for ourselves otherwise, both now and in the future.

I have removed nodynlink.ml, well spotted.

Come on, the "work" we're talking here is one-line alterations in driver/main_args.ml and utils/clflags.ml* to change plugin to be internally a boolean flag followed by an error message in driver/main.ml below the vmthreads deprecation warning.

dra27@eb1ab30 is what I was meaning which gives:

dra@cosmic:~/ocaml-$ ./ocamlc.opt -plugin foo
-plugin is only supported up to OCaml 4.08.0
dra@cosmic:~/ocaml$ echo $?
2

This seems to have the additional desirable side-effect that -prefix can't be re-added in a driver which tries to extend the default one.

I could use an explanation of Dynlink.unsafe_get_global_value, a function that this PR adds.

@xavierleroy This function is needed to support existing use cases where clients of the Dynlink library look up global values by name, instead of what I understand to be the more usual situation, where the module initialisers in the shared library make various registrations with the main program.

The two existing use cases, which now both go via this function, are:

1. Printers in the debugger. This is bytecode-only at present and used Symtable.get_global_value. Since this pull request makes the Dynlink library linked into the debugger use the compilerlibs modules from the dynlink-specific pack (the Compdynlink machinery has been removed) then the relevant Symtable module is no longer accessible from the debugger code itself [*]. As such, some interface in Dynlink is needed here.

2. The native toplevel. This currently binds one of the external C functions in the natdynlink code, which is ugly.

The Dynlink.unsafe_get_global_value provides a uniform interface for both use cases. (I would like to improve the type of its argument to something better than string, but that should wait until after my other patches relating to symbol types.)

[*] This seems good, because Dynlink now can't interfere with other clients of Symtable, as used to happen in the debugger when a single Symtable was shared between Dynlink and the debugger code itself. Special-case code was needed to work around this, which can now be removed.

@xavierleroy If you are happy with my explanation above, please merge.

Thank you @mshinwell for the detailed explanations. With them I could review the dynlink + debugger part of this PR. It looks good to me and I agree the debugger's use of Symtable is nicer and clearer.

About Dynlink + Symtable: the following is probably obvious for @mshinwell but perhaps not clear to everyone. Even though proper packing of Dynlink hides the copy of Symtable that it uses, making it possible to link it along Symtable, there is still only one table of global values in the bytecode VM and changes to this table are NOT synchronized between Dynlink and the Symtable functions that change the global value table (update_global_table, assign_global_value). That's why we can't use Dynlink from the toplevel interactive loop, in particular. I think this GPR doesn't run into this trap, since the debugger doesn't seem to use those Dynlink functions.

Coming back to this GPR: I think it's in good shape and will merge soon. I still encourage @dra27 to submit a PR that will 1- remove Clflags.plugin entirely, and 2- print a specific error message if -plugin is given on the command line, in the style of #2312.

@xavierleroy I was not fully clear on all the details of that actually, so thanks for the explanation. Let me just add a comment based on your text before we merge this.

I was writing the log entry for the squashed merge :-) Ping me when you're done.

@xavierleroy - no problem to follow it up with a Clflags.plugin removal.

@xavierleroy Please see the comment, which is a slightly expanded version of your text. That's all from me.

@dra27 don't forget to open a PR for the removal of Clflags.plugin.

I was advised from https://discuss.ocaml.org/t/compiler-plugins/3898/2?u=overminddl1 to voice my issues upon the compiler plugins being removed, it's essentially killing my codebase and preventing me from upgrading past 4.8.0 (which I just completed doing).

I'm not finding any documentation on any migration path or anything to be able to keep the workflow this uses in a backwards compatible way using the stock installed compiler, otherwise it seems I'll need to essentially fork the compiler, which I'm exceptionally not wanting to do...