LGTM - changes need to go in ocamlnat as well

Thanks for the quick review. I pushed the missing changes (ocamlnat, but also a few more in Toploop and Topdirs). Now there are no other calls to exit apart from the ones in Topstart, Opttopstart.

One thought - it's easy to accidentally add code which calls exit (especially exit 2) in future, which would be annoying for users with alternate drivers relying on this new behaviour. Might it be worth adding an at_exit guard which reports an incorrect exit? Something along the lines of...

let main () =
  match main () with
  | exception Exit_compiler n -> n
  | () -> 0

let main () =
  let incorrect_exit = ref true in
  let _ = at_exit (fun () -> if !incorrect_exit then Printf.eprintf "Stop the world; report a bug;\n"; exit 2) in
  let result = main () in
  incorrect_exit := false;
  result

@nojb Didn't look what you are doing, I'm drive-by commenting. But is this related to #7202 ?

One thought - it's easy to accidentally add code which calls exit (especially exit 2) in future, which would be annoying for users with alternate drivers relying on this new behaviour. Might it be worth adding an at_exit guard which reports an incorrect exit? Something along the lines of...

let main () =
  let incorrect_exit = ref true in
  let _ = at_exit (fun () -> if !incorrect_exit then Printf.eprintf "Stop the world; report a bug;\n"; exit 2) in
  let result = main () in
  incorrect_exit := false;
  result

The at_exit hook will be triggered if the user calls exit in the repl, right?

More generally, I don't think this kind of thing is necessary in compiler-libs which is for "experts".

@nojb Didn't look what you are doing, I'm drive-by commenting. But is this related to #7202 ?

Not really, this just fills in some missing cases in #9688, which changed some direct calls to exit by an exception so that it can be handled by a user of compilerlibs.

Naive question: why would the toplevel use Exit_compiler, when it's not a compiler?

Naive question: why would the toplevel use Exit_compiler, when it's not a compiler?

Fair question, I guess... Are you suggesting that it should use a different exception or that the name should be adjusted? For a name adjustment, I could think of something more general like Exit_status. Using a different exception seems a bit overkill to me.

Not having looked in details at the PR that introduced the change, I cannot tell what was the intended scope of the exit-protocol change, but the name suggests that it should have been restricted to ocaml{c,opt}. The fact that it silently broke something in the toplevel is a bit worrying. One fix may be to propagate the exit-protocol change in the toplevel as you suggest (but then we probably need a better name, and also to think twice about what the scope of the change really was, to understand whether other places (ocamldep ?) are affected as well). But maybe another fix would be to restrict the scope of the change, so that the toplevel is not affected as originally intended? (And if the latter also works, which of the two approaches do we believe is the better one?)

Not having looked in details at the PR that introduced the change, I cannot tell what was the intended scope of the exit-protocol change, but the name suggests that it should have been restricted to ocaml{c,opt}. The fact that it silently broke something in the toplevel is a bit worrying. One fix may be to propagate the exit-protocol change in the toplevel as you suggest (but then we probably need a better name, and also to think twice about what the scope of the change really was, to understand whether other places (ocamldep ?) are affected as well). But maybe another fix would be to restrict the scope of the change, so that the toplevel is not affected as originally intended? (And if the latter also works, which of the two approaches do we believe is the better one?)

Using an exception instead of calling exit seems like a good thing to me, independently of other considerations. But I agree that the change should be considered in the context of the original PR. For reference, the issue reported in #9699 (comment) is caused by

We would of course severely frown on any user libraries which called exit directly, so it's certainly good to be getting rid of it in OCaml's internal libraries too. The name isn't supposed to leak, but I guess is there any reason for it not to be called Compenv.Exit instead?

For the at_exit, I hadn't considered code intentionally calling exit 😊

@nojb thanks!

I cannot tell what was the intended scope of the exit-protocol change, but the name suggests that it should have been restricted to ocaml{c,opt}.

It was intended to only affect ocaml{c,opt}, but I forgot that other programs use the compiler's Main_args for options parsing. (Main_args exits the process during option parsing if it sees -version).

The same problem appears in other programs that use the compiler's option parsing. The list is:

ocaml
ocamlnat
ocamlcp
ocamloptp
ocamldoc

(ocamldep is fine, it has its own special argument parsing because of the ocamlc -depend mode, so it got fixed at the time)

The name isn't supposed to leak, but I guess is there any reason for it not to be called Compenv.Exit instead?

I wanted to avoid confusion with Stdlib.Exit, but @nojb's suggestion of Exit_status works.

@nojb thanks!

I cannot tell what was the intended scope of the exit-protocol change, but the name suggests that it should have been restricted to ocaml{c,opt}.

It was intended to only affect ocaml{c,opt}, but I forgot that other programs use the compiler's Main_args for options parsing. (Main_args exits the process during option parsing if it sees -version).

The same problem appears in other programs that use the compiler's option parsing. The list is:

ocaml
ocamlnat
ocamlcp
ocamloptp
ocamldoc

(ocamldep is fine, it has its own special argument parsing because of the ocamlc -depend mode, so it got fixed at the time)

