Remove warning 30 from the standard set. by gasche · Pull Request #9568 · ocaml/dune

gasche · 2023-12-25T14:14:11Z

Warning 30 (same constructor/label name used in two mutually-recursive declarations) is an annoying warning that makes no sense since we introduced type-based disambiguation of fields and constructors. It was disabled by default in OCaml 4.10

ocaml/ocaml#9046

and there is no reason to keep it in Dune.

Note: I wasn't sure how warning changes are supposed to be versioned (or not). 3eff810 implements versioning for the new warnings 67, 69, but then c967d8a earlier removed a bunch of warning without any specific versioning code. I went with the simplest approach of the two, but happy to take feedback. (This could benefit from a comment in the source besides (* New warnings should be introduced here *).

rgrinberg · 2023-12-26T19:40:12Z

but then c967d8a earlier removed a bunch of warning without any specific versioning code

This is just an issue with our suboptimal warning handling code. A new version of OCaml that introduces new warnings will have them enabled in dune by default. So the commit above simply disabled these warnings.

I agree with the change this PR proposes, but it needs to be version guarded.

gasche · 2023-12-26T21:27:24Z

@rgrinberg I edited the PR to add versioning. I had to massage the code a bit to make it read nicely enough with 3 different cases instead of 2. (If there was a simple way to ask dune to memoize functions, we could use it instead of the lazy soup, but I stayed true to the existing code style.)

src/dune_rules/ocaml_flags.ml

gasche · 2023-12-28T22:29:45Z

@rgrinberg I see that you fixed a conditional, dune_version <= foo should have been dune_version < foo, but this occurs twice in the code and both places should be fixed. Let me force-push a more complete fix.

rgrinberg · 2023-12-28T22:30:27Z

Sure, please take over.

Warning 30 (same constructor/label name used in two mutually-recursive declarations) is an annoying warning that makes no sense since we introduced type-based disambiguation of fields and constructors. It was disabled by default in OCaml 4.10 ocaml/ocaml#9046 and there is no reason to keep it in Dune. Signed-off-by: Gabriel Scherer <gabriel.scherer@gmail.com> Co-authored-by: Rudi Grinberg <me@rgrinberg.com>

gasche · 2024-01-08T08:33:50Z

Thanks!

@rgrinberg

