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

NFData instances for unboxed arrays from array #102

Closed
meooow25 opened this issue Jun 7, 2024 · 13 comments
Closed

NFData instances for unboxed arrays from array #102

meooow25 opened this issue Jun 7, 2024 · 13 comments

Comments

@meooow25
Copy link
Contributor

meooow25 commented Jun 7, 2024

NFData instances could be provided for UArray, STUArray and IOUArray from the array package: https://hackage.haskell.org/package/array

PS: I find it a little odd that deepseq depends on array and has to provide these, rather than the other way around.

@Bodigrim
Copy link
Contributor

Bodigrim commented Jun 7, 2024

PS: I find it a little odd that deepseq depends on array and has to provide these, rather than the other way around.

It should rather be other way around indeed.

@meooow25
Copy link
Contributor Author

meooow25 commented Jun 11, 2024

I looked into this a bit, and it seems there are two ways we can go about it.

A. deepseq provides the instances

This is simple enough. I can make a PR for it.

B. Reverse the deepseq->array dependency

It turns out that the Array type is defined not in array, but in base's GHC.Arr. So deepseq never required a dependency on array in the first place. The steps here are:

  1. deepseq drops the dependency on array and provides NFData for Array using GHC.Arr. This is released as version deepseq-x.
  2. array-y provides NFData instances for unboxed arrays by taking a dependency on deepseq >= x. Users of array >= y get the instances.

Sidenote: I found that there was a plan to reverse deepseq->containers and deepseq->array dependencies in GHC #5468, but they stopped at containers due to undocumented reasons.

What do you think?

@meooow25
Copy link
Contributor Author

Hi, any thoughts on this?

@Bodigrim
Copy link
Contributor

Given that deepseq is a dependency of template-haskell, the former is practically non-reinstallable and such are its dependencies as well, including array. I think "B. Reverse the deepseq->array dependency" would be most beneficial.

The steps here are:

I think the same could be achieved with automatic Cabal flags: a dependency cycle should be automatically rejected by Cabal solver, forcing flipping a flag. But this requires testing, and being boot packages is likely to complicate things.

@Bodigrim
Copy link
Contributor

(@mixphix is a maintainer here, I'm just passing by)

@meooow25
Copy link
Contributor Author

Could you explain what you mean be automatic Cabal flags? I didn't get that, but then again I'm not too familiar with Cabal stuff.
As I see it, for B the only thing deepseq needs to do is simply drop the array dependency. Nothing would be lost with this. Further changes are required only to add new NFData instances.

@Bodigrim
Copy link
Contributor

Bodigrim commented Jul 14, 2024

Ah, that's even simpler then. I guess the only relevant part of my comment is that array-y does not even need to demand deepseq >= x, because Cabal solver would imply such bound automatically to avoid circular dependencies.

@meooow25
Copy link
Contributor Author

meooow25 commented Sep 7, 2024

@mixphix would you please provide your opinion on this?

If we're good with dropping the array dependency, I can make the PR here, then take up the rest of the work on the array side.

@mixphix
Copy link
Collaborator

mixphix commented Sep 7, 2024

Dropping the array dependency is fine by me. Thanks for volunteering to do that!

@mixphix
Copy link
Collaborator

mixphix commented Sep 8, 2024

The array dependency has been dropped for version 1.6.0.0.

@mixphix mixphix closed this as completed Sep 8, 2024
@meooow25
Copy link
Contributor Author

meooow25 commented Sep 8, 2024

Thanks for the fast release! Why a major version bump though?

@Bodigrim
Copy link
Contributor

Bodigrim commented Sep 8, 2024

@mixphix thanks for a quick turnaround!

Did I miss a breaking change since 1.5.0.0? If there were none, could it be released as 1.5.1.0 (and 1.6.0.0 marked as deprecated so that it does not pop up in build plans, see how it was done for binary)? Sorry for asking to do more work, but upgrading version bounds to allow deepseq-1.6.0.0 across the ecosystem is somewhat expensive.

@mixphix
Copy link
Collaborator

mixphix commented Sep 9, 2024

My apologies, I should have reviewed the PVP before releasing. I was under the impression that dropping a dependency required a major version bump. I'll re-release this as 1.5.1.0 and mark deepseq-1.6.0.0 as deprecated. Any breaking changes to be released in the next version will be released as 1.6.0.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants