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

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

@@ -0,0 +1,9 @@
+- For `(lang dune 3.23)` or newer with `(generate_opam_files true)`,
+  the generated opam file now enforces the menhir lower bound
+  (`>= "20180523"`) that dune's menhir rules require, even when the
+  package already lists a menhir constraint. A warning is emitted when
+  the user's lower bound is below the minimum. The 3.23 gate also
+  covers the bare-`(menhir)` fill from #14434 (previously unversioned),
+  mirroring #14466 to avoid silent changes to opam output on
+  dune-binary upgrade.
+  (#14453, @robinbb)

src/dune_rules/opam_create.ml

@@ -216,23 +216,60 @@ let dune_name = Package.Name.of_string "dune"
 let odoc_name = Package.Name.of_string "odoc"
 let menhir_name = Package.Name.of_string "menhir"
 
+let rec has_sufficient_lower_bound ~min_version : Package_constraint.t -> bool = function
+  | Uop (Gte, String_literal v) | Uop (Eq, String_literal v) ->
+    OpamVersionCompare.compare v min_version >= 0
+  | And ts -> List.exists ts ~f:(has_sufficient_lower_bound ~min_version)
+  | Or ts -> List.for_all ts ~f:(has_sufficient_lower_bound ~min_version)
+  | Uop _ | Bop _ | Bvar _ | Not _ -> false
+;;
+
+(* Walk [c] replacing each [>= "v"] literal where [v < min_version]
+   with [>= min_version]. Returns the rewritten constraint and a
+   flag indicating whether any rewrite occurred (so the caller can
+   emit a single user-facing warning). [Not], [Bop], and [Bvar]
+   pass through unchanged. *)
+let normalize_lower_bounds ~min_version c =
+  let rewrote = ref false in
+  let min_constraint : Package_constraint.t = Uop (Gte, String_literal min_version) in
+  let rec loop : Package_constraint.t -> Package_constraint.t = function
+    | Uop (Gte, String_literal v) as c ->
+      if OpamVersionCompare.compare v min_version >= 0
+      then c
+      else (
+        rewrote := true;
+        min_constraint)
+    | And ts -> And (List.map ts ~f:loop)
+    | Or ts -> Or (List.map ts ~f:loop)
+    | Not t -> Not (loop t)
+    | (Uop _ | Bop _ | Bvar _) as c -> c
+  in
+  let c' = loop c in
+  c', !rewrote
+;;
+
 let merge_dune_constraints lang_constraint user_constraint =
-  match lang_constraint, user_constraint with
-  | ( Package_constraint.Uop (Gte, String_literal lang_v)
-    , Package_constraint.Uop (Gte, String_literal user_v) ) ->
-    if OpamVersionCompare.compare lang_v user_v <= 0
-    then user_constraint
-    else (
+  match lang_constraint with
+  | Package_constraint.Uop (Gte, String_literal lang_v) ->
+    let user_constraint, rewrote =
+      normalize_lower_bounds ~min_version:lang_v user_constraint
+    in
+    if rewrote
+    then
       User_warning.emit
         [ Pp.textf
-            "The lower bound >= %s on dune in the depends field is less than the dune \
-             language version %s. The generated opam file will use >= %s instead."
-            user_v
+            "A lower bound on dune in the depends field is less than the dune language \
+             version %s. The generated opam file will use >= %s instead."
             lang_v
             lang_v
         ];
-      lang_constraint)
-  | _ -> And [ lang_constraint; user_constraint ]
+    if has_sufficient_lower_bound ~min_version:lang_v user_constraint
+    then user_constraint
+    else And [ lang_constraint; user_constraint ]
+  | _ ->
+    (* [insert_dune_dep] always supplies the [Gte] shape; this arm
+       is defensive. *)
+    And [ lang_constraint; user_constraint ]
 ;;
 
 let insert_dune_dep depends dune_version =
@@ -300,20 +337,46 @@ 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. Tightens any nested [>= "v"] literal below
+   the minimum, then AND's [menhir_constraint] at the top level
+   unless the (post-tightening) expression already implies the
+   lower bound on every satisfying valuation. *)
+let merge_menhir_constraint user_constraint =
+  let user_constraint, rewrote =
+    normalize_lower_bounds ~min_version:menhir_min_version user_constraint
+  in
+  if rewrote
+  then
+    User_warning.emit
+      [ Pp.textf
+          "A lower bound on menhir in the depends field is less than the version dune's \
+           menhir rules require. The generated opam file will use >= %s instead."
+          menhir_min_version
+      ];
+  if has_sufficient_lower_bound ~min_version:menhir_min_version user_constraint
+  then user_constraint
+  else 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 +405,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/dune-project-meta/dune-dep.t

@@ -118,9 +118,8 @@ is emitted and the lang constraint takes precedence:
   > EOF
 
   $ dune build foo.opam
-  Warning: The lower bound >= 3.20 on dune in the depends field is less than
-  the dune language version 3.23. The generated opam file will use >= 3.23
-  instead.
+  Warning: A lower bound on dune in the depends field is less than the dune
+  language version 3.23. The generated opam file will use >= 3.23 instead.
   $ dune_cmd print-from 'depends' < _build/default/foo.opam | dune_cmd print-until ']'
   depends: [
     "dune" {>= "3.23"}