CHANGES: - Do not ignore `(formatting ..)` settings in context or workspace files (ocaml/dune#8447, @rgrinberg) - Add command `dune cache clear` to completely delete all traces of the Dune cache. (ocaml/dune#8975, @nojb) - Fixed a bug where Dune was incorrectly parsing the output of coqdep when it was escaped, as is the case on Windows. (ocaml/dune#9231, fixes ocaml/dune#9218, @Alizter) - Copying mode for sandboxes will now follow symbolic links (ocaml/dune#9282, @rgrinberg) - Forbid the empty `(binaries ..)` field in the `env` stanza in the workspace file unless language version is at least 3.2. - [coq] Fix bug in computation of flags when composed with boot theories. (ocaml/dune#9347, fixes ocaml/dune#7909, @ejgallego) - Fixed a bug where the `(select)` field of the `(libraries)` field of the `(test)` stanza wasn't working properly. (ocaml/dune#9387, fixes ocaml/dune#9365, @Alizter) - Allow to disable Coq 0.8 deprecation warning (ocaml/dune#9439, @ejgallego) - Fix handling of the `PATH` argument to `dune init proj NAME PATH`. An intermediate directory called `NAME` is no longer created if `PATH` is supplied, so `dune init proj my_project .` will now initialize a project in the current working directory. (ocaml/dune#9447, fixes ocaml/dune#9209, @shonfeder) - Allow `OCAMLFIND_TOOLCHAIN` to be set per context in the workspace file through the `env` stanza. (ocaml/dune#9449, @rgrinberg) - Experimental doc rules: Correctly handle the case when a package depends upon its own sublibraries (ocaml/dune#9461, fixes ocaml/dune#9456, @jonludlam) - Resolve various public binaries to their build location, rather than to where they're copied in the `_build/install` directory (ocaml/dune#9496, fixes ocaml/dune#7908, @rgrinberg). - Menhir: generate `.conflicts` file by default. Add new field to the `(menhir)` stanza to control the generation of this file: `(explain <blang expression>)`. Introduce `(menhir (flags ...) (explain ...))` field in the `(env)` stanza, delete `(menhir_flags)` field. All changes are guarded under a new version of the Menhir extension, 3.0. (ocaml/dune#9512, @nojb) - Correctly ignore warning flags in vendored projects (ocaml/dune#9515, @rgrinberg) - Directory targets can now be caches. (ocaml/dune#9535, @rleshchinskiy) - Remove warning 30 from default set for projects where dune lang is at least 3.13 (ocaml/dune#9568, @gasche) - It is now possible to use special forms such as `(:include)` and variables `%{read-lines:}` in `(modules)` and similar fields. Note that the dependencies introduced in this way (ie the files being read) must live in a different directory than the stanza making use of them. (ocaml/dune#9578, @nojb) - Use watch exclusions in watch mode on MacOS (ocaml/dune#9643, fixes ocaml/dune#9517, @PoorlyDefinedBehaviour) - Fix merlin configuration for `(include_subdirs qualified)` modules (ocaml/dune#9659, fixes ocaml/dune#8297, @rgrinberg) - Fix handling of `enabled_if` in binary install stanzas. Previously, we'd ignore the result of `enabled_if` when evaluating `%{bin:..}` (ocaml/dune#9707, @rgrinberg) - Add `coqdoc_flags` field to `coq` field of `env` stanza allowing the setting of workspace-wide defaults for `coqdoc_flags`. (ocaml/dune#9280, fixes ocaml/dune#9139, @Alizter) - ctypes: fix an error where `(ctypes)` with no `(function_description)` would cause an error trying refer to a nonexistent `_stubs.a` dependency (ocaml/dune#9302, fix ocaml/dune#9300, @emillon)

@nojb

CHANGES: ### Added - Add command `dune cache clear` to completely delete all traces of the Dune cache. (ocaml/dune#8975, @nojb) - Allow to disable Coq 0.8 deprecation warning (ocaml/dune#9439, @ejgallego) - Allow `OCAMLFIND_TOOLCHAIN` to be set per context in the workspace file through the `env` stanza. (ocaml/dune#9449, @rgrinberg) - Menhir: generate `.conflicts` file by default. Add new field to the `(menhir)` stanza to control the generation of this file: `(explain <blang expression>)`. Introduce `(menhir (flags ...) (explain ...))` field in the `(env)` stanza, delete `(menhir_flags)` field. All changes are guarded under a new version of the Menhir extension, 3.0. (ocaml/dune#9512, @nojb) - Directory targets can now be cached. (ocaml/dune#9535, @rleshchinskiy) - It is now possible to use special forms such as `(:include)` and variables `%{read-lines:}` in `(modules)` and similar fields. Note that the dependencies introduced in this way (ie the files being read) must live in a different directory than the stanza making use of them. (ocaml/dune#9578, @nojb) - Remove warning 30 from default set for projects where dune lang is at least 3.13 (ocaml/dune#9568, @gasche) - Add `coqdoc_flags` field to `coq` field of `env` stanza allowing the setting of workspace-wide defaults for `coqdoc_flags`. (ocaml/dune#9280, fixes ocaml/dune#9139, @Alizter) - ctypes: fix an error where `(ctypes)` with no `(function_description)` would cause an error trying refer to a nonexistent `_stubs.a` dependency (ocaml/dune#9302, fix ocaml/dune#9300, @emillon) ### Changed - Check that package names in `(depends)` and related fields in `dune-project` are well-formed. (ocaml/dune#9472, fixes ocaml/dune#9270, @ElectreAAS) ### Fixed - Do not ignore `(formatting ..)` settings in context or workspace files (ocaml/dune#8447, @rgrinberg) - Fixed a bug where Dune was incorrectly parsing the output of coqdep when it was escaped, as is the case on Windows. (ocaml/dune#9231, fixes ocaml/dune#9218, @Alizter) - Copying mode for sandboxes will now follow symbolic links (ocaml/dune#9282, @rgrinberg) - Forbid the empty `(binaries ..)` field in the `env` stanza in the workspace file unless language version is at least 3.2. - [coq] Fix bug in computation of flags when composed with boot theories. (ocaml/dune#9347, fixes ocaml/dune#7909, @ejgallego) - Fixed a bug where the `(select)` field of the `(libraries)` field of the `(test)` stanza wasn't working properly. (ocaml/dune#9387, fixes ocaml/dune#9365, @Alizter) - Fix handling of the `PATH` argument to `dune init proj NAME PATH`. An intermediate directory called `NAME` is no longer created if `PATH` is supplied, so `dune init proj my_project .` will now initialize a project in the current working directory. (ocaml/dune#9447, fixes ocaml/dune#9209, @shonfeder) - Experimental doc rules: Correctly handle the case when a package depends upon its own sublibraries (ocaml/dune#9461, fixes ocaml/dune#9456, @jonludlam) - Resolve various public binaries to their build location, rather than to where they're copied in the `_build/install` directory (ocaml/dune#9496, fixes ocaml/dune#7908, @rgrinberg). - Correctly ignore warning flags in vendored projects (ocaml/dune#9515, @rgrinberg) - Use watch exclusions in watch mode on MacOS (ocaml/dune#9643, fixes ocaml/dune#9517, @PoorlyDefinedBehaviour) - Fix merlin configuration for `(include_subdirs qualified)` modules (ocaml/dune#9659, fixes ocaml/dune#8297, @rgrinberg) - Fix handling of `enabled_if` in binary install stanzas. Previously, we'd ignore the result of `enabled_if` when evaluating `%{bin:..}` (ocaml/dune#9707, @rgrinberg)

@nojb

CHANGES: ### Added - Add command `dune cache clear` to completely delete all traces of the Dune cache. (ocaml/dune#8975, @nojb) - Allow to disable Coq 0.8 deprecation warning (ocaml/dune#9439, @ejgallego) - Allow `OCAMLFIND_TOOLCHAIN` to be set per context in the workspace file through the `env` stanza. (ocaml/dune#9449, @rgrinberg) - Menhir: generate `.conflicts` file by default. Add new field to the `(menhir)` stanza to control the generation of this file: `(explain <blang expression>)`. Introduce `(menhir (flags ...) (explain ...))` field in the `(env)` stanza, delete `(menhir_flags)` field. All changes are guarded under a new version of the Menhir extension, 3.0. (ocaml/dune#9512, @nojb) - Directory targets can now be cached. (ocaml/dune#9535, @rleshchinskiy) - It is now possible to use special forms such as `(:include)` and variables `%{read-lines:}` in `(modules)` and similar fields. Note that the dependencies introduced in this way (ie the files being read) must live in a different directory than the stanza making use of them. (ocaml/dune#9578, @nojb) - Remove warning 30 from default set for projects where dune lang is at least 3.13 (ocaml/dune#9568, @gasche) - Add `coqdoc_flags` field to `coq` field of `env` stanza allowing the setting of workspace-wide defaults for `coqdoc_flags`. (ocaml/dune#9280, fixes ocaml/dune#9139, @Alizter) - ctypes: fix an error where `(ctypes)` with no `(function_description)` would cause an error trying refer to a nonexistent `_stubs.a` dependency (ocaml/dune#9302, fix ocaml/dune#9300, @emillon) ### Changed - Check that package names in `(depends)` and related fields in `dune-project` are well-formed. (ocaml/dune#9472, fixes ocaml/dune#9270, @ElectreAAS) ### Fixed - Do not ignore `(formatting ..)` settings in context or workspace files (ocaml/dune#8447, @rgrinberg) - Fixed a bug where Dune was incorrectly parsing the output of coqdep when it was escaped, as is the case on Windows. (ocaml/dune#9231, fixes ocaml/dune#9218, @Alizter) - Copying mode for sandboxes will now follow symbolic links (ocaml/dune#9282, @rgrinberg) - Forbid the empty `(binaries ..)` field in the `env` stanza in the workspace file unless language version is at least 3.2. - [coq] Fix bug in computation of flags when composed with boot theories. (ocaml/dune#9347, fixes ocaml/dune#7909, @ejgallego) - Fixed a bug where the `(select)` field of the `(libraries)` field of the `(test)` stanza wasn't working properly. (ocaml/dune#9387, fixes ocaml/dune#9365, @Alizter) - Fix handling of the `PATH` argument to `dune init proj NAME PATH`. An intermediate directory called `NAME` is no longer created if `PATH` is supplied, so `dune init proj my_project .` will now initialize a project in the current working directory. (ocaml/dune#9447, fixes ocaml/dune#9209, @shonfeder) - Experimental doc rules: Correctly handle the case when a package depends upon its own sublibraries (ocaml/dune#9461, fixes ocaml/dune#9456, @jonludlam) - Resolve various public binaries to their build location, rather than to where they're copied in the `_build/install` directory (ocaml/dune#9496, fixes ocaml/dune#7908, @rgrinberg). - Correctly ignore warning flags in vendored projects (ocaml/dune#9515, @rgrinberg) - Use watch exclusions in watch mode on MacOS (ocaml/dune#9643, fixes ocaml/dune#9517, @PoorlyDefinedBehaviour) - Fix merlin configuration for `(include_subdirs qualified)` modules (ocaml/dune#9659, fixes ocaml/dune#8297, @rgrinberg) - Fix handling of `enabled_if` in binary install stanzas. Previously, we'd ignore the result of `enabled_if` when evaluating `%{bin:..}` (ocaml/dune#9707, @rgrinberg)

gasche force-pushed the disable-warning-30 branch from d68d872 to c4e41fb Compare December 26, 2023 21:26

gasche force-pushed the disable-warning-30 branch from c4e41fb to a77cf33 Compare December 26, 2023 21:36

rgrinberg added this to the 3.13.0 milestone Dec 27, 2023

rgrinberg reviewed Dec 27, 2023

View reviewed changes

src/dune_rules/ocaml_flags.ml Outdated Show resolved Hide resolved

gasche force-pushed the disable-warning-30 branch from a77cf33 to 27eeb9c Compare December 27, 2023 23:06

rgrinberg force-pushed the disable-warning-30 branch from 27eeb9c to cfcc396 Compare December 28, 2023 22:27

gasche force-pushed the disable-warning-30 branch 2 times, most recently from 0ebf6ea to 59abf8e Compare January 4, 2024 20:24

rgrinberg force-pushed the disable-warning-30 branch from 59abf8e to bc320e4 Compare January 8, 2024 05:39

rgrinberg force-pushed the disable-warning-30 branch from bc320e4 to 5438881 Compare January 8, 2024 05:52

rgrinberg merged commit 4f274af into ocaml:main Jan 8, 2024

emillon mentioned this pull request Jan 12, 2024

[new release] dune (15 packages) (3.13.0~alpha1) ocaml/opam-repository#25067

Closed

emillon mentioned this pull request Jan 16, 2024

[new release] dune (15 packages) (3.13.0) ocaml/opam-repository#25081

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove warning 30 from the standard set.#9568

Remove warning 30 from the standard set.#9568
rgrinberg merged 1 commit intoocaml:mainfrom
gasche:disable-warning-30

gasche commented Dec 25, 2023

Uh oh!

rgrinberg commented Dec 26, 2023

Uh oh!

gasche commented Dec 26, 2023

Uh oh!

Uh oh!

gasche commented Dec 28, 2023

Uh oh!

rgrinberg commented Dec 28, 2023

Uh oh!

gasche commented Jan 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

gasche commented Dec 25, 2023

Uh oh!

rgrinberg commented Dec 26, 2023

Uh oh!

gasche commented Dec 26, 2023

Uh oh!

Uh oh!

gasche commented Dec 28, 2023

Uh oh!

rgrinberg commented Dec 28, 2023

Uh oh!

gasche commented Jan 8, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants