browser-ppx: process stritems

[jchavarri]

Allows to do things like:

module T = struct
  type t = Js.Json.t
end [@@platform js]

module U = struct
  type t = string
end [@@platform native]

Both platform or browser-ppx can be used as the attribute tag (this change applies to every existing preprocessing of browser-ppx as well).

[jchavarri]

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

[jchavarri]

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

[davesnx]

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

[davesnx]

Happy to merge it as it is, we can push for a better impl with:

• usage of context free
• have proper errors when one is missing
• more test cases

[davesnx]

It should remove the annotation no?

[jchavarri]

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

[jchavarri]

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...)

[davesnx]

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

[jchavarri]

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

[davesnx]

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.

[jchavarri]

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

[jchavarri]

have proper errors when one is missing

is this necessary? I can imagine scenarios where one might want to run something only on one platform (e.g. some side effect). I also think it'll be quite challenging to implement "exhaustiveness" considering how this ppx is integrating at the str item level 🤔

more test cases

I added a bunch of them in cb61e51. Lmk what you think!

[jchavarri]

usage of context free

I took a stab at this too. It seems non-trivial because context-free:

• only supports extensions, not attributes, so we should change the API somehow to adapt to that
• it expects to return something from the rule function, but we need to erase nodes fully from the ast (Some / None)

I am not sure how to work around that 🤔