fix(opam-create): only upgrade existing menhir deps; do not add new

Fixes #14428.

The 3.23 menhir auto-injection (#14168) was triggered by the
project-wide `(using menhir ...)` extension and added `menhir` as a
new dependency to every package's generated opam file, regardless of
whether the package's own `(depends ...)` field declared it. In
multi-package projects where only some packages used menhir, every
package gained a spurious build-time dependency.

The original ask in #10707 was narrower: when a generated opam file
*already* lists `menhir`, fill in the lower bound `{>= "20180523"}`
that dune's menhir rules require. Restrict the behaviour to that.

- A package that does not declare `(depends menhir)` no longer has
  `menhir` added by dune.
- A package that declares bare `(depends menhir)` has the lower
  bound filled in, as before.
- A package whose `(depends ...)` already specifies a constraint
  (version bound, `{with-test}`, etc.) is preserved verbatim.

The convenience of auto-adding `menhir` for users who use a
`(menhir ...)` stanza but forget to declare `(depends menhir)` is
deliberately removed: the dependency-inference logic that supported
it was the source of #14428 and out of scope for #10707.

Signed-off-by: Robin Bate Boerop <me@robinbb.com>

Author: robinbb

doc/changes/fixed/14434.md

@@ -0,0 +1,9 @@
+- Fix the `menhir` opam dependency injection introduced in 3.23. Dune
+  now only fills in the lower bound `{>= "20180523"}` on an existing
+  user-declared `menhir` dependency; it no longer adds `menhir` as a
+  new dependency to packages that did not declare it themselves. The
+  3.23 behaviour, which auto-injected `menhir` into every package's
+  generated opam file whenever `(using menhir ...)` was enabled,
+  added spurious build-time dependencies in multi-package projects
+  where only some packages used menhir. (#14434, fixes #14428,
+  @robinbb)

src/dune_rules/opam_create.ml

@@ -300,22 +300,19 @@ let insert_odoc_dep depends =
    which dune's menhir rules rely on unconditionally. *)
 let menhir_constraint : Package_constraint.t = Uop (Gte, String_literal "20180523")
 
-let insert_menhir_dep depends =
-  let menhir_dep =
-    { Package_dependency.name = menhir_name; constraint_ = Some menhir_constraint }
-  in
-  if
-    List.exists depends ~f:(fun (dep : Package_dependency.t) ->
-      Package.Name.equal dep.name menhir_name)
-  then
-    List.map depends ~f:(fun (dep : Package_dependency.t) ->
-      if Package.Name.equal dep.name menhir_name
-      then
-        { dep with
-          constraint_ = Some (Option.value dep.constraint_ ~default:menhir_constraint)
-        }
-      else dep)
-  else depends @ [ menhir_dep ]
+(* If the package's [(depends ...)] field already lists [menhir]
+   without a constraint, fill in the lower bound. Existing user-
+   written constraints (whether version bounds, [{with-test}], or
+   anything else) are preserved verbatim. We do not add menhir as a
+   new dependency: that was the over-injection class fixed by #14428. *)
+let upgrade_menhir_constraint depends =
+  List.map depends ~f:(fun (dep : Package_dependency.t) ->
+    if Package.Name.equal dep.name menhir_name
+    then
+      { dep with
+        constraint_ = Some (Option.value dep.constraint_ ~default:menhir_constraint)
+      }
+    else dep)
 ;;
 
 let maintenance_intent dune_version info =
@@ -344,7 +341,7 @@ let opam_fields project (package : Package.t) =
   let package =
     match Dune_project.find_extension_version project Dune_lang.Menhir.syntax with
     | None -> package
-    | Some _ -> Package.map_depends package ~f:insert_menhir_dep
+    | Some _ -> Package.map_depends package ~f:upgrade_menhir_constraint
   in
   let package_fields = package_fields package ~project in
   let open Opam_file.Create in

test/blackbox-tests/test-cases/menhir/opam-menhir-dep.t

@@ -1,55 +1,57 @@
-When a project uses (using menhir ...), the generated opam file should include
-a lower bound on the menhir version, since dune assumes menhir supports
---infer-write-query and --infer-read-reply (available since menhir 20180523).
+When a package's [(depends ...)] field lists [menhir] without a
+version constraint, the generated opam file should fill in the
+lower bound [{>= "20180523"}], since dune's menhir rules rely on
+features available since menhir 20180523.
 
-See https://github.com/ocaml/dune/issues/10707
+See https://github.com/ocaml/dune/issues/10707.
 
-Case 1: menhir used, no explicit menhir dependency declared.
-The generated opam file should include "menhir" {>= "20180523"}.
+Case 0: package does not declare menhir. Dune does not add it.
 
   $ cat > dune-project << EOF
-  > (lang dune 3.23)
+  > (lang dune 3.24)
   > (using menhir 2.1)
   > (generate_opam_files true)
   > (package (name foo) (allow_empty))
   > EOF
 
-  $ dune build @opam --auto-promote > /dev/null 2>&1 || true
+  $ dune build @opam --auto-promote > /dev/null 2>&1
+  [1]
+  $ dune build @opam
   $ grep menhir foo.opam
-    "menhir" {>= "20180523"}
+  [1]
 
-Case 2: user already declared menhir with a sufficient lower bound.
-The auto-injected constraint should not duplicate it.
+Case 1: bare [(depends menhir)]. Dune fills in the lower bound.
 
   $ cat > dune-project << EOF
-  > (lang dune 3.23)
+  > (lang dune 3.24)
   > (using menhir 2.1)
   > (generate_opam_files true)
   > (package
   >  (name foo)
   >  (allow_empty)
-  >  (depends (menhir (>= 20211128))))
+  >  (depends menhir))
   > EOF
 
   $ dune build @opam --auto-promote > /dev/null 2>&1
   [1]
+  $ dune build @opam
   $ grep menhir foo.opam
-    "menhir" {>= "20211128"}
+    "menhir" {>= "20180523"}
 
-Case 3: user declared menhir with no version constraint.
-The auto-injected lower bound should be merged in.
+Case 2: user-written version bound is preserved verbatim.
 
   $ cat > dune-project << EOF
-  > (lang dune 3.23)
+  > (lang dune 3.24)
   > (using menhir 2.1)
   > (generate_opam_files true)
   > (package
   >  (name foo)
   >  (allow_empty)
-  >  (depends menhir))
+  >  (depends (menhir (>= 20211128))))
   > EOF
 
   $ dune build @opam --auto-promote > /dev/null 2>&1
   [1]
+  $ dune build @opam
   $ grep menhir foo.opam
-    "menhir" {>= "20180523"}
+    "menhir" {>= "20211128"}