Fix migration of 5.4's Pexp_pack

[dra27-js]

In 5.4, ocaml/ocaml#13809 effectively changed where attributes appear on package types, and the migration back to 5.3 drops them:

let () =
  List.iter
    (fun source ->
       let structure = Ppxlib.Parse.implementation (Lexing.from_string source) in
       Format.printf "in:  %s@." source;
       Format.printf "out: %a@.@." Ppxlib.Pprintast.structure structure)
    [ "let f (module K : S with type t = int [@foo]) = ()"
    ; "let g (x : int [@foo]) = ()"
    ]

gives:

in:  let f (module K : S with type t = int [@foo]) = ()
out: let f ((module K)  : (module S with type t = int)) = ()

in:  let g (x : int [@foo]) = ()
out: let g (x : ((int)[@foo ])) = ()

for OCaml 5.4/5.5 before this patch and the correct out: let f ((module K) : (((module S with type t = int))[@foo ])) = () with it.

[patricoferris]

Thanks for the report @dra27-js -- taking a look :-)

[patricoferris]

Hmm we do drop the package attributes here:

ppxlib/astlib/migrate_504_503.ml

Lines 460 to 468 in b9646cc

	and copy_package_type :
	Ast_504.Parsetree.package_type -> Ast_503.Parsetree.package_type =
	fun { ppt_path; ppt_cstrs; ppt_loc = _; ppt_attrs = _ } ->
	( copy_loc copy_Longident_t ppt_path,
	List.map
	(fun x ->
	let x0, x1 = x in
	(copy_loc copy_Longident_t x0, copy_core_type x1))
	ppt_cstrs )

And your fix re-adds them in the two places where a package_type might be used: a Pexp_unpack and a Ptyp_package.

It would be good to add a quick test case for this, something like this in test/504_migrations.

+ Check package_type attributes roundtrip:
+
+  $ 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])) = ()
+  > type t = (module S [@foo])
+  >
+  > EOF
+
+  $ ./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 ]))) = ()
+  type t = (((module S))[@foo ])
+  $ ./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 ]))) = ()
+  type t = (((module S))[@foo ])

Happy to do that for you if you like @dra27-js ?

[NathanReb]

It would be good to ensure the attributes are correctly preserved through round trips as well! Happy to take a look at that if needed!

[dra27-js]

Sure - feel free to push this branch (or I can have a look later in the week)

[patricoferris]

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]

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]

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