Unexpectedly restrictive type signature for `toStrict_`?

[vdukhovni]

Commit 7a9df8b2561e4d4271383a2430c317994295e822 (back in 2015), adds the toStrict_ function, giving it a signature of:

toStrict_ :: Monad m => ByteString m () -> m (S.ByteString)

However, the same commit also adds the toLazy_ function, this time with a signature of:

toLazy_ :: Monad m => ByteString m r -> m BI.ByteString

I see no obvious reason why the toLazy_ function is willing to discard a non-trivial r value, but the toStrict_ function is not. The comments in the export list of the module are no help, bearing little resemblance to reality:

  , toLazy           -- toLazy :: Monad m => ByteStream m () -> m ByteString
  , toLazy_          -- toLazy' :: Monad m => ByteStream m () -> m (Of ByteString r)
  ...
  , toStrict         -- toStrict :: Monad m => ByteStream m () -> m ByteString
  , toStrict_        -- toStrict_ :: Monad m => ByteStream m r -> m (Of ByteString r)

If the goal is to protect users from accidentally truncating Stream (ByteStream m) m r by inadvertently applying toStrict_, rather than toStrict, then the protection should likely also apply to toLazy_. If, on the other hand, it is up to the user to not use the wrong function, then both should probably have an unrestricted r type.

[ Edit, actually, given that the return type of the toStrict_ function has a rather different form than the toStrict function, no accidental confusion is possible. So I think the simplest solution is to unrestrict the input type, replacing () with r. Just doing that compiles and passes all tests, so I think it should be done. ]

[fosskers]

Given that one form is in Of-land and the other is not, it's probably hard to confuse the two during general use. I agree that the restrictive () seems strange. I personally have nothing against relaxing it, but perhaps we should get more feedback first.

[vdukhovni]

Do you have anyone else in mind to ask for a review?

[fosskers]

You, @chessai and I seem to be the active members of this project at the moment :)

[vdukhovni]

You, @chessai and I seem to be the active members of this project at the moment :)

That does limit the options somewhat. :-) Meanwhile, I'm proceeding with the README WIP on the basis that this change will be made. It looks right to me.

Speaking of minor nits, I am also surprised that Q.length returns Of Int r and not Of Int64 r, especially given that other functions like splitAt take Int64. This does rather look like an oversight, but fixing it would be a breaking change... Any thoughts about that?

[ Edit: Same issue for length_, count, count_ and perhaps also the various hGet functions that really should support Int64 lengths. That said, the difference is increasingly irrelevant as 32-bit systems fade from living memory, ... :-) ]

[fosskers]

Leaving aside the Int issues, shall we move ahead with this?

[vdukhovni]

Leaving aside the Int issues, shall we move ahead with this?

Sure. The simplest thing is to merge this into #42, if that works for you, while I guess leaving the Int <-> Int64 nits for a subsequent discussion (or just do nothing).

[vdukhovni]

Well, I found out why the signature was what it was. Turns out that prior to streaming-2.2.0 the toList_ signature in streaming also took only r ~ (), and we need to apply void to toChunks to make it work with older versions of streaming. I added the necessary CPP guards.