Prepare for 5.2 AST bump

[patricoferris]

In preparation for ocaml/opam-repository#27478 there is WIP documentation about the AST bump on the ppxlib wiki: https://github.com/ocaml-ppx/ppxlib/wiki/Upgrading-to-ppxlib-0.36.0

[jmid]

Thanks! So IIUC, the previous versions need a ppxlib.0.35 upper bound, and then it would be nice to release a version with this patch once ppxlib.0.36 is available from the opam-repo, right?

[patricoferris]

That's correct :)) Once we 0.36.0 is released, having a release with >=0.36.0 would be great. That opam file change should be a part of this PR but let's wait for the actual release so CI will pick it up.

[jmid]

I just merged #335 (incl. suitable opam updates) and rebased on main.

The CI is still failing unfortunately:
https://github.com/c-cube/qcheck/actions/runs/13708003567/job/38337945815?pr=331

[ERROR] Package conflict!
  * No agreement on the version of ppxlib:
    - deps-of-ppx_deriving_qcheck → ppx_deriving >= 5.2.1 → ppxlib < 0.36.0
    - deps-of-ppx_deriving_qcheck → ppxlib >= 0.36.0

AFAICS this is because the latest ppx_deriving needs ppxlib < 0.36.0
https://github.com/ocaml/opam-repository/blob/master/packages/ppx_deriving/ppx_deriving.6.0.3/opam#L22
as such we need that one to support ppxlib.0.36.0 too... 😬

[patricoferris]

Hot off the press! ppx_deriving 6.1.0 has just been released which you can use :)

[jmid]

Thanks for all your work on this Patrick! 🙏
ATM I'm awaiting the opam repo merge of ocaml/opam-repository#27621
so that I can retrigger the CI and pick up the release.

[jmid]

OK, I rebased and added a ppx_deriving boundary too.

I see the ocaml 5.2 and 5.3 workflows passing now, but all 4.08 to 5.0 workflows are failing the same test,
namely a textual test of the derived recursive function for generating polymorphic variants:
https://github.com/c-cube/qcheck/actions/runs/13972035078/job/39116089432?pr=331#step:7:115

  ┌──────────────────────────────────────────────────────────────────────────────┐
  │ [FAIL]        deriving generator good         30   deriving rec poly var...  │
  └──────────────────────────────────────────────────────────────────────────────┘
  ASSERT deriving recursive polymorphic variants
  FAIL deriving recursive polymorphic variants
  
     Expected: `"let rec gen_tree_sized gen_a n : tree QCheck.Gen.t=\n  match n with\n  | 0 -> QCheck.Gen.map (fun gen0 -> `Leaf gen0) gen_a\n  | _ ->\n      QCheck.Gen.frequency\n        [(1, (QCheck.Gen.map (fun gen0 -> `Leaf gen0) gen_a));\n        (1,\n          (QCheck.Gen.map (fun gen0 -> `Node gen0)\n             (QCheck.Gen.map (fun (gen0, gen1) -> (gen0, gen1))\n                (QCheck.Gen.pair ((gen_tree_sized gen_a) (n / 2))\n                   ((gen_tree_sized gen_a) (n / 2))))))]\nlet gen_tree gen_a = QCheck.Gen.sized (gen_tree_sized gen_a)\nlet arb_tree_sized gen_a n = QCheck.make @@ ((gen_tree_sized gen_a) n)\nlet arb_tree gen_a = QCheck.make @@ (gen_tree gen_a)"'
  
     Received: `"let rec gen_tree_sized gen_a n =\n  (match n with\n   | 0 -> QCheck.Gen.map (fun gen0 -> `Leaf gen0) gen_a\n   | _ ->\n       QCheck.Gen.frequency\n         [(1, (QCheck.Gen.map (fun gen0 -> `Leaf gen0) gen_a));\n         (1,\n           (QCheck.Gen.map (fun gen0 -> `Node gen0)\n              (QCheck.Gen.map (fun (gen0, gen1) -> (gen0, gen1))\n                 (QCheck.Gen.pair ((gen_tree_sized gen_a) (n / 2))\n                    ((gen_tree_sized gen_a) (n / 2))))))] : tree QCheck.Gen.t)\nlet gen_tree gen_a = QCheck.Gen.sized (gen_tree_sized gen_a)\nlet arb_tree_sized gen_a n = QCheck.make @@ ((gen_tree_sized gen_a) n)\nlet arb_tree gen_a = QCheck.make @@ (gen_tree gen_a)"'
  
  Raised at file "src/alcotest-engine/test.ml", line 216, characters 4-261
  Called from file "src/alcotest-engine/core.ml", line 186, characters 17-23
  Called from file "src/alcotest-engine/monad.ml", line 24, characters 31-35

It seems to be due to the type annotation following the function header in the Expected: part: let rec gen_tree_sized gen_a n : tree QCheck.Gen.t=... whereas it follows after the function body in the Received: part.

The thing is, the choice of printing it first in the header seems to be caused by Ppxlib.Pprintast.string_of_structure [%stri ...]

qcheck/test/ppx_deriving_qcheck/deriver/qcheck/test_textual.ml

Lines 16 to 18 in a15d5de

	let check_eq ~expected ~actual name =
	let f = Ppxlib.Pprintast.string_of_structure in
	Alcotest.(check string) name (f expected) (f actual)

as the expected string has the type annotation last:

qcheck/test/ppx_deriving_qcheck/deriver/qcheck/test_textual.ml

Lines 776 to 799 in a15d5de

	let test_recursive_poly_variant () =
	let expected =
	[
	[%stri
	let rec gen_tree_sized gen_a n =
	(match n with
	| 0 -> QCheck.Gen.map (fun gen0 -> `Leaf gen0) gen_a
	| _ ->
	QCheck.Gen.frequency
	[
	( 1,
	QCheck.Gen.map (fun gen0 -> `Leaf gen0) gen_a
	);
	( 1,
	QCheck.Gen.map
	(fun gen0 -> `Node gen0)
	(QCheck.Gen.map
	(fun (gen0, gen1) -> (gen0, gen1))
	(QCheck.Gen.pair
	((gen_tree_sized gen_a) (n / 2))
	((gen_tree_sized gen_a) (n / 2))))
	);
	]
	: tree QCheck.Gen.t)];

Since the test works on 5.2 and 5.3 with the 5.2-AST and worked with ppxlib and ppx_deriving over the previous AST, could this be a regression caused by the latest cross-version-AST-mappings? 🤔

[patricoferris]

Thanks for the report @jmid -- taking a look :)

[patricoferris]

Yeah, I think this is a known thing in the roundtripping of this code. See this comment in the migration from 501 to 502 in ppxlib

https://github.com/ocaml-ppx/ppxlib/blob/37d7ee13f4dcac44de5244a1c1e19652a5880075/astlib/migrate_501_502.ml#L173-L181

I think that's what we're seeing here, the pexp_constraint is now fully around the body of the function and not in the function argument section. CC-ing @NathanReb though in case I've missed something here.

[jmid]

I've just force pushed after rebasing on main (to get CI green again),
disabled the textual diff test that was failing on 5.1 and earlier due to a moved type annotation, and
added a CHANGELOG entry (we don't have a separate one for ppx_deriving_qcheck yet)

Still red, but now we are down to pre-4.14 failures because Sys.ocaml_release was added there...

[jmid]

Finally all green - I'll merge. Thanks again @patricoferris 🙏 🙂