Forbid %{targets} in pp action

CHANGES.md

@@ -123,6 +123,10 @@ unreleased
   This means that `foo.c` and `foo.cpp` can no longer exist in the same library.
   (#1788, @rgrinberg)
 
+- Forbid use of `%{targets}` (or `${@}` in jbuild files) inside
+  preprocessing actions
+  (#1812, fixes #1811, @diml)
+
 1.6.2 (05/12/2018)
 ------------------
 

src/build.ml

@@ -221,11 +221,12 @@ let action ?dir ~targets action =
   | Some dir -> Action.Chdir (dir, action)
 
 let action_dyn ?dir ~targets () =
-  Targets targets
-  >>^ fun action ->
   match dir with
-  | None -> action
-  | Some dir -> Action.Chdir (dir, action)
+  | None -> Targets targets
+  | Some dir ->
+    Targets targets
+    >>^ fun action ->
+    Action.Chdir (dir, action)
 
 let write_file fn s =
   action ~targets:[fn] (Write_file (fn, s))

src/expander.ml

@@ -170,10 +170,12 @@ module Resolved_forms = struct
     None
 end
 
-type targets =
-  | Static of Path.t list
-  | Infer
-  | Alias
+module Targets = struct
+  type t =
+    | Static of Path.t list
+    | Infer
+    | Forbidden of string
+end
 
 let path_exp path = [Value.Path path]
 let str_exp  str  = [Value.String str]
@@ -324,13 +326,13 @@ let expand_and_record_deps acc ~dir ~read_package ~dep_kind
         | Var (First_dep | Deps | Named_local) -> None
         | Var Targets ->
           let loc = String_with_vars.Var.loc pform in
-          begin match targets_written_by_user with
+          begin match (targets_written_by_user : Targets.t) with
           | Infer ->
             Errors.fail loc "You cannot use %s with inferred rules."
               (String_with_vars.Var.describe pform)
-          | Alias ->
-            Errors.fail loc "You cannot use %s in aliases."
-              (String_with_vars.Var.describe pform)
+          | Forbidden context ->
+            Errors.fail loc "You cannot use %s in %s."
+              (String_with_vars.Var.describe pform) context
           | Static l ->
             Some (Value.L.dirs l) (* XXX hack to signal no dep *)
           end

src/expander.mli

@@ -76,17 +76,19 @@ module Resolved_forms : sig
   val empty : unit -> t
 end
 
-type targets =
-  | Static of Path.t list
-  | Infer
-  | Alias
+module Targets : sig
+  type t =
+    | Static of Path.t list
+    | Infer
+    | Forbidden of string (** context *)
+end
 
 val with_record_deps
   :  t
   -> Resolved_forms.t
   -> read_package:(Package.t -> (unit, string option) Build.t)
   -> dep_kind:Lib_deps_info.Kind.t
-  -> targets_written_by_user:targets
+  -> targets_written_by_user:Targets.t
   -> map_exe:(Path.t -> Path.t)
   -> t
 

src/inline_tests.ml

@@ -231,7 +231,7 @@ include Sub_system.Register_end_point(
                SC.Action.run sctx action ~loc
                  ~expander
                  ~dep_kind:Required
-                 ~targets:Alias
+                 ~targets:(Forbidden "inline test generators")
                  ~targets_dir:dir)))
         >>^ (fun actions ->
           Action.with_stdout_to target

src/preprocessing.ml

@@ -495,7 +495,6 @@ let get_ppx_driver sctx ~loc ~scope ~dir_kind pps =
   >>| fun driver ->
   (ppx_driver_exe (SC.host sctx) libs ~dir_kind, driver)
 
-let target_var         = String_with_vars.virt_var __POS__ "targets"
 let workspace_root_var = String_with_vars.virt_var __POS__ "workspace_root"
 
 let cookie_library_name lib_name =
@@ -653,16 +652,16 @@ let make sctx ~dir ~expander ~dep_kind ~lint ~preprocess
                 >>^ (fun _ -> Bindings.empty)
                 >>>
                 SC.Action.run sctx
-                  (Redirect
-                     (Stdout,
-                      target_var,
-                      Chdir (workspace_root_var,
-                             action)))
+                  (Chdir (workspace_root_var, action))
                   ~loc
                   ~expander
                   ~dep_kind
-                  ~targets:(Static [dst])
-                  ~targets_dir:(Path.parent_exn dst)))
+                  ~targets:(Forbidden "preprocessing actions")
+                  ~targets_dir:(Path.parent_exn dst)
+                >>>
+                Build.action_dyn () ~targets:[dst]
+                >>^ fun action ->
+                Action.with_stdout_to dst action))
            |> setup_reason_rules sctx in
          if lint then lint_module ~ast ~source:m;
          ast)

