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

Data.Array.Byte should also export MutableByteArray #49

Closed
AndreasPK opened this issue Mar 4, 2022 · 18 comments
Closed

Data.Array.Byte should also export MutableByteArray #49

AndreasPK opened this issue Mar 4, 2022 · 18 comments
Labels
approved Approved by CLC vote base-4.17 Implemented in base-4.17 (GHC 9.4)

Comments

@AndreasPK
Copy link

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

That's all.

@AndreasPK
Copy link
Author

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
Copy link

bgamari commented Mar 4, 2022

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
Copy link
Author

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
Copy link

bgamari commented Mar 4, 2022

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
Copy link
Collaborator

Bodigrim commented Mar 5, 2022

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
Copy link
Member

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
Copy link
Collaborator

Bodigrim commented Mar 7, 2022

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
Copy link
Author

@Bodigrim
Copy link
Collaborator

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


+1 from me

@tomjaguarpaw
Copy link
Member

tomjaguarpaw commented Mar 16, 2022

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
Copy link
Author

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

@tomjaguarpaw
Copy link
Member

@Bodigrim
Copy link
Collaborator

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

@chessai
Copy link
Member

chessai commented Mar 22, 2022

+1

2 similar comments
@mixphix
Copy link
Collaborator

mixphix commented Mar 22, 2022

+1

@cgibbard
Copy link
Contributor

+1

@Bodigrim
Copy link
Collaborator

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

@chshersh
Copy link
Member

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 🙂


@chshersh chshersh added the base-4.17 Implemented in base-4.17 (GHC 9.4) label Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Approved by CLC vote base-4.17 Implemented in base-4.17 (GHC 9.4)
Projects
None yet
Development

No branches or pull requests

8 participants