From 927713804e667348dc89e85c09477bfe9abda945 Mon Sep 17 00:00:00 2001
From: David Allsopp <david.allsopp@metastack.com>
Date: Sat, 1 Jun 2024 23:07:31 +0100
Subject: [PATCH 1/2] Add test showing that ?var doesn't work

?var & var = "foo" should not trigger a warning if var is undefined.
---
 tests/reftests/resolve-variables.test | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/tests/reftests/resolve-variables.test b/tests/reftests/resolve-variables.test
index c26148aec95..c25087fe39e 100644
--- a/tests/reftests/resolve-variables.test
+++ b/tests/reftests/resolve-variables.test
@@ -81,3 +81,16 @@ The following actions will be performed:
 -> installed d.4
 -> installed foo.1
 Done.
+### OPAMYES=1 OPAMSTRICT=1
+### <REPO2/packages/undef/undef.1/opam>
+opam-version: "2.0"
+depends: "d" {?undefined-global & undefined-global != ""}
+### <REPO2/repo>
+opam-version: "2.0"
+### opam switch create undefined-globals --empty
+### opam repository add second ./REPO2 --this-switch
+[second] Initialised
+### opam repo remove default --this-switch
+### opam install undef
+[ERROR] Undefined filter variable undefined-global in dependencies of undef.1
+# Return code 30 #

From 01c317d78076805da58a226988e0033f1ccc1487 Mon Sep 17 00:00:00 2001
From: David Allsopp <david.allsopp@metastack.com>
Date: Sat, 1 Jun 2024 22:28:14 +0100
Subject: [PATCH 2/2] Don't trigger undefined variable warning for ?var

The implementation of the OpamSwitchState.get_dependencies_t displays
warnings on identifier resolution (which, with --strict enabled, are
fatal). This was incorrect, as this includes the resolution of variables
underneath the `?` operator (which tests for undefined variables).

The revised test relies on the fact that everything should have been
resolved except for `post` and displays errors after the filter has been
reduced.
---
 master_changes.md                     |  1 +
 src/state/opamSwitchState.ml          | 30 ++++++++++++++++++---------
 tests/reftests/resolve-variables.test |  9 ++++++--
 3 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/master_changes.md b/master_changes.md
index 71f88495ba6..8f559742d8a 100644
--- a/master_changes.md
+++ b/master_changes.md
@@ -51,6 +51,7 @@ users)
 ## Var/Option
 
 ## Update / Upgrade
+  * [BUG] Stop triggering "Undefined filter variable variable" warning for `?variable` [#5983 @dra27]
 
 ## Tree
   * [BUG] Fix `opam tree --with-*` assigning the `with-*` variables to unrequested packages [#5919 @kit-ty-kate @rjbou - fix #5755]
diff --git a/src/state/opamSwitchState.ml b/src/state/opamSwitchState.ml
index 1f1649062b2..6d4b3e33883 100644
--- a/src/state/opamSwitchState.ml
+++ b/src/state/opamSwitchState.ml
@@ -884,8 +884,17 @@ let avoid_version st nv =
            has_avoid_flag)
           +! false)
 
+let undefined_filter_variable nv v =
+  (if OpamFormatConfig.(!r.strict) then
+     OpamConsole.error_and_exit `File_error
+       "Undefined filter variable %s in dependencies of %s"
+   else
+     log
+       "ERR: Undefined filter variable %s in dependencies of %s")
+    (OpamVariable.Full.to_string v) (OpamPackage.to_string nv)
+
 let package_env_t st ~force_dev_deps ~test ~doc ~dev_setup
-    ~requested_allpkgs nv v =
+    ~requested_allpkgs ?(err_undefined=true) nv v =
   if List.mem v OpamPackageVar.predefined_depends_variables then
     match OpamVariable.Full.to_string v with
     | "dev" ->
@@ -899,19 +908,17 @@ let package_env_t st ~force_dev_deps ~test ~doc ~dev_setup
     | _ -> None (* Computation delayed to the solver *)
   else
   let r = OpamPackageVar.resolve_switch ~package:nv st v in
-  if r = None then
-    (if OpamFormatConfig.(!r.strict) then
-       OpamConsole.error_and_exit `File_error
-         "Undefined filter variable %s in dependencies of %s"
-     else
-       log
-         "ERR: Undefined filter variable %s in dependencies of %s")
-      (OpamVariable.Full.to_string v) (OpamPackage.to_string nv);
+  if err_undefined && r = None then
+    undefined_filter_variable nv v;
   r
 
 let get_dependencies_t st ~force_dev_deps ~test ~doc ~dev_setup
     ~requested_allpkgs deps opams =
   let filter_undefined nv =
+    let warn_undefined v =
+      if not (List.mem v OpamPackageVar.predefined_depends_variables) then
+        undefined_filter_variable nv v
+    in
     OpamFormula.map (fun (name, fc) ->
         let fc =
           OpamFormula.map (function
@@ -923,6 +930,9 @@ let get_dependencies_t st ~force_dev_deps ~test ~doc ~dev_setup
                   "Undefined filter variable %s in dependencies of %s"
                   (OpamVariable.to_string v) (OpamPackage.to_string nv);
                 Atom (Filter (FBool false))
+              | (Filter filter) as f ->
+                List.iter warn_undefined (OpamFilter.variables filter);
+                Atom f
               | f -> Atom f)
             fc
         in
@@ -931,7 +941,7 @@ let get_dependencies_t st ~force_dev_deps ~test ~doc ~dev_setup
   OpamPackage.Map.mapi (fun nv opam ->
       OpamFilter.partial_filter_formula
         (package_env_t st ~force_dev_deps ~test ~doc
-           ~dev_setup ~requested_allpkgs nv)
+           ~dev_setup ~requested_allpkgs ~err_undefined:false nv)
         (deps opam)
       |> filter_undefined nv) opams
 
diff --git a/tests/reftests/resolve-variables.test b/tests/reftests/resolve-variables.test
index c25087fe39e..328cf53c819 100644
--- a/tests/reftests/resolve-variables.test
+++ b/tests/reftests/resolve-variables.test
@@ -92,5 +92,10 @@ opam-version: "2.0"
 [second] Initialised
 ### opam repo remove default --this-switch
 ### opam install undef
-[ERROR] Undefined filter variable undefined-global in dependencies of undef.1
-# Return code 30 #
+The following actions will be performed:
+=== install 1 package
+  - install undef 1
+
+<><> Processing actions <><><><><><><><><><><><><><><><><><><><><><><><><><><><>
+-> installed undef.1
+Done.