refactor: cleanups in the opam solver (ocaml#11439)

* Use |> to improve readability along with labels
* Use `Package_version.dev` for the version

Signed-off-by: Rudi Grinberg <[email protected]>

Author: rgrinberg

src/dune_pkg/opam_solver.ml

@@ -148,11 +148,9 @@ module Context = struct
       let available = OpamFile.OPAM.available opam in
       match
         OpamFilter.partial_eval
-          (add_self_to_filter_env
-             package
-             (Solver_stats.Updater.wrap_env
-                t.stats_updater
-                (Solver_env.to_env t.solver_env)))
+          (Solver_env.to_env t.solver_env
+           |> Solver_stats.Updater.wrap_env t.stats_updater
+           |> add_self_to_filter_env package)
           available
         |> eval_to_bool
       with
@@ -244,30 +242,34 @@ module Context = struct
   ;;
 
   let filter_deps t package filtered_formula =
-    let name = OpamPackage.name package |> Package_name.of_opam_package_name in
     (* Add additional constraints to the formula. This works in two steps.
        First identify all the additional constraints applied to packages which
        appear in the current package's dependency formula. Then each additional
        constnraint is and-ed with the current package's dependency formula. *)
     let filtered_formula =
       OpamFormula.fold_left
         (fun additional_formulae (pkg, _) ->
-          let name = Package_name.of_opam_package_name pkg in
-          match Package_name.Map.find t.constraints name with
+          match
+            Package_name.of_opam_package_name pkg |> Package_name.Map.find t.constraints
+          with
           | None -> additional_formulae
           | Some additional -> additional :: additional_formulae)
         []
         filtered_formula
       |> List.fold_left ~init:filtered_formula ~f:(fun additional acc ->
         OpamFormula.And (acc, additional))
     in
-    let package_is_local = Package_name.Map.mem t.local_packages name in
-    Resolve_opam_formula.apply_filter
-      (add_self_to_filter_env
-         package
-         (Solver_stats.Updater.wrap_env t.stats_updater (Solver_env.to_env t.solver_env)))
-      ~with_test:package_is_local
-      filtered_formula
+    let package_is_local =
+      OpamPackage.name package
+      |> Package_name.of_opam_package_name
+      |> Package_name.Map.mem t.local_packages
+    in
+    Solver_env.to_env t.solver_env
+    |> Solver_stats.Updater.wrap_env t.stats_updater
+    |> add_self_to_filter_env package
+    |> Resolve_opam_formula.apply_filter
+         ~with_test:package_is_local
+         ~formula:filtered_formula
   ;;
 
   let count_expanded_packages t = Table.fold t.expanded_packages ~init:0 ~f:( + )
@@ -701,13 +703,13 @@ module Solver = struct
       let+ () =
         let rec lookup_impl expand_deps role =
           let impls = ref [] in
-          let* clause =
+          let* candidates =
             Fiber_cache.find_or_add impl_cache role ~f:(fun () ->
-              let+ clause, impls' =
+              let+ candidates, impls' =
                 Candidates.make_impl_clause sat context ~dummy_impl role
               in
               impls := impls';
-              clause)
+              candidates)
           in
           let+ () =
             Fiber.parallel_iter !impls ~f:(fun { var = impl_var; impl } ->
@@ -727,7 +729,7 @@ module Solver = struct
                     deferred := (impl_var, dep) :: !deferred;
                     Fiber.return ()))
           in
-          clause
+          candidates
         and process_dep expand_deps user_var (dep : Input.dependency) : unit Fiber.t =
           (* Process a dependency of [user_var]:
              - find the candidate implementations to satisfy it
@@ -807,8 +809,10 @@ module Solver = struct
          3) we follow every dependency of every selected implementation
       *)
       let sat = Sat.create () in
-      let dummy_impl = if closest_match then Some Input.Dummy else None in
-      let* impl_clauses = build_problem context root_req sat ~dummy_impl in
+      let* impl_clauses =
+        let dummy_impl = if closest_match then Some Input.Dummy else None in
+        build_problem context root_req sat ~dummy_impl
+      in
       let+ impl_clauses = Fiber_cache.to_table impl_clauses in
       (* Run the solve *)
       let decider () =
@@ -846,9 +850,7 @@ module Solver = struct
         Some
           (Table.to_list impl_clauses
            |> List.filter_map ~f:(fun (key, v) ->
-             match Candidates.selected v with
-             | None -> None
-             | Some v -> Some (key, v))
+             Candidates.selected v |> Option.map ~f:(fun v -> key, v))
            |> Input.Role.Map.of_list_exn)
     ;;
   end
@@ -1634,8 +1636,8 @@ let opam_package_to_lock_file_pkg
     let resolve what =
       Resolve_opam_formula.filtered_formula_to_package_names
         ~with_test:false
-        (add_self_to_filter_env opam_package (Solver_env.to_env solver_env))
-        version_by_package_name
+        ~packages:version_by_package_name
+        ~env:(add_self_to_filter_env opam_package (Solver_env.to_env solver_env))
         what
     in
     let depends =
@@ -1802,9 +1804,7 @@ let reject_unreachable_packages =
             "package is both local and returned by solver"
             [ "name", Package_name.to_dyn name ]
         | Some (lock_dir_pkg : Lock_dir.Pkg.t), None -> Some lock_dir_pkg.info.version
