Fix https://github.com/haskell/vector/issues/223 #224

idontgetoutmuch · 2018-10-27T13:20:43Z

No description provided.

RyanGlScott · 2018-10-27T13:21:47Z

This is a good idea. From a quick search in GHCi, it looks like the following Vector data types have roles that are too permissive:

λ> :info Primitive.Vector
type role Primitive.Vector phantom -- should be `representational`
data Primitive.Vector a

λ> :info Primitive.MVector
type role Primitive.MVector nominal phantom -- should be `nominal representational`
data Primitive.MVector s a

λ> :info Storable.Vector 
type role Storable.Vector phantom -- should be `representational`
data Storable.Vector a

λ> :info Storable.MVector 
type role Storable.MVector phantom phantom -- should be `nominal representational`
data Storable.MVector s a

Can you fix these as well?

I'll leave some more specific comments inline.

idontgetoutmuch · 2018-10-27T13:22:54Z

No problem - I'll do it now

RyanGlScott · 2018-10-27T13:22:34Z

Data/Vector.hs

@@ -5,6 +5,7 @@
           , TypeFamilies
           , Rank2Types
           , BangPatterns
+           , RoleAnnotations


This extension is only available on GHC 7.8 and later, so you'll need to use CPP to ensure that older GHCs can continue to compile this.

Why is nothing in life simple?

RyanGlScott · 2018-10-27T13:22:55Z

Data/Vector.hs

@@ -204,6 +205,9 @@ import qualified Control.Applicative as Applicative
 import qualified Data.Foldable as Foldable
 import qualified Data.Traversable as Traversable

+import Data.Coerce
+import Foreign.C.Types


Why do you need to import these? This seems like an unforced change.

Oh that was to test my change actually gave a type error - they should be removed

RyanGlScott · 2018-10-27T13:23:05Z

Data/Vector.hs

@@ -212,6 +216,7 @@ import Data.Monoid   ( Monoid(..) )
 import qualified GHC.Exts as Exts (IsList(..))
 #endif

+type role Vector representational


Similarly, wrap this in CPP.

RyanGlScott · 2018-10-27T13:23:26Z

vector.cabal

@@ -179,6 +179,8 @@ source-repository head

 test-suite vector-tests-O0
  Default-Language: Haskell2010
+  if os(OSX)
+    extra-libraries: iconv


I don't understand what this has to do with the original issue. Perhaps this would be better suited to a separate patch.

Sure - the tests wouldn't build for me without this

idontgetoutmuch · 2018-10-27T14:03:56Z

Hopefully all is now well

RyanGlScott · 2018-10-27T15:38:17Z

LGTM. Thanks!

ekmett · 2018-10-28T12:17:21Z

Isn't representational too weak for safety? In general when you have a container that behaves based on the instances associated with a type (e.g. Map w/ Ord, etc) we want a nominal role for the argument. As it stands a user could still coerce one of them that was made representational. Having the same representation does not mean the Storable instances are compatible. They could well have different alignments or formats.

RyanGlScott · 2018-10-28T12:30:38Z

Yes, that is a good observation. I'm somewhat reluctant to just slam on the brakes and make the arguments to Primitive.Vector and Storable.Vector nominal since it's possible that there is code in the wild that relies on coerce-ing underneath them (I would wager that @idontgetoutmuch probably has some examples of such code, in fact). Still, the safety argument is quite concerning...

Do you have an example of some code that would go wrong at runtime due to these roles being representational? That would make for a pretty compelling counterexample.

Shimuuar · 2018-10-28T18:44:26Z

Totally contrived example for Storable. Data type with phantom type which mirrors C structure and phantom type encode layout (packed, standard C alignment, etc.).

cartazio · 2018-10-29T16:03:53Z

