Option to turn warnings into errors

[Julow]

Closes #284

This PR adds the option --warn-error to turn warnings into errors.
It is implemented using a global flag and failing after warnings are printed.

This is the easier implementation I found and it has some problems:

• Very few warnings are accumulated before shed_warnings is called and the error is raised.
• It's a global variable that is very deep in the dependency chain

I also started looking into other ways that require refactoring:

• Passing the warning accumulator everywhere
  It's how the parser works but makes the code very verbose and introduce a lot of diffs. (~300 adds)
• Change Error.catch to also "catch" warnings
  This is also a lot of changes and requires to change the API of Error to avoid bad uses, which requires even more refactoring.
• Monad style would drastically change the whole codebase

What do you think?

[jonludlam]

Looks like the failures are due to the expect tests expecting exact line numbers in the backtraces.

[Julow]

This error message is very bad anyway, I wanted feedback before changing it.
Failure is raised in odoc/* when handling errors from Odoc_loader.read_* and are not catch.
I changed the code to properly handle that error but the output is still an unhandled exception.

[aantron]

This looks fine. We can refactor it later, either passing a warning accumulator, or converting to monadic style.

How are you invoking odoc with --warn-error?

The test should be fixed to pass, which I guess will happen when you fix up the error message :)

[Julow]

The option is added to odoc compile, odoc html and odoc html-fragment. Do other commands need it ?
I commited a small refactor that propagate the errors up to the main and print/exit there. This is only for the 3 subcommands, so it's not very consistent. I can work on doing that for all the subcommands in an other PR if you think it's a good approach ?

[aantron]

The option is added to odoc compile, odoc html and odoc html-fragment. Do other commands need it ?

What I meant is, how are you driving odoc? I am just curious for comparison. AFAIK when driving odoc through Dune, users still won't have easy access to the option.

I commited a small refactor that propagate the errors up to the main and print/exit there. This is only for the 3 subcommands, so it's not very consistent. I can work on doing that for all the subcommands in an other PR if you think it's a good approach ?

The refactor looks okay at first glance, it is at least more consistent than what we had before :) I would accept a second PR for this. However, a lot of this code is probably in need of even more fundamental refactoring with respect to error propagation and reporting.

[Julow]

What I meant is, how are you driving odoc? I am just curious for comparison. AFAIK when driving odoc through Dune, users still won't have easy access to the option.

My first thought was that is Dune's job. Just like libraries, dune build @doc is for dev builds and dune build -p my_package @doc for release builds. Also, the (documentation ..) stanza could allow to enable or disable explicitly this flag (or allow to pass flags directly).
I don't known about other tools calling Odoc. Do you think a different API is needed ? (eg. an env variable)

The refactor looks okay at first glance, it is at least more consistent than what we had before :) I would accept a second PR for this. However, a lot of this code is probably in need of even more fundamental refactoring with respect to error propagation and reporting.

I'd like to split this in an other PR and go a little bit further to make things a little cleaner.

[aantron]

Ok, that all sounds good.

[aantron]

Is this PR ready for merge? It looks good to me.

[Julow]

Sorry for the delay and thanks for merging :)
Do you think I should open an issue on the limitations of this PR ?
Only a few warnings are printed before the error is raised terminating the program instead of collecting all the warnings.

[aantron]

Do you think I should open an issue on the limitations of this PR ?
Only a few warnings are printed before the error is raised terminating the program instead of collecting all the warnings.

Yes, please :)