fix(pkg): substitute absent package variables in string interpolation

Signed-off-by: Ali Caglayan <alizter@gmail.com>

Author: Alizter

src/dune_lang/package_variable_name.ml

@@ -45,8 +45,13 @@ let name = of_string "name"
 let build = of_string "build"
 let post = of_string "post"
 let dev = of_string "dev"
+let installed = of_string "installed"
 let one_of t xs = List.mem xs ~equal t
 
+let absent_package_value t =
+  if equal t installed then Some "false" else None
+;;
+
 let platform_specific =
   Set.of_list [ arch; os; os_version; os_distribution; os_family; sys_ocaml_version ]
 ;;

src/dune_lang/package_variable_name.mli

@@ -32,8 +32,14 @@ val version : t
 val post : t
 val build : t
 val dev : t
+val installed : t
 val one_of : t -> t list -> bool
 
+(** Returns the value of a package variable when the package is not in the
+    solution. Some variables have well-defined values for absent packages
+    (e.g., "installed" is "false"), while others are undefined and return None. *)
+val absent_package_value : t -> string option
+
 (** The set of variable names whose values are expected to differ depending on
     the current platform. *)
 val platform_specific : Set.t

src/dune_pkg/lock_pkg.ml

@@ -54,22 +54,42 @@ let invalid_variable_error ~loc variable =
     [ Pp.textf "Variable %S is not supported." (OpamVariable.to_string variable) ]
 ;;
 
-let opam_variable_to_slang ~loc packages variable =
+let opam_variable_to_slang ~loc ~packages_in_solution ~for_string_interp packages variable =
   let variable_string = OpamVariable.to_string variable in
+  let variable_name = Package_variable_name.of_string variable_string in
   let convert_with_package_name package_name =
     match is_valid_package_variable_name variable_string with
     | false -> Error (invalid_variable_error ~loc variable)
     | true ->
-      let pform =
-        let name = Package_variable_name.of_string variable_string in
-        let scope : Package_variable.Scope.t =
-          match package_name with
-          | None -> Self
-          | Some p -> Package (Package_name.of_opam_package_name p)
-        in
-        Package_variable.to_pform { Package_variable.name; scope }
-      in
-      Ok (Slang.pform pform)
+      (match package_name with
+       | Some p ->
+         let pkg_name = Package_name.of_opam_package_name p in
+         if Package_name.Map.mem packages_in_solution pkg_name
+         then (
+           let pform =
+             Package_variable.to_pform
+               { Package_variable.name = variable_name; scope = Package pkg_name }
+           in
+           Ok (Slang.pform pform))
+         else if for_string_interp
+         then (
+           let value =
+             Package_variable_name.absent_package_value variable_name
+             |> Option.value ~default:""
+           in
+           Ok (Slang.text value))
+         else (
+           let pform =
+             Package_variable.to_pform
+               { Package_variable.name = variable_name; scope = Package pkg_name }
+           in
+           Ok (Slang.pform pform))
+       | None ->
+         let pform =
+           Package_variable.to_pform
+             { Package_variable.name = variable_name; scope = Self }
+         in
+         Ok (Slang.pform pform))
   in
   match packages with
   | [] ->
@@ -114,12 +134,21 @@ let desugar_special_string_interpolation_syntax
   | _ -> fident
 ;;
 
-let opam_fident_to_slang ~loc fident =
+let opam_fident_to_slang ~loc ~packages_in_solution ~for_string_interp fident =
   let open Result.O in
   let packages, variable, string_converter =
     OpamFilter.desugar_fident fident |> desugar_special_string_interpolation_syntax
   in
-  let+ slang = opam_variable_to_slang ~loc packages variable in
+  (* When there's a string_converter (?then:else syntax), don't substitute
+     absent package values - let catch_undefined_var handle it at build time. *)
+  let for_string_interp =
+    match string_converter with
+    | Some _ -> false
+    | None -> for_string_interp
+  in
+  let+ slang =
+    opam_variable_to_slang ~loc ~packages_in_solution ~for_string_interp packages variable
+  in
   match string_converter with
   | None -> slang
   | Some (then_, else_) ->
@@ -133,11 +162,12 @@ let opam_fident_to_slang ~loc fident =
     Slang.if_ condition ~then_:(Slang.text then_) ~else_:(Slang.text else_)
 ;;
 
