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

RFC: Make Some into a newtype around Some from package some #142

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

langston-barrett
Copy link
Contributor

The Some type from package some is a newtype, and is correspondingly more performant than our previous data-based encoding.

This change will require a major version bump and an easy but still mandatory migration for downstream codebases. Downstream code will either need to switch to using the eliminator (viewSome), or using pattern synonyms. Example migrations to use pattern synonyms can be seen in this commit.

This patch also uses -XGeneralizedNewtypeDeriving to derive the upstream Semigroup and Monoid instances, which are a good demonstration of the utility of sharing an implementation with a well-thought-out upstream package.

In the long run, I hope this paves the way towards #62. I'm not 100% sure this PR is the right way to go, but I realized that this was possible and thought it was worth a discussion.

@langston-barrett langston-barrett marked this pull request as ready for review September 28, 2022 15:45
The `Some` type from package `some` is a newtype, and is correspondingly more
performant than our previous data-based encoding.

This change will require a major version bump and an easy but still mandatory
migration for downstream codebases. Downstream code will either need to switch
to using the eliminator, or using pattern synonyms. Example migrations to use
pattern synonyms can be seen in this commit.

This patch also uses GeneralizedNewtypeDeriving to derive the upstream
`Semigroup` and `Monoid` instances, which are a good demonstration of the
utility of sharing an implementation with a well-thought-out upstream package.
@RyanGlScott
Copy link
Contributor

Is #62 the primary motivation for this? If so, I wonder if making parameterized-utils' Some a newtype around the one in some is worth it, since the one in parameterized-utils would still be distinct. We could, I suppose, eventually switch over to directly using the one in some, but that would require yet another breaking change.

The performance benefits from using a newtype encoding of Some might be nice, but I'm unclear if this is actually a performance bottleneck in practice.

@langston-barrett
Copy link
Contributor Author

Is #62 the primary motivation for this?

Let me make a more comprehensive pro/con list, as I should have done in the PR description:

Cons:

  • Major version bump, code churn in dependencies
  • Dependency on a new package (possible corresponding increase in build times, lead times when switching to a new GHC, cache sizes, etc.). Counterpoint: We already depend on all of its transitive dependencies, so the impact is likely to be small.

Pros:

  • Possible performance benefit from newtype representation. Counterpoint: We have no indication that this is a significant performance hit.
  • Makes it easier to "import ideas from upstream" such as the Monoid instance
  • Potential for improved interoperability with some, including zero-cost conversions (newtype (un)wrapping) between Data.Some and Data.Parameterized.Some.
  • Potential for an easier migration path towards Combine efforts on Data.Parameterized.Map and Data.Parameterized.Sum #62 - downstream packages could individually start migrating to Data.Some (with zero-cost conversions to Data.Parameterized.Some), and if we can figure out how to migrate typeclasses accordingly we could eventually deprecate Data.Parameterized.Some.

@RyanGlScott
Copy link
Contributor

OK. If I'm reading the pros correctly, most of the benefits involve the some package. If that is true, wouldn't it be more direct to just re-export some's Some from Data.Parameterized.Some? I'm honestly not sure what benefit it would provide to define our own newtype. If anything, that makes the situation more complicated, since now you have to keep track of which library's some you are using.

@langston-barrett
Copy link
Contributor Author

If that is true, wouldn't it be more direct to just re-export some's Some from Data.Parameterized.Some?

Right, great question. The problem is instances. For example, we have GEq f => Eq (Data.Some.Some f), but TestEquality f => Eq (Data.Parameterized.Some f). It is desireable that we move towards (something like) the former (#129), but doing so would cause major downstream breakage and I wanted to start by proposing as minimal of a change as I could. I think we probably should move in the direction of reexporting in the long term, but we would want to have a migration plan in place for these instances.

@RyanGlScott
Copy link
Contributor

The problem is instances. For example, we have GEq f => Eq (Data.Some.Some f), but TestEquality f => Eq (Data.Parameterized.Some f).

Ah, OK.

If we do decide to go down this path, it would be worthwhile to nail down exactly what the desired end state would be. I'm still unclear on what we actually want: Do we want to use a redefined version of EqF in the Eq instance for Some? Do we want to use GEq from some? Something else? I'm a bit nervous about committing to this change without having an agreement on the direction.

@kquick
Copy link
Member

kquick commented Sep 28, 2022

I share the concern about GEq/TestEquality differences (as well as API changes). I wonder if maybe first we should deprecate Parameterized.Some in favor of some and let client code selectively switch wherever possible first to minimize the impact. Ultimately, we might even then just remove Parameterized.Some and everywhere use some, but if we start from the top rather than the bottom we can iterate on this and ensure each conversion is valid.

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

Successfully merging this pull request may close these issues.

3 participants