Add regression tests for qualified type

[bobot]

Qualified type are currently concatenated into a unique identifier, so it becomes a constructor:

module Q = struct
   type t = int
    [@@deriving qcheck]
end

type t = Q.t
 [@@deriving qcheck]

dune runtest
File "test/ppx_deriving_qcheck/deriver/test_qualified_names.ml", lines 6-7, characters 0-20:
6 | type t = Q.t
7 |  [@@deriving qcheck]
Error: Unbound constructor Q.gen

I added the test outside test.mlbecause string comparison doesn't show the difference (let gen = Q.gen in both case). I tried to see if the bug was obvious but I haven't found anything.

[c-cube]

Not sure either, what do you think @vch9 ?

[c-cube]

for reference, the generated code:

module Q =
  struct
    type t = int[@@deriving qcheck]
    include
      struct let _ = fun (_ : t) -> ()
             let gen = QCheck.Gen.int
             let _ = gen end[@@ocaml.doc "@inline"][@@merlin.hide ]
  end
type t = Q.t[@@deriving qcheck]
include struct let _ = fun (_ : t) -> ()
               let gen = Q.gen
               let _ = gen end[@@ocaml.doc "@inline"][@@merlin.hide ]

[vch9]

Qualified type are currently concatenated into a unique identifier, so it becomes a constructor:

Yes this is a bug, thank you for reporting it. I translated Module.gen to a Pexp_construct where it should be a Pexp_ident.

I tried to see if the bug was obvious but I haven't found anything.

Based on my last discovery, the bug is pretty much easy to fix. We can begin by having a function which create an identifier for a qualified type:

let name_lg = function
  | Lident s -> name s
  | Ldot (lg, s) -> longident_to_str lg ^ "." ^ name s (* Q.t becomes Q.gen *)
  | Lapply _ -> raise (Invalid_argument "We do not produce gen name for Lapply")

Then we change the precedent creation of generator from

let gen ~loc ?(env = TypeGen.empty) lg =
  let (module A) = Ast_builder.make loc in
  match lg with
  | Lident s ->
      Option.value ~default:(name s |> A.evar) @@ TypeGen.find_opt s env
  | Ldot (lg, s) -> A.(pexp_construct (Located.mk @@ Ldot (lg, name s)) None)
  | Lapply (_, _) -> raise (Invalid_argument "gen received an Lapply")

to

let gen ~loc ?(env = TypeGen.empty) lg =
  let (module A) = Ast_builder.make loc in
  let s = longident_to_str lg in
  let name = name_lg lg in
  Option.value ~default:(A.evar name) @@ TypeGen.find_opt s env

Adding a regressing test is a good idea (I should already have done it tbh). What would be more convenient, put our trust in the compiler ? put regression tests on the generated code ?
This last seems complicated in particular cases like this one:

for reference, the generated code:

module Q =
  struct
    type t = int[@@deriving qcheck]
    include
      struct let _ = fun (_ : t) -> ()
             let gen = QCheck.Gen.int
             let _ = gen end[@@ocaml.doc "@inline"][@@merlin.hide ]
  end
type t = Q.t[@@deriving qcheck]
include struct let _ = fun (_ : t) -> ()
               let gen = Q.gen
               let _ = gen end[@@ocaml.doc "@inline"][@@merlin.hide ]

it would not catch the difference between a Pexp_construct and a Pexp_ident.

[bobot]

Thanks for finding the erroneous pexp_constructor. But I don't understand your fix. I prefer not to convert the identifier to string. I pushed a fix.

[vch9]

But I don't understand your fix. I prefer not to convert the identifier to string. I pushed a fix.

I also prefer your fix :)

Maybe we could have a test such as

module A = struct
   type a = (int [@gen QCheck.Gen.pure 0])
    [@@deriving qcheck]

  module B = struct
    type b = (int [@gen QCheck.Gen.pure 1])
    [@@deriving qcheck]
  end
end

type a = A.t
 [@@deriving qcheck]
 
type b = A.B.t
[@@deriving qcheck] 

with a unit (or pbt?) to test assert(QCheck.Gen.generate1 gen_a = 0) and assert(QCheck.Gen.generate1 gen_b = 1)

[bobot]

Adding more tests is a good idea, you can push in the branch, but It also could go in another MR. This MR has a fix and a regresssion test.

Is the falling test normal?

[vch9]

Adding more tests is a good idea, you can push in the branch, but It also could go in another MR.

Unfortunately I can't, I'm not a QCheck's maintainer. This will be done in a follow-up MR.

[c-cube]

I don't think anyone can push onto your branch, @bobot, it's from your fork :)

[vch9]

I don't think anyone can push onto your branch, @bobot, it's from your fork :)

Can't we on github allow modifications from maintainers?

[c-cube]

Maybe, but I've never understood how to push on such a branch 😅

[bobot]

The last time I did that I believe I just added the remote of the branch author. But except for the failing test the branch is done for me, and I don't understand why the test is failing.

[vch9]

Rebase on master, #203 should fix the test.

[vch9]

LGTM, thank you for the fix :)

[vch9]

@c-cube If you also agree we could merge this

[c-cube]

let's go!

[bobot]

Is there a release planned?

[c-cube]

I kind of lost context, but I think there were just a few PRs blocking 0.19, and @jmid is going through them like a hurricane 😁

[jmid]

I most likely lost track myself! Feel free to ping me on any blocking PRs that need my input.

I've spent my weekend spring cleaning PRs #172 #174 #176 #177 to avoid bit rot. Input on #233 #234 #235 is therefore welcome!