warning 9 enabled by default in dev mode

[avsm]

Xavier Leroy notes in ocaml/ocaml#12201 (comment) that warning 9 being enabled by default in dune is not a good default. Is there a possibility of adjusting the default build flags to be more in line with the decisions taken by upstream OCaml in this regard?

[rgrinberg]

Sure, I see nothing wrong with doing it. It will require users to explicitly bump the dune language in their project file though.

Do you have any other warnings you'd like to change?

[avsm]

Agreed on the change only happening with the dune language version bump, so it's optin. My preference is generally for the defaults to be the same as upstream OCaml, to reduce the surprise factor for users. We could have the current defaults be available in another (non-default) profile to ease the transition as well.

[rgrinberg]

The upstream defaults disable warnings such as

6 labels-omitted
Label omitted in function application.

and basically all the unused X warnings:

32 unused-value-declaration
Unused value declaration. (since 4.00)
33 unused-open
Unused open statement. (since 4.00)
34 unused-type-declaration
Unused type declaration. (since 4.00)
35 unused-for-index
Unused for-loop index. (since 4.00)
36 unused-ancestor
Unused ancestor variable. (since 4.00)
37 unused-constructor
Unused constructor. (since 4.00)
38 unused-extension
Unused extension constructor. (since 4.00)
39 unused-rec-flag
Unused rec flag. (since 4.00)

In principle, I don't mind. I just wonder if we're really improving UX by doing this.

[SkySkimmer]

Disabling 42 is good though.

[rgrinberg]

Disabling 42 is good though.

Indeed, my copy paste was a little bit too wide. Fixed.

[kit-ty-kate]

In principle, I don't mind. I just wonder if we're really improving UX by doing this.

I think the main thing that is bothering users is less which specific warnings are enabled by default, and more that they are enabled as errors.

As a first step, why not at least remove -warn-error and only enable -warn-error on a potential dune lint command (or dune build @check) instead so that the errors are not fatal to increase productivity and leave CI systems to enable them as errors.

I also would personally prefer dune to stick with OCaml's standard set of warnings/errors and make them evolve there instead (e.g. ocaml/ocaml#12475) but if it's not possible, at least removing -warn-error is to me a much better user-friendly default compared to now.

[c-cube]

Seems like a good time to advocate for the deprecation warning to not be a warn-error in any profile, again.

[avsm]

@kit-ty-kate wrote:

I also would personally prefer dune to stick with OCaml's standard set of warnings/errors and make them evolve there instead (e.g. ocaml/ocaml#12475) but if it's not possible, at least removing -warn-error is to me a much better user-friendly default compared to now.

I agree. It's the divergence to upstream that's causing confusion, so a good default is to start there and ensure that changes are done in lockstep with the compiler. Non-default Dune profiles can still be available with the current settings, though.

[rgrinberg]

I actually don't mind resetting the set of warnings to OCaml's, but removing -warn-error is far more work than it looks. The issue is that removing this flag is that compilation will display the warning only when the file needs to be recompiled. So:

$ dune build # this warns
$ dune build # no warning appears because compilation already succeeded

This is highly undesirable and is going to bring us back to the world before dune where every project was littered with random warnings.

The correct way to fix this is to remove the -warn-error but make compilation fail at the end of the build, and replay all the stderr emitted by the various compilation commands. This is all much more work than just removing a flag.

[gridbugs]

compilation will display the warning only when the file needs to be recompiled

Is that really that bad of a problem? OCaml-LSP/Merlin will still show warnings. And since people seem to just workaround this by manually downgrading all the "unused" errors back to warnings anyway we're kind of already in this world (at least for those people).

I have two different use cases for warnings:

1. While I'm writing code I often do things that cause warnings temporarily like introducing variables that I haven't used yet and it's frustrating to have to prefix them with _ just to be able to build/run the code. I copy/paste custom flags to downgrade errors into warnings in every dune project I work on.
2. When I'm shipping code (either by PR or releasing a project) I want to make sure it has no warnings but not everyone feels this way (see most C projects) and lots of hobby code never gets released anyway so they don't benefit from the current strict default.

I like the idea of making dune attractive to people who just want to mess around with ocaml for fun and that scenario only sees case 1 above. For people using dune in production and who do need case 2, they will still be spending much more time writing code than shipping it so I claim that case 1 what we should be optimizing for when designing UX.

My proposal would be to change the defaults to match the compiler but introduce a flag that treats warnings as errors similar to C's -werror or rust's --deny warnings.

[xavierleroy]

"Upstream" speaking! I'm glad to see this point being discussed and the discussion being constructive so far.

FYI, warning 6 "labels-omitted" is now active by default: ocaml/ocaml#10140 . I would not be surprised if it becomes an error (or maybe a warning that triggers an error by default) at some point in the future.

My feeling is that the default should be "few warnings and very few warnings as errors", to encourage discovery of the language and quick prototyping. On the other hand, it is currently rather painful to select a reasonable set of warnings to use for production-quality code, so we "upstream" should have more "presets" and better documentation. For example, did you know that -W K activates all the "unused" warnings? I did not, until I checked the OCaml manual before writing this reply...

[SkySkimmer]

Individual warnings got names a few versions ago, maybe the warning groups could have names too?

[rgrinberg]

Is that really that bad of a problem? OCaml-LSP/Merlin will still show warnings. And since people seem to just workaround this by manually downgrading all the "unused" errors back to warnings anyway we're kind of already in this world (at least for those people).

It's pretty bad. If you want to make sure your project is warnings-free you need to do a clean build. If you have the dune cache enabled, you also need to remember to turn it off for the clean build.

Okay, so I think the plan of action is pretty clear:

1. We revert to OCaml's default set of warnings
2. Introduce a mechanism to set the warning level to dune's current default so people that prefer the old default will have an easy path forward
3. We replace -warn-error with something that will replay all the active warnings at the end of every build (without failing).

[xavierleroy]

I tried to start an "upstream" discussion here: ocaml/ocaml#12557 . Please contribute!

[ejgallego]

I do like that dune is pretty strict about warnings in dev mode, unaddressed warnings seem pretty common in the OCaml world and are a pain, so IMHO the default setup should be the one that promotes better quality code.

It seems to me that devs take always the quicker path, which means that if Dune's dev profile becomes weaker few devs will invest time in going back to a sane warning dev setup.

I do agree that it is sometimes annoying in rapid prototyping, so how to address this should be documented more prominently.