Support containers-0.8.

[jonathanknowles]

This PR adds support for containers version 0.8.

It:

1. Removes the custom implementation of Data.IntMap.NonEmpty.Internal.traverseMapWithKey and updates call sites to use Data.IntMap.traverseWithKey instead, which correctly traverses in ascending order of keys for containers >= 0.6.3.1 (see below.)

2. Adjusts Data.Set.NonEmpty.combineEq so that:

   • for containers < 0.8: combineEq chooses the first occurrence of each duplicated element (consistent with behaviour of containers < 0.8).
   • for containers >= 0.8: combineEq chooses the last occurrence of each duplicated element (consistent with behaviour of containers >= 0.8).

   This is to account for a change in behaviour of Data.Set.from{Asc,Desc}List between containers-0.7 and containers-0.8:

   • for containers < 0.8: Set.from{Asc,Desc}List chooses the first occurrence of any duplicated element.
   • for containers >= 0.8: Set.from{Asc,Desc}List chooses the last occurrence of any duplicated element.

3. Updates both lower and upper bounds for containers to reflect the set of versions for which all tests pass (containers >= 0.6.3.1 && < 0.9)

4. Removes the compatibility layer for containers < 0.5.11. Assuming the package is only buildable for containers >= 0.6.3.1, then it should be safe to remove this layer.

Testing

I have confirmed that the test suite passes for all versions of containers between 0.6.3.1 and 0.8 (inclusively):

• ✅ 0.8
• ✅ 0.7
• ✅ 0.6.8
• ✅ 0.6.7
• ✅ 0.6.6
• ✅ 0.6.5.1
• ✅ 0.6.4.1
• ✅ 0.6.3.1 (released in 2020-07-14)

The test suite fails for the following older versions of containers:

• 💥 0.6.2.1 (released in 2019-06-24)
• 💥 0.6.1.1
• 💥 0.6.0.1
• 💥 0.5.11.0

Tests that fail with older versions of containers:

• Tests.IntMap.mapAccumWithKey
• Tests.IntMap.sequenceA
• Tests.IntMap.sequence1
• Tests.IntMap.traverseWithKey
• Tests.IntMap.traverseWithKey1

[meooow25]

(Just passing by)
I know this is not from this PR, but this does not seem right. IntMap traverses in ascending order of keys, this implementation doesn't.

λ> import qualified Data.IntMap.Lazy as IM
λ> import qualified Data.IntMap.NonEmpty as NEIM
λ> () <$ IM.traverseWithKey (curry print) (IM.fromList [(i,i) | i <- [-3..2]])
(-3,-3)
(-2,-2)
(-1,-1)
(0,0)
(1,1)
(2,2)
λ> () <$ NEIM.traverseWithKey (curry print) (NEIM.unsafeFromMap (IM.fromList [(i,i) | i <- [-3..2]]))
(-3,-3)
(-1,-1)
(-2,-2)
(2,2)
(1,1)
(0,0)

[jonathanknowles]

(Just passing by) I know this is not from this PR, but this does not seem right. IntMap traverses in ascending order of keys, this implementation doesn't.

Hi @meooow25, many thanks for commenting, I really appreciate your analysis.

Could it be that in the past, these traversal functions were not always so well-behaved?

For example, as recently as containers-0.6.2.1, we can see a discrepancy between Data.IntMap and Data.IntMap.Strict in the behaviour of traverseWithKey (borrowing your example code):

$ cabal repl --constraint=containers==0.6.2.1
> import qualified Data.IntMap as IM
> import qualified Data.IntMap.Strict as IMS
> () <$ IM.traverseWithKey (curry print) (IM.fromList [(i,i) | i <- [-3..2]])
(-3,-3)
(-2,-2)
(-1,-1)
(0,0)
(1,1)
(2,2)
> () <$ IMS.traverseWithKey (curry print) (IM.fromList [(i,i) | i <- [-3..2]])
(0,0)
(1,1)
(2,2)
(-3,-3)
(-2,-2)
(-1,-1)

But with containers-0.6.3.1, this discrepancy is fixed:

$ cabal repl --constraint=containers==0.6.3.1
> import qualified Data.IntMap as IM
> import qualified Data.IntMap.Strict as IMS
> () <$ IM.traverseWithKey (curry print) (IM.fromList [(i,i) | i <- [-3..2]])
(-3,-3)
(-2,-2)
(-1,-1)
(0,0)
(1,1)
(2,2)
> () <$ IMS.traverseWithKey (curry print) (IM.fromList [(i,i) | i <- [-3..2]])
(-3,-3)
(-2,-2)
(-1,-1)
(0,0)
(1,1)
(2,2)

Though in any case, I think it's clear that the current implementation of traverseMapWithKey does not work as expected. Many thanks for pointing this out! I will run some more tests and revise this PR.

[jonathanknowles]

I've updated this PR to remove the custom implementation of Data.IntMap.NonEmpty.Internal.traverseMapWithKey, updating all call sites to use Data.IntMap.traverseWithKey instead.

According to my testing, the test suite now passes for all the following versions of containers:

• 0.6.3.1
• 0.6.4.1
• 0.6.5.1
• 0.6.6
• 0.6.7
• 0.6.8
• 0.7
• 0.8

[meooow25]

But with containers-0.6.3.1, this discrepancy is fixed:

Good find, the code here is likely an (incorrect) attempt to work around the containers issue.

I also think that bumping to a fixed version of containers is the best thing to do here, and one can avoid dealing with the internals 👍

[mstksg]

Thanks for reviewing -- your assement was correct, this was odd behavior in containers that was actually caught and updated by this package's property tests haskell/containers#579

[jonathanknowles]

Hi @mstksg

Hope it's okay for me to have created this PR. I had some spare time over the weekend, so I took a look at how nonempty-containers could be upgraded to support containers-0.8.

(I use nonempty-containers quite a bit—thanks for providing it!)

If there's anything you'd like me to adjust or if you’d prefer to handle things differently, just let me know—I’d be happy to help if I can.

All the best,
Jonathan

[jonathanknowles]

Hi @mstksg

Just checking in on this PR. It's been a couple of months since I opened it, so I wasn't sure if it might've been missed, or if things have just been a bit busy on your end.

If there's anything you need from my side, feel free to let me know. Thanks again!

[mstksg]

Hi jonathan -- yes, I did miss the notification initially. Appreciate this very much, thanks for following up as well! I'll see if I can push this in this week