Unseal InjectK to allow for extension by other libraries

[andyscott]

It would be extremely useful to open up the injection helpers in Free to support any target coproduct type. This is the simplest solution to address this "issue".

Related gitter conversation: https://gitter.im/typelevel/cats?at=590627c9d32c6f2f0956189d

For discussion:

• Should this be converted to a trait?
• Should apply/unapply be removed?
• Should a test be added to ensure injection into type constructors other than EitherK can occur?

[edmundnoble]

Looks good, I don't think practically sealing this buys us anything. This type class is much more useful as "injective natural transformations", rather than "how to get in and out of EitherK".

[andyscott]

The only benefit, IMHO, of leaving it sealed is that the implementation stays true to the reference paper.

That being said, also I prefer that InjectK be a generalized injection type class for natural transformation.

[codecov-io]

Codecov Report

Merging #1642 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #1642   +/-   ##
=======================================
  Coverage   93.37%   93.37%           
=======================================
  Files         240      240           
  Lines        3941     3941           
  Branches      138      138           
=======================================
  Hits         3680     3680           
  Misses        261      261

Impacted Files	Coverage Δ
core/src/main/scala/cats/InjectK.scala	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a90b61...11ddf9e. Read the comment docs.

[kailuowang]

👍 for the change. Since we'd like to treat this as a typeclass, how about making it a trait? We may decide to add a SurjectK and BijectK later, BijectK can extend both SurjectK and InjectK.

Edit: another question: why not making apply and unapply final?

[andyscott]

Update: I switched the class to a trait and added final to apply/unapply.

[kailuowang]

Thanks very much @andyscott. It's probably too much to ask for this PR but I started to wonder if we should/can add laws to this new typeclass. We don't have typeclasses that take two type constructors yet, so it will be a first.

[andyscott]

Adding laws seems reasonable. Would it be okay to handle that with a separate PR (and maybe an issue for discussion purposes)?

[kailuowang]

do you want to create an issue for this law or I can create one? @andyscott

[kailuowang]

LGTM, I'll wait for another one or two signoff. @edmundnoble do you want to do another quick round?

[edmundnoble]

I actually don't think we should change the "abstract class" part. Nobody should be using mixin inheritance with a class with an apply and an unapply; practically I don't believe anyone will, so the trait conceptual and bytecode overhead isn't worth it.

[kailuowang]

@edmundnoble I was thinking the possibility of having a SurjectK typeclass and BijectK typeclass mixing both SurjectK and InjectK (would BijectK be a natural isomorphism between F[_] and G[_]?).
The unapply is convenient but I feel that it should go to something like an InjectK syntax now that we treat it more like a typeclass? @sellout, since you added those two methods, I am curious to hear your opinion over this.
That said, I don't feel strongly about making it a trait, the BijectK and SurjectK maybe too long a shot to be a factor in this design.

[andyscott]

I've changed it back to an abstract class.
RE: SurjectK-- I like it.
Also, I am bullish on splitting InjectK apart.

[kailuowang]

@andyscott how do you split Inject apart? You mean taking the apply and unapply out or split inj and prj ?

[andyscott]

@kailuowang I like the idea of splitting inj and prj apart, although I don't know if it has any practical benefit.

[kailuowang]

@andyscott it appears to me that you have to have them both available to write laws right?
Something like forall a, prj(inj(a)) == Some(a)

[sellout]

@kailuowang I would be fine with unapply moving to syntax, provided it’s still possible to use the unapply for pattern matching. We basically use it for peephole optimizations over a coproduct, so we often need to match a couple levels deep, and having to do that with match/prj/match/prj is painful and unreadable.

[andyscott]

@kailuowang Yep, for any general roundtrip law(s) you need both.

[andyscott]

@edmundnoble Are changes still needed? Should we move unapply to syntax?

[edmundnoble]

@andyscott This gets a 👍 from me after the abstract class change.

[andyscott]

@edmundnoble I've changed it back to abstract class already ☺️

[edmundnoble]

I know. Sorry for the ambiguity ;)

[kailuowang]

@andyscott thanks! Let's merge now and we can work on making it a type class for Functors later.