Add additional arbitrary instances

[MaximilianAlgehed]

There are a bunch of types that have been added to base in recent years. The goal here is to bring Test.QuickCheck.Arbitrary up to speed with these new types.

closes #347
closes #360

[phadej]

This breaks contract with quickcheck-instances. Now the code will be duplicated for instances using compatibility packages.

I'm not a fan.

[MaximilianAlgehed]

Well, quickcheck-instances provides orphan instances for things in base that should have been provided by QuickCheck from the start. Updating the version bounds on quickcheck-instances is trivial and this avoids some (many) use cases having to depend on additional packages.

It does make sense to have a compatibility packages for things not in base. It makes no sense to have one for things in base.

[MaximilianAlgehed]

@nick8325 thoughts?

[nick8325]

I think that quickcheck-instances is very useful and we shouldn't break it (thank you @phadej for maintaining it all these years!)

IIRC, the reason why Arbitrary instances for Data.List.NonEmpty and friends are defined in quickcheck-instances is because, although they live in base now, they originally lived in semigroups. And it wasn't reasonable to add semigroups as a dependency of QuickCheck as it has a whole ton of dependencies. I think the same situation applies to Natural.

So it's not as simple as "the type is in base so we should have it in QuickCheck". Somewhere there also needs to be an Arbitrary instance for older versions of GHC, before the type was moved to base. I tend to agree with @phadej that having instances live in different packages depending on GHC version would be awkward.

But I do think it's unfortunate that these instances don't live in QuickCheck just for the sake of some rather old GHC version and it would be really nice to fix that. One option would be to (1) move the NonEmpty instance into QuickCheck, but (2) add a dependency on semigroups for old GHCs so that it still works there. We could perhaps have a general policy that we do this once the type has been in base for a while. The downside would be long CI times for users of old GHCs, but if we're a bit conservative there shouldn't be many of them. What do you think about this approach, @phadej, @MaximilianAlgehed?

[phadej]

We could perhaps have a general policy that we do this once the type has been in base for a while.

What is "for a while"?

I have added instances to quickcheck-instances as soon as there are types in base (or at least as soon as I became aware of it). E.g. recently added Solo and ByteArray. Having instances for them somewhere is a necessity for testing thing using the new types.

