fix: only fill lower bound on existing menhir deps; do not auto-add

doc/changes/fixed/14434.md

@@ -0,0 +1,5 @@
+- 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.
+  (#14434, fixes #14428, @robinbb)

src/dune_rules/opam_create.ml

@@ -300,22 +300,20 @@ 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: doing so unconditionally is the over-injection
+   bug reported in #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 +342,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,28 +1,47 @@
-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
+  [1]
+
+Case 1: bare [(depends menhir)]. Dune fills in the lower bound.
+
+  $ cat > dune-project << EOF
+  > (lang dune 3.24)
+  > (using menhir 2.1)
+  > (generate_opam_files true)
+  > (package
+  >  (name foo)
+  >  (allow_empty)
+  >  (depends menhir))
+  > EOF
+
+  $ dune build @opam --auto-promote > /dev/null 2>&1
+  [1]
+  $ dune build @opam
   $ grep menhir foo.opam
     "menhir" {>= "20180523"}
 
-Case 2: user already declared menhir with a sufficient lower bound.
-The auto-injected constraint should not duplicate it.
+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
@@ -33,23 +52,73 @@ The auto-injected constraint should not duplicate it.
 
   $ dune build @opam --auto-promote > /dev/null 2>&1
   [1]
+  $ dune build @opam
   $ grep menhir foo.opam
     "menhir" {>= "20211128"}
 
-Case 3: user declared menhir with no version constraint.
-The auto-injected lower bound should be merged in.
+Case 3: gate on [(using menhir ...)]. Without the menhir extension
+enabled, dune does not run menhir's rules and so must not impose
+the lower bound on a user-declared menhir dependency that exists
+for an unrelated reason (e.g. runtime).
+
+  $ cat > dune-project << EOF
+  > (lang dune 3.24)
+  > (generate_opam_files true)
+  > (package
+  >  (name foo)
+  >  (allow_empty)
+  >  (depends menhir))
+  > EOF
+
+  $ dune build @opam --auto-promote > /dev/null 2>&1
+  [1]
+  $ dune build @opam
+  $ grep menhir foo.opam
+    "menhir"
+
+Case 4: multi-package regression for #14428. Package [foo] declares
+[(depends menhir)]; package [bar] declares no menhir dep. The
+generated opam files must reflect this: [foo.opam] gets the lower
+bound; [bar.opam] has no [menhir] line at all.
 
   $ 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))
+  > (package (name bar) (allow_empty))
   > EOF
 
   $ dune build @opam --auto-promote > /dev/null 2>&1
   [1]
+  $ dune build @opam
   $ grep menhir foo.opam
     "menhir" {>= "20180523"}
+  $ test -f bar.opam && grep -c '^opam-version' bar.opam
+  1
+  $ grep menhir bar.opam
+  [1]
+
+Case 5: a [{with-test}] filter on the menhir dep is a non-version
+constraint and is preserved verbatim — the lower bound is not
+combined with it.
+
+  $ rm -f bar.opam
+  $ cat > dune-project << EOF
+  > (lang dune 3.24)
+  > (using menhir 2.1)
+  > (generate_opam_files true)
+  > (package
+  >  (name foo)
+  >  (allow_empty)
+  >  (depends (menhir :with-test)))
+  > EOF
+
+  $ dune build @opam --auto-promote > /dev/null 2>&1
+  [1]
+  $ dune build @opam
+  $ grep menhir foo.opam
+    "menhir" {with-test}