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

fix(melange): run melc ppx with merlin #6476

Merged
merged 25 commits into from
Nov 30, 2022
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
0d971e5
fix(melange): run melc ppx with merlin
jchavarri Nov 15, 2022
c866b7e
Merge branch 'main' into melange/fix-melc-ppx
jchavarri Nov 16, 2022
5de0797
pass compiler flags to ppx directive
jchavarri Nov 16, 2022
b5c979e
Merge branch 'main' into melange/fix-melc-ppx
jchavarri Nov 16, 2022
993c0ff
Revert "pass compiler flags to ppx directive"
jchavarri Nov 16, 2022
4fd850e
Merge branch 'main' into melange/fix-melc-ppx
jchavarri Nov 23, 2022
7abc35c
Merge branch 'main' into melange/fix-melc-ppx
jchavarri Nov 24, 2022
4acd9e1
use dune ocaml merlin dump-config /home/me/code/dune
jchavarri Nov 24, 2022
858dac6
remove flags from dune file
jchavarri Nov 24, 2022
c4a6d2b
fix dump-dot-merlin
jchavarri Nov 24, 2022
5c34973
quote melc ppx flag
jchavarri Nov 24, 2022
f2b7a96
fix test
jchavarri Nov 28, 2022
c16eec2
Merge branch 'main' into melange/fix-melc-ppx
jchavarri Nov 28, 2022
c6e8bd6
merlin: print absolute path to melc
jchavarri Nov 29, 2022
f698bd6
merlin: lift melc_flags to Processed.config
jchavarri Nov 29, 2022
9146f1b
Merge branch 'main' into melange/fix-melc-ppx
jchavarri Nov 29, 2022
e759821
merlin: fix after breakage upstream
jchavarri Nov 29, 2022
97c0115
_
rgrinberg Nov 29, 2022
437fabd
merlin: fix tests
jchavarri Nov 29, 2022
dfff493
merlin: remove opt
jchavarri Nov 29, 2022
dadc896
merlin: don't add melange flags to ocaml builds
jchavarri Nov 30, 2022
9bc6c7d
Merge branch 'main' into melange/fix-melc-ppx
jchavarri Nov 30, 2022
8bacbb7
merlin: pass all flags at once
jchavarri Nov 30, 2022
2d27c73
Merge branch 'main' into melange/fix-melc-ppx
jchavarri Nov 30, 2022
8ac63ba
Merge branch 'main' into melange/fix-melc-ppx
rgrinberg Nov 30, 2022
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
47 changes: 34 additions & 13 deletions src/dune_rules/merlin.ml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ module Processed = struct
; src_dirs : Path.Set.t
; flags : string list
; extensions : string Ml_kind.Dict.t list
; mode : [ `Ocaml | `Melange ]
}

(* ...but modules can have different preprocessing specifications*)
Expand Down Expand Up @@ -68,7 +69,7 @@ module Processed = struct

let serialize_path = Path.to_absolute_filename

let to_sexp ~pp { stdlib_dir; obj_dirs; src_dirs; flags; extensions } =
let to_sexp ~pp { stdlib_dir; obj_dirs; src_dirs; flags; extensions; mode } =
let make_directive tag value = Sexp.List [ Atom tag; value ] in
let make_directive_of_path tag path =
make_directive tag (Sexp.Atom (serialize_path path))
Expand All @@ -94,11 +95,20 @@ module Processed = struct
(Sexp.List (List.map ~f:(fun s -> Sexp.Atom s) flags))
]
in
match pp with
| None -> flags
| Some { flag; args } ->
let flags =
match pp with
| None -> flags
| Some { flag; args } ->
make_directive "FLG"
(Sexp.List [ Atom (Pp_kind.to_flag flag); Atom args ])
:: flags
in
match mode with
| `Ocaml -> flags
| `Melange ->
make_directive "FLG"
(Sexp.List [ Atom (Pp_kind.to_flag flag); Atom args ])
(Sexp.List
[ Atom (Pp_kind.to_flag Ppx); Atom "melc -as-ppx -bs-jsx 3" ])
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the melange ppx come first?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Isn't the merlin syntax something like FLG -ppx melc -as-ppx -bs-jsx 3? The first atom is what renders the -ppx flag next to FLG.

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 make a note to remove the -bs-jsx-3 stuff eventually. Here's my rough plan:

:: flags
in
let suffixes =
Expand All @@ -120,7 +130,8 @@ module Processed = struct
in
if String.need_quoting s then Filename.quote s else s

