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.

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

browser-ppx: process stritems #127

Merged
merged 3 commits into from
Apr 18, 2024
Merged

browser-ppx: process stritems #127

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dc7b324
browser-ppx: process stritems
jchavarri Apr 10, 2024
fb5769c
fix tests
jchavarri Apr 10, 2024
cb61e51
+ more exhaustive tests
jchavarri Apr 18, 2024
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
193 changes: 192 additions & 1 deletion packages/browser-ppx/ppx.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ module Builder = Ast_builder.Default
type target = Native | Js

let mode = ref Native
let tag = "browser_ppx"
let alt_tag = "platform"
let is_platform_tag str = String.equal str tag || String.equal str alt_tag

module Platform = struct
let pattern = Ast_pattern.(__')
Expand Down Expand Up @@ -273,14 +276,202 @@ module Browser_only = struct
extractor structure_item_handler)
end

module Preprocess = struct
(* This module is heavily based on leostera `config.ml` PPX:
https://github.com/ocaml-sys/config.ml/blob/d248987cc1795de99d3735c06635dbd355d4d642/config/cfg_ppx.ml*)

let eval_attr attr =
if not (is_platform_tag attr.attr_name.txt) then `keep
else
match (attr.attr_payload, !mode) with
| ( PStr
[
{
pstr_desc =
Pstr_eval
({ pexp_desc = Pexp_ident { txt = Lident "js" } }, []);
_;
};
],
Native )
| ( PStr
[
{
pstr_desc =
Pstr_eval
({ pexp_desc = Pexp_ident { txt = Lident "native" } }, []);
_;
};
],
Js ) ->
`drop
| _ -> `keep

let rec should_keep attrs =
match attrs with
| [] -> `keep
| attr :: attrs ->
if eval_attr attr = `drop then `drop else should_keep attrs

let rec should_keep_many list fn =
match list with
| [] -> `keep
| item :: list ->
if should_keep (fn item) = `drop then `drop
else should_keep_many list fn

let apply_config_on_types (tds : type_declaration list) =
List.filter_map
(fun td ->
match td with
| {
ptype_kind = Ptype_abstract;
ptype_manifest =
Some
({ ptyp_desc = Ptyp_variant (rows, closed_flag, labels); _ } as
manifest);
_;
} ->
let rows =
List.filter_map
(fun row ->
if should_keep row.prf_attributes = `keep then Some row
else None)
rows
in

if rows = [] then None
else
Some
{
td with
ptype_manifest =
Some
{
manifest with
ptyp_desc = Ptyp_variant (rows, closed_flag, labels);
};
}
| { ptype_kind = Ptype_variant cstrs; _ } ->
let cstrs =
List.filter_map
(fun cstr ->
if should_keep cstr.pcd_attributes = `keep then Some cstr
else None)
cstrs
in

if cstrs = [] then None
else Some { td with ptype_kind = Ptype_variant cstrs }
| { ptype_kind = Ptype_record labels; _ } ->
let labels =
List.filter_map
(fun label ->
if should_keep label.pld_attributes = `keep then Some label
else None)
labels
in

if labels = [] then None
else Some { td with ptype_kind = Ptype_record labels }
| _ -> Some td)
tds

let apply_config_on_structure_item stri =
match stri.pstr_desc with
| Pstr_typext { ptyext_attributes = attrs; _ }
| Pstr_modtype { pmtd_attributes = attrs; _ }
| Pstr_open { popen_attributes = attrs; _ }
| Pstr_include { pincl_attributes = attrs; _ }
| Pstr_exception { ptyexn_attributes = attrs; _ }
| Pstr_primitive { pval_attributes = attrs; _ }
| Pstr_eval (_, attrs)
| Pstr_module { pmb_attributes = attrs; _ } ->
if should_keep attrs = `keep then Some stri else None
| Pstr_value (_, vbs) ->
if should_keep_many vbs (fun vb -> vb.pvb_attributes) = `keep then
Some stri
else None
| Pstr_type (recflag, tds) ->
if should_keep_many tds (fun td -> td.ptype_attributes) = `keep then
let tds = apply_config_on_types tds in
Some { stri with pstr_desc = Pstr_type (recflag, tds) }
else None
| Pstr_recmodule md ->
if should_keep_many md (fun md -> md.pmb_attributes) = `keep then
Some stri
else None
| Pstr_class cds ->
if should_keep_many cds (fun cd -> cd.pci_attributes) = `keep then
Some stri
else None
| Pstr_class_type ctds ->
if should_keep_many ctds (fun ctd -> ctd.pci_attributes) = `keep then
Some stri
else None
| Pstr_extension _ | Pstr_attribute _ -> Some stri