The name isn't supposed to leak, but I guess is there any reason for it not to be called Compenv.Exit instead?

I wanted to avoid confusion with Stdlib.Exit, but @nojb's suggestion of Exit_status works.

Unless there is an objection, I can do the changes for the rest of the tools and switch to the new name. Speak up if you think it is a bad idea!

(I'm happy with Exit_status.)

I renamed the exception and adapted the rest of the tools in @stedolan's list. Review welcome!

The code looks fine, but #9688 having essentially leaked to utilities as well is giving me second thoughts about how this is being done. It's fine to have to handle the Exit_status exception when you're doing something main-like and need to convert the teardown effect of exit into a return code, but it looks odd to me having to handle it subtly as a result of argument parsing. Might it be better to add a parameter to Main_args.options_with_command_line_syntax indicating whether the arguments should call exit n or raise (Exit_status n)? The drivers and toplevel can then specify the exception and the utilities can then have exit 0 for ocamlcp -version as expected?

The code looks fine, but #9688 having essentially leaked to utilities as well is giving me second thoughts about how this is being done. It's fine to have to handle the Exit_status exception when you're doing something main-like and need to convert the teardown effect of exit into a return code, but it looks odd to me having to handle it subtly as a result of argument parsing. Might it be better to add a parameter to Main_args.options_with_command_line_syntax indicating whether the arguments should call exit n or raise (Exit_status n)? The drivers and toplevel can then specify the exception and the utilities can then have exit 0 for ocamlcp -version as expected?

I agree that the exception handling becomes too subtle and unintuitive.

Perhaps another solution is simply not to "exit" after handling -version...

Instead of exiting, the main command-line interpreter could return a (unit, int) result, where the error case is an error code that would have been the exit value?

(I agree that it's an API smell to have a function named print_version : unit -> unit implicitly abort the computation, but then the behavior of -version stopping early has been around for so long, uniformly across all tools, that I think it would be surprising and dangerous to change it now.)

Instead of exiting, the main command-line interpreter could return a (unit, int) result, where the error case is an error code that would have been the exit value?

That would be nicer, but is incompatible with the API of Arg.parse_and_expand_argv_dynamic. While not widely used outside the compiler, this API is part of the stdlib and therefore tricky to change. In particular, I don't think there's any way to make Arg stop parsing further options other than exceptions or exit, so it will be tricky to preserve the existing behaviour of ocamlc -version -asdfasdf.

Alternatively, we could move a copy of Arg into the compiler proper and give it a result-based API, but that would be a larger change.

My idea was to expose an entry point somewhere to process command-line arguments, that would internally catch the exception and expose a result interface (so Exit_status would be designed to never escape the corresponding module). Other tools and compiler-libs would use it, and decide on their own semantics for the exit case. (Just like with a specific exception, but there the type forces users to decide.) But then I don't know how the code is structured today, maybe it's not realistic to expect Main_args user to use this wrapper instead of calling Arg directly?

That makes sense to me. There will be a bit of work needed because there are several different option-parsing entrypoints at the moment (some tools use Arg directly, some go via Clflags.parse_arguments), but this sounds like a worthwhile refactoring.

If you do this refactoring, it would also be nice to refactor the handling of -help so that it works in a similar way to -version. Currently, -help is hardcoded in Arg and converted into a special exception, while -version is a normal option that happens to call exit / raise Exit_status.

It looks like this PR started as a fix for an actual bug and is now lost in "wouldn't it be nicer if..." territory. Could the author and reviewers agree quickly on something that at least fixes the bug?

I think there are three options:

• Go with this PR and a slight API smell (with no offence intended to @nojb), and a possible (if maybe unlikely?!) refactoring to something more elegant in future
• Go ahead with @gasche's suggestion in time for 4.12
• Revert Expose the main entrypoint in compilerlibs #9688 pending a comprehensive fix for 4.13

I like the refactoring, but I don't have time before 4.12, and I expect that if the more temporary fix goes in then the refactoring will be a loooong way off. Thoughts?

Sorry for having dropped the ball on this one. I am looking into implementing @gasche's suggestion. Something I noticed is that currently Compenv.fatal is using the Exit_compiler/Exit_status exception, but this function is used outside of the argument-parsing context. So any refactoring of the argument-parsing API will not be enough to also solve the uses of this function.

At the risk of sounding like a broken record: we have a bug here (e.g. Coq's autoconfiguration calls ocaml -version, which fails with an uncaught exception in the current trunk) and we need to fix it. If the fix looks nice it's a plus, but a clunky fix is already better than the broken code we have today. Thank you all for your attention.

OK, I rebased the current version of this PR (the "clunky fix").

No Changes entry because nothing has been released yet. A formal review and approval would be appreciated, thanks!

From a quick reading of the code, this looks OK. I'm not a fan of the name Exit_status for the exception. Exit_with_status would be slightly better. Just Exit would work fine, and there's already enough qualification Compenv.Exit to avoid confusion with the Exit exception from the standard library.

Thanks. I went with Exit_with_status because Exit conflicted with a use of Stdlib.Exit in Compenv itself.