let to_dot_merlin stdlib_dir pp_configs flags obj_dirs src_dirs extensions =
let to_dot_merlin stdlib_dir pp_configs flags obj_dirs src_dirs extensions
(* TODO print melange flag *) _mode =
let b = Buffer.create 256 in
let printf = Printf.bprintf b in
let print = Buffer.add_string b in
Expand Down Expand Up @@ -183,32 +194,42 @@ module Processed = struct
| Error msg -> Printf.eprintf "%s\n" msg
| Ok [] -> Printf.eprintf "No merlin configuration found.\n"
| Ok (init :: tl) ->
let pp_configs, obj_dirs, src_dirs, flags, extensions =
let pp_configs, obj_dirs, src_dirs, flags, extensions, mode =
(* We merge what is easy to merge and ignore the rest *)
List.fold_left tl
~init:
( [ init.pp_config ]
, init.config.obj_dirs
, init.config.src_dirs
, [ init.config.flags ]
, init.config.extensions )
, init.config.extensions
, init.config.mode )
~f:(fun
(acc_pp, acc_obj, acc_src, acc_flags, acc_ext)
(acc_pp, acc_obj, acc_src, acc_flags, acc_ext, acc_mode)
{ modules = _
; pp_config
; config =
{ stdlib_dir = _; obj_dirs; src_dirs; flags; extensions }
{ stdlib_dir = _
; obj_dirs
; src_dirs
; flags
; extensions
; mode
}
}
->
( pp_config :: acc_pp
, Path.Set.union acc_obj obj_dirs
, Path.Set.union acc_src src_dirs
, flags :: acc_flags
, extensions @ acc_ext ))
, extensions @ acc_ext
, match acc_mode with
| `Melange -> `Melange
| `Ocaml -> mode ))
in
Printf.printf "%s\n"
(to_dot_merlin init.config.stdlib_dir pp_configs flags obj_dirs src_dirs
extensions)
extensions mode)
end

let obj_dir_of_lib kind mode obj_dir =
Expand Down Expand Up @@ -418,7 +439,7 @@ module Unprocessed = struct
Path.Set.union src_dirs
(Path.Set.of_list_map ~f:Path.source more_src_dirs)
in
{ Processed.stdlib_dir; src_dirs; obj_dirs; flags; extensions }
{ Processed.stdlib_dir; src_dirs; obj_dirs; flags; extensions; mode }
Copy link
Member

Choose a reason for hiding this comment

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

Processed shouldn't include the mode. The flags should already contain what's needed.

Copy link
Collaborator Author

@jchavarri jchavarri Nov 16, 2022

Choose a reason for hiding this comment

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

Sorry, I spent some time trying to implement this, but I don't understand how it is possible.

flags includes only the compiler flags, and it's dumped in a single FLG directive here:

| flags ->
[ make_directive "FLG"
(Sexp.List (List.map ~f:(fun s -> Sexp.Atom s) flags))
]

We want melange FLG to be a separate directive. I can replace mode field in processed with something like melange_ppx_flag of type string list option or similar. In any case, something else needs to be passed to Processed in order to add this new directive.

Copy link
Member

Choose a reason for hiding this comment

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

Why does it need to be a separate FLG directive? We manage to put all the other flags in one directive.

Copy link
Collaborator Author

@jchavarri jchavarri Nov 18, 2022

Choose a reason for hiding this comment

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

I don't think so, there were two separate FLG directives already.

One for the OCaml flags:

[ make_directive "FLG"
(Sexp.List (List.map ~f:(fun s -> Sexp.Atom s) flags))
]

Another for the pp (or ppx) flags:

make_directive "FLG"
(Sexp.List [ Atom (Pp_kind.to_flag flag); Atom args ])
:: flags

I don't understand how to inject the melc ppx in either of those. The first one (afaiu) is to tell Merlin which flags are used for the OCaml compiler. The second one is for user-defined ppxs. We need a new directive to tell Merlin "you also need to run this ppx", as it's a separate process: it's not part of the compiler flags, nor a ppx that is user defined.

Do you or @anmonteiro see a way to "blend" it with any of the two other existing directives?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rgrinberg Any thoughts on the above? Considering there were already two FLG directives, should melc ppx be made part of any of them or could it exist on its own? If anything, it should be part of the second, but I don't think it's possible at the moment as it's not a user defined ppx (so can't be compiled with ppx driver).

Copy link
Member

Choose a reason for hiding this comment

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

This is hacky but it looks good enough for now. A few points:

  1. We shouldn't hard code the path of melc. We should use the absolute path that was resolved by dune
  2. We should make sure that melc is actually built if it's defined in the same workspace.

In any case, if @anmonteiro is fine with this temporary hack then we can merge it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree we should fix 1. & 2. before merging.

What's the temporary hack? the 2 FLG directives?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. I would expect all these melange ppx's to be ported to ppxlib and enabled explicitly.

Copy link
Collaborator

Choose a reason for hiding this comment

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

that's the goal for the ReactJS PPX, as highlighted in #6476 (comment).

I don't yet have a clear plan for the builtin PPX, which currently is very tightly integrated into the compiler.

and+ pp_config = pp_config t sctx ~expander in
let modules =
(* And copy for each module the resulting pp flags *)
Expand Down
13 changes: 13 additions & 0 deletions test/blackbox-tests/test-cases/melange/merlin.t
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
> (library
> (name $lib)
> (private_modules bar)
> (flags :standard -bs-jsx 3)
jchavarri marked this conversation as resolved.
Show resolved Hide resolved
> (modes melange))
> EOF

Expand All @@ -25,6 +26,13 @@
$TESTCASE_ROOT/_build/default/.foo.objs/melange)
Foo__

All 3 entries (Foo, Foo__ and Bar) contain a ppx directive

$ dune ocaml-merlin --dump-config="$(pwd)" | grep -i "ppx"
(FLG (-ppx "melc -as-ppx -bs-jsx 3"))
(FLG (-ppx "melc -as-ppx -bs-jsx 3"))
(FLG (-ppx "melc -as-ppx -bs-jsx 3"))

$ target=output
$ cat >dune <<EOF
> (melange.emit
Expand All @@ -37,3 +45,8 @@
$ dune build @check
$ dune ocaml-merlin --dump-config="$PWD" | grep -i "$target"
$TESTCASE_ROOT/_build/default/.output.mobjs/melange)

The melange.emit entry contains a ppx directive

$ dune ocaml-merlin --dump-config="$(pwd)" | grep -i "ppx"
jchavarri marked this conversation as resolved.
Show resolved Hide resolved
(FLG (-ppx "melc -as-ppx -bs-jsx 3"))