Bounds checking in array functions

[osa1]

We discovered today that array functions do not do bounds checking, which was surprising to us. The expectation was that unless the function is prefixed with "unsafe" it's supposed to do bounds checking.

After discovering this I checked the haddocks, but there's literally no mention to bounds checking in any of the array modules or at the top level. At the very least we should mention in bold letters that the array modules do not do bounds checking, and are completely unsafe.

For long-time primitive users and maintainers this is probably obvious, but it's far from being obvious from a user's point of view.

I realize that it's hard to change this behavior now, so perhaps we should just update the documentation. Ideally it'd be great to have writeArray that does bounds checking, and unsafeWriteArray that omits it though.

[treeowl]

Functions in this package are generally not safe, and are typically named after the underlying primitives. Improving documentation is always helpful. I would oppose changing the semantics now, because packages using this one rely on the efficiency of unchecked operations. I'm not sure if the checked operations belong in this package or not.

[cartazio]

i have to agree with David here, use Vector or something similar. the cost/perf model for primitive needs to be brutally simple and transparent. Any changes to that would otherwise impact performance / semantics of eg vector (which has bounds checking / flags for it)

…

On Tue, Oct 9, 2018 at 5:11 PM David Feuer ***@***.***> wrote: Functions in this package are generally *not* safe, and are typically named after the underlying primitives. Improving documentation is always helpful. I would oppose changing the semantics now, because packages using this one rely on the efficiency of unchecked operations. I'm not sure if the checked operations belong in this package or not. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#212 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAQwhA44y1D-WgqmicvD6HGfD7WqBkYks5ujREIgaJpZM4XUGm2> .

[cartazio]

if you want safe arrays, use vector or array.

[treeowl]

I *disagree* with Carter's suggestion. I think it's perfectly reasonable to write checked operations for the arrays in this package. Not everyone wants the stream fusion and slicing of vector, and not everyone wants the flexible indexing of array. The question is whether those checked operations belong in this package or in a separate (safe-primitive?) one. I don't have a strong opinion either way.

…

On Tue, Oct 9, 2018 at 6:03 PM Carter Tazio Schonwald < ***@***.***> wrote: if you want safe arrays, use vector or array. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#212 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABzi_Wu7o3kRHngTaqDywh7It4vdMe7dks5ujR0kgaJpZM4XUGm2> .

[cartazio]

we could add check ones, or an opt in checking flag, i'm merely opposed to default built for current ops to do the checking ...

…

On Tue, Oct 9, 2018 at 6:30 PM David Feuer ***@***.***> wrote: I *disagree* with Carter's suggestion. I think it's perfectly reasonable to write checked operations for the arrays in this package. Not everyone wants the stream fusion and slicing of vector, and not everyone wants the flexible indexing of array. The question is whether those checked operations belong in this package or in a separate (safe-primitive?) one. I don't have a strong opinion either way. On Tue, Oct 9, 2018 at 6:03 PM Carter Tazio Schonwald < ***@***.***> wrote: > if you want safe arrays, use vector or array. > > — > You are receiving this because you commented. > Reply to this email directly, view it on GitHub > <#212 (comment) >, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/ABzi_Wu7o3kRHngTaqDywh7It4vdMe7dks5ujR0kgaJpZM4XUGm2 > > . > — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#212 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAQwqPVMO4zLvGNo9SmsuFLmgiNWzzFks5ujSNngaJpZM4XUGm2> .

[treeowl]

We could even do both, adding `safe` and `unsafe` variants, and making the bare name depend on a flag. That should give maximal flexibility. On Tue, Oct 9, 2018 at 6:48 PM Carter Tazio Schonwald < notifications@github.com> wrote:

…

we could add check ones, or an opt in checking flag, i'm merely opposed to default built for current ops to do the checking ... On Tue, Oct 9, 2018 at 6:30 PM David Feuer ***@***.***> wrote: > I *disagree* with Carter's suggestion. I think it's perfectly reasonable to > write checked operations for the arrays in this package. Not everyone wants > the stream fusion and slicing of vector, and not everyone wants the > flexible indexing of array. The question is whether those checked > operations belong in this package or in a separate (safe-primitive?) one. I > don't have a strong opinion either way. > > On Tue, Oct 9, 2018 at 6:03 PM Carter Tazio Schonwald < > ***@***.***> wrote: > > > if you want safe arrays, use vector or array. > > > > — > > You are receiving this because you commented. > > Reply to this email directly, view it on GitHub > > < #212 (comment) > >, > > or mute the thread > > < > https://github.com/notifications/unsubscribe-auth/ABzi_Wu7o3kRHngTaqDywh7It4vdMe7dks5ujR0kgaJpZM4XUGm2 > > > > . > > > > — > You are receiving this because you modified the open/close state. > Reply to this email directly, view it on GitHub > <#212 (comment) >, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/AAAQwqPVMO4zLvGNo9SmsuFLmgiNWzzFks5ujSNngaJpZM4XUGm2 > > . > — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#212 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABzi_TcmC3k74YpV3kC-UYTraGzNEnv5ks5ujSfZgaJpZM4XUGm2> .

[cartazio]

true. I'd rather keep primitive a tad simpler in that respect. (eg have a build flag for checked mode or something)

but for now, lets keep this as a backburner todo :)

[cartazio]

for now lets approach this as "add a package flag for bounds checking operations "

[chessai]

@osa1 this isn't precisely what you want (i think), but may be of interest to you: http://hackage.haskell.org/package/primitive-checked

[chessai]

To clarify, that package mimics the API of primitive, but adds bounds checking and gives useful runtime error messages

[Shimuuar]

I don't think flag will work well. Every nontrivial program have primitive as dependency which means that every user of primitive will have to pay price of bounds checking even if they do them anyway. So if one package need that flag it will impose performance penalty on all other packages. And AFAIR it's not possible to depend flag selection of package.

It could function as debug flag at best with purpose of catching indexing bug in package that depends on it

[andrewthad]

I agree that we should be more explicit in the documentation about the lack of bounds-checking. Having a flag would be nice. As @chessai pointed out, http://hackage.haskell.org/package/primitive-checked is useful. It gets me 95% of where I want to be (plus, someone added CallStack everywhere, which makes the error messages pretty good). I would be in favor of pulling this into primitive itself behind a checked flag (similar to the unsafechecks that vector has). This would make it easier to use.

Specifically responding to @Shimuuar's concern:

So if one package needs that flag it will impose a performance penalty on all other packages

The whole purpose of the flag would be for when you are debugging an application. So, it would be running locally or in a staging environment. I've never had the need to perform bounds-checking at runtime in production. If you have an out-of-bounds access in production, your program is will still crash regardless of whether or not you are doing bounds-checking. You just get a better error message if you are bounds checking.

[Shimuuar]

As debugging flag it would be useful indeed. Also program wouldn't necessarily crash it may just produce garbage if you just have out of bounds reads and never runs into segfault

[cartazio]

I think adding bounds checking behind a flag for debugging is fine. The part that might be a problem is that we’d only want to include has call stack constraints in the types when that flag is toggled ... because the change in arguments structure does change register pressure and optimization in core etc. (at least on some ghc Versions) So yeah, near term sounds like the -checked package is what you should use

…

On Wed, Oct 10, 2018 at 8:22 AM Aleksey Khudyakov ***@***.***> wrote: As debugging flag it would be useful indeed. Also program wouldn't necessarily crash it may just produce garbage if you just have out of bounds reads and never runs into segfault — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#212 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAAQwgxF82EiksNyAhnavEDsR7LACod9ks5ujeaCgaJpZM4XUGm2> .