Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add (explain) field to (menhir) stanza #9512

Merged
merged 20 commits into from
Dec 17, 2023
Merged

Conversation

nojb
Copy link
Collaborator

@nojb nojb commented Dec 15, 2023

A simple-minded attempt at resolving #6865: add an (explain) field to the (menhir) stanza.

Fixes #6865

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb nojb mentioned this pull request Dec 15, 2023
@gasche
Copy link
Member

gasche commented Dec 15, 2023

Nice!

It would be nice to also update the documentation.

Would there be a way for users to access the .conflicts file that does not involve foraging into the build artifact directory? (For example, is it the case that dune exec -- less foo/Parser.conflicts would work?)

@nojb
Copy link
Collaborator Author

nojb commented Dec 15, 2023

It would be nice to also update the documentation.

Yes, will do.

For example, is it the case that dune exec -- less foo/Parser.conflicts would work?

This seems to work.

Would there be a way for users to access the .conflicts file that does not involve foraging into the build artifact directory?

One can also add (mode (promote (until-clean))) in the (menhir) stanza to "promote" the .conflicts file to the current directory, but this will also promote the generated .ml/.mli files (Dune does not appear to be able to promote only a subset of the generated files by a rule at the moment).

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb
Copy link
Collaborator Author

nojb commented Dec 15, 2023

I pushed another commit 0b46891 detecting the --explain flag in the (flags) field. I'm in two minds whether this one is better than (explain).

@gasche
Copy link
Member

gasche commented Dec 15, 2023

I don't like the idea of seeing the generated .ml and .mli in my build directory. For the time being I would rather document dune exec -- less.

@gasche
Copy link
Member

gasche commented Dec 15, 2023

I wonder whether explain should be the default. This sounds like a very convenient thing to have, and I don't see any immediate cost -- presumably this has a performance cost, but this is neglectible for not-too-large grammars.

Does your current implementation support (explain false) to disable the option (should be a no-op for now)? Having this working today would be convenient if (explain) becomes the default now or in the future.

cc @fpottier: do you see an obvious issue with Dune passing --explain by default to Menhir? (Are there realistic use-cases where this is problematic?)

@ejgallego
Copy link
Collaborator

Explain being the default would make sense, note however the case where the user does something like (flags :standard \ --explain) should be either documented as not supported, or supported.

For the Coq mode, understanding flags would have been very convenient, but there were too many corner cases in the end.

This reverts commit 0b46891.

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb
Copy link
Collaborator Author

nojb commented Dec 15, 2023

I wonder whether explain should be the default. This sounds like a very convenient thing to have, and I don't see any immediate cost -- presumably this has a performance cost, but this is neglectible for not-too-large grammars.

I think this makes sense; I switched the default (but we can switch back if there are good reasons to do so).

Does your current implementation support (explain false) to disable the option (should be a no-op for now)? Having this working today would be convenient if (explain) becomes the default now or in the future.

This is now supported.

understanding flags would have been very convenient, but there were too many corner cases in the end.

I reverted the auto-detection of the --explain flag; too many corner cases and the explicit (explain <bool>) (especially if the default is true) seems simpler and clearer.

@rgrinberg
Copy link
Member

Dune does not appear to be able to promote only a subset of the generated files by a rule at the moment

There's actually (promote (only ..)). Although I don't know whether it's good to use this in the end. Also, I guess it can't be combined with until-clean?

Would there be a way for users to access the .conflicts file that does not involve foraging into the build artifact directory? (For example, is it the case that dune exec -- less foo/Parser.conflicts would work?)

We can also add a specialized dune subcommand for menhir. I don't know if using dune exec is all that obvious.

A couple of improvements I would like to suggest:

  1. We should add a warning or error if the user passes --explain manually and point them to this feature (only when the newer version is enabled)
  2. We should allow blang expressions inside (explain ..) to allow conditionally enabling this feature. It can always be added later, but it creates complications due to versioning.

{ merge_into; flags; modules; mode; loc; infer; enabled_if })
let explain =
match explain with
| None -> dune_syntax >= (3, 13)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should use the version of the menhir syntax to control this behavior.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, done.

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb
Copy link
Collaborator Author

nojb commented Dec 15, 2023

2. We should allow blang expressions inside (explain ..) to allow conditionally enabling this feature. It can always be added later, but it creates complications due to versioning.

This should now be working.

@rgrinberg rgrinberg added this to the 3.13.0 milestone Dec 15, 2023
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb
Copy link
Collaborator Author

nojb commented Dec 15, 2023

  1. We should add a warning or error if the user passes --explain manually and point them to this feature (only when the newer version is enabled)

I pushed something along these lines, let me know if it looks OK.

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb
Copy link
Collaborator Author

nojb commented Dec 15, 2023

@rgrinberg just to confirm before this gets merged: do you think it is OK to switch the default to generate the .conflicts file?

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@rgrinberg
Copy link
Member

If the change is guarded behind a version flag, I see no issues from the rules side. I don't know enough about menhir to know whether this is a good idea though. I'll trust Gabriel's opinion on this one.

@nojb nojb force-pushed the menhir_explain branch 2 times, most recently from e1090b1 to f28c50e Compare December 16, 2023 13:57
@nojb
Copy link
Collaborator Author

nojb commented Dec 16, 2023

I pushed one more commit that:

  • Adds a new (menhir (flags ...) (explain ...)) field to the (env) stanza allowing to set the new (explain) setting using (env) as well.
  • Removes the old (menhir_flags ...) field of (env) (as it is replaced by (menhir (flags ...))).

The CRAM test shows these features in action.

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@Alizter
Copy link
Collaborator

Alizter commented Dec 16, 2023

@nojb could you test that this works with a dune-workspace file?

@rgrinberg
Copy link
Member

There's no need for such a test, there's already one that demonstrates the menhir flags cannot be set in the workspace file.

src/dune_lang/blang.ml Outdated Show resolved Hide resolved
@rgrinberg
Copy link
Member

I pushed one more commit that:

  • Adds a new (menhir (flags ...) (explain ...)) field to the (env) stanza allowing to set the new (explain) setting using (env) as well.
  • Removes the old (menhir_flags ...) field of (env) (as it is replaced by (menhir (flags ...))).

The CRAM test shows these features in action.

The addition looks good. Given that the new version of the menhir syntax isn't strictly backward compatible, how about bumping it to (3, 0)?

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb
Copy link
Collaborator Author

nojb commented Dec 16, 2023

The addition looks good. Given that the new version of the menhir syntax isn't strictly backward compatible, how about bumping it to (3, 0)?

Done, thanks.

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb
Copy link
Collaborator Author

nojb commented Dec 16, 2023

(This is ready from my side.)

@rgrinberg
Copy link
Member

This is ready from my end as well.

@rgrinberg
Copy link
Member

Will take the liberty to merge this one to avoid a merge conflict with some of my own work. Thanks @nojb!

@rgrinberg rgrinberg merged commit 5088f61 into ocaml:main Dec 17, 2023
27 checks passed
@nojb nojb deleted the menhir_explain branch December 17, 2023 09:14
@nojb
Copy link
Collaborator Author

nojb commented Dec 17, 2023

Thanks!

@gasche
Copy link
Member

gasche commented Dec 17, 2023

Thanks as well!

emillon added a commit to emillon/opam-repository that referenced this pull request Jan 12, 2024
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)
emillon added a commit to emillon/opam-repository that referenced this pull request Jan 16, 2024
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)
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using menhir --explain
5 participants