Merge pull request ocaml#3496 from voodoos/github3494

[FIX ocaml#3494] `enabled_if` corrections

CHANGES.md

@@ -4,9 +4,11 @@ next
 - Fix a bug where valid lib names in `dune init exec --libs=lib1,lib2`
   results in an error. (#3444, fix #3443, @bikallem)
 
-- Add and `enabled_ if` field to the `install` stanza. Enforce the same
-  variable restrictions for `enabled_if` fields in the `executable` and `install`
-  stanzas than in the `library` stanza. (#3408, fixes #3354, @voodoos)
+- Add and `enabled_ if` field to the `install` stanza. Enforce the same variable
+  restrictions for `enabled_if` fields in the `executable` and `install` stanzas
+  than in the `library` stanza. When using dune lang < 2.6, the usage of
+  forbidden variables in executables stanzas with only trigger a warning to
+  maintain compatibility. (#3408 and #3496, fixes #3354, @voodoos)
 
 - Insert a constraint one the version of dune when the user explicitly
   specify the dependency on dune in the `dune-project` file (#3434 ,

src/dune/coq_stanza.ml

@@ -128,7 +128,7 @@ module Theory = struct
        and+ boot =
          field_b "boot" ~check:(Dune_lang.Syntax.since coq_syntax (0, 2))
        and+ modules = modules_field "modules"
-       and+ enabled_if = Enabled_if.decode ~since:None ()
+       and+ enabled_if = Enabled_if.decode ~allowed_vars:Any ~since:None ()
        and+ buildable = Buildable.decode in
        let package = select_deprecation ~package ~public in
        { name

src/dune/dune_file.ml

@@ -1484,8 +1484,12 @@ module Executables = struct
              [ Pp.text "This field is reserved for Dune itself" ];
          fname)
     and+ enabled_if =
-      let allowed_vars = Enabled_if.common_vars ~since:(2, 6) in
-      Enabled_if.decode ~allowed_vars ~since:(Some (2, 3)) ()
+      let allowed_vars = Enabled_if.common_vars ~since:(2, 3) in
+      let* syntax_version = Dune_lang.Syntax.get_exn Stanza.syntax in
+      let is_error =
+        Dune_lang.Syntax.Version.Infix.(syntax_version >= (2, 6))
+      in
+      Enabled_if.decode ~allowed_vars ~is_error ~since:(Some (2, 3)) ()
     in
     fun names ~multi ->
       let has_public_name = Names.has_public_name names in
@@ -1668,7 +1672,8 @@ module Rule = struct
          dune. *)
       assert (not fallback)
     and+ mode = field "mode" Mode.decode ~default:Mode.Standard
-    and+ enabled_if = Enabled_if.decode ~since:(Some (1, 4)) ()
+    and+ enabled_if =
+      Enabled_if.decode ~allowed_vars:Any ~since:(Some (1, 4)) ()
     and+ package =
       field_o "package"
         (Dune_lang.Syntax.since Stanza.syntax (2, 0) >>> Pkg.decode)
@@ -1706,7 +1711,9 @@ module Rule = struct
     <|> fields
           (let+ modules = field "modules" (repeat string)
            and+ mode = Mode.field
-           and+ enabled_if = Enabled_if.decode ~since:(Some (1, 4)) () in
+           and+ enabled_if =
+             Enabled_if.decode ~allowed_vars:Any ~since:(Some (1, 4)) ()
+           in
            { modules; mode; enabled_if })
 
   let ocamlyacc = ocamllex
@@ -1790,7 +1797,8 @@ module Menhir = struct
          field_o_b "infer"
            ~check:(Dune_lang.Syntax.since Menhir_stanza.syntax (2, 0))
        and+ menhir_syntax = Dune_lang.Syntax.get_exn Menhir_stanza.syntax
-       and+ enabled_if = Enabled_if.decode ~since:(Some (1, 4)) ()
+       and+ enabled_if =
+         Enabled_if.decode ~allowed_vars:Any ~since:(Some (1, 4)) ()
        and+ loc = loc in
        let infer =
          match infer with
@@ -1859,7 +1867,8 @@ module Tests = struct
            ~default:Executables.Link_mode.Map.default_for_tests
        and+ deps =
          field "deps" (Bindings.decode Dep_conf.decode) ~default:Bindings.empty
-       and+ enabled_if = Enabled_if.decode ~since:(Some (1, 4)) ()
+       and+ enabled_if =
+         Enabled_if.decode ~allowed_vars:Any ~since:(Some (1, 4)) ()
        and+ action =
          field_o "action"
            ( Dune_lang.Syntax.since ~fatal:false Stanza.syntax (1, 2)

src/dune/enabled_if.ml

@@ -21,33 +21,41 @@ let common_vars_list =
 let common_vars ~since =
   Only (List.map ~f:(fun var -> (var, since)) common_vars_list)
 
-let decode ?(allowed_vars = Any) ~since () =
+let emit_warning allowed_vars is_error var =
+  let loc = String_with_vars.Var.loc var in
+  let var_names = List.map ~f:fst allowed_vars in
+  User_warning.emit ~loc ~is_error
+    [ Pp.textf
+        "Only %s variables are allowed in this 'enabled_if' field. If you \
+         think that %s should also be allowed, please file an issue about it."
+        (String.enumerate_and var_names)
+        (String_with_vars.Var.name var)
+    ];
+  return ()
+
+let decode ~allowed_vars ?(is_error = true) ~since () =
+  let check_var ~allowed_vars var decoder_acc =
+    ( match String_with_vars.Var.payload var with
+    | Some _ -> emit_warning allowed_vars is_error var
+    | None -> (
+      let name = String_with_vars.Var.name var in
+      match List.assoc allowed_vars name with
+      | None -> emit_warning allowed_vars is_error var
+      | Some min_ver ->
+        let* current_ver = Dune_lang.Syntax.get_exn Stanza.syntax in
+        if min_ver > current_ver then
+          let loc = String_with_vars.Var.loc var in
+          let what = "This variable" in
+          Dune_lang.Syntax.Error.since loc Stanza.syntax min_ver ~what
+        else
+          return () ) )
+    >>> decoder_acc
+  in
   let check_vars blang =
     match allowed_vars with
     | Any -> return blang
     | Only allowed_vars ->
-      Blang.fold_vars blang ~init:(return blang) ~f:(fun var dec ->
-          let raise_error () =
-            let loc = String_with_vars.Var.loc var in
-            let var_names = List.map ~f:fst allowed_vars in
-            User_error.raise ~loc
-              [ Pp.textf "Only %s are allowed in this 'enabled_if' field."
-                  (String.enumerate_and var_names)
-              ]
-          in
-          match String_with_vars.Var.(name var, payload var) with
-          | _, Some _ -> raise_error ()
-          | name, None -> (
-            match List.assoc allowed_vars name with
-            | None -> raise_error ()
-            | Some min_ver ->
-              let* current_ver = Dune_lang.Syntax.get_exn Stanza.syntax in
-              if min_ver > current_ver then
-                let loc = String_with_vars.Var.loc var in
-                let what = "This variable" in
-                Dune_lang.Syntax.Error.since loc Stanza.syntax min_ver ~what
-              else
-                return () >>> dec ))
+      Blang.fold_vars blang ~init:(return blang) ~f:(check_var ~allowed_vars)
   in
   let decode =
     ( match since with

src/dune/enabled_if.mli

@@ -5,7 +5,8 @@ type allowed_vars =
 val common_vars : since:Dune_lang.Syntax.Version.t -> allowed_vars
 
 val decode :
-     ?allowed_vars:allowed_vars
+     allowed_vars:allowed_vars
+  -> ?is_error:bool
   -> since:Dune_lang.Syntax.Version.t option
   -> unit
   -> Blang.t Dune_lang.Decoder.fields_parser

test/blackbox-tests/test-cases/enabled_if-exec/dune

@@ -10,3 +10,10 @@
  (public_name dis)
  (modules dis)
  (enabled_if false))
+
+; Github3494 : Enabled if with allowed variable
+; in dune 2.3 to 2.5 should not raise an error
+(executable
+ (name foo)
+ (modules foo)
+ (enabled_if (<> %{os_type} "")))

test/blackbox-tests/test-cases/enabled_if-exec/foo.ml

@@ -0,0 +1 @@
+Printf.eprintf "Ping"

test/blackbox-tests/test-cases/enabled_if-exec/forbidden_var/dune

@@ -1,3 +1,3 @@
 (executable
  (name foo)
- (enabled_if (= %{project_root} "")))
+ (enabled_if (<> %{project_root} "")))

test/blackbox-tests/test-cases/enabled_if-exec/run.t

@@ -16,16 +16,6 @@ This one is enabled
 Installing should silently ignore disabled executables
   $ dune build @install
 
-This one unse forbidden variables
-  $ dune exec ./foo.exe --root forbidden_var
-  Entering directory 'forbidden_var'
-  File "dune", line 3, characters 18-31:
-  3 |  (enabled_if (= %{project_root} "")))
-                        ^^^^^^^^^^^^^
-  Error: Only architecture, system, model, os_type, ccomp_type, profile and
-  ocaml_version are allowed in this 'enabled_if' field.
-  [1]
-
 Tests for enabled_if in install stanza. Only bar.x should be installed.
   $ dune build @install --root install
   Entering directory 'install'
@@ -39,5 +29,36 @@ Tests for enabled_if in install stanza using forbidden variable.
   6 |  (enabled_if (= %{project_root} ""))
                         ^^^^^^^^^^^^^
   Error: Only architecture, system, model, os_type, ccomp_type, profile and
-  ocaml_version are allowed in this 'enabled_if' field.
+  ocaml_version variables are allowed in this 'enabled_if' field. If you think
+  that project_root should also be allowed, please file an issue about it.
+  [1]
+
+The next ones use forbidden variables
+For dune 2.3 -> 2.5 it is a warning
+  $ cat > forbidden_var/dune-project <<EOF
+  > (lang dune 2.3)
+  > EOF
+  $ dune exec ./foo.exe --root forbidden_var
+  Entering directory 'forbidden_var'
+  File "dune", line 3, characters 19-32:
+  3 |  (enabled_if (<> %{project_root} "")))
+                         ^^^^^^^^^^^^^
+  Warning: Only architecture, system, model, os_type, ccomp_type, profile and
+  ocaml_version variables are allowed in this 'enabled_if' field. If you think
+  that project_root should also be allowed, please file an issue about it.
+  Entering directory 'forbidden_var'
+  bar
+
+For dune >= 2.6 it is an error
+  $ cat > forbidden_var/dune-project <<EOF
+  > (lang dune 2.6)
+  > EOF
+  $ dune exec ./foo.exe --root forbidden_var
+  Entering directory 'forbidden_var'
+  File "dune", line 3, characters 19-32:
+  3 |  (enabled_if (<> %{project_root} "")))
+                         ^^^^^^^^^^^^^
+  Error: Only architecture, system, model, os_type, ccomp_type, profile and
+  ocaml_version variables are allowed in this 'enabled_if' field. If you think
+  that project_root should also be allowed, please file an issue about it.
   [1]

test/blackbox-tests/test-cases/enabled_if/run.t

@@ -58,5 +58,6 @@ This one unse forbidden variables
   3 |  (enabled_if (= %{project_root} "")))
                         ^^^^^^^^^^^^^
   Error: Only architecture, system, model, os_type, ccomp_type, profile and
-  ocaml_version are allowed in this 'enabled_if' field.
+  ocaml_version variables are allowed in this 'enabled_if' field. If you think
+  that project_root should also be allowed, please file an issue about it.
   [1]