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

Consider adding some form of newtypes to cats #1800

Closed
LukaJCB opened this issue Aug 8, 2017 · 28 comments
Closed

Consider adding some form of newtypes to cats #1800

LukaJCB opened this issue Aug 8, 2017 · 28 comments
Milestone

Comments

@LukaJCB
Copy link
Member

LukaJCB commented Aug 8, 2017

We have https://github.com/alexknvl/newtypes, but we could also look at other options :)

@sir-wabbit
Copy link

sir-wabbit commented Sep 20, 2017

Once scala/scala#6074 is merged in and there is a scalameta release with https://github.com/scalameta/paradise/pull/207 (maybe M10 already has it), that technique will be more usable. There are also some issues with IJ and newtypes with existential parameters at the moment. scala/scala#6088 could improve the performance a bit (?).

@LukaJCB
Copy link
Member Author

LukaJCB commented Sep 21, 2017

Maybe we should wait out this: http://docs.scala-lang.org/sips/opaque-types.html

@LukaJCB LukaJCB changed the title Consider using alexknvl's newtypes Consider adding some form newtypes to cats Sep 26, 2017
@kailuowang
Copy link
Contributor

kailuowang commented Sep 26, 2017

Options that I see so far.

  1. depends on https://github.com/alexknvl/newtypes Update: can't use this as is unless we drop 2.10 support
  2. depends on an adaption of https://github.com/estatico/scala-newtype which doesn't support type parameter as of now. @alexknvl might be able to do some experiments with it.
  3. fold a newtype project (one of above) into a cats module (could be cats.kernel or a new one) that other cats modules depends.
  4. wait for opaque-types, basically it means no newtype for cats 1.0

@kailuowang
Copy link
Contributor

kailuowang commented Sep 26, 2017

I am okay with either 3 or 4.
2 is still a question mark. but generally speaking, if cats.core has depend on something it has to be something that we have control over binary compat, otherwise Cats' bin compat promise becomes rather useless. so neither 1 or 2 should on the table.

@edmundnoble
Copy link
Contributor

I prefer 3. I'm not really okay with 4 myself. I think we're going to get stuck with a lot of things we don't want to be stuck with if we wait that long.

@oscar-stripe
Copy link
Contributor

I prefer 4 since we are in the midst of this currently unfolding.

Also, can we have some concrete proposals of where we want newtype in cats? For things like EitherT or OptionT or something? Can we actually make a benchmark that shows some win first? Usually one more allocation in a stack 2-deep of monads is not actually going to be a big problem.

I don't want to complicate things, nor consider dropping 2.10, if we don't have a concrete win to look at.

I added this in gitter:

the newtype macro is very new.
it would be safe IF:
1) the encoding is binary compatible from release to release. Which it may or may not be.
and
2) it leaves no runtime calls in the generated code that make visible changes to the class files.
even if that is true today, it might not be true tomorrow. I think it is a risky bet, unless you are willing to stay on a fixed version after some change comes.
in my experience, binary compatibility with scala is a big pain.

@kailuowang
Copy link
Contributor

For context, the use cases for newtype in in cats that I noticed recently.

  1. In Consider Reader-style instance for MonoidK of Kleisli #1928, to represent the endmorphic Kleisli
  2. Several newtypes in Add Parallel type class #1837
  3. SemigroupK and MonoidK as pointed out in Removing MonoidK and SemigroupK #1932
  4. OptionT, EitherT as suggested above by @oscar-stripe . Although I am not sure if they are the original incentive by @edmundnoble.

@LukaJCB
Copy link
Member Author

LukaJCB commented Sep 26, 2017

To add to those, we also have NonEmptyVector and Nested come to mind. Also those defined in newts. :)

@edmundnoble
Copy link
Contributor

I think I can try for a stab at the use-cases we have in addition to the ones presented so far.

Type class disambiguation

  • First and Last semigroup, as in x |+| y == x and x |+| y == y
  • FirstOption, LastOption monoids, FirstEither, LastEither semigroups. Same for transformers
  • Sum and Product monoids for numeric types
  • Validated as a newtype over Either

Zero-overhead data types

  • All monad transformers
  • NonEmptyList and NonEmptyVector
  • Every data type we have, all of it, which has a single-member

@kailuowang
Copy link
Contributor

@LukaJCB I am curious if you still plan to do the benchmark?

@LukaJCB
Copy link
Member Author

LukaJCB commented Oct 4, 2017

I haven't made a lot of progress yet. Tried for about an hour last weekend, but ran into some errors with JMH in combination with @alexknvl newtypes library, will have to do more research on that

@kailuowang
Copy link
Contributor

Given the scope mentioned above, and the fact that the new type mechanism in Scala doesn't seem to be stable yet, also once the opaque type is out, we probably have to change it again. Now I incline to not include it in Cats 1.0 and lock it down. Users can choose their own new type lib for now. Cats internal usage of new type might be easier to manage (IRT to backward compat) if we keep the new type mechanism also internal.

@johnynek
Copy link
Contributor

I agree this is premature for cats 1.0.

@kailuowang kailuowang mentioned this issue Oct 16, 2017
12 tasks
@LukaJCB LukaJCB changed the title Consider adding some form newtypes to cats Consider adding some form of newtypes to cats Jun 13, 2018
@LukaJCB
Copy link
Member Author

LukaJCB commented Jun 13, 2018

I would like to reopen this discussion :)
Especially in light of 2.13 adding opaque types, I think we should consider our options again.
I don't remember who, but someone said somewhere that a macro internal to cats would be good because we could target pre-existing solutions (like those already done for NonEmptySet and NonEmptyMap) for older versions, while at the same time making use of the new functionality in 2.13.

I'll also try to get those benchmarks I promised a long time ago, to see what we can gain here :)

@kailuowang
Copy link
Contributor

kailuowang commented Jun 13, 2018

somewhere that a macro internal to cats would be good because we could target pre-existing solutions

It might be me, a macro based solution can probably support cross build. It could be internal to cats or since cross building new type would be very commonly used, cats can depend on https://github.com/estatico/scala-newtype and help support it.

@LukaJCB
Copy link
Member Author

LukaJCB commented Jun 14, 2018

I like the idea! I'm not super sure how stable that project is, but I've used it a lot and had only good experiences so far 👍

@LukaJCB
Copy link
Member Author

LukaJCB commented Jun 14, 2018

I did those benchmarks for a very simple parTraverse over Either, results seem very promising:

[info] Benchmark                   Mode  Cnt   Score   Error  Units
[info] ParallelBench.currentBench  avgt   15  49.324 ± 0.299  ms/op
[info] ParallelBench.newtypeBench  avgt   15   2.798 ± 0.028  ms/op

Full code here:
https://github.com/LukaJCB/newtype-bench

I can imagine results being even more dramatic when using nested Monad transformers.

@kailuowang
Copy link
Contributor

kailuowang commented Jun 14, 2018

This will be a good reason to go cats 3.0 (assuming 2.x maintains BC on Scala 2.12). Do we know the timeline of adding opaque type to 2.13?

@LukaJCB
Copy link
Member Author

LukaJCB commented Jun 14, 2018

IIRC, the timeline is Q3 for 2.13.0 and Q4 for 2.13.1 and I think Opaque types will be in the 2.13.1 version (it turned out to be harder to implement than originally thought)

On another note, do we think we'll release cats 2.0 and 3.0 at the same time?

@kailuowang
Copy link
Contributor

@LukaJCB Just created #2296

@LukaJCB
Copy link
Member Author

LukaJCB commented Aug 3, 2018

@carymrobbins What do you think of the idea of having cats depend on scala-newtype? Do you think it will be stable enough, or a bad idea? Would love to hear your thoughts :)

@carymrobbins
Copy link
Contributor

carymrobbins commented Aug 3, 2018

@LukaJCB I like it! Do you think cats will look to use mostly the annotation macros (@newtype / @newsubtype)? I'm thinking before newtype gets a 1.0 release (which I think should happen before we have cats depend on it) we should consider removing the legacy encoding. If that works for you, then at this point the API should be considered pretty stable. A good plan of action may be to -

  • Start adding newtype 0.x (latest is 0.4.2) to cats (unreleased)
  • Along the way, determine if changes are needed from newtype (ideally this won't happen, but considering it for pragmatism)
  • Make additional desired changes, if any
  • Finish up work on newtype arrays (Support Array casting estatico/scala-newtype#10)
  • Remove the legacy encoding from newtype (if this is a problem for folks, I could split it out into a newtype-legacy library, but hopefully not)
  • Release newtype 1.0
  • Before releasing cats, bump its newtype dep to 1.0 (should "just work" at this point)

Let me know what you think. Also, feel free to adjust the above points if either I'm over/underthinking things.

@LukaJCB
Copy link
Member Author

LukaJCB commented Aug 4, 2018

I think we'll primarily use @newtype, I don't think we'd use @newsubtype anywhere really.
I really like that plan and hope we can get it started ASAP :)

@kailuowang
Copy link
Contributor

If we go with the option 1 in this roadmap, this will be Cats 3.0 right?

@LukaJCB
Copy link
Member Author

LukaJCB commented Aug 6, 2018

Right, yeah it depends on how we handle our next release. :)

@diesalbla
Copy link
Contributor

diesalbla commented Mar 19, 2021

@LukaJCB Good evening. I have been sweeping a bit through old issues of cats. It has now been three and a half years since you once opened this issue, and IIUC it never got to fruition. Given that Scala 3 is to provide its own solution for Opaque Type Aliases, should we close this one?

@joroKr21
Copy link
Member

I think we should consider Cats 3.0 for Scala 3 using opaque types

@armanbilge
Copy link
Member

Yep, I think we can close this one. Anyone who needs something for Scala 2 can look at https://github.com/monix/newtypes.

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

10 participants