fix: AND user-written menhir constraint with dune's required lower bound

doc/changes/changed/menhir-merge-bound.md

@@ -0,0 +1,9 @@
+- When generating opam files for `(lang dune 3.23)` or newer, AND
+  the user-written menhir dep constraint with the lower bound
+  (`>= "20180523"`) that dune's menhir rules require. Take the
+  higher of two `>=` literals (warn when the user's bound is below
+  the minimum); combine with formula operators like `:with-test`.
+  The 3.23 gate also covers the bare-`(menhir)` fill from #14434
+  (previously unversioned), mirroring the fix in #14466 to avoid
+  silent changes to opam output on dune-binary upgrade. Follows up
+  #14434. (@robinbb)

src/dune_rules/opam_create.ml

@@ -300,20 +300,48 @@ let insert_odoc_dep depends =
 
 (* Menhir 20180523 added --infer-write-query and --infer-read-reply,
    which dune's menhir rules rely on unconditionally. *)
-let menhir_constraint : Package_constraint.t = Uop (Gte, String_literal "20180523")
+let menhir_min_version = "20180523"
+let menhir_constraint : Package_constraint.t = Uop (Gte, String_literal menhir_min_version)
 
-(* 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
+(* Merge an existing user-written menhir constraint with dune's
+   required lower bound. Mirrors [merge_dune_constraints]:
+   - two [>= "v"] literals collapse to the higher of the two
+     (warn when the user's bound is below dune's required minimum);
+   - any other shape (e.g. [{with-test}]) is ANDed with the lower
+     bound. *)
+let merge_menhir_constraint user_constraint =
+  match user_constraint with
+  | Package_constraint.Uop (Gte, String_literal user_v) ->
+    if OpamVersionCompare.compare user_v menhir_min_version >= 0
+    then user_constraint
+    else (
+      User_warning.emit
+        [ Pp.textf
+            "The lower bound >= %s on menhir in the depends field is less than the \
+             version dune's menhir rules require. The generated opam file will use >= %s \
+             instead."
+            user_v
+            menhir_min_version
+        ];
+      menhir_constraint)
+  | _ -> And [ menhir_constraint; user_constraint ]
+;;
+
+(* If the package's [(depends ...)] field already lists [menhir],
+   fill in the lower bound — combining it with any user-written
+   constraint via [merge_menhir_constraint]. 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)
+        constraint_ =
+          Some
+            (match dep.constraint_ with
+             | None -> menhir_constraint
+             | Some user_constraint -> merge_menhir_constraint user_constraint)
       }
     else dep)
 ;;
@@ -342,9 +370,12 @@ let opam_fields project (package : Package.t) =
     else Package.map_depends package ~f:insert_odoc_dep
   in
   let package =