Storable needs to be nominal. Because of binary encoding things. Like network order vs host order due to storable being for c interop The semantics for prim is “the only instances should directly map to their unlifted sibling”. Anything else should just be a new type. Funkier / more interesting things should be in unboxed. Prim can be representational afaict. Also this could have used more review before merge for that very reason... Somehow didn’t see the thread this weekend :(

…

On Sun, Oct 28, 2018 at 2:44 PM Aleksey Khudyakov ***@***.***> wrote: Totally contrived example for Storable. Data type with phantom type which mirrors C structure and phantom type encode layout (packed, standard C alignment, etc.). — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#224 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAQwmmrmC8AplQ1ERNkqbANlFwZHyuaks5upfsKgaJpZM4X9bVW> .

RyanGlScott · 2018-10-29T16:32:13Z

My apologies for pushing it through quickly—I legitimately thought this was a no-brainer at the time. And in any case, this is a move in the right direction, since the roles used to be even more permissive than they are now! The remaining question is whether we should go further in restricting the roles.

@idontgetoutmuch, out of curiosity, which flavor of Vector were you coerce-ing under? (I'm not sure if it's the Prim- or Storable-based one.)

ekmett · 2018-10-29T22:20:32Z

We could always offer a function ourself for the coercion, an unsafeCoerce if you will. ;)

idontgetoutmuch · 2018-11-08T11:26:27Z

import qualified Data.Vector.Storable as V

Lots of use here: https://github.com/haskell-numerics/hmatrix-sundials/blob/281-build-failure/src/Numeric/Sundials/CVode/ODE.hs#L205

RyanGlScott · 2018-11-08T14:15:50Z

I see. Unfortunately, that's exactly the flavor of Vector that ought to be nominally roled, as I've now come to realize, which means that this code would stop working if we were to give it the role signature it really deserves :(

I think that if we are going to do this, we should follow @ekmett's advice and offer something like:

unsafeVectorCoerce :: Coercible a b => Storable.Vector a -> Storable.Vector b
unsafeVectorCoerce = unsafeCoerce

As the name suggests, it would still be unsound to use this in general, but if you do know what you're doing, then you can at least have some assurance that the type parameters are representationally equal.

cartazio · 2018-11-08T14:19:56Z

Yeah. For storable anything ever, you can’t convert without actually looking at the exact actual binary / memory encoding

…

On Thu, Nov 8, 2018 at 9:15 AM Ryan Scott ***@***.***> wrote: I see. Unfortunately, that's exactly the flavor of Vector that ought to be nominally roled, as I've now come to realize, which means that this code would stop working if we were to give it the role signature it really deserves :( I think that if we are going to do this, we should follow @ekmett <https://github.com/ekmett>'s advice and offer something like: unsafeVectorCoerce :: Coercible a b => Storable.Vector a -> Storable.Vector b unsafeVectorCoerce = unsafeCoerce As the name suggests, it would still be unsound to use this in general, but if you *do* know what you're doing, then you can at least have some assurance that the type parameters are representationally equal. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#224 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAQwh6UEbt4zFYP_TGOly7kXIdwO5qRks5utDyXgaJpZM4X9bVW> .

idontgetoutmuch · 2018-11-09T06:39:33Z

@RyanGlScott Yes I feared as much when I looked at my code but I was beguiled by the the word "safe" in the documentation. Yes I should use unsafeVectorCoerce and presumably have a test such that should ghc ever change the representation for Double it will get caught.

to avoid accidentally coercing vectors. See haskell/vector#224.

RyanGlScott · 2018-12-30T17:15:16Z

I've submitted #235 to make Storable vectors nominally roled.

Reasoning is identical to one for storable vectors. See discussion in haskell#224, and PR haskell#235. Fixes haskell#277

# 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)

RyanGlScott suggested changes Oct 27, 2018

View reviewed changes

idontgetoutmuch force-pushed the master branch 2 times, most recently from beda2f7 to 9700d0b Compare October 27, 2018 13:59

Fix incorrect coercion haskell#223

6bfe81b

idontgetoutmuch force-pushed the master branch from 9700d0b to 6bfe81b Compare October 27, 2018 14:01

RyanGlScott approved these changes Oct 27, 2018

View reviewed changes

RyanGlScott merged commit e80833e into haskell:master Oct 27, 2018

RyanGlScott mentioned this pull request Oct 27, 2018

Vector should be representational #223

Closed

UnkindPartition added a commit to UnkindPartition/hmatrix-sundials that referenced this pull request Nov 26, 2018

Use the latest version of 'vector'

b4c6c77

to avoid accidentally coercing vectors. See haskell/vector#224.

RyanGlScott mentioned this pull request Dec 30, 2018

Make Storable vectors nominally roled, and add unsafeCoerceVector functions #235

Merged

Shimuuar mentioned this pull request Jan 31, 2020

vector Jan 2020 RC #266

Closed

Shimuuar added a commit to Shimuuar/vector that referenced this pull request Jun 13, 2020

Make primitive vectors nominally roled

3874631

Reasoning is identical to one for storable vectors. See discussion in haskell#224, and PR haskell#235. Fixes haskell#277

Shimuuar mentioned this pull request Jun 13, 2020

Make primitive vectors nominally roled #316

Merged

Shimuuar added a commit to Shimuuar/vector that referenced this pull request Jun 14, 2020

Make primitive vectors nominally roled

1fdfea1

Reasoning is identical to one for storable vectors. See discussion in haskell#224, and PR haskell#235. Fixes haskell#277

Shimuuar added a commit to Shimuuar/vector that referenced this pull request Jun 14, 2020

Make primitive vectors nominally roled

b35a6d3

Reasoning is identical to one for storable vectors. See discussion in haskell#224, and PR haskell#235. Fixes haskell#277

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix https://github.com/haskell/vector/issues/223 #224

Fix https://github.com/haskell/vector/issues/223 #224

idontgetoutmuch commented Oct 27, 2018

RyanGlScott commented Oct 27, 2018

idontgetoutmuch commented Oct 27, 2018

RyanGlScott Oct 27, 2018

idontgetoutmuch Oct 27, 2018

RyanGlScott Oct 27, 2018

idontgetoutmuch Oct 27, 2018

RyanGlScott Oct 27, 2018

RyanGlScott Oct 27, 2018

idontgetoutmuch Oct 27, 2018

idontgetoutmuch commented Oct 27, 2018

RyanGlScott commented Oct 27, 2018

ekmett commented Oct 28, 2018

RyanGlScott commented Oct 28, 2018

Shimuuar commented Oct 28, 2018

cartazio commented Oct 29, 2018 via email

RyanGlScott commented Oct 29, 2018

ekmett commented Oct 29, 2018

idontgetoutmuch commented Nov 8, 2018 •

edited

Loading

RyanGlScott commented Nov 8, 2018

cartazio commented Nov 8, 2018 via email

idontgetoutmuch commented Nov 9, 2018

RyanGlScott commented Dec 30, 2018

Fix https://github.com/haskell/vector/issues/223 #224

Fix https://github.com/haskell/vector/issues/223 #224

Conversation

idontgetoutmuch commented Oct 27, 2018

RyanGlScott commented Oct 27, 2018

idontgetoutmuch commented Oct 27, 2018

RyanGlScott Oct 27, 2018

Choose a reason for hiding this comment

idontgetoutmuch Oct 27, 2018

Choose a reason for hiding this comment

RyanGlScott Oct 27, 2018

Choose a reason for hiding this comment

idontgetoutmuch Oct 27, 2018

Choose a reason for hiding this comment

RyanGlScott Oct 27, 2018

Choose a reason for hiding this comment

RyanGlScott Oct 27, 2018

Choose a reason for hiding this comment

idontgetoutmuch Oct 27, 2018

Choose a reason for hiding this comment

idontgetoutmuch commented Oct 27, 2018

RyanGlScott commented Oct 27, 2018

ekmett commented Oct 28, 2018

RyanGlScott commented Oct 28, 2018

Shimuuar commented Oct 28, 2018

cartazio commented Oct 29, 2018 via email

RyanGlScott commented Oct 29, 2018

ekmett commented Oct 29, 2018

idontgetoutmuch commented Nov 8, 2018 • edited Loading

RyanGlScott commented Nov 8, 2018

cartazio commented Nov 8, 2018 via email

idontgetoutmuch commented Nov 9, 2018

RyanGlScott commented Dec 30, 2018

idontgetoutmuch commented Nov 8, 2018 •

edited

Loading