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.

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

Remove CPP driven error reporting in favor of HasCallStack #397

Merged
merged 3 commits into from
Aug 10, 2021

Conversation

lehins
Copy link
Contributor

@lehins lehins commented Aug 4, 2021

This PR does the world a favor by reducing CPP usage in vector. 😄

The idea behind this PR is that all Bounds checks get the HasCallStack constraint, so they can propagate to the user's callsite, while Internal and Unsafe do not, nevertheless the error message will still contain the module name and line number as before with CPP approach, because of the ghc's callstack innovation.

For ghc-7.10 it will fall back to call-stack package, which is a small price to pay for backwards compatibility with such and old ghc version.

Next step will be to adding HasCallStack to all other partial functions transitively, such PR in itself will supersede #184

vector/src/Data/Vector/Fusion/Stream/Monadic.hs Outdated Show resolved Hide resolved
vector/src/Data/Vector/Generic.hs Outdated Show resolved Hide resolved
Co-authored-by: konsumlamm <44230978+konsumlamm@users.noreply.github.com>
@Shimuuar
Copy link
Contributor

Shimuuar commented Aug 5, 2021

Excellent!

I added HasCallStack constraint to other check functions. Otherwise call stack is truncated early and there's no way to know which function failed:

*Data.Vector> generate 10 id ! 11
*** Exception: index out of bounds (11,10)
CallStack (from HasCallStack):
  error, called at src/Data/Vector/Internal/Check.hs:101:12 in vector-0.13.0.1-inplace:Data.Vector.Internal.Check
  checkError, called at src/Data/Vector/Internal/Check.hs:107:17 in vector-0.13.0.1-inplace:Data.Vector.Internal.Check
  check, called at src/Data/Vector/Internal/Check.hs:120:5 in vector-0.13.0.1-inplace:Data.Vector.Internal.Check

Also monomophic functions lack HasCallStack constraint which again causes call stack to terminate early:

*Data.Vector> generate 10 id ! 11
*** Exception: index out of bounds (11,10)
CallStack (from HasCallStack):
  error, called at src/Data/Vector/Internal/Check.hs:101:12 in vector-0.13.0.1-inplace:Data.Vector.Internal.Check
  checkError, called at src/Data/Vector/Internal/Check.hs:107:17 in vector-0.13.0.1-inplace:Data.Vector.Internal.Check
  check, called at src/Data/Vector/Internal/Check.hs:120:5 in vector-0.13.0.1-inplace:Data.Vector.Internal.Check

or if it's added:

*Data.Vector> generate 10 id ! 11
*** Exception: index out of bounds (11,10)
CallStack (from HasCallStack):
  error, called at src/Data/Vector/Internal/Check.hs:101:12 in vector-0.13.0.1-inplace:Data.Vector.Internal.Check
  checkError, called at src/Data/Vector/Internal/Check.hs:107:17 in vector-0.13.0.1-inplace:Data.Vector.Internal.Check
  check, called at src/Data/Vector/Internal/Check.hs:120:5 in vector-0.13.0.1-inplace:Data.Vector.Internal.Check
  checkIndex, called at src/Data/Vector/Generic.hs:238:11 in vector-0.13.0.1-inplace:Data.Vector.Generic
  !, called at src/Data/Vector.hs:505:7 in vector-0.13.0.1-inplace:Data.Vector
  !, called at <interactive>:47:1 in interactive:Ghci1

@lehins
Copy link
Contributor Author

lehins commented Aug 5, 2021

I added HasCallStack constraint to other check functions.

Yes, this is the next step. I wanted to keep this PR to a minimum.

@lehins
Copy link
Contributor Author

lehins commented Aug 5, 2021

check{Index,Slice,Length} require HasCallStack constraint too

Good catch!!

lehins and others added 2 commits August 6, 2021 13:31
Otherwise call stack is terminated early

> CallStack (from HasCallStack):
>   error, called at src/Data/Vector/Internal/Check.hs:101:12 in ...
>   checkError, called at src/Data/Vector/Internal/Check.hs:107:17 in ...
>   check, called at src/Data/Vector/Internal/Check.hs:120:5 in ...

Compare with

> CallStack (from HasCallStack):
>   error, called at src/Data/Vector/in ...
>   checkError, called at src/Data/Vector/in ...
>   check, called at src/Data/Vector/in ...
>   checkIndex, called at src/Data/Vector/Generic.hs:238:11 in ...
>   !, called at src/Data/Vector.hs:505:7 in ...
@lehins
Copy link
Contributor Author

lehins commented Aug 9, 2021

@Bodigrim Do you have input on this ticket? If not, then I'll merge it tomorrow.

@Bodigrim
Copy link
Contributor

Bodigrim commented Aug 9, 2021

@lehins any chance to check performance impact? I guess all these functions are non-recursive and inline well, so there is no associated cost to HasCallStack, but it would be nice to check. Otherwise looks very good indeed.

