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

jbuilder doesn't support menhir with --infer #305

Closed
vsiles opened this issue Nov 3, 2017 · 7 comments · Fixed by ocaml/opam-repository#12774
Closed

jbuilder doesn't support menhir with --infer #305

vsiles opened this issue Nov 3, 2017 · 7 comments · Fixed by ocaml/opam-repository#12774

Comments

@vsiles
Copy link
Contributor

vsiles commented Nov 3, 2017

Hi ! I'm trying to move from ocamlbuild to jbuilder, but I can't invoke menhir with the --infer flag:

$ jbuilder build foobar.exe
      menhir parser.{ml,mli} (exit 1)
(cd _build/default && .../menhir --infer --explain parser.mly)
File "parser.mly", line 3, characters 13-19
Error: Unbound module Syntax

The following configuration

(menhir
 ((flags (--explain))
  (modules (parser))))

(ocamllex (lexer))

(executable
    ((name foobar)
     (libraries (core))))

works but

(menhir
 ((flags (--infer --explain))
  (modules (parser))))

(ocamllex (lexer))
(executable
    ((name foobar)
     (libraries (core))))

doesn't.

The issue is described in menhir manual: by using --infer, some calls to ocamlc are done to typecheck the file, so some modules have to be compiled before the parser. It is advised to use --depend to get the correct dependencies.

For the record, my parser.mly starts with:

%{
  open Lexing
  module S = Syntax
%}

and I use S.Fobaar in the mly file (see here)
I'm using jbuilder 1.0+beta14 on ubuntu 16.04.3

@ghost ghost mentioned this issue Jan 23, 2018
@ghost
Copy link

ghost commented Jan 23, 2018

We need to change a few things in jbuilder to support that. But before we look into this, I'd just like to understand better why menhir is calling the compiler. IIUC, we get better error locations when using the --infer flag, but for instance we get the right locations as well when using ocamlyacc or a ppx rewriter without calling the compiler. /cc @fpottier

@fpottier
Copy link
Collaborator

Hello! The initial motivation for menhir --infer was indeed to typecheck the semantic actions in isolation, so as to prevent the OCaml typechecker from reporting type errors at random places in the generated code. Then, later on, the so-called inspection API was added. When --inspection is turned on, Menhir generates additional code, which (among other things) includes a GADT of all nonterminal symbols indexed with their OCaml type. This requires the OCaml type of every symbol to be known to Menhir. The simplest way to achieve this is to use --infer, but it is also possible to manually annotate every symbol with an explicit %type declaration.

The reason why Menhir directly invokes ocamlc is that this was deemed simpler. One could conceivably instead imagine a scheme where Menhir is invoked in two stages; the first stage dumps an .ml file; then ocamlc -i is invoked (by the build system); then Menhir is invoked again and reads the types inferred by the OCaml compiler. This was suggested already by Fabrice Le Fessant but has not been implemented, by lack of demand. If there are strong technical arguments why that would be a good idea, I could look into it.

@ghost
Copy link

ghost commented Jan 23, 2018

Thanks for the explanations! I understand better why we need this now. We'll add this to the TODO list.

The reason why Menhir directly invokes ocamlc is that this was deemed simpler. One could conceivably instead imagine a scheme where Menhir is invoked in two stages; the first stage dumps an .ml file; then ocamlc -i is invoked (by the build system); then Menhir is invoked again and reads the types inferred by the OCaml compiler. This was suggested already by Fabrice Le Fessant but has not been implemented, by lack of demand. If there are strong technical arguments why that would be a good idea, I could look into it.

The only advantage I can think of is that we would avoid quoting issues since we wouldn't have to construct a shell command for --ocamlc/--ocamldep. Apart from that it's the same complexity from the point of view of jbuilder, so I don't think it's worth changing menhir.

@ghost ghost mentioned this issue Feb 5, 2018
@vsiles
Copy link
Contributor Author

vsiles commented Mar 6, 2018

Today, my opam updated to jbuilder.1.0+beta18 where the merge is located (if I'm correct), so I tried to see if the issue was addressed. My project is located here, and here is a copy of my jbuild file:

(menhir
 ((flags (--explain --infer))
  (modules (parser))))

(ocamllex (lexer))


(executable
    ((name tigercc)
     (modules_without_implementation (frame)) 
     (libraries (core))))

Upon executing jbuilder build tigercc.exe in the src directory, I get:

      menhir parser.{ml,mli} (exit 1)
(cd _build/default && /home/vsiles/.opam/system/bin/menhir --explain --infer parser.mly)

Did I forgot something to trigger the dependency computation ?

@ghost
Copy link

ghost commented Mar 6, 2018

--infer is not yet supported. We only did some preliminary changes in order to be able to support it

@vsiles
Copy link
Contributor Author

vsiles commented Mar 6, 2018

Ok ! Great to here you're working on it :)

rgrinberg added a commit to rgrinberg/opam-repository that referenced this issue Oct 4, 2018
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)
@rgrinberg
Copy link
Member

This is now in master and will be available in 1.4.0 with (using menhir 2.0). Please open new tickets if there are issues with this feature.

rgrinberg added a commit to rgrinberg/opam-repository that referenced this issue Oct 10, 2018
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants