fix ocaml#3431 opam_constraint add parenthesis to respect precedenc…

…e of operations

Signed-off-by: Lucccyo <cha.git@mailo.fr>

otherlibs/site/test/run.t

@@ -150,7 +150,7 @@ Test with an opam like installation
   opam-version: "2.0"
   version: "0.a"
   depends: [
-    "dune" {>= "3.0"}
+    "dune" {(>= "3.0")}
     "odoc" {with-doc}
   ]
   build: [

otherlibs/site/test/run_2_9.t

@@ -138,7 +138,7 @@ Test with an opam like installation
   opam-version: "2.0"
   version: "0.a"
   depends: [
-    "dune" {>= "2.9"}
+    "dune" {(>= "2.9")}
     "odoc" {with-doc}
   ]
   build: [

src/dune_rules/package.ml

@@ -215,23 +215,73 @@ module Dependency = struct
     <|> let+ name = Name.decode in
         { name; constraint_ = None }
 
-  let rec opam_constraint : Constraint.t -> OpamParserTypes.FullPos.value =
+  (** [decide context p value] returns [true] whenever [value] should be
+      parenthesized inside [context] at position [p] ([false] means left-hand
+      side, [true] means right-hand side). *)
+  let decide context p value =
     let open OpamParserTypes.FullPos in
-    function
-    | Bvar v -> Constraint.Var.to_opam v
-    | Uop (op, x) ->
-      nopos (Prefix_relop (Op.to_relop op, Constraint.Var.to_opam x))
-    | Bop (op, x, y) ->
-      nopos
-        (Relop
-           (Op.to_relop op, Constraint.Var.to_opam x, Constraint.Var.to_opam y))
-    | And [ c ] -> opam_constraint c
-    | And (c :: cs) ->
-      nopos (Logop (nopos `And, opam_constraint c, opam_constraint (And cs)))
-    | Or [ c ] -> opam_constraint c
-    | Or (c :: cs) ->
-      nopos (Logop (nopos `Or, opam_constraint c, opam_constraint (And cs)))
-    | And [] | Or [] -> Code_error.raise "opam_constraint" []
+    match (context, value) with
+    (* [ atom (prefix_relop atom) ] *)
+    | (List _ | Group _), Prefix_relop _ -> true
+    | Option _, Prefix_relop _ when p -> true
+    (* otherwise, juxtaposition inside lists is unambiguous *)
+    | (List _ | Group _), _ -> false
+    | Option _, _ when p -> false
+    (* atoms cannot have subtrees *)
+    | (Bool _ | Int _ | String _ | Ident _), _ -> assert false
+    (* atoms and bracketed values should never be parenthesized *)
+    | _, (Bool _ | Int _ | String _ | Ident _ | List _ | Group _) -> false
+    (* [or] and [and] are left-associative, so we check [p] *)
+    | Logop ({ pelem = `Or; _ }, _, _), Logop ({ pelem = `Or; _ }, _, _) -> p
+    | Logop ({ pelem = `And; _ }, _, _), Logop ({ pelem = `And; _ }, _, _) -> p
+    (* [or] is the weakest binary operator, no need to parenthize its children *)
+    | Logop ({ pelem = `Or; _ }, _, _), _ -> false
+    (* [or] contained in anything else should be parenthesized *)
+    | _, Logop ({ pelem = `Or; _ }, _, _) -> true
+    (* [and] in the next weakest, no need to parenthesize its children *)
+    | Logop ({ pelem = `And; _ }, _, _), _ -> false
+    (* [and] contained in anything else should be parenthesized *)
+    | _, Logop ({ pelem = `And; _ }, _, _) -> true
+    (* other cases should not be reached *)
+    | _, _ -> false
+
+  let rec add_group context p value =
+    let open OpamParserTypes.FullPos in
+    let v =
+      match value.pelem with
+      | Bool _ | Int _ | String _ | Ident _ -> value
+      | Relop (op, lvk, rvk) ->
+        nopos @@ Relop (op, add_group value false lvk, add_group value true rvk)
+      | Prefix_relop (op, lvk) ->
+        nopos @@ Prefix_relop (op, add_group value false lvk)
+      | Logop (op, lvk, rvk) ->
+        nopos @@ Logop (op, add_group value false lvk, add_group value true rvk)
+      | Group l ->
+        nopos @@ Group (nopos @@ List.map ~f:(add_group value false) l.pelem)
+      | _ -> assert false
+    in
+    if decide context.pelem p v.pelem then nopos (Group (nopos [ v ])) else v
+
+  let opam_constraint t : OpamParserTypes.FullPos.value =
+    let open OpamParserTypes.FullPos in
+    let rec opam_constraint = function
+      | Constraint.Bvar v -> Constraint.Var.to_opam v
+      | Uop (op, x) ->
+        nopos (Prefix_relop (Op.to_relop op, Constraint.Var.to_opam x))
+      | Bop (op, x, y) ->
+        nopos
+          (Relop
+             (Op.to_relop op, Constraint.Var.to_opam x, Constraint.Var.to_opam y))
+      | And [ c ] -> opam_constraint c
+      | And (c :: cs) ->
+        nopos (Logop (nopos `And, opam_constraint c, opam_constraint (And cs)))
+      | Or [ c ] -> opam_constraint c
+      | Or (c :: cs) ->
+        nopos (Logop (nopos `Or, opam_constraint c, opam_constraint (And cs)))
+      | And [] | Or [] -> Code_error.raise "opam_constraint" []
+    in
+    let root_fake_context = nopos (Group (nopos [])) in
+    add_group root_fake_context false (opam_constraint t)
 
   let opam_depend { name; constraint_ } =
     let constraint_ = Option.map ~f:opam_constraint constraint_ in

test/blackbox-tests/test-cases/describe.t

@@ -1348,7 +1348,7 @@ opam file listing
   opam-version: \"2.0\"
   synopsis: \"foo bar baz\"
   depends: [
-    \"dune\" {>= \"2.3\"}
+    \"dune\" {(>= \"2.3\")}
   ]
   build: [
     [\"dune\" \"subst\"] {pinned}

test/blackbox-tests/test-cases/dune-init.t/run.t

@@ -369,7 +369,7 @@ And the opam file will be generated as expected
   bug-reports: "https://github.com/username/reponame/issues"
   depends: [
     "ocaml"
-    $dune {>= "3.8"}
+    $dune {(>= "3.8")}
     "odoc" {with-doc}
   ]
   build: [
@@ -479,7 +479,7 @@ And the opam file will be generated as expected
   bug-reports: "https://github.com/username/reponame/issues"
   depends: [
     "ocaml"
-    "dune" {>= "3.8"}
+    "dune" {(>= "3.8")}
     "odoc" {with-doc}
   ]
   build: [

test/blackbox-tests/test-cases/dune-package.t/run.t

@@ -160,7 +160,7 @@ Install as opam does
   # This file is generated by dune, edit dune-project instead
   opam-version: "2.0"
   depends: [
-    "dune" {>= "3.0"}
+    "dune" {(>= "3.0")}
     "odoc" {with-doc}
   ]
   build: [

test/blackbox-tests/test-cases/dune-project-meta/basic-generate.t

@@ -75,11 +75,11 @@ The `dune build` should generate the opam file
   depends: [
     "alcotest" {with-test}
     "dune" {build & > "1.5"}
-    "foo" {dev & > "1.5" & < "2.0"}
-    "uri" {>= "1.9.0"}
-    "uri" {< "2.0.0"}
-    "fieldslib" {> "v0.12"}
-    "fieldslib" {< "v0.13"}
+    "foo" {dev & (> "1.5" & < "2.0")}
+    "uri" {(>= "1.9.0")}
+    "uri" {(< "2.0.0")}
+    "fieldslib" {(> "v0.12")}
+    "fieldslib" {(< "v0.13")}
   ]
 
   $ cat cohttp-async.opam
@@ -103,10 +103,10 @@ The `dune build` should generate the opam file
   make sure that the rendering of long strings stays compatible.
   """
   depends: [
-    "cohttp" {>= "1.0.2"}
-    "conduit-async" {>= "1.0.3"}
-    "async" {>= "v0.10.0"}
-    "async" {< "v0.12"}
+    "cohttp" {(>= "1.0.2")}
+    "conduit-async" {(>= "1.0.3")}
+    "async" {(>= "v0.10.0")}
+    "async" {(< "v0.12")}
   ]
 
   $ cat cohttp-lwt.opam
@@ -131,8 +131,8 @@ The `dune build` should generate the opam file
   newline since it doesn't escape the line break.
   """
   depends: [
-    "cohttp" {>= "1.0.2"}
-    "conduit-lwt" {>= "1.0.3"}
-    "lwt" {>= "5.0.0"}
-    "lwt" {< "6.0.0"}
+    "cohttp" {(>= "1.0.2")}
+    "conduit-lwt" {(>= "1.0.3")}
+    "lwt" {(>= "5.0.0")}
+    "lwt" {(< "6.0.0")}
   ]

test/blackbox-tests/test-cases/dune-project-meta/dune-dep.t

@@ -14,7 +14,7 @@ generate a dune dependency with a constraint:
   $ dune build foo.opam
   $ grep -A2 ^depends: foo.opam
   depends: [
-    "dune" {>= "2.1"}
+    "dune" {(>= "2.1")}
   ]
 
 With the dune dependency declared in the dune-project file and version
@@ -43,7 +43,7 @@ Same with version of the language >= 2.6, we now add the constraint:
   $ dune build foo.opam
   $ grep -A2 ^depends: foo.opam
   depends: [
-    "dune" {>= "2.6"}
+    "dune" {(>= "2.6")}
   ]
 
 When the version of the language >= 2.7 we use dev instead of pinned
@@ -86,7 +86,7 @@ the doc dependencies:
   $ dune build foo.opam
   $ grep -A3 ^depends: foo.opam
   depends: [
-    "dune" {>= "2.7"}
+    "dune" {(>= "2.7")}
     "odoc" {with-doc}
   ]
 
@@ -100,7 +100,7 @@ the doc dependencies:
   $ dune build foo.opam
   $ grep -A4 ^depends: foo.opam
   depends: [
-    "dune" {>= "2.7"}
+    "dune" {(>= "2.7")}
     "something"
     "odoc" {with-doc}
   ]
@@ -115,7 +115,7 @@ the doc dependencies:
   $ dune build foo.opam
   $ grep -A4 ^depends: foo.opam
   depends: [
-    "dune" {>= "2.7"}
+    "dune" {(>= "2.7")}
     "odoc"
     "something"
   ]
@@ -130,7 +130,7 @@ the doc dependencies:
   $ dune build foo.opam
   $ grep -A4 ^depends: foo.opam
   depends: [
-    "dune" {>= "2.7"}
+    "dune" {(>= "2.7")}
     "odoc" {with-doc}
     "something"
   ]
@@ -145,7 +145,7 @@ the doc dependencies:
   $ dune build foo.opam
   $ grep -A4 ^depends: foo.opam
   depends: [
-    "dune" {>= "2.7"}
+    "dune" {(>= "2.7")}
     "odoc" {with-doc & >= "1.5.0"}
     "something"
   ]
@@ -160,7 +160,7 @@ the doc dependencies:
   $ dune build foo.opam
   $ grep -A5 ^depends: foo.opam
   depends: [
-    "dune" {>= "2.7"}
+    "dune" {(>= "2.7")}
     "odoc" {with-test}
     "something"
     "odoc" {with-doc}

test/blackbox-tests/test-cases/dune-project-meta/github2927.t

@@ -14,7 +14,7 @@ Reproduction case for #2927
   # This file is generated by dune, edit dune-project instead
   opam-version: "2.0"
   depends: [
-    "dune" {>= "2.0"}
+    "dune" {(>= "2.0")}
   ]
   build: [
     ["dune" "subst"] {pinned}

test/blackbox-tests/test-cases/dune-project-meta/override.t

@@ -30,7 +30,7 @@ Package information fields can be overridden per-package:
   homepage: "https://my.home.page"
   bug-reports: "https://github.com/mirage/foo/issues"
   depends: [
-    "dune" {>= "2.5"}
+    "dune" {(>= "2.5")}
   ]
   build: [
     ["dune" "subst"] {pinned}

test/blackbox-tests/test-cases/dune-project-meta/v11-1.t

@@ -13,7 +13,7 @@ Generation of opam files with lang dune >= 1.11
   # This file is generated by dune, edit dune-project instead
   opam-version: "2.0"
   depends: [
-    "dune" {>= "1.11"}
+    "dune" {(>= "1.11")}
   ]
   build: [
     ["dune" "subst"] {pinned}

test/blackbox-tests/test-cases/opam-constraints.t

@@ -36,22 +36,22 @@ constraints.
   # This file is generated by dune, edit dune-project instead
   opam-version: "2.0"
   depends: [
-    "dune" {>= "2.1"}
-    "p_eq" {= "v"}
-    "p_lt" {< "v"}
-    "p_gt" {> "v"}
-    "p_ge" {>= "v"}
-    "p_le" {<= "v"}
-    "p_ne" {!= "v"}
+    "dune" {(>= "2.1")}
+    "p_eq" {(= "v")}
+    "p_lt" {(< "v")}
+    "p_gt" {(> "v")}
+    "p_ge" {(>= "v")}
+    "p_le" {(<= "v")}
+    "p_ne" {(!= "v")}
     "p_bin" {os != "win32"}
     "p_and2" {a & b}
     "p_and1" {a}
-    "p_and3" {a & b & c}
+    "p_and3" {a & (b & c)}
     "p_or2" {a | b}
     "p_or1" {a}
     "p_or3" {a | b & c}
     "p_and_in_or" {a | b & c}
-    "p_or_in_and" {a & b | c}
+    "p_or_in_and" {a & (b | c)}
   ]
   build: [
     ["dune" "subst"] {pinned}