@lehins
Copy link
Contributor Author

lehins commented Aug 10, 2021

I did compare benchmarks included in the repo and there was no difference in performance. As you mentioned all these functions inline well, so it is expected that there would be no regressions (also mentioned in this comment by Ben: #184 (comment))

As an extra confirmation I did a similar switch in massiv recently and there was no impact on performance either, so I am pretty confident in this approach.

@Bodigrim
Copy link
Contributor

This is a breaking change, because downstream packages could have included vector.h, e. g., Bodigrim/bitvec@f6e96e3. Could someone add a changelog entry please?

@lehins
Copy link
Contributor Author

lehins commented May 23, 2022

@Bodigrim I beg to differ. There is not a single place in haddock where vector.h or the included CPP pragmas are advertised as public API.

However, I will add this ticket into changelog before making a release. I am sure there are other things that we have missed that didn't make it into the changelog, so I'll go through all of the PRs since version 0.12 and catalog them with PR numbers in the changelog. (I wish I didn't have to do that and all changelog entries were added together with the PR, but we are all human and it is easy to forget it.)

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Aug 19, 2022
# Changes in version 0.13.0.0

 * `mkType` from `Data.Vector.Generic` is deprecated in favor of
   `Data.Data.mkNoRepType`
 * The role signatures on several `Vector` types were too permissive, so they
   have been tightened up:
   * The role signature for `Data.Vector.Mutable.MVector` is now
     `type role MVector nominal representational` (previously, both arguments
     were `phantom`). [#224](haskell/vector#224)
   * The role signature for `Data.Vector.Primitive.Vector` is now
     `type role Vector nominal` (previously, it was `phantom`).
     The role signature for `Data.Vector.Primitive.Mutable.MVector` is now
     `type role MVector nominal nominal` (previously, both arguments were
     `phantom`). [#316](haskell/vector#316)
   * The role signature for `Data.Vector.Storable.Vector` is now
     `type role Vector nominal` (previous, it was `phantom`), and the signature
     for `Data.Vector.Storable.Mutable.MVector` is now
     `type role MVector nominal nominal` (previous, both arguments were
     `phantom`). [#235](haskell/vector#235)

     We pick `nominal` for the role of the last argument instead of
     `representational` since the internal structure of a `Storable` vector is
     determined by the `Storable` instance of the element type, and it is not
     guaranteed that the `Storable` instances between two representationally
     equal types will preserve this internal structure.  One consequence of this
     choice is that it is no longer possible to `coerce` between
     `Storable.Vector a` and `Storable.Vector b` if `a` and `b` are nominally
     distinct but representationally equal types. We now provide
     `unsafeCoerce{M}Vector` and `unsafeCast` functions to allow this (the onus
     is on the user to ensure that no `Storable` invariants are broken when
     using these functions).
 * Methods of type classes `Data.Vector.Generic.Mutable.MVector` and
   `Data.Vector.Generic.Vector` use concrete monads (`ST`, etc) istead of being
   polymorphic (`PrimMonad`, etc). [#335](haskell/vector#335).
   This makes it possible to derive `Unbox` with:
   * `GeneralizedNewtypeDeriving`
   * via `UnboxViaPrim` and `Prim` instance
   * via `As` and `IsoUnbox` instance: [#378](haskell/vector#378)
 * Add `MonadFix` instance for boxed vectors: [#312](haskell/vector#312)
 * Re-export `PrimMonad` and `RealWorld` from mutable vectors:
   [#320](haskell/vector#320)
 * Add `maximumOn` and `minimumOn`: [#356](haskell/vector#356)
 * The functions `scanl1`, `scanl1'`, `scanr1`, and `scanr1'` for immutable
   vectors are now defined when given empty vectors as arguments,
   in which case they return empty vectors. This new behavior is consistent
   with the one of the corresponding functions in `Data.List`.
   Prior to this change, applying an empty vector to any of those functions
   resulted in an error. This change was introduced in:
   [#382](haskell/vector#382)
 * Change allocation strategy for `unfoldrN`: [#387](haskell/vector#387)
 * Remove `CPP` driven error reporting in favor of `HasCallStack`:
   [#397](haskell/vector#397)
 * Remove redundant `Storable` constraints on to/from `ForeignPtr` conversions:
   [#394](haskell/vector#394)
 * Add `unsafeCast` to `Primitive` vectors: [#401](haskell/vector#401)
 * Make `(!?)` operator strict: [#402](haskell/vector#402)
 * Add `readMaybe`: [#425](haskell/vector#425)
 * Add `groupBy` and `group` for `Data.Vector.Generic` and the specialized
   version in `Data.Vector`, `Data.Vector.Unboxed`, `Data.Vector.Storable` and
   `Data.Vector.Primitive`. [#427](haskell/vector#427)
 * Add `toArraySlice` and `unsafeFromArraySlice` functions for conversion to and
   from the underlying boxed `Array`: [#434](haskell/vector#434)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants