haskell / core-libraries-committee

95 stars 16 forks source link

Data.Array.Byte should also export `MutableByteArray` #49

Closed AndreasPK closed 2 years ago

AndreasPK commented 2 years ago

Because it makes no sense to do this for immutable ones but not mutable ones.

That's all.

AndreasPK commented 2 years ago

The prompt was https://gitlab.haskell.org/ghc/ghc/-/issues/20620 where I wondered if it could be extended to MutableByteArray. But that would require this change.

bgamari commented 2 years ago

The rationale for the decision not to export MutableByteArray is documented in https://gitlab.haskell.org/ghc/ghc/-/issues/20044#note_403703.

In short, Data.Array.Byte currently seeks to provide the ByteArray type, some instances, and nothing more. In particular, exporting MutableByteArray with any sensible operations would require that MonadPrim be moved into base. Understandably, this is a larger change than @Bodigrim wanted to advocate for when introducing ByteArray. Making progress on this ticket will require that someone put together a proposal to move MonadPrim.

AndreasPK commented 2 years ago

In my opinion base can reasonably provide a lifted wrapper for a unlifted type without providing operations on the lifted variant.

This would cut down on different names/types being used in different libraries for what amounts to the same thing. Even if these libraries have to define their own operations for working with such a type, they could still pass such a type between them easier if a common definition in base existed.

If the CLC considers it better to have no lifted wrapper over a lifted wrapper with no operations so be it.
But I think there are advantages to defining a common type for wrapping a MutableByteArray.

bgamari commented 2 years ago

Yes, I suppose you could simply export the type given that none of the instances for MutableByteArray provided by primitive (Eq and Data) appear to rely on MonadPrim.

Bodigrim commented 2 years ago

Originally I decided against exporting MutableByteArray, because there are no utilities available without MonadPrim in base. But I agree with @AndreasPK that it makes sense to export even a type itself + instance Eq + instance Data (similar to primitive).

tomjaguarpaw commented 2 years ago

This sounds sensible to me too, to prevent proliferation of third party wrappers. However, it seems that MutableByteArray has an NFData instance too. Presumably that would have to be moved into deepseq?

I'm not particularly familiar with this corner of the ecosystem, so apologies if my question makes no sense.

Bodigrim commented 2 years ago

Presumably that would have to be moved into deepseq?

Yes, same as ByteArray.


@AndreasPK could you please prepare an MR, adding MutableByteArray with Eq and Data instances? We can trigger a vote once such MR is available.

AndreasPK commented 2 years ago

@Bodigrim MR is here: https://gitlab.haskell.org/ghc/ghc/-/merge_requests/7785/diffs

Bodigrim commented 2 years ago

Dear CLC members, please vote on https://gitlab.haskell.org/ghc/ghc/-/merge_requests/7785 @chessai @cgibbard @cigsender @emilypi @tomjaguarpaw


+1 from me

tomjaguarpaw commented 2 years ago

Now that we have a concrete PR to look at, I realise that I am confused by the purpose of the Data instance.

instance Typeable s => Data (MutableByteArray s) where
  toConstr _ = error "toConstr"
  gunfold _ _ = error "gunfold"
  dataTypeOf _ = mkNoRepType "Data.Data.Array.Byte.MutableByteArray"
  1. Do Data.Data-using people (I am not one myself) find this kind of thing useful? What use does it serve to be able to get the data type but not actually be able to use any of the boilerplate-reducing functions (because they're replaced with error), that are, after all, the point of Data.Data?

  2. If we're going to keep this instance and the error calls, would it not make more sense to attach more information to them? For example toConstr _ = error "toConstr of Data (MutableByteArray s)"?

    (I appreciate that the PR as written matches the instance for ByteArray but I wasn't around for that decision ...)

  3. The s in that type signature is the phantom ST type parameter, right? How does it make sense to require that it is Typeable? Doesn't its use mean that if you use it in an ST computation you will end up with a result of type Typeable s => ST s result, which, as it is not parametrically polymorphic in s, cannot be run by runST?

AndreasPK commented 2 years ago

For reference I simply kept the instances currently provided by primitive.

tomjaguarpaw commented 2 years ago

Ah I see, i.e. defined at https://www.stackage.org/haddock/lts-18.27/primitive-0.7.3.0/src/Data.Primitive.ByteArray.html#line-532

In that case, +1 from me.

Bodigrim commented 2 years ago

@chessai @cgibbard @cigsender @emilypi may I have your votes please?

chessai commented 2 years ago

+1

mixphix commented 2 years ago

+1

cgibbard commented 2 years ago

+1

Bodigrim commented 2 years ago

Thanks all, with 5 votes in favor the proposal is approved.

chshersh commented 1 year ago

I'm trying to summarise the state of this proposal as part of my volunteering effort to track the progress of all approved CLC proposals.

Field Value
Authors @AndreasPK
Status merged
base version 4.17.0.0
Merge Request (MR) https://gitlab.haskell.org/ghc/ghc/-/merge_requests/7785
Blocked by nothing
CHANGELOG entry present
Migration guide not needed

Please, let me know if you find any mistakes 🙂