Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a reproduction case for issue #4682 #4683

Merged
merged 2 commits into from
Jun 18, 2021
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Fix root_module and preprocessing bug
do not try to preprocess root_module. Do this by excluding it from the
set of "user written" modules.

Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
rgrinberg committed Jun 18, 2021
commit f6531b09fcbe310f0c611ecd5c6984da0ed84465
3 changes: 3 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
@@ -154,6 +154,9 @@ Unreleased
- Fix `root_module` when used in public libraries (#4685, fixes #4684,
@rgrinberg, @CraigFe)

- Fix `root_module` when used with preprocessing (#4683, fixes #4682,
@rgrinberg, @CraigFe)

2.9.0 (unreleased)
------------------

4 changes: 2 additions & 2 deletions src/dune_rules/ml_sources.ml
Original file line number Diff line number Diff line change
@@ -46,7 +46,7 @@ module Modules = struct
let rev_map =
let rev_modules =
let by_name buildable =
Modules.fold_user_written ~init:[] ~f:(fun m acc ->
Modules.fold_user_available ~init:[] ~f:(fun m acc ->
(Module.name m, buildable) :: acc)
in
List.rev_append
@@ -104,7 +104,7 @@ module Artifacts = struct
in
let modules =
let by_name modules obj_dir =
Modules_group.fold_user_written ~init:modules ~f:(fun m modules ->
Modules_group.fold_user_available ~init:modules ~f:(fun m modules ->
Module_name.Map.add_exn modules (Module.name m) (obj_dir, m))
in
let init =
33 changes: 33 additions & 0 deletions src/dune_rules/modules.ml
Original file line number Diff line number Diff line change
@@ -656,7 +656,34 @@ let wrapped_compat = function
Module_name.Map.empty
| Wrapped w -> w.wrapped_compat

let rec fold_user_available t ~f ~init =
match t with
| Stdlib w -> Stdlib.fold w ~init ~f
| Singleton m -> f m init
| Wrapped { modules; _ }
| Unwrapped modules ->
Module_name.Map.fold modules ~init ~f
| Impl { impl; vlib = _ } ->
(* XXX shouldn't we folding over [vlib] as well? *)
fold_user_available impl ~f ~init
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not fold over [vlib] because its modules are part of the virtual library and not its implementation?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should be folding over vlib modules here as well. Those modules are also "visible"

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are they not visible in a way from the virtual library? It depends how we use this function. Currently it is used for the mapping module name -> Build.t I presume.


let is_user_written m =
match Module.kind m with
| Root -> false
| Wrapped_compat
| Alias ->
(* Logically, this shold be [acc]. But this is unreachable these are stored
separately *)
assert false
| _ -> true

let rec fold_user_written t ~f ~init =
let f m acc =
if is_user_written m then
f m acc
else
acc
in
match t with
| Stdlib w -> Stdlib.fold w ~init ~f
| Singleton m -> f m init
@@ -666,6 +693,12 @@ let rec fold_user_written t ~f ~init =
| Impl { impl; vlib = _ } -> fold_user_written impl ~f ~init

let rec map_user_written t ~f =
let f m =
if is_user_written m then
f m
else
Memo.Build.return m
in
let open Memo.Build.O in
match t with
| Singleton m ->
2 changes: 2 additions & 0 deletions src/dune_rules/modules.mli
Original file line number Diff line number Diff line change
@@ -55,6 +55,8 @@ val fold_user_written : t -> f:(Module.t -> 'acc -> 'acc) -> init:'acc -> 'acc
val map_user_written :
t -> f:(Module.t -> Module.t Memo.Build.t) -> t Memo.Build.t

val fold_user_available : t -> f:(Module.t -> 'acc -> 'acc) -> init:'acc -> 'acc

(** Returns all the compatibility modules. *)
val wrapped_compat : t -> Module.Name_map.t

1 change: 0 additions & 1 deletion test/blackbox-tests/test-cases/github4682.t/ppx/dune
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
(library
(name ppx)
(kind ppx_rewriter)
(libraries lib)
(ppx.driver (main Ppx.main)))
13 changes: 12 additions & 1 deletion test/blackbox-tests/test-cases/github4682.t/ppx/ppx.ml
Original file line number Diff line number Diff line change
@@ -1 +1,12 @@
let main () = ()
let main () =
let out = ref "" in
let args =
[ ("-o", Arg.Set_string out, "")
; ("--impl", Arg.Set_string (ref ""), "")
; ("--as-ppx", Arg.Set (ref false), "")
]
in
let anon _ = () in
Arg.parse (Arg.align args) anon "";
let out = open_out !out in
close_out out;
4 changes: 0 additions & 4 deletions test/blackbox-tests/test-cases/github4682.t/run.t
Original file line number Diff line number Diff line change
@@ -2,7 +2,3 @@ Attempting to use `(root_module ...)` with an executable that uses PPX results
in an unexpected build failure:

$ dune build
Error: Multiple rules generated for _build/default/root.pp.ml-gen:
- dune:5
- <none>:1
[1]