fromSetA follow up changes

[L0neGamer]

Finally getting to some of the changes requested by @meooow25 on #1163

In this PR we remove some unused instances, and try and share the implementations for some other instances. We also do some cleanup for some of the testing code. We use liftA3 instead of manually combining three actions (which uses liftA2 in the default implementation), and we also verify that the result of fromSetA is correct in the case of the action ordering property test.

[treeowl]

Both fromSet and fromSetA seem to be missing validity (i.e., data structure invariant) tests. I don't understand your (partial?) collection of Arbitrary instances, or why that's in this PR.

[L0neGamer]

I would appreciate guidance on what to look for for data structure invariant tests.

I moved those arbitrary instances to a separate module because, at least for the Set one, the instance was needlessly duplicated across modules and had increased complexity for no reason. If there was some shared containers-testing lib, then this module could be compiled once for all testing executables, but that is not the case currently.

I would've moved the Map ones as well but of course the strict and lazy variants would have to have the odd cpp-based substitution, and that was complexity I wanted to avoid.

[treeowl]

Understood. The function you're looking for is called valid. For largely hysterical raisins, it's actually exposed from Data.Set. (In modern Haskell library design, there'd probably be a separate Unsafe module for functions that can break the invariants, and we'd probably expose the functions for diagnosing said breakage there.)

[treeowl]

I'm generally okay with this. Do you intend to reorganize the other Arbitrary instances in a follow-up?

[L0neGamer]

I can if wanted but I don't think it would be the cleanest solution.

Since this was triggered by @meooow25's request, I ask that they approve as well before merging.

[meooow25]

Looks good to remove the Bot instances, improve the tests, and use liftA3.

But I also don't think the other Arbitrary changes belong here.
IMO there is no benefit in touching those instances at all. That little bit of duplication is harmless.

[L0neGamer]

@meooow25 got around to undoing those changes. I keep my removal of IsInt in favour of Int ~. If it somehow fails the CI, then I'll revert that as well.

[treeowl]

You seem to have done some substantial reworking of Arbitrary instances here, none of which are related to the purpose of the PR. Please don't do that. You can propose those changes (with explanations of why, and preferably some more source comments too) in a separate PR.

[treeowl]

The purpose of IsInt is to support potential future Haskell compilers that don't support GHC-style type equality constraints. I believe @ekmett is among those who thinks those are a plausible future target. I suppose we could just wait for them to actually exist....

[L0neGamer]

Happy to discuss IsInt on the other PR; one option is that we could use mhs in the testing pipeline as well if we do want to commit to non-ghc testing. My impression was that while the testing is ghc only, we may as well make it more readable.

[treeowl]

The last I heard, MHS wasn't ready for QuickCheck. But maybe @augustss can give us an update.

[treeowl]

I definitely don't think that change belongs in this pull request; it's a much better fit for the other one.

[augustss]

Mhs can compile quickcheck, but not many of the testing frameworks. Yet.

[Bodigrim]

Mhs can compile quickcheck, but not many of the testing frameworks. Yet.

(I'm trying to get tasty compiling with MicroHs, that's why my recent questions in MicroHs / MicroCabal repos)

[meooow25]

Thanks!