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

test(melange): show crash when melange.emit depends on transitive PPX #7948

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
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
6 changes: 4 additions & 2 deletions src/dune_rules/melange/melange_rules.ml
Original file line number Diff line number Diff line change
Expand Up @@ -469,7 +469,6 @@ let setup_emit_js_rules ~dir_contents ~dir ~scope ~sctx mel =
let* modules_for_js, _obj_dir =
modules_for_js_and_obj_dir ~sctx ~dir_contents ~scope mel
in
Resolve.push_frames resolve_error @@ fun () ->
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure why the error is fixed by not pushing the stack frames.

For reference, the error is:

  Description:
    ("Implicit_output.produce called without any handler in dynamic scope",
    { type = "Rules" })
  Raised at Stdune__Code_error.raise in file
    "otherlibs/stdune/src/code_error.ml", line 11, characters 30-62
  Called from Fiber__Core.O.(>>|).(fun) in file "otherlibs/fiber/src/core.ml",
    line 250, characters 36-41

    ...

  -> required by ("<unnamed>", ())
  -> required by ("<unnamed>", ())
  -> required by ("<unnamed>", ())
  -> required by ("<unnamed>", ())
  -> required by
     ("load-dir", In_build_dir "default/lib/test/dist/lib/test/.dist.mobjs")
  -> required by
     ("build-file",
     In_build_dir "default/lib/test/dist/lib/test/.dist.mobjs/melange.js")
  -> required by ("<unnamed>", ())
  -> required by
     ("build-alias", { dir = In_build_dir "default/lib/test"; name = "all" })
  -> required by ("<unnamed>", ())
  -> required by
     ("build-alias", { dir = In_build_dir "default"; name = "default" })
  -> required by ("toplevel", ())

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, i think we need to go to the old approach of shoving the backtrace inside the Action_builder.fail

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pushed a new version that exposes Report_error.format_memo_stack and uses it to format the error stack trace.

let module_systems = mel.module_systems in
let output = `Private_library_or_emit target_dir in
let loc = mel.loc in
Expand All @@ -478,5 +477,8 @@ let setup_emit_js_rules ~dir_contents ~dir ~scope ~sctx mel =
let file_targets = [ make_js_name ~output ~js_ext m ] in
Super_context.add_rule sctx ~dir ~loc ~mode
(Action_builder.fail
{ fail = (fun () -> Resolve.raise_error resolve_error) }
{ fail =
(fun () ->
Resolve.raise_error_with_stack_trace resolve_error)
}
|> Action_builder.with_file_targets ~file_targets)))
17 changes: 14 additions & 3 deletions src/dune_rules/resolve.ml
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,20 @@ let push_frames { stack_frames; message = _ } f =
in
loop stack_frames

let raise_error { message; _ } = raise (User_error.E message)

let error_to_memo error = push_frames error (fun () -> raise_error error)
let error_to_memo error =
push_frames error (fun () -> raise (User_error.E error.message))

let raise_error_with_stack_trace { message; stack_frames } =
match
Dune_util.Report_error.format_memo_stack
(List.map stack_frames ~f:Lazy.force)
with
| None -> raise (User_error.E message)
| Some stack ->
let message =
{ message with paragraphs = message.paragraphs @ [ stack ] }
in
raise (User_error.E message)

let read_memo = function
| Ok x -> Memo.return x
Expand Down
4 changes: 1 addition & 3 deletions src/dune_rules/resolve.mli
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,7 @@ type error

val to_result : 'a t -> ('a, error) result

val raise_error : error -> 'a

val push_frames : error -> (unit -> 'a Memo.t) -> 'a Memo.t
val raise_error_with_stack_trace : error -> 'a

(** Read a [Resolve.t] value inside the action builder monad. *)
val read : 'a t -> 'a Action_builder.t
Expand Down
3 changes: 3 additions & 0 deletions src/dune_util/report_error.mli
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,6 @@ exception Already_reported

(** Print the memo stacks of errors. *)
val print_memo_stacks : bool ref

(** Format a list of Memo stack frames into a user-friendly presentation *)
val format_memo_stack : 'a Pp.t list -> 'a Pp.t option
49 changes: 49 additions & 0 deletions test/blackbox-tests/test-cases/melange/transitive-ppx.t
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
Test interaction of melange.emit library ppx dependencies

$ mkdir lib test ppx
$ cat > dune-project <<EOF
> (lang dune 3.8)
> (package (name mel-subdir))
> (using melange 0.1)
> EOF

$ mkdir lib/test
$ touch lib/dune

$ mkdir lib/impl
$ cat > lib/impl/dune <<EOF
> (library
> (name subdir)
> (modes melange)
> (public_name mel-subdir)
> (preprocess (pps not-present)))
> EOF
$ touch lib/impl/subdir.ml

$ cat > lib/test/dune <<EOF
> (melange.emit
> (target dist)
> (emit_stdlib false)
> (modules)
> (libraries subdir))
> EOF

$ dune build
File "lib/impl/dune", line 5, characters 18-29:
5 | (preprocess (pps not-present)))
^^^^^^^^^^^
Error: Library "not-present" not found.
-> required by library "mel-subdir" in _build/default/lib/impl
-> required by melange target dist
-> required by alias lib/test/all
-> required by alias default
File "lib/impl/dune", line 5, characters 18-29:
5 | (preprocess (pps not-present)))
^^^^^^^^^^^
Error: Library "not-present" not found.
-> required by melange target dist
-> required by library "mel-subdir" in _build/default/lib/impl
-> required by _build/default/lib/test/dist/lib/test/.dist.mobjs/melange.js
-> required by alias lib/test/all
-> required by alias default
[1]