src/simple_rules.ml

@@ -17,7 +17,7 @@ let dep_bindings ~extra_bindings deps =
 
 let user_rule sctx ?extra_bindings ~dir ~expander (rule : Rule.t) =
   if Expander.eval_blang expander rule.enabled_if then begin
-    let targets : Expander.targets =
+    let targets : Expander.Targets.t =
       match rule.targets with
       | Infer -> Infer
       | Static fns ->
@@ -139,7 +139,7 @@ let alias sctx ?extra_bindings ~dir ~expander (alias_conf : Alias_conf.t) =
            ~loc
            ~expander
            ~dep_kind:Required
-           ~targets:Alias
+           ~targets:(Forbidden "aliases")
            ~targets_dir:dir)
   else
     add_alias sctx

src/super_context.ml

@@ -498,14 +498,17 @@ module Action = struct
     : (Path.t Bindings.t, Action.t) Build.t =
     let dir = Expander.dir expander in
     let map_exe = map_exe sctx in
-    if targets_written_by_user = Expander.Alias then begin
-      match U.Infer.unexpanded_targets t with
-      | [] -> ()
-      | x :: _ ->
-        let loc = String_with_vars.loc x in
-        Errors.warn loc
-          "Aliases must not have targets, this target will be ignored.\n\
-           This will become an error in the future.";
+    begin match (targets_written_by_user : Expander.Targets.t) with
+    | Static _ | Infer -> ()
+    | Forbidden context ->
+         match U.Infer.unexpanded_targets t with
+         | [] -> ()
+         | x :: _ ->
+           let loc = String_with_vars.loc x in
+           Errors.warn loc
+             "%s must not have targets, this target will be ignored.\n\
+              This will become an error in the future."
+             (String.capitalize context)
     end;
     let t, forms =
       partial_expand sctx ~expander ~dep_kind
@@ -520,7 +523,7 @@ module Action = struct
           U.Infer.partial t ~all_targets:false
         in
         { deps; targets = Path.Set.union targets targets_written_by_user }
-      | Alias ->
+      | Forbidden _ ->
         let { U.Infer.Outcome. deps; targets = _ } =
           U.Infer.partial t ~all_targets:false
         in

src/super_context.mli

@@ -156,7 +156,7 @@ module Action : sig
     -> loc:Loc.t
     -> expander:Expander.t
     -> dep_kind:Lib_deps_info.Kind.t
-    -> targets:Expander.targets
+    -> targets:Expander.Targets.t
     -> targets_dir:Path.t
     -> Action_unexpanded.t
     -> (Path.t Bindings.t, Action.t) Build.t

test/blackbox-tests/dune.inc

@@ -503,6 +503,14 @@
    test-cases/github1616
    (progn (run %{exe:cram.exe} -test run.t) (diff? run.t run.t.corrected)))))
 
+(alias
+ (name github1811)
+ (deps (package dune) (source_tree test-cases/github1811))
+ (action
+  (chdir
+   test-cases/github1811
+   (progn (run %{exe:cram.exe} -test run.t) (diff? run.t run.t.corrected)))))
+
 (alias
  (name github20)
  (deps (package dune) (source_tree test-cases/github20))
@@ -1317,6 +1325,7 @@
   (alias github1549)
   (alias github1560)
   (alias github1616)
+  (alias github1811)
   (alias github20)
   (alias github24)
   (alias github25)
@@ -1470,6 +1479,7 @@
   (alias github1549)
   (alias github1560)
   (alias github1616)
+  (alias github1811)
   (alias github20)
   (alias github24)
   (alias github25)

test/blackbox-tests/test-cases/github1811/dune

@@ -0,0 +1,3 @@
+(library
+ (name foo)
+ (preprocess (action (with-stdout-to %{targets} (run cat %{input-file})))))

test/blackbox-tests/test-cases/github1811/dune-project

@@ -0,0 +1 @@
+(lang dune 1.7)

test/blackbox-tests/test-cases/github1811/foo.ml

@@ -0,0 +1 @@
+let x = 42

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

@@ -0,0 +1,13 @@
+Check that %{targets} is forbidden in preprocessing actions
+
+  $ dune build @all
+  File "dune", line 3, characters 37-47:
+  3 |  (preprocess (action (with-stdout-to %{targets} (run cat %{input-file})))))
+                                           ^^^^^^^^^^
+  Warning: Preprocessing actions must not have targets, this target will be ignored.
+  This will become an error in the future.
+  File "dune", line 3, characters 39-47:
+  3 |  (preprocess (action (with-stdout-to %{targets} (run cat %{input-file})))))
+                                             ^^^^^^^^
+  Error: You cannot use %{targets} in preprocessing actions.
+  [1]