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

Figure out how to handle SI-2712 fix #1073

Closed
ceedubs opened this issue May 30, 2016 · 9 comments · Fixed by #1583
Closed

Figure out how to handle SI-2712 fix #1073

ceedubs opened this issue May 30, 2016 · 9 comments · Fixed by #1583

Comments

@ceedubs
Copy link
Contributor

ceedubs commented May 30, 2016

The infamous SI-2712 is fixed under the "experimental" compiler flag in scala 2.12. The fix is available via a compiler plugin for scala 2.10 and 2.11. We should figure out how cats is going to handle this change. Should it remove Unapply and ask people to use one of these two approaches if they want an SI-2712 workaround? Keeping around other SI-2712 workarounds such as Unapply could potentially cause issues with this fix in place.

cc @milessabin

@ceedubs ceedubs added this to the cats-1.0.0 milestone May 30, 2016
@kailuowang
Copy link
Contributor

If the SI2712 fix is required on the use site, would that mean that all libraries they use must be also at least SI 2712 fix compatible (i.e. hide their unapply properly) ?

@ceedubs
Copy link
Contributor Author

ceedubs commented May 31, 2016

If the SI2712 fix is required on the use site, would that mean that all libraries they use must be also at least SI 2712 fix compatible (i.e. hide their unapply properly) ?

I'm hoping @milessabin can weigh in, as he probably has a better idea on when problems can arise. At work we added the SI-2712 fix plugin to a project that uses scalaz heavily, and we were able to change a bunch of traverseU calls into just traverse and didn't seem to have any issues. I'm guessing that potential issues come up if you call code that uses something like Unapply (instead of changing traverseU calls to traverse), resulting in now-ambiguous instances.

@milessabin
Copy link
Member

The plugin would be required downstream for 2.10.x and 2.11.x. The fix can be backported (basically already has been), and we might be able to persuade Lightbend to make a 2.10.7 and 2.11.9 release with it included.

@kailuowang
Copy link
Contributor

BTW, if nobody has reported yet, I tested cats with SI-2712 fix plugin and some tests failed to compile due to ambiguous instance. Maybe the first step should be fixing those?

@non
Copy link
Contributor

non commented Jun 11, 2016

Here's my 2¢:

I think the easiest thing is to leave the unapply machinery in, and not use the SI-2712 fix in Cats, but to try to ensure that everything we provide can be used with the option/plugin by downstream libraries and end users. Until there are early adopter reports for the fix I think it's likely that there will be other people who are nervous about it, so I'd like to support them in the short term (but not necessarily the long term).

Also right now library authors who want to support cross versioning will have to do a fairly tricky SBT set up to conditionally enable compiler plugins or flags depending on the Scala version. This is definitely possible, but it's harder than I'd like. I think a big first step toward standardizing on this fix would be creating an SBT plugin that papers over the version distinction (i.e. it knows if a given version requires a plugin or a -Y option), allowing library authors to cross-build and publish easily.

Once we reach a place where either the fix is available in all versions we care about (2.10 through 2.12 currently), or we have an SBT plugin that figures it out for us, I'm much more open to tearing out the unapply machinery and having everyone standardize on the SI-2712 fix.

ceedubs added a commit to ceedubs/cats that referenced this issue Jun 20, 2016
Fixes typelevel#1146.

I followed the convention for `Functor` (okay I copied it and did a find/replace). Considering the recent fix for SI-2712, we'll probably want to handle the `Unapply` differently, but I'd be inclined to solve all of that at once under typelevel#1073.
@kailuowang
Copy link
Contributor

conniptions @djspiewak generally use to "just give me partial unification" from 2.10 through to 2.12
https:/djspiewak/base.g8/blob/master/src/main/g8/build.sbt#L81-L88 and
https:/djspiewak/base.g8/blob/master/src/main/g8/build.sbt#L96-L102

@tpolecat
Copy link
Member

My vote is to remove Unapply for cats 1.0 … the 2712 fix is well known at this point and available for all versions. I don't think it's worth the effort to maintain the machinery any longer (much less until 2.0).

@mpilquist
Copy link
Member

👍 for removing for 1.0.

@kailuowang
Copy link
Contributor

It looks like that the consensus is to tear out the Unapply machinery and having everyone standardize on the SI-2712 fix. I am going to work on a PR.

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

Successfully merging a pull request may close this issue.

6 participants