let apply_config_on_signature_item sigi =
match sigi.psig_desc with
| Psig_typext { ptyext_attributes = attrs; _ }
| Psig_modtype { pmtd_attributes = attrs; _ }
| Psig_open { popen_attributes = attrs; _ }
| Psig_include { pincl_attributes = attrs; _ }
| Psig_exception { ptyexn_attributes = attrs; _ }
| Psig_value { pval_attributes = attrs; _ }
| Psig_modtypesubst { pmtd_attributes = attrs; _ }
| Psig_modsubst { pms_attributes = attrs; _ }
| Psig_module { pmd_attributes = attrs; _ } ->
if should_keep attrs = `keep then Some sigi else None
| Psig_typesubst tds ->
if should_keep_many tds (fun td -> td.ptype_attributes) = `keep then
let tds = apply_config_on_types tds in
Some { sigi with psig_desc = Psig_typesubst tds }
else None
| Psig_type (recflag, tds) ->
if should_keep_many tds (fun td -> td.ptype_attributes) = `keep then
let tds = apply_config_on_types tds in
Some { sigi with psig_desc = Psig_type (recflag, tds) }
else None
| Psig_recmodule md ->
if should_keep_many md (fun md -> md.pmd_attributes) = `keep then
Some sigi
else None
| Psig_class cds ->
if should_keep_many cds (fun cd -> cd.pci_attributes) = `keep then
Some sigi
else None
| Psig_class_type ctds ->
if should_keep_many ctds (fun ctd -> ctd.pci_attributes) = `keep then
Some sigi
else None
| Psig_extension _ | Psig_attribute _ -> Some sigi

let preprocess_impl _exp_ctxt str =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure what the _exp_ctxt variable is about, it was introduced in ppxlib v2, @davesnx do you know?

match str with
| { pstr_desc = Pstr_attribute attr; _ } :: rest
when is_platform_tag attr.attr_name.txt ->
if eval_attr attr = `keep then rest else []
| _ -> List.filter_map apply_config_on_structure_item str
Copy link
Member

Choose a reason for hiding this comment

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

I know you might not implemented this part, but why do we have 2 passes?

Probably [@platform] can be applied to any structure, not only modules in this case.

Copy link
Contributor 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 I understand the q, where are the two passes? What I understand this code is doing is:

  • If the str item itself has an the platform attr, process it
  • Otherwise, process whatever is inside the str item


let preprocess_intf _exp_ctxt sigi =
match sigi with
| { psig_desc = Psig_attribute attr; _ } :: rest
when is_platform_tag attr.attr_name.txt ->
if eval_attr attr = `keep then rest else []
| _ -> List.filter_map apply_config_on_signature_item sigi
end

let () =
Driver.add_arg "-js"
(Unit (fun () -> mode := Js))
~doc:"preprocess for js build";
Driver.V2.register_transformation "browser_ppx"
Driver.V2.register_transformation tag
~rules:
[
Browser_only.expression_rule;
Browser_only.structure_item_rule;
Platform.rule;
]
~preprocess_impl:Preprocess.preprocess_impl
~preprocess_intf:Preprocess.preprocess_intf
Comment on lines +476 to +477
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a way to do this processing using just rules so no preprocess_impl or preprocess_intf is used?

Copy link
Member

Choose a reason for hiding this comment

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

Haven't looked into the code, yet, but would be nice

23 changes: 23 additions & 0 deletions packages/browser-ppx/tests/preprocess.t
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
$ cat > input.ml << EOF
> include struct
> type t = Js.Json.t
> end [@@platform js]
>
> include struct
> type t = string
> end [@@platform native]
Copy link
Member

