Skip to content

fix(module_compilation): add dep to original source file #12774

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.

Sign up for GitHub

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

Merged
anmonteiro merged 2 commits into from
Nov 30, 2025
Merged

fix(module_compilation): add dep to original source file #12774

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d7b3254
fix(module_compilation): add dep to original source file
anmonteiro Nov 23, 2025
4ea04ec
add comment, expand the test
anmonteiro Nov 30, 2025
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
9 changes: 9 additions & 0 deletions src/dune_rules/module.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,6 +336,15 @@ let sources_without_pp t =
~f:(Option.map ~f:(fun (x : File.t) -> x.original_path))
;;

let source_without_pp ~ml_kind t =
let source =
match (ml_kind : Ml_kind.t) with
Copy link
Member

@rgrinberg rgrinberg Nov 23, 2025

Choose a reason for hiding this comment

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

I think this might need an adjustment for sources without mli's. That is, when ml_kind = Intf but there's not t.source.files.intf we should fall back to the .impl.

Would be great to have a test for this as well if you have the time.

Copy link
Collaborator Author

@anmonteiro anmonteiro Nov 25, 2025

Choose a reason for hiding this comment

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

Thanks, will have a look.

Copy link
Collaborator Author

@anmonteiro anmonteiro Nov 30, 2025

Choose a reason for hiding this comment

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

hey @rgrinberg I've expanded the test case to account for .mli files as well, but I think I'm confused about your suggestion here: when's ever the case that we want to find source files when compiling interfaces?

Copy link
Collaborator Author

@anmonteiro anmonteiro Nov 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I've merged this to unblock #12688 but open to fixing once I have a better understanding.

Copy link
Member

@rgrinberg rgrinberg Nov 30, 2025

Choose a reason for hiding this comment

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

I think you have a point. When there's no explicit mli, and there's an error message to display, it will always be in the .ml anyway.

| Impl -> t.source.files.impl
| Intf -> t.source.files.intf
in
Option.map source ~f:(fun (x : File.t) -> x.original_path)
;;

module Obj_map = struct
include Map.Make (struct
type nonrec t = t
Expand Down
1 change: 1 addition & 0 deletions src/dune_rules/module.mli
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ end

val sources : t -> Path.t list
val sources_without_pp : t -> Path.t list
val source_without_pp : ml_kind:Ml_kind.t -> t -> Path.t option
val visibility : t -> Visibility.t
val encode : t -> src_dir:Path.t -> Dune_lang.t list
val decode : src_dir:Path.t -> t Dune_lang.Decoder.t
Expand Down
5 changes: 5 additions & 0 deletions src/dune_rules/module_compilation.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ let build_cm
let* compiler = compiler in
let ml_kind = Lib_mode.Cm_kind.source cm_kind in
let+ src = Module.file m ~ml_kind in
let original = Module.source_without_pp m ~ml_kind in
let dst = Obj_dir.Module.cm_file_exn obj_dir m ~kind:cm_kind in
let obj =
Obj_dir.Module.obj_file obj_dir m ~kind:(Ocaml Cmx) ~ext:ocaml.lib_config.ext_obj
Expand Down Expand Up @@ -324,6 +325,10 @@ let build_cm
; A "-c"
; Command.Ml_kind.flag ml_kind
; Dep src
; (* We add a hidden dependency on the original, pre-PPX source
file, which the compiler wants to find to display error
location snippets. *)
Hidden_deps (Dep.Set.of_files (Option.to_list original))
; other_targets
]
>>| Action.Full.add_sandbox sandbox))
Expand Down
33 changes: 24 additions & 9 deletions test/blackbox-tests/test-cases/melange/ppx-preview.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,26 +15,41 @@ Show PPX snippet preview is shown in Dune
> let x: nope = "hello"
> EOF

$ cat > dune <<EOF
> (melange.emit
> (target output)
> (libraries the_lib)
> (emit_stdlib false))
> EOF

$ export DUNE_SANDBOX=symlink
$ dune build @melange
$ dune build @all
File "lib/the_lib.ml", line 1, characters 7-11:
1 | let x: nope = "hello"
^^^^
Error: Unbound type constructor nope
[1]

Works if the sandbox is disabled

$ export DUNE_SANDBOX=none
$ dune build @melange
$ dune build @all
File "lib/the_lib.ml", line 1, characters 7-11:
1 | let x: nope = "hello"
^^^^
Error: Unbound type constructor nope
[1]

$ cat > lib/the_lib.mli <<EOF
> val x: nope
> EOF

$ export DUNE_SANDBOX=symlink
$ dune build @all
File "lib/the_lib.mli", line 1, characters 7-11:
1 | val x: nope
^^^^
Error: Unbound type constructor nope
[1]

$ export DUNE_SANDBOX=none
$ dune build @all
File "lib/the_lib.mli", line 1, characters 7-11:
1 | val x: nope
^^^^
Error: Unbound type constructor nope
[1]

Loading