-let opam_raw_fident_to_slang ~loc raw_ident =
-  OpamTypesBase.filter_ident_of_string raw_ident |> opam_fident_to_slang ~loc
+let opam_raw_fident_to_slang ~loc ~packages_in_solution ~for_string_interp raw_ident =
+  OpamTypesBase.filter_ident_of_string raw_ident
+  |> opam_fident_to_slang ~loc ~packages_in_solution ~for_string_interp
 ;;
 
-let opam_string_to_slang ~package ~loc opam_string =
+let opam_string_to_slang ~packages_in_solution ~package ~loc opam_string =
   Re.Seq.split_full OpamFilter.string_interp_regex opam_string
   |> Seq.map ~f:(function
     | `Text text -> Ok (Slang.text text)
@@ -148,7 +178,7 @@ let opam_string_to_slang ~package ~loc opam_string =
          when String.starts_with ~prefix:"%{" interp
               && String.ends_with ~suffix:"}%" interp ->
          let ident = String.sub ~pos:2 ~len:(String.length interp - 4) interp in
-         opam_raw_fident_to_slang ~loc ident
+         opam_raw_fident_to_slang ~loc ~packages_in_solution ~for_string_interp:true ident
        | other ->
          Error
            (User_error.make
@@ -220,11 +250,13 @@ let resolve_depopts ~resolve depopts =
    These two Slang operators are used to emulate Opam's undefined value
    semantics.
 *)
-let filter_to_blang ~package ~loc filter =
+let filter_to_blang ~packages_in_solution ~package ~loc filter =
   let filter_to_slang (filter : OpamTypes.filter) =
     match filter with
-    | FString s -> opam_string_to_slang ~package ~loc s
-    | FIdent fident -> opam_fident_to_slang ~loc fident
+    | FString s -> opam_string_to_slang ~packages_in_solution ~package ~loc s
+    | FIdent fident ->
+      (* FIdent in filter context is truthy, so don't substitute absent values *)
+      opam_fident_to_slang ~loc ~packages_in_solution ~for_string_interp:false fident
     | other ->
       Code_error.raise
         "The opam file parser should only allow identifiers and strings in places where \
@@ -273,6 +305,7 @@ let filter_to_blang ~package ~loc filter =
 ;;
 
 let opam_commands_to_actions
+      ~packages_in_solution
       get_solver_var
       loc
       package
@@ -293,8 +326,13 @@ let opam_commands_to_actions
             let slang =
               let+ slang =
                 match simple_arg with
-                | CString s -> opam_string_to_slang ~package ~loc s
-                | CIdent ident -> opam_raw_fident_to_slang ~loc ident
+                | CString s -> opam_string_to_slang ~packages_in_solution ~package ~loc s
+                | CIdent ident ->
+                  opam_raw_fident_to_slang
+                    ~loc
+                    ~packages_in_solution
+                    ~for_string_interp:true
+                    ident
               in
               Slang.simplify slang
             in
@@ -304,7 +342,8 @@ let opam_commands_to_actions
                  | None -> slang
                  | Some filter ->
                    let+ filter_blang =
-                     filter_to_blang ~package ~loc filter >>| Slang.simplify_blang
+                     filter_to_blang ~packages_in_solution ~package ~loc filter
+                     >>| Slang.simplify_blang
                    and+ slang = slang in
                    let filter_blang_handling_undefined =
                      (* Wrap the blang filter so that if any undefined
@@ -331,7 +370,8 @@ let opam_commands_to_actions
           | None -> Ok action
           | Some filter ->
             let+ condition =
-              filter_to_blang ~package ~loc filter >>| Slang.simplify_blang
+              filter_to_blang ~packages_in_solution ~package ~loc filter
+              >>| Slang.simplify_blang
             in
             Action.When (condition, action)
         in
@@ -358,14 +398,15 @@ let opam_commands_to_actions
    solving. Opam allows depexts to be filtered by arbitrary filter expressions,
    which is why the slang dsl is needed as opposed to (say) a map from
    distro/version to depext name. *)
-let depexts_to_conditional_external_dependencies package depexts =
+let depexts_to_conditional_external_dependencies ~packages_in_solution package depexts =
   let open Result.O in
   List.map depexts ~f:(fun (sys_pkgs, filter) ->
     let external_package_names =
       OpamSysPkg.Set.to_list_map OpamSysPkg.to_string sys_pkgs
     in
     let+ condition =
-      filter_to_blang ~package ~loc:Loc.none filter >>| Slang.simplify_blang
+      filter_to_blang ~packages_in_solution ~package ~loc:Loc.none filter
+      >>| Slang.simplify_blang
     in
     let enabled_if =
       if Slang.Blang.equal condition Slang.Blang.true_
@@ -468,6 +509,7 @@ let opam_package_to_lock_file_pkg
     Solver_stats.Updater.expand_variable stats_updater variable_name;
     Solver_env.get solver_env variable_name
   in
+  let packages_in_solution = version_by_package_name in
   let* build_command =
     if Resolved_package.dune_build resolved_package
     then Ok (Some Lock_dir.Build_command.Dune)
@@ -491,13 +533,18 @@ let opam_package_to_lock_file_pkg
           | None -> Ok action
           | Some filter ->
             let+ blang =
-              filter_to_blang ~package:opam_package ~loc:Loc.none filter
+              filter_to_blang
+                ~packages_in_solution
+                ~package:opam_package
+                ~loc:Loc.none
+                filter
               >>| Slang.simplify_blang
             in
             Action.When (blang, action))
         |> Result.List.all
       and+ build_step =
         opam_commands_to_actions
+          ~packages_in_solution
           get_solver_var
           loc
           opam_package
@@ -528,6 +575,7 @@ let opam_package_to_lock_file_pkg
     if portable_lock_dir
     then
       depexts_to_conditional_external_dependencies
+        ~packages_in_solution
         opam_package
         (OpamFile.OPAM.depexts opam_file)
     else (
@@ -549,7 +597,7 @@ let opam_package_to_lock_file_pkg
   in
   let+ install_command =
     OpamFile.OPAM.install opam_file
-    |> opam_commands_to_actions get_solver_var loc opam_package
+    |> opam_commands_to_actions ~packages_in_solution get_solver_var loc opam_package
     >>| make_action
     >>| Option.map ~f:(fun action -> lockfile_field_choice (build_env action))
     >>| Option.value ~default:Lock_dir.Conditional_choice.empty

test/blackbox-tests/test-cases/pkg/opam-var/absent-pkg-installed.t

@@ -15,14 +15,14 @@ First, test the variable in string interpolation context (command argument):
   Solution for dune.lock:
   - string-context.0.0.1
 
-Currently the variable is left as a pform. It should resolve to "false" at
-solve time:
+The variable resolves to "false" at solve time:
 
   $ cat dune.lock/string-context.0.0.1.pkg
   (version 0.0.1)
 
   (build
-   (all_platforms ((action (run echo %{pkg:not-in-lock:installed})))))
+   (all_platforms ((action (run echo false)))))
+
 
 
 

test/blackbox-tests/test-cases/pkg/opam-var/absent-pkg-version.t

@@ -0,0 +1,53 @@
+Test the "version" variable for packages not in the solution in different
+contexts: string interpolation vs truthy/filter context.
+
+  $ mkrepo
+
+First, test the variable in string interpolation context (command argument):
+
+  $ mkpkg "string-context" <<'EOF'
+  > build: [
+  >   [ "echo" "version=%{not-in-lock:version}%" ]
+  > ]
+  > EOF
+
+  $ solve string-context
+  Solution for dune.lock:
+  - string-context.0.0.1
+
+The variable resolves to empty string at solve time:
+
+  $ cat dune.lock/string-context.0.0.1.pkg
+  (version 0.0.1)
+  
+  (build
+   (all_platforms ((action (run echo "version=")))))
+
+
+Now test the variable in truthy/filter context (conditional on command):
+
+  $ mkpkg "truthy-context" <<'EOF'
+  > build: [
+  >   [ "echo" "has version" ] {not-in-lock:version}
+  >   [ "echo" "no version" ] {!not-in-lock:version}
+  > ]
+  > EOF
+
+  $ solve truthy-context
+  Solution for dune.lock:
+  - truthy-context.0.0.1
+
+In truthy context, the variable is left for build time evaluation:
+
+  $ cat dune.lock/truthy-context.0.0.1.pkg
+  (version 0.0.1)
+  
+  (build
+   (all_platforms
+    ((action
+      (progn
+       (when %{pkg:not-in-lock:version} (run echo "has version"))
+       (when (not %{pkg:not-in-lock:version}) (run echo "no version")))))))
+
+
+