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

Jump to bottom

Implement new module type syntax #87

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Implement new module type syntax #87

wants to merge 6 commits into from
+525 −49

Conversation

tatchi
Copy link
Collaborator

@tatchi tatchi commented Feb 21, 2023 •
edited
Loading

Note: This contains extra commits from #80, #81 and #82 that should be merged first

This PR implements the new module type syntax, so instead of

module type S_rec = [%import: (module Stuff.S_rec)]

we now write:

module type%import S_rec = Stuff.S_rec or [%%import: module type S_rec = Stuff.S_rec]

I didn't keep the old syntax, but it should be possible to have both coexist if desired.

As for the implementation, I first tried to declare an extender applied to structure_item with the following pattern psig (psig_modtype __ ^:: nil) ||| pstr (pstr_modtype __ ^:: nil) )

let module_declaration_extension =
  Ppxlib.Extension.V3.declare "import" Ppxlib.Extension.Context.structure_item
    Ppxlib.Ast_pattern.(
      psig (psig_modtype __ ^:: nil) ||| pstr (pstr_modtype __ ^:: nil) )
    module_declaration_expand

Unfortunately, this results in the following build error:

❯ dune build
File "src_test/ppx_deriving/.test_ppx_import.eobjs/_unknown_", line 1, characters 0-0:
Fatal error: exception Failure("Some ppx-es tried to register conflicting transformations: Extension 'import' on structure items declared at src/ppx_import.ml:616 matches extension 'import' declared at src/ppx_import.ml:599")

This is a bit more annoying, because we have to manually pattern-match to extract the cases we want to support, and return an error in all other cases.

let type_declaration_extension =
  Ppxlib.Extension.V3.declare "import" Ppxlib.Extension.Context.structure_item
    Ppxlib.Ast_pattern.(
      psig (psig_type __ __ ^:: nil) ||| pstr (pstr_type __ __ ^:: nil) )
    type_declaration_expand

I tried to combine both but psig (psig_modtype __ ^:: nil) ||| pstr (pstr_modtype __ ^:: nil) ) and psig (psig_type __ __ ^:: nil) ||| pstr (pstr_type __ __ ^:: nil) ) don't extract the same value. The only solution I found is to relax the pattern and extract the whole structure_item

let type_declaration_extension =
  Ppxlib.Extension.V3.declare "import" Ppxlib.Extension.Context.structure_item
    Ppxlib.Ast_pattern.(__)
    type_declaration_expander

That's a bit more annoying because we manually have to pattern match to extract the cases we want to support, and return an error in all the other cases.

I wonder if we could build a more "refined" pattern here. I've tried a few combinators to build patterns, but I haven't managed to build anything that works. I'm not sure I understood all of them though, so it could be that I'm missing something.
Maybe @pitag-ha or @panglesd could help? Or confirm that there is no other way than using the whole structure_item like I did.

Fixes #74

@tatchi tatchi requested review from ejgallego and pitag-ha February 21, 2023 07:04
@panglesd
Copy link

I wonder if we could build a more "refined" pattern here.

Yes, I think it is possible. The problem here is that you want, in one case, to extract something of some type t1, in some other case, to extract something of type t2
But this is not possible: what would be the type of the continuation?

The idea is to use a sum type to merge the two cases. Let me give an example (where the complex types are replaced with int and char to simplify):

type input = Case1 of int | Case2 of char

let extractor1 = Ast_pattern.(eint __ |> map1 ~f:(fun i -> Case1 i))
let extractor2 = Ast_pattern.(echar __ |> map1 ~f:(fun c -> Case2 c))
(* Ast_pattern.map1 maps the first argument of the continuation before applying it.
   So while [eint __] has type [(expression, int -> _, _) Ast_pattern.t],
         [extractor1] has type [(expression, input -> _, _) Ast_pattern.t] *)

let extractor = Ast_pattern.(extractor1 ||| extractor2)

let handle_case_1 (i:int) = ...
let handle_case_2 (c:char) = ...

let handle input =
  match input with
  | Case1 i -> handle_case_1 i
  | Case2 c -> handle_case_2 c

@tatchi
add test for multiple signature items
2e9a5a0 
<!-- ps-id: 597fd8ce-d920-40ad-b1e3-2bad11222b23 -->
@tatchi
add test for module type decl in signature item
2fea5f2 
<!-- ps-id: d0989bfd-2d2a-4182-89bb-40edf8006617 -->
@tatchi
add test for module type with
de40fc0 
<!-- ps-id: c4f408bb-cab8-4ea2-8486-702917493cfd -->
@tatchi
add more module type error cases
807ae22 
<!-- ps-id: d73095c3-be81-4a9c-8ad0-fddf33272dd4 -->
@tatchi
implement new module type syntax
4ebeb19 
<!-- ps-id: 144ef8fa-2d04-4b5a-9077-11afcc4283c7 -->
@tatchi tatchi force-pushed the new-module-type-syntax branch from 7b55928 to bb907f0 Compare February 23, 2023 17:14
@tatchi
Copy link
Collaborator Author

tatchi commented Feb 23, 2023

@panglesd amazing, that's exactly what I was looking for. Thanks so much for you help! 🙏🤗

@tatchi tatchi force-pushed the new-module-type-syntax branch 2 times, most recently from 8f895e3 to 2b3175a Compare February 24, 2023 05:45
@tatchi tatchi force-pushed the new-module-type-syntax branch from 2b3175a to ab733da Compare February 24, 2023 06:08
@tatchi
Copy link
Collaborator Author

tatchi commented Feb 24, 2023

Okay, so the only "downside" is that the errors are less verbose. See for example

Error: [%%import] Type pattern (PTyp) is not supported, only type and module
         type declarations are allowed

vs

Error: PStr expected

But I think that's okay. A bit more annoying is that some locations in the errors are different for OCaml versions 4.08-4.12, which is why the CI fails for these versions. We can duplicate the tests in a new folder errors_408_412 like we did for versions below 4.08, but we'll end up with 3 copies of the tests, which will be more annoying to maintain.
The other solution might be to just drop the 3 failing tests. What do you think?

@panglesd
Copy link

the only "downside" is that the errors are less verbose

I totally agree, I think that is clearly something that we should improve! I'll open an issue on the ppxlib repo.

A bit more annoying is that some locations in the errors are different for OCaml versions

It's not only that: some errors are actually syntax errors in 4.08 -> 4.12...
To keep those tests without maintaining 3 versions, you could test them only in the latest version of the compiler.

@panglesd panglesd mentioned this pull request Feb 28, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ejgallego ejgallego Awaiting requested review from ejgallego

@pitag-ha pitag-ha Awaiting requested review from pitag-ha

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update the module type syntax
2 participants
@tatchi @panglesd