Skip to content

Fix migration of 5.4's Pexp_pack #650

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

Open
dra27-js wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix migration of 5.4's Pexp_pack #650

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bf3917b
Fix migration of 5.4's Pexp_pack
dra27 Sep 7, 2026
ed03c4d
Add package_type attributes roundtrip tests for 5.4
patricoferris Sep 8, 2026
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
2 changes: 2 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ unreleased
- Remove reduntant module type copy from migrate_504_503, preventing
exponential blowup with many "with" constraints. (#644, @smuenzel)

- Fix the 5.4/5.3 migration of attributes on package types (#650, @dra27)

0.38.0
------

Expand Down
11 changes: 9 additions & 2 deletions astlib/migrate_504_503.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ and copy_expression_desc ~loc :
ptyp_desc = package;
ptyp_loc = Location.none;
ptyp_loc_stack = [];
ptyp_attributes = [];
ptyp_attributes = copy_attributes c.Ast_504.Parsetree.ppt_attrs;
}
in
Ast_503.Parsetree.Pexp_constraint (exp, ct)
Expand Down Expand Up @@ -403,11 +403,18 @@ and copy_core_type : Ast_504.Parsetree.core_type -> Ast_503.Parsetree.core_type
Ast_504.Parsetree.ptyp_attributes;
} ->
let loc = copy_location ptyp_loc in
let package_attrs =
match ptyp_desc with
| Ast_504.Parsetree.Ptyp_package { Ast_504.Parsetree.ppt_attrs; _ } ->
copy_attributes ppt_attrs
| _ -> []
in
{
Ast_503.Parsetree.ptyp_desc = copy_core_type_desc ~loc ptyp_desc;
Ast_503.Parsetree.ptyp_loc = loc;
Ast_503.Parsetree.ptyp_loc_stack = copy_location_stack ptyp_loc_stack;
Ast_503.Parsetree.ptyp_attributes = copy_attributes ptyp_attributes;
Ast_503.Parsetree.ptyp_attributes =

@patricoferris patricoferris Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm concerned that this doesn't roundtrip very well, and this is a fact of not being able to unmix the core type attributes from the package type attributes. For example:

type t = (module S [@inner])[@outer]

Has the following (summarised for clarity), AST on OCaml 5.4:

type_declaration "t" 
  ptype_manifest =
    Some
      core_type
        attribute "outer"
          []
        Ptyp_package
          package_type "S"
          []
            attribute "inner"
              []

When we pass it through ppxlib (using the fixes here) we get type t = (((module S))[@outer ][@inner ]) with the following AST:

type_declaration "t"
  ptype_manifest =
    Some
      core_type
        attribute "outer"
          []
        attribute "inner"
          []
        Ptyp_package "S"
        []

Which is better than without the fix where we were just dropping attributes! But maybe is not ideal. I wonder if we shouldn't have an encoding for package types with attributes that we can use instead in order to safely preserve these attributes when we migrate back from 5.3 to 5.4 (which should be doable via Ptyp_extension and Pexp_extension)? What do you think @NathanReb?

I have added some tests in ed03c4d @dra27-js.

@NathanReb NathanReb Sep 8, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I would avoid wrapping in an extension here since there's a valid migration (modulo attributes). If we do encode those with an extension, that means than building and preprocessing old code with new compiler won't work, which is what we're trying to avoid.

It's not the most statisfying of ways but could we split the attributes from the types and the attributes from package by inserting our own [@ppxlib.migration.split_package_attributes]?

We just add an extra attribute that everyone will ignore and when migrating back to 5.4, the migration knows to treat all attributes after this one differently.

Does that sound doable?

@patricoferris patricoferris Sep 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

That sounds good to me @NathanReb -- if you're happy enough I can push a commit to that effect @dra27-js.

@dra27-js dra27-js Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Go for it - I can double-check again, but we're not actually hitting this particular case anymore (I can possibly check against an older tree if it would helpful to test it against "real" code that's hitting it?)

copy_attributes ptyp_attributes @ package_attrs;
}

and copy_location_stack :
Expand Down
9 changes: 9 additions & 0 deletions test/504_migrations/package-types/dune
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
(executable
(name id_driver)
(modules id_driver)
(libraries ppxlib))

(cram
(enabled_if
(>= %{ocaml_version} "5.4"))
(deps id_driver.exe))
1 change: 1 addition & 0 deletions test/504_migrations/package-types/id_driver.ml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
let () = Ppxlib.Driver.standalone ()
28 changes: 28 additions & 0 deletions test/504_migrations/package-types/run.t
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
The signature for package types, as well as packed expressions
changed inbetween.

Originally we had a bug in the migrations that would silently drop some
attributes.

$ cat > test.ml << EOF
> module type S = sig type t end
> let f (module K : S with type t = int [@foo]) = ()
> let f (module K : S with type t = (int [@foo])) = ()
> let f (module K : S with type t = (int [@foo])[@bar]) = ()
> type t = (module S [@inner])[@outer]
>
> EOF

$ ./id_driver.exe test.ml
module type S = sig type t end
let f ((module K) : (((module S with type t = int))[@foo ])) = ()
let f ((module K) : (module S with type t = ((int)[@foo ]))) = ()
let f ((module K) : (((module S with type t = ((int)[@foo ])))[@bar ])) = ()
type t = (((module S))[@outer ][@inner ])
$ ./id_driver.exe --use-compiler-pp test.ml
module type S = sig type t end
let f ((module K) : (((module S with type t = int))[@foo ])) = ()
let f ((module K) : (module S with type t = ((int)[@foo ]))) = ()
let f ((module K) : (((module S with type t = ((int)[@foo ])))[@bar ])) = ()
type t = (((module S))[@outer ][@inner ])

Loading