Choose a reason for hiding this comment

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

wondering what happens (if ppx raises) when you add platform to only one

Copy link
Contributor Author

@jchavarri jchavarri Apr 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Nothing happens (in terms of errors), added a test here:

server-reason-react/packages/browser-ppx/tests/preprocess.t

Lines 27 to 51 in cb61e51

Use only one of the platforms
$ cat > input.ml << EOF
> include struct
> type t = Js.Json.t
> end [@@platform js]
>
> include struct
> type t = string
> end
> EOF
$ ./standalone.exe -impl input.ml | ocamlformat - --enable-outside-detected-project --impl
include struct
type t = string
end
$ ./standalone.exe -impl input.ml -js | ocamlformat - --enable-outside-detected-project --impl
include struct
type t = Js.Json.t
end [@@platform js]
include struct
type t = string
end

> EOF

With -js flag it picks the block with `[@@platform js]`

$ ./standalone.exe -impl input.ml -js | ocamlformat - --enable-outside-detected-project --impl
include struct
type t = Js.Json.t
end [@@platform js]

Without -js flag, it picks the block with `[@@platform native]`

$ ./standalone.exe -impl input.ml | ocamlformat - --enable-outside-detected-project --impl
include struct
type t = string
end [@@platform native]
Copy link
Member

Choose a reason for hiding this comment

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

It should remove the annotation no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think it should. Will look into this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried removing the attributes but it's quite cumbersome. The approach I took is to transform should_keep into st like this:

  let rec should_keep attrs =
    let rec f acc attrs =
      match attrs with
      | [] -> `keep (List.rev acc)
      | attr :: attrs -> (
          match eval_attr attr with
          | `found_drop -> `drop
          | `found_keep -> f acc attrs
          | `not_found -> f (attr :: acc) attrs)
    in
    f [] attrs

The main issue I found is on the apply_config_on_structure_item function. Instead of the current simple approach, it becomes this monster:

  let apply_config_on_structure_item stri =
    match stri.pstr_desc with
    | Pstr_typext ({ ptyext_attributes = attrs; _ } as payload) -> (
        match should_keep attrs with
        | `keep attrs ->
            Some (Pstr_typext { payload with ptyext_attributes = attrs })
        | `drop -> None)
    | Pstr_modtype ({ pmtd_attributes = attrs; _ } as payload) -> (
        match should_keep attrs with
        | `keep attrs ->
            Some (Pstr_modtype { payload with pmtd_attributes = attrs })
        | `drop -> None)
    | Pstr_open ({ popen_attributes = attrs; _ } as payload) -> (
        match should_keep attrs with
        | `keep attrs ->
            Some (Pstr_open { payload with popen_attributes = attrs })
        | `drop -> None)
    | Pstr_include ({ pincl_attributes = attrs; _ } as payload) -> (
        match should_keep attrs with
        | `keep attrs ->
            Some (Pstr_include { payload with pincl_attributes = attrs })
        | `drop -> None)
    | Pstr_exception ({ ptyexn_attributes = attrs; _ } as payload) -> (
        match should_keep attrs with
        | `keep attrs ->
            Some (Pstr_exception { payload with ptyexn_attributes = attrs })
        | `drop -> None)
    | Pstr_primitive ({ pval_attributes = attrs; _ } as payload) -> (
        match should_keep attrs with
        | `keep attrs ->
            Some (Pstr_primitive { payload with pval_attributes = attrs })
        | `drop -> None)
    | Pstr_eval (str_desc, attrs) -> (
        match should_keep attrs with
        | `keep attrs -> Some (Pstr_eval (str_desc, attrs))
        | `drop -> None)
    | Pstr_module ({ pmb_attributes = attrs; _ } as payload) -> (
        match should_keep attrs with
        | `keep attrs ->
            Some (Pstr_module { payload with pmb_attributes = attrs })
        | `drop -> None)
      ...

I am sure there are ways in which it could be simplified, but after discussing with @davesnx we're punting on this for later (maybe we end up using extensions instead of attributes...)

Loading