Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move slice to Partial, add sliceMaybe function #201

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Move slice to Partial, add sliceMaybe function #201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
67994da
Move vector `slice` to Partial.hs modules.
bjornbm Sep 22, 2019
1932046
`sliceMaybe` function and specs.
bjornbm Sep 22, 2019
71922d7
`slice` updates in changelog.
bjornbm Sep 22, 2019
27ed332
Overflow-safe guard for slice upper bound.
bjornbm Sep 23, 2019
0e59957
Property testing.
bjornbm Sep 23, 2019
8673eb7
Shim `slice` to protect against overflow.
bjornbm Sep 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions rio/ChangeLog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
# Changelog for rio

## 0.1.13.0

* Move vector `slice` functions to `Partial` modules and add `RIO.Vector.sliceMaybe` function.

## 0.1.12.0

* Add `logFormat` and `setLogFormat` for `LogOptions`.
Expand Down
19 changes: 18 additions & 1 deletion rio/src/RIO/Vector.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ module RIO.Vector
, (Data.Vector.Generic.!?)

-- ** Extracting subvectors
, Data.Vector.Generic.slice
, sliceMaybe
, Data.Vector.Generic.take
, Data.Vector.Generic.drop
, Data.Vector.Generic.splitAt
Expand Down Expand Up @@ -257,3 +257,20 @@ module RIO.Vector
) where

import qualified Data.Vector.Generic
import Data.Vector.Generic (Vector)
import Control.Monad (guard)

-- | /O(1)/ Yield a slice of the vector without copying it. If the vector
-- cannot satisfy the specificed slice 'Nothing' is returned.
--
-- @since 0.1.13.0
sliceMaybe :: Vector v a
=> Int -- ^ @i@ starting index
-> Int -- ^ @n@ length
-> v a
-> Maybe (v a)
sliceMaybe i n v = do
guard $ i >= 0
guard $ n >= 0
guard $ Data.Vector.Generic.length v - n >= i
pure $ Data.Vector.Generic.slice i n v
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're doing the checking ourselves, moving to unsafeSlice is a good idea.

3 changes: 2 additions & 1 deletion rio/src/RIO/Vector/Boxed.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ module RIO.Vector.Boxed
, (Data.Vector.!?)

-- ** Extracting subvectors
, Data.Vector.slice
, V.sliceMaybe
, Data.Vector.take
, Data.Vector.drop
, Data.Vector.splitAt
Expand Down Expand Up @@ -222,3 +222,4 @@ module RIO.Vector.Boxed
) where

import qualified Data.Vector
import qualified RIO.Vector as V
2 changes: 2 additions & 0 deletions rio/src/RIO/Vector/Boxed/Partial.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ module RIO.Vector.Boxed.Partial
-- ** Extracting subvectors
, Data.Vector.init
, Data.Vector.tail
, RIO.Vector.Partial.slice -- Pending <https://gitlab.haskell.org/ghc/ghc/issues/17233>

-- * Modifying vectors
-- ** Bulk updates
Expand Down Expand Up @@ -62,3 +63,4 @@ module RIO.Vector.Boxed.Partial
) where

import qualified Data.Vector
import qualified RIO.Vector.Partial
21 changes: 21 additions & 0 deletions rio/src/RIO/Vector/Partial.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ module RIO.Vector.Partial
-- ** Extracting subvectors
, Data.Vector.Generic.init
, Data.Vector.Generic.tail
, slice -- Pending <https://gitlab.haskell.org/ghc/ghc/issues/17233>

-- * Modifying vectors
-- ** Bulk updates
Expand Down Expand Up @@ -62,3 +63,23 @@ module RIO.Vector.Partial
) where

import qualified Data.Vector.Generic

-- | /O(1)/ Yield a slice of the vector without copying it. The vector must
-- contain at least @i+n@ elements.
slice :: Data.Vector.Generic.Vector v a
=> Int -- ^ @i@ starting index
-> Int -- ^ @n@ length
-> v a
-> v a
slice i n v = if i > 0 && n > 0 && i + n < 0 -- `i+n` overflows
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather see a patch like this land in vector than here. I'm also concerned about surprising behavior of error types changing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I will remove 8673eb7 then.

-- Special case handling for cases when `i+n` overflows. This is
-- required due to <https://gitlab.haskell.org/ghc/ghc/issues/17233>.
-- Once that GHC issue is closed this function can be replaced by
-- `Data.Vector.Generic.slice`.
-- (Negative overflow is not an issue as an `Date.Vector.Generic.slice`
-- throws an exception is thrown for negative arguments.)
then error $ "slice: invalid slice ("
++ show i ++ ","
++ show n ++ ","
++ show (Data.Vector.Generic.length v) ++ ")"
else Data.Vector.Generic.slice i n v
3 changes: 2 additions & 1 deletion rio/src/RIO/Vector/Storable.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ module RIO.Vector.Storable
, (Data.Vector.Storable.!?)

-- ** Extracting subvectors
, Data.Vector.Storable.slice
, V.sliceMaybe
, Data.Vector.Storable.take
, Data.Vector.Storable.drop
, Data.Vector.Storable.splitAt
Expand Down Expand Up @@ -188,3 +188,4 @@ module RIO.Vector.Storable
) where

