ocaml / dune

A composable build system for OCaml.
https://dune.build/
MIT License
1.64k stars 409 forks source link

`dune fmt` unexpectedly triggers menhir rules #7454

Open voodoos opened 1 year ago

voodoos commented 1 year ago

Expected Behavior

I would expect dune fmt or dune build @fmt to never build anything. (This is an assumption made by OCaml-CI for the lint-fmt job. cc @maiste)

Actual Behavior

Menhir parser that have a corresponding .mli file are generated when calling dune build @fmt

Reproduction

It was a bit subtle to reproduce and minimize the issue we encountered in Merlin. I made a PR with what I think is a fairly minimal reproduction: https://github.com/ocaml/dune/pull/7455

Specifications

voodoos commented 1 year ago

I investigated a bit and I think the issue lies not in the formatting rules but in the menhir rules.

The formatting rules do only work for files in the source tree:

image

https://github.com/ocaml/dune/blob/main/src/dune_rules/format_rules.ml#L107-L108

But in the test, if I show the rules for the mli file that is in the source-tree:

image

It says that the file is generated by Menhir, not that is is simply in the source tree. I think this is why dune runs menhir when trying to format the mli files: it is in the source tree so it is formatted, but a rule says that is generated thus it is generated along with the ml file.

The target configuration in menhir:

image

https://github.com/ocaml/dune/blob/main/src/dune_rules/menhir/menhir_rules.ml#L77-L80

If I remove the mli from the targets the test pass and the rules for the mli files are correct:

image

Of course this is not an acceptable patch, we should conditionally have that mli file as target if it is not already in the source tree. Any ideas how I could do that ?

cc @rgrinberg @anmonteiro @emillon who intervened in the reproduction PR

voodoos commented 1 year ago

In fact, it is not linked to Menhir either. The issue appear when a promoted rule have (at least) two targets and only one of them is in the source-tree. In normal function it is expected that both files are re-generated, but when formatting we don't want that to happen.

Here is a simpler reproduction:

See issue 7454

  $ cat >dune <<EOF
  > (library 
  >  (name foo)
  >  (modules other_gen))
  > 
  > (rule
  >  (targets other_gen.ml other_gen.mli)
  >  (mode promote)
  >  (action 
  >   (progn
  >    (with-stdout-to other_gen.ml
  >          (echo "let x = 42"))
  >    (with-stdout-to other_gen.mli
  >          (echo "val x : int")))))
  > EOF

  $ cat >dune-project <<EOF
  > (lang dune 2.9)
  > (formatting (enabled_for ocaml))
  > EOF

  $ touch .ocamlformat 

In the above project, one module have is generated: 
- [Other_gen]'s impl and signature are generated by a rule stanza

Formatting the codebase should not trigger the generation of missing modules
  $ dune build @fmt

As expected, Dune did not try to generate the missing implem for [Other_gen]
  $ dune_cmd exists _build/default/other_gen.ml
  false

  $ dune clean

Now we add [mli] files for the module whose implementation is generated:

  $ touch other_gen.mli

We format again.
  $ dune build @fmt
  File "other_gen.mli", line 1, characters 0-0:
  Error: Files _build/default/other_gen.mli and
  _build/default/.formatted/other_gen.mli differ.
  [1]

FIXME: unexpectedly, Dune generated the missing implem. for [Other_gen]
  $ dune_cmd exists _build/default/other_gen.ml
  true
rgrinberg commented 1 year ago

Okay, so the generated parser is promoted. Okay, then about improving our source file check to take promotion into account. In particular, we don't setup formatting rules for any source files that has a promotion rule.