Add regression tests for qualified type

[bobot]

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

moduleQ=structtypet = int
[@@deriving qcheck]
endtypet = 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:

letname_lg=function|Lidents -> 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

letgen~loc?(env = TypeGen.empty)lg=let (moduleA) =Ast_builder.make loc inmatch lg with|Lidents ->
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

letgen~loc?(env = TypeGen.empty)lg=let (moduleA) =Ast_builder.make loc inlet s = longident_to_str lg inlet name = name_lg lg inOption.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

moduleA=structtypea = (int [@gen QCheck.Gen.pure0])
[@@deriving qcheck]
moduleB=structtypeb = (int [@gen QCheck.Gen.pure1])
[@@deriving qcheck]
endendtypea = A.t
[@@deriving qcheck]
typeb = 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!