-
Notifications
You must be signed in to change notification settings - Fork 455
New interpretation of (menhir ...) stanzas. #863
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
Conversation
rgrinberg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Francois, this looks good. A few points to get the PR rolling:
-
Make sure to sign off your commit with
-s. We require a DCO for all contributions to dune: https://github.com/ocaml/dune/blob/master/CONTRIBUTING.md -
You can run the tests with
$ make test. If you're happy with the changes to expect tests, you can accept them with$ make accept-correctionsand commit the result. -
If it's not too much trouble, could you run your code through ocp-indent? I have nothing against your formatting, but I'm afraid it will be hard to keep it up without any kind of formatting tool.
src/menhir.ml
Outdated
| | "--infer" | ||
| | "--infer-write-query" | ||
| | "--infer-read-reply" -> | ||
| Loc.fail stanza.loc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly nicer would be to use a more precise location for the flags. For example, here's what you can do attach a loc to every flag:
diff --git a/src/jbuild.ml b/src/jbuild.ml
index c1717d63..dd9e3a3d 100644
--- a/src/jbuild.ml
+++ b/src/jbuild.ml
@@ -1077,7 +1077,7 @@ end
module Menhir = struct
type t =
{ merge_into : string option
- ; flags : string list
+ ; flags : (Loc.t * string) list
; modules : string list
; mode : Rule.Mode.t
; loc : Loc.t
@@ -1086,7 +1086,7 @@ module Menhir = struct
let v1 =
record
(field_o "merge_into" string >>= fun merge_into ->
- field "flags" (list string) ~default:[] >>= fun flags ->
+ field "flags" (list (located string)) ~default:[] >>= fun flags ->
field "modules" (list string) >>= fun modules ->
Rule.Mode.field >>= fun mode ->
return
diff --git a/src/jbuild.mli b/src/jbuild.mli
index 609ccece..a0a60e98 100644
--- a/src/jbuild.mli
+++ b/src/jbuild.mli
@@ -298,7 +298,7 @@ end
module Menhir : sig
type t =
{ merge_into : string option
- ; flags : string list
+ ; flags : (Loc.t * string) list
; modules : string list
; mode : Rule.Mode.t
; loc : Loc.t
And if that is a bit fine grained, we can consider Loc.t * (string list). But it's really nice to have precise error messages for your editor to jump to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding make test: (updated comment) after I installed all required packages, this command succeeds with just a couple warnings (warning 40) in upstream/master.
I will make sure that my PR does not cause new failures to appear.
src/menhir.ml
Outdated
| List.mem "--only-tokens" ~set:stanza.flags | ||
| in | ||
| rule (menhir args) | ||
| if ocaml_type_inference_disabled then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a bit sad to lose the ability to use Ordered_set_lang.t because of this. Would it make sense for this option to be toggled with a separate field in the stanza if there's no workaround?
The reason why it's nice to have Ordered_set_lang.t btw is that it lets you change the set of flags in a future proof way. Eventually, we'll have the ability to change what :standard will mean for a project and it would be nice if such changes propagated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could revert to Ordered_set_lang.t if you wish.
Then, how would I implement your other suggestion of giving a precise error location when a reserved flag is given by the user? It seems that Super_context.expand_and_eval_set returns an action of type (unit, string list) Build.t which does not give me access to the location of each individual flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest the following: we add a function in Ordered_set_lang.Unexpanded.t to scan the the static strings. Then in menhir we do the following:
- scan the flags for a plain
--only-tokensflag and select the behaviour accordingly - if
--only-tokenswasn't present, scan the flags after they are fully evaluated and error out if--only-tokensis present
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds good. It should still allow catching the cases where people pass incorrect static flags, and would still allow using your expansion mechanism inside flags.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, this behavior seems good to me. Supporting Ordered_ste_lang is now even more important as it's going to be a mechanism for controlling bulk builds in conjunction with (env ..)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
master now has Ordered_set_lang.Unexpanded.fold_strings and String_with_vars.text_only which should allow implementing this.
|
Oh and 1 more thing, this PR will also require update CHANGES and the manual. But that doesn't have to come right away. I'm also thinking that we should give the menhir support its own page in the manual page. Our manual is written in a markdown like markup called rst and you can preview the manual locally by installing sphinx. The manual is updated on every commit and is available here: http://dune.readthedocs.io/. Don't worry about the formatting too much if you're unfamiliar with rst, we can fix that up. |
|
@fpottier okay now if you rebase this on master, you will have menhir.rst where you can update the docs. |
|
Hey there. I would love to see that happening as the current state of Dune prevents me from using it in many of my projects that require Menhir! |
|
Note: for merlin we use the I'd be happy to make a PR for that once this one gets merged, but as I suspect it's just a few more lines of code it could probably be directly added to this PR. |
|
@fpottier would you mind signing your commit here? I'd like to push some of the improvements we discussed to this branch, but that would make signing the commit for you later harder. |
|
Sorry, what exactly should I do? |
|
According to https://github.com/ocaml/dune/blob/master/CONTRIBUTING.md, every dev must sign their commits for them to be considered for inclusion. This is done by creating the commit with Enjoy your vacation! |
|
OK, I have signed my commit (I think). I haven't changed the code to use There are more small improvements that I could make in the future. |
Thanks, it is signed.
I will take care of it.
You might as well just create a |
I would put this in a comment in src/menhir.ml directly. The closer it is to the actual source code, the more likely it is to stay in sync. |
|
@fpottier I've made the adjustment to use the ordered set language. We might still want to consider using an explicit field to control this however. Inferring user intent from flags isn't the most reliable thing and it's now how we want to handle things with opaque for example. I don't have such a strong opinion about this however |
|
I can't reproduce the syntax error in 4.06.1, but I can reproduce it in 4.05.0. Here's the offending line: The |
|
For reference, this is what menhir generates for 4.06.1: and this is indeed valid. |
|
@rgrinberg: regarding the syntax error in your last two messages, how is it produced? These two messages look as though they belong in some other thread (which I would be interested to read, if it concerns a bug in Menhir). |
|
@fpottier yeah, it certainly doesn't look like it's related to this PR. You can reproduce the bug if you have opam2 to install the deps: |
|
@rgrinberg isn't the error introduced by this PR? The compilation of doc-ock-xml in travis seems to work fine with master. |
|
Well the new rules are a bit different now. We're now using |
|
We are also reading the output of |
|
I mean menhir is reading it. I don't know what menhir is doing with it though |
|
@fpottier does menhir parses the output of |
|
Menhir reads the output of |
|
I haven't checked, but that would explain the issue. @rgrinberg could you check the contents of the inferred mli file generated by the menhir rules with OCaml 4.05? |
|
Here's the output ( |
|
And here's the diff in the file above between 4.05 and 4.06. Clearly shows the incorrect source produced by 4.05: |
|
I suggest that we simply disable the infer mode when OCaml <= 4.05. |
|
@diml: this seems safe, but perhaps a bit drastic? Some features of Menhir, such as the inspection API, require type inference to be turned on. (That said, it is still possible to provide every type by hand using Another approach would be to add an option inside a |
|
No problem |
dune.opam:
Add a constraint that Menhir (if present) must be >= 20180523.
src/jbuild.{ml,mli}:
Change the type of the flags in a (menhir ... stanza)
from [Ordered_set_lang.Unexpanded.t] to [string list].
This makes it possible for the build rules to depend
on the presence of certain flags (such as --only-tokens).
src/menhir.ml:
Update the build rules to take advantage of the commands
[--infer-write-query] and [--infer-read-reply] offered by Menhir.
Signed-off-by: François Pottier <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
latest releasd versions of menhir and reason aren't compatible Signed-off-by: Rudi Grinberg <[email protected]>
New directories will test other features Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
Rename Loc.fail to Errors.fail Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
Infer is disabled by default and can be re-enabled only in syntax (2, 0) of menhir Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
|
I don't quite follow what is going on, but I think |
|
@fpottier agreed. Changing the default of the
In both cases, users will be able to add |
|
Okay. |
Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
Signed-off-by: Rudi Grinberg <[email protected]>
CHANGES: - Do not fail if the output of `ocamlc -config` doesn't include `standard_runtime` (ocaml/dune#1326, @diml) - Let `Configurator.V1.C_define.import` handle negative integers (ocaml/dune#1334, @Chris00) - Re-execute actions when a target is modified by the user inside `_build` (ocaml/dune#1343, fix ocaml/dune#1342, @diml) - Pass `--set-switch` to opam (ocaml/dune#1341, fix ocaml/dune#1337, @diml) - Fix bad interaction between multi-directory libraries the `menhir` stanza (ocaml/dune#1373, fix ocaml/dune#1372, @diml) - Integration with automatic formatters (ocaml/dune#1252, fix ocaml/dune#1201, @emillon) - Better error message when using `(self_build_stubs_archive ...)` and `(c_names ...)` or `(cxx_names ...)` simultaneously. (ocaml/dune#1375, fix ocaml/dune#1306, @nojb) - Improve name detection for packages when the prefix isn't an actual package (ocaml/dune#1361, fix ocaml/dune#1360, @rgrinberg) - Support for new menhir rules (ocaml/dune#863, fix ocaml/dune#305, @fpottier, @rgrinberg) - Do not remove flags when compiling compatibility modules for wrapped mode (ocaml/dune#1382, fix ocaml/dune#1364, @rgrinberg) - Fix reason support when using `staged_pps` (ocaml/dune#1384, @charlesetc) - Add support for `enabled_if` in `rule`, `menhir`, `ocamllex`, `ocamlyacc` (ocaml/dune#1387, @diml) - Exit gracefully when a signal is received (ocaml/dune#1366, @diml) - Load all defined libraries recursively into utop (ocaml/dune#1384, fix ocaml/dune#1344, @rgrinberg) - Allow to use libraries `bytes`, `result` and `uchar` without `findlib` installed (ocaml/dune#1391, @nojb)
CHANGES: - Do not fail if the output of `ocamlc -config` doesn't include `standard_runtime` (ocaml/dune#1326, @diml) - Let `Configurator.V1.C_define.import` handle negative integers (ocaml/dune#1334, @Chris00) - Re-execute actions when a target is modified by the user inside `_build` (ocaml/dune#1343, fix ocaml/dune#1342, @diml) - Pass `--set-switch` to opam (ocaml/dune#1341, fix ocaml/dune#1337, @diml) - Fix bad interaction between multi-directory libraries the `menhir` stanza (ocaml/dune#1373, fix ocaml/dune#1372, @diml) - Integration with automatic formatters (ocaml/dune#1252, fix ocaml/dune#1201, @emillon) - Better error message when using `(self_build_stubs_archive ...)` and `(c_names ...)` or `(cxx_names ...)` simultaneously. (ocaml/dune#1375, fix ocaml/dune#1306, @nojb) - Improve name detection for packages when the prefix isn't an actual package (ocaml/dune#1361, fix ocaml/dune#1360, @rgrinberg) - Support for new menhir rules (ocaml/dune#863, fix ocaml/dune#305, @fpottier, @rgrinberg) - Do not remove flags when compiling compatibility modules for wrapped mode (ocaml/dune#1382, fix ocaml/dune#1364, @rgrinberg) - Fix reason support when using `staged_pps` (ocaml/dune#1384, @charlesetc) - Add support for `enabled_if` in `rule`, `menhir`, `ocamllex`, `ocamlyacc` (ocaml/dune#1387, @diml) - Exit gracefully when a signal is received (ocaml/dune#1366, @diml) - Load all defined libraries recursively into utop (ocaml/dune#1384, fix ocaml/dune#1344, @rgrinberg) - Allow to use libraries `bytes`, `result` and `uchar` without `findlib` installed (ocaml/dune#1391, @nojb) - Take argument to self_build_stubs_archive into account. (ocaml/dune#1395, @nojb) - Fix bad interaction between `env` customization and vendored projects: when a vendored project didn't have its own `env` stanza, the `env` stanza from the enclosing project was in effect (ocaml/dune#1408, @diml) - Fix stop early bug when scanning for watermarks (ocaml/dune#1423, @diml)
dune.opam:
Add a constraint that Menhir (if present) must be >= 20180523.
src/jbuild.{ml,mli}:
Change the type of the flags in a (menhir ... stanza)
from [Ordered_set_lang.Unexpanded.t] to [string list].
This makes it possible for the build rules to depend
on the presence of certain flags (such as --only-tokens).
src/menhir.ml:
Update the build rules to take advantage of the commands
[--infer-write-query] and [--infer-read-reply] offered by Menhir.