First adding to quickcheck-instances and then moving to QuickCheck after some time would be quite inconvenient. (and as a user I'd just depend on quickcheck-instances, so moving them later to QuickCheck would benefit dependency footprint much).

The downside would be long CI times for users of old GHCs, but if we're a bit conservative there shouldn't be many of them.

That's actually a non-argument. The dependencies are very well cached by build tools and CIs.

---

My opinion is that perfectly QuickCheck should have instances for types in all libraries bundled with GHC (e..g. Text, ByteString, Day, UTCTime etc not only types in containers) and used compatibility packages (like semigroups, data-array-byte, time-compat) to have instances uniformly across different GHC versions, and add these instances ASAP after the libraries are updated.

Then quickcheck-instances would have instances for types in packages like vector, primitive etc which aren't bundled with GHC but are widely used anyway.

But that puts pressure on you to work on QuickCheck for a bit every six months or so, which is the GHCs release cadence.

I think that all intermediate solutions between that and current extreme approaches sub-optimal and "arbitrary".

---

Another not so bad solution is to require to use GHC-8+ version if the compiler is GHC, i.e. base-4.9.0.0 as the lower bound. That GHC / base is 7 years old and have quite a lot of stuff (types from semigroups, Natural, Void). That would remove most of the CPP guards. Saying "if you use GHC you'll get the instances always, but you need a "recent" GHC though". That is IMO fair compromise.

But this approach is less clear, not "continuous". At some point in the future you'd want to bump the lower bound again. (FWIW GHC-9.0 and possible future GHC-10.0 are very arbitrary. It just happens that in GHC-8.0 there was actually a fair bit of stuff in base changing, not so for GHC-9.0; the supermajor GHC version has mostly marketing value AFAICT).

[MaximilianAlgehed]

My take on this is that base changing makes things awkward and there is no good way around that. Most people will use QuickCheck on a recent GHC version and that seems like a reasonable thing to support out of the box.

Those users are currently hurt by this policy (as evidenced by multiple open issues that have been around for over a year) and we would be right to fix that.

Now the choice is just where we put the awkwardness. I don't mind putting it in QuickCheck but I think it would be much more reasonable to put it in quickcheck-instances - that way no users see longer CI times at the small cost of adding an ifdef for the version of QuickCheck around the relevant instances in quickcheck-instances. Another benefit of this approach is that it centralises the problem of dealing with things moving between auxiliary packages and base to quickcheck-instances instead of spreading it between QuickCheck and quickcheck-instances. Isn't that the whole point of having a compatibility shim - for it to solve the compatibility problems?

If having these things in two different places is a problem I suggest moving the compatibility shim and the main packages into the same repo - that way there won't be any issues around communication and synchronisation to worry about.

[phadej]

that way no users see longer CI times

Again, that is not a valid argument in 2024.

[MaximilianAlgehed]

Fine, longer build times then.

[MaximilianAlgehed]

Considering we released a new version of QuickCheck a while ago and quickcheck-instances hasn't kept up (I even made a PR that was summarily rejected to bump the versions), I'm more inclined to figure out a solution where we don't have two repositories that are holding up deploying new versions of QuickCheck.

@nick8325 and @UlfNorell can we try to figure out a way to either move quickcheck-instances into this repository as a separate package or fork it? We'll need to figure out what to do about hackage, perhaps @phadej has some thoughts?

[phadej]

@MaximilianAlgehed QuickCheck-2.15 was released 3 weeks ago. GHC-9.10.1 last weekend. I was deliberately waiting for the latter. (And cannot do things any faster ATM, as GHC-9.10.1 is not supported by all quickcheck-instances dependencies yet)

If about month is not acceptable latency in catching up in support, then our maintenance philosophy disagrees so fundamentally, our co-operation won't be fruitful.

---

Previously, the reasonable latency haven't been any problem. What had changed recently?

---

Remember, most people in Hackage ecosystem maintain libraries on their free time, maybe partly (hardly ever fully) sponsored by their employer. Be reasonable in your expectations.

[MaximilianAlgehed]

I take issue with the idea that someone complains about changes here breaking things downstream, then don't accept help downstream.

QuickCheck has been functionally neglected for some time. I'm trying to make sure that isn't the case any more. The point I was making is that the three week delay is unnecessary and an artifact of having two different repositories. It would not be necessary.

[MaximilianAlgehed]

If you don't have the time to support a library, and you don't want help. Then I ask you to be reasonable about the expectations you put on others to take your input seriously.

We've been delaying resolving this PR for some time due to your comments about "breaking constract" with quickcheck-instances. But if you want to impose constraints like that upstream I would suggest you either:

1. Accept help doing trivial things like bumping version bounds, or
2. At the very least communicate your timeline when rejecting PRs

I want to resolve this in a way that doesn't require keeping things in synch across multiple different repositories with a maintainer who doesn't communicate.

[phadej]

I take issue with the idea that someone complains about changes here breaking things downstream, then don't accept help downstream.

Yes, I don't want things to change unnecessarily.

I also don't need help in bumping bounds. That's trivial change, I'll do it eventually. But I don't want to be doing trivial changes every other week either.

The point I was making is that the three week delay is unnecessary and an artifact of having two different repositories.

If Quickcheck-2.15 were released somewhere else in GHC release cycle, then things would been different., I'd updated quickcheck-instances promptly (and other packages which I maintain which depend on QuickCheck directly). But as GHC releases cause complete scan through everything, I opted to wait.

Accept help doing trivial things like bumping version bounds, or

That is tricky to explain. On the other hand this is trivial, on the other hand I need to trust the person I'd give them commit and upload rights.

At the very least communicate your timeline when rejecting PRs

My work on quickcheck-instances is currently blocked on haskell-unordered-containers/unordered-containers#489

[MaximilianAlgehed]

My work on quickcheck-instances is currently blocked on haskell-unordered-containers/unordered-containers#489

My PR was not blocked on this. And you don't have to trust me to review the PR - you just need to be able to read code.

[phadej]

@MaximilianAlgehed your PR stricltly isn't. But as I said, i don't want to do trivial stuff every other week, so when I'll be able to do GHC-9.10.1 compatible release of quickcheck-instances it will support QC-2.15 too. I hope that will happen in a week.

[MaximilianAlgehed]

Fine.

Getting back to the PR at hand. Clearly adding missing instances from base isn't unnecessary as the current policy is (as evidenced by multiple issues on this repo) confusing to users. Furthermore, telling users "btw, if you want to use this library with base you have to go get this other package too" is terrible UX.

And evidently having two different repoa for these two packages is also terrible DX (as evidenced by it causing such extreme disagreement between the two of us). So..? We need some solution to this.

I propose having one repo with two packages (to decrease migration burden downstream) and putting the instances for base in the main QuickCheck package.

[phadej]

quickcheck-instances is not only adding instances for types from base package. There's plenty of more.

You are free to add instances from to base to QuickCheck if you wish (or any package for that matter), no need to take my opinions into account. I'll adapt quickcheck-instances accordingly. Don't worry about code duplication.

I'd kindly ask that you'd then depend on compatiblity packages in QuickCheck though (e.g. data-array-byte for ByteArray, semigroups for NonEmpty and so one), if your support window include base versions yet without these packages. That would reduce the code duplication.

I have written a blog post about compatibility packages https://oleg.fi/gists/posts/2019-06-03-compat-packages.html, but it's a bit dated. Luckily, IIRC, only ByteArray is recently added type to base which makes sense to have QuickCheck instances.