-        | None, Some _pkg ->
-          let version = Package_version.of_string "dev" in
-          Some version)
+        | None, Some _pkg -> Some Package_version.dev)
     in
     let pkgs_by_name =
       Package_name.Map.merge pkgs_by_name local_packages ~f:(fun name lhs rhs ->
@@ -1816,21 +1816,24 @@ let reject_unreachable_packages =
             [ "name", Package_name.to_dyn name ]
         | Some (pkg : Lock_dir.Pkg.t), None -> Some (List.map pkg.depends ~f:snd)
         | None, Some (pkg : Local_package.For_solver.t) ->
-          let formula = Dependency_formula.to_filtered_formula pkg.dependencies in
-          (* Use `dev` because at this point we don't have any version *)
-          let opam_package =
-            OpamPackage.of_string (sprintf "%s.dev" (Package_name.to_string pkg.name))
-          in
-          let env = add_self_to_filter_env opam_package (Solver_env.to_env solver_env) in
-          let resolved =
-            Resolve_opam_formula.filtered_formula_to_package_names
-              env
-              ~with_test:true
-              (Package_name.Map.set pkgs_by_version Dune_dep.name dune_version)
-              formula
-          in
           let deps =
-            match resolved with
+            match
+              let env =
+                let opam_package =
+                  OpamPackage.create
+                    (Package_name.to_opam_package_name pkg.name)
+                    (* Use [dev] because at this point we don't have any version *)
+                    (Package_version.to_opam_package_version Package_version.dev)
+                in
+                Solver_env.to_env solver_env |> add_self_to_filter_env opam_package
+              in
+              Dependency_formula.to_filtered_formula pkg.dependencies
+              |> Resolve_opam_formula.filtered_formula_to_package_names
+                   ~env
+                   ~with_test:true
+                   ~packages:
+                     (Package_name.Map.set pkgs_by_version Dune_dep.name dune_version)
+            with
             | Ok { regular; post = _ (* discard post deps *) } ->
               (* remove Dune from the formula as we remove it from solutions *)
               List.filter regular ~f:(fun pkg ->

src/dune_pkg/package_universe.ml

@@ -48,14 +48,12 @@ let version_by_package_name local_packages (lock_dir : Lock_dir.t) =
 let concrete_dependencies_of_local_package t local_package_name ~with_test =
   let local_package = Package_name.Map.find_exn t.local_packages local_package_name in
   match
-    Local_package.(
-      for_solver local_package
-      |> (fun x -> x.dependencies)
-      |> Dependency_formula.to_filtered_formula)
+    (Local_package.for_solver local_package).dependencies
+    |> Dependency_formula.to_filtered_formula
     |> Resolve_opam_formula.filtered_formula_to_package_names
          ~with_test
-         (Solver_env.to_env t.solver_env)
-         t.version_by_package_name
+         ~env:(Solver_env.to_env t.solver_env)
+         ~packages:t.version_by_package_name
   with
   | Ok { regular; post = _ } -> regular
   | Error (`Formula_could_not_be_satisfied unsatisfied_formula_hints) ->

src/dune_pkg/resolve_opam_formula.ml

@@ -1,17 +1,18 @@
 open! Import
 module Relop = Dune_lang.Relop
 
-let apply_filter env ~with_test (opam_filtered_formula : OpamTypes.filtered_formula)
+let apply_filter env ~with_test ~(formula : OpamTypes.filtered_formula)
   : OpamTypes.formula
   =
   OpamFilter.gen_filter_formula
-    (OpamFormula.partial_eval (function
-      | OpamTypes.Filter flt ->
-        `Formula (Atom (OpamTypes.Filter (OpamFilter.partial_eval env flt)))
-      | Constraint (relop, filter) ->
-        let filter = OpamFilter.partial_eval env filter in
-        `Formula (Atom (Constraint (relop, filter)))))
-    opam_filtered_formula
+    (OpamFormula.partial_eval (fun (form : _ OpamTypes.filter_or_constraint) ->
+       match form with
+       | Filter flt ->
+         `Formula (Atom (OpamTypes.Filter (OpamFilter.partial_eval env flt)))
+       | Constraint (relop, filter) ->
+         let filter = OpamFilter.partial_eval env filter in
+         `Formula (Atom (Constraint (relop, filter)))))
+    formula
   |> OpamFilter.filter_deps
        ~build:true
        ~post:false
@@ -204,17 +205,17 @@ type deps =
   ; regular : Package_name.t list
   }
 
-let filtered_formula_to_package_names env ~with_test version_by_package_name formula =
+let filtered_formula_to_package_names ~env ~with_test ~packages formula =
   let open Result.O in
-  let+ all =
-    formula_to_package_names version_by_package_name (apply_filter ~with_test env formula)
-  in
-  let regular_set =
-    formula_to_package_names_allow_missing
-      version_by_package_name
-      (apply_filter ~with_test (override_post (Some false) env) formula)
-    |> Package_name.Set.of_list
+  let+ all = apply_filter ~with_test env ~formula |> formula_to_package_names packages in
+  let regular, post =
+    let regular_set =
+      override_post (Some false) env
+      |> apply_filter ~with_test ~formula
+      |> formula_to_package_names_allow_missing packages
+      |> Package_name.Set.of_list
+    in
+    List.partition all ~f:(Package_name.Set.mem regular_set)
   in
-  let regular, post = List.partition all ~f:(Package_name.Set.mem regular_set) in
   { regular; post }
 ;;

src/dune_pkg/resolve_opam_formula.mli

@@ -4,7 +4,7 @@ open! Import
 val apply_filter
   :  OpamFilter.env
   -> with_test:bool
-  -> OpamTypes.filtered_formula
+  -> formula:OpamTypes.filtered_formula
   -> OpamTypes.formula
 
 module Version_constraint : sig
@@ -54,8 +54,8 @@ type deps =
     list of package names, split into post and regular (ie. non-post)
     dependencies. *)
 val filtered_formula_to_package_names
-  :  OpamFilter.env
+  :  env:OpamFilter.env
   -> with_test:bool
-  -> Package_version.t Package_name.Map.t
+  -> packages:Package_version.t Package_name.Map.t
   -> OpamTypes.filtered_formula
   -> (deps, unsatisfied_formula) result