import qualified Data.Vector.Storable
import qualified RIO.Vector as V
2 changes: 2 additions & 0 deletions rio/src/RIO/Vector/Storable/Partial.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ module RIO.Vector.Storable.Partial
-- ** Extracting subvectors
, Data.Vector.Storable.init
, Data.Vector.Storable.tail
, RIO.Vector.Partial.slice -- Pending <https://gitlab.haskell.org/ghc/ghc/issues/17233>

-- * Modifying vectors
-- ** Bulk updates
Expand Down Expand Up @@ -60,3 +61,4 @@ module RIO.Vector.Storable.Partial
) where

import qualified Data.Vector.Storable
import qualified RIO.Vector.Partial
3 changes: 2 additions & 1 deletion rio/src/RIO/Vector/Unboxed.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ module RIO.Vector.Unboxed
, (Data.Vector.Unboxed.!?)

-- ** Extracting subvectors
, Data.Vector.Unboxed.slice
, V.sliceMaybe
, Data.Vector.Unboxed.take
, Data.Vector.Unboxed.drop
, Data.Vector.Unboxed.splitAt
Expand Down Expand Up @@ -211,3 +211,4 @@ module RIO.Vector.Unboxed
) where

import qualified Data.Vector.Unboxed
import qualified RIO.Vector as V
2 changes: 2 additions & 0 deletions rio/src/RIO/Vector/Unboxed/Partial.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ module RIO.Vector.Unboxed.Partial
-- ** Extracting subvectors
, Data.Vector.Unboxed.init
, Data.Vector.Unboxed.tail
, RIO.Vector.Partial.slice -- Pending <https://gitlab.haskell.org/ghc/ghc/issues/17233>

-- * Modifying vectors
-- ** Bulk updates
Expand Down Expand Up @@ -62,3 +63,4 @@ module RIO.Vector.Unboxed.Partial
) where

import qualified Data.Vector.Unboxed
import qualified RIO.Vector.Partial
27 changes: 27 additions & 0 deletions rio/test/RIO/VectorSpec.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
{-# LANGUAGE NoImplicitPrelude #-}
module RIO.VectorSpec where

import Test.Hspec
import Test.Hspec.QuickCheck (prop)
import qualified Test.QuickCheck as QC
import RIO
import qualified RIO.Vector as V
import qualified RIO.Vector.Partial as V'

spec :: Spec
spec =
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some quickchecks/property tests would be a great addition here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I didn't write any because I didn't see any in ListSpec.hs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was about to replace all the tests with:

spec :: Spec
spec =
  describe "sliceMaybe" $ do
    prop "is consistent with `slice` (pathological cases)" $
      \(QC.Large i) (QC.Large n) v
        -> sliceProp i n (DV.fromList v)
    prop "is consistent with `slice` (less pathological cases)" $
      \(QC.NonNegative i) (QC.NonNegative n) (QC.NonEmpty v)
        -> sliceProp i n (DV.fromList v)
  where
    sliceProp :: Int -> Int -> DV.Vector Char -> QC.Property
    sliceProp i n v = QC.withMaxSuccess 10000 $ case VB.sliceMaybe i n v of
      Just v' -> DV.slice i n v `shouldBe` v'
      Nothing -> evaluate (DV.slice i n v) `shouldThrow` anyException

(The second property could be skipped since it is implicit in the first and the number of cases could be reduced to the default of 100, but the coverage may not be extensive.)

However, these tests resulted in:

rio        > RIO.Vector       
rio        >   sliceMaybe     
rio        >     is consistent with `slice` (pathological cases) FAILED [1]
rio        >     is consistent with `slice` (less pathological cases)
rio        >       +++ OK, passed 10000 tests.
rio        >                  
rio        > Failures:        
rio        >                  
rio        >   test/RIO/VectorSpec.hs:23:18: 
rio        >   1) RIO.Vector.sliceMaybe is consistent with `slice` (pathological cases)
rio        >        Falsifiable (after 82 tests and 25 shrinks):
rio        >          Large {getLarge = 2104973872246037021}
rio        >          Large {getLarge = 7118398164608738787}
rio        >          ""      
rio        >        did not get expected exception: SomeException

Further investigation led me to open https://gitlab.haskell.org/ghc/ghc/issues/17233.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What makes you think this is a GHC bug instead of a vector library bug?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just used the heuristic that if it is a segfault or the error message says Please report this as a GHC bug it should be reported here. But it may well be (probably is?) a vector bug so I reported it as haskell/vector#257 too.

describe "sliceMaybe" $ do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's necessary to replace the unit tests with property tests, I was just hoping to include property tests as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was understood. However, I felt the property test was more to the point and completely specified the intended behavior of sliceMaybe (in terms of slice), thereby making the unit tests redundant.

Things became a bit complicated with the revealed bug of course.

prop "is consistent with `slice` (pathological cases)" $
\(QC.Large i) (QC.Large n) v
-> sliceTest i n (V.fromList v)
-- The next property is a subset of the previous one but with
-- significantly greater likelyhood of having "realistic"
-- arguments to `slice`.
prop "is consistent with `slice` (more realistic cases)" $
\(QC.NonNegative i) (QC.NonNegative n) (QC.NonEmpty v)
-> sliceTest i n (V.fromList v)
where
sliceTest :: Int -> Int -> Vector Char -> QC.Property
sliceTest i n v = QC.withMaxSuccess 1000 $ case V.sliceMaybe i n v of
Just v' -> V'.slice i n v `shouldBe` v'
Nothing -> evaluate (V'.slice i n v) `shouldThrow` anyException