-    match Dune_project.find_extension_version project Dune_lang.Menhir.syntax with
-    | None -> package
-    | Some _ -> Package.map_depends package ~f:upgrade_menhir_constraint
+    if dune_version < (3, 23)
+    then package
+    else (
+      match Dune_project.find_extension_version project Dune_lang.Menhir.syntax with
+      | None -> package
+      | 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

@@ -8,7 +8,7 @@ See https://github.com/ocaml/dune/issues/10707.
 Case 0: package does not declare menhir. Dune does not add it.
 
   $ cat > dune-project << EOF
-  > (lang dune 3.24)
+  > (lang dune 3.23)
   > (using menhir 2.1)
   > (generate_opam_files true)
   > (package (name foo) (allow_empty))
@@ -22,7 +22,7 @@ Case 0: package does not declare menhir. Dune does not add it.
 Case 1: bare [(depends menhir)]. Dune fills in the lower bound.
 
   $ cat > dune-project << EOF
-  > (lang dune 3.24)
+  > (lang dune 3.23)
   > (using menhir 2.1)
   > (generate_opam_files true)
   > (package
@@ -39,7 +39,7 @@ Case 1: bare [(depends menhir)]. Dune fills in the lower bound.
 Case 2: user-written version bound is preserved verbatim.
 
   $ cat > dune-project << EOF
-  > (lang dune 3.24)
+  > (lang dune 3.23)
   > (using menhir 2.1)
   > (generate_opam_files true)
   > (package
@@ -59,7 +59,7 @@ 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)
+  > (lang dune 3.23)
   > (generate_opam_files true)
   > (package
   >  (name foo)
@@ -78,7 +78,7 @@ 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.24)
+  > (lang dune 3.23)
   > (using menhir 2.1)
   > (generate_opam_files true)
   > (package
@@ -98,16 +98,16 @@ bound; [bar.opam] has no [menhir] line at all.
   [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.
+constraint; the generated opam file ANDs dune's required lower
+bound with it.
 
 Case 4 left a [bar.opam] behind; remove it first, otherwise dune
 errors because the new dune-project below has no [bar] package
 stanza for the existing opam file.
 
   $ rm -f bar.opam
   $ cat > dune-project << EOF
-  > (lang dune 3.24)
+  > (lang dune 3.23)
   > (using menhir 2.1)
   > (generate_opam_files true)
   > (package
@@ -119,4 +119,80 @@ stanza for the existing opam file.
   $ dune build @opam --auto-promote > /dev/null 2>&1
   [1]
   $ grep menhir foo.opam
-    "menhir" {with-test}
+    "menhir" {>= "20180523" & with-test}
+
+Case 6: user-written lower bound below dune's required minimum.
+Dune warns and the generated opam file uses [>= "20180523"]
+instead of the user's bound.
+
+Case 5 left [foo.opam] behind; remove it so this case starts
+fresh.
+
+  $ rm -rf _build foo.opam
+  $ cat > dune-project << EOF
+  > (lang dune 3.23)
+  > (using menhir 2.1)
+  > (generate_opam_files true)
+  > (package
+  >  (name foo)
+  >  (allow_empty)
+  >  (depends (menhir (>= 20100101))))
+  > EOF
+
+Skip [--auto-promote]: the generated file lives at
+[_build/default/foo.opam.generated] under [(lang dune 3.23)] or
+newer, so we can read it without promoting to source. Assert two
+things: the warning is emitted with its full text, and the
+generated opam file uses dune's bound rather than the user's.
+
+  $ dune build @opam 2>&1 | dune_cmd print-from '^Warning:' | dune_cmd print-until 'instead\.$'
+  Warning: The lower bound >= 20100101 on menhir in the depends field is less
+  than the version dune's menhir rules require. The generated opam file will
+  use >= 20180523 instead.
+  [1]
+  $ grep menhir _build/default/foo.opam.generated
+    "menhir" {>= "20180523"}
+
+Case 7: lang version below 3.23. Dune does nothing to the menhir
+dep constraint; the generated opam file preserves the user's input
+verbatim. The whole upgrade step is gated on [(lang dune 3.23)] or
+newer to avoid changing opam output on dune-binary upgrade for
+projects pinned to an older lang version (parallels #14466 / the
+fix for #14436). The gate also covers the bare-[(menhir)] fill
+from #14434, which previously applied unversioned.
+
+  $ rm -rf _build foo.opam
+  $ cat > dune-project << EOF
+  > (lang dune 3.22)
+  > (using menhir 2.1)
+  > (generate_opam_files true)
+  > (package
+  >  (name foo)
+  >  (allow_empty)
+  >  (depends (menhir (>= 20100101))))
+  > EOF
+  $ dune build @opam --auto-promote > /dev/null 2>&1
+  $ grep menhir foo.opam
+    "menhir" {>= "20100101"}
+
+Case 8: lang version below 3.23 with bare [(menhir)]. The 3.23
+gate also covers #14434's bare-fill — dune does not inject
+[>= "20180523"]; the opam file lists [menhir] without a
+constraint. Together with Case 7, this verifies both branches of
+[upgrade_menhir_constraint] are gated: the bare branch (from
+#14434) and the user-constraint branch (from this PR's merge
+logic).
+
+  $ rm -rf _build foo.opam
+  $ cat > dune-project << EOF
+  > (lang dune 3.22)
+  > (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
+  $ grep menhir foo.opam
+    "menhir"