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

Split interruptible #2343

Merged
merged 20 commits into from
Oct 10, 2021
Merged

Split interruptible #2343

merged 20 commits into from
Oct 10, 2021

Conversation

alexandrustana
Copy link
Contributor

@alexandrustana alexandrustana commented Sep 10, 2021

This PR tries to fix #2268.
Running mimaReportBinaryIssues doesn't seem to report anything, so I hope the changes are binary compatible 😄

TODO:

  • add scalafix migration

Copy link
Member

@djspiewak djspiewak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really good apart from my one note! And I would generally trust mima: the approached you used is indeed binary compatible.

core/shared/src/main/scala/cats/effect/IO.scala Outdated Show resolved Hide resolved
@vasilmkd
Copy link
Member

Thanks for taking this on.

@djspiewak djspiewak added this to the v3.3.0 milestone Sep 13, 2021
@alexandrustana alexandrustana marked this pull request as ready for review September 13, 2021 11:00
@djspiewak
Copy link
Member

for the output test folder a new version which is not yet released and will contain these changes)

Can we solve this by publishing a snapshot?

@@ -1466,9 +1466,13 @@ object IO extends IOCompanionPlatform with IOLowPriorityImplicits {

override def blocking[A](thunk: => A): IO[A] = IO.blocking(thunk)

override def interruptible[A](many: Boolean)(thunk: => A): IO[A] =
private[effect] override def interruptible[A](many: Boolean)(thunk: => A): IO[A] =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait you can do that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It technically doesn't change the visibility in the bytecode, so yes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure it does what we want downstream?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, let's not take that risk.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scala> import cats.effect.IO
import cats.effect.IO

scala> IO.interruptible(false) { println("hi") }
val res0: cats.effect.IO[Unit] = IO(...)

Looks like it doesn't mask it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, thanks for confirming. Good to know.

@djspiewak
Copy link
Member

For posterity, the problem here is the following:

[error] cats-effect: Failed binary compatibility check against org.typelevel:cats-effect_2.13:3.1.0! Found 1 potential problems (filtered 7)
[error]  * static method interruptible(Boolean,scala.Function0)cats.effect.IO in class cats.effect.IO does not have a correspondent in current version
[error]    filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("cats.effect.IO.interruptible")

Looks like the private[effect] overload isn't getting peered into a static forwarder. I'm not totally certain how to trick the compiler into doing that…

@djspiewak
Copy link
Member

IMO, it's worth breaking binary compatibility with Java interop on this one function in order to get the better API for everyone. Anyone have any objections? Paging @rossabaker and @johnynek for opposing opinions.

@johnynek
Copy link

just to chime in:

My feeling is that runtime errors really suck, and if you want to be a super core library, you will be implicated in many diamonds, so try to never break binary compatibility.

But this is your project, of course, so make your own calls.

@djspiewak
Copy link
Member

I do very much agree with this. The only reason I'm even pondering the breakage is the fact that it's confined to the Java interop exclusively. Scala code paths never hit this forwarder and remain binary compatible.

@armanbilge
Copy link
Member

FWIW http4s recently made a similar decision to eschew Java forwarders in http4s/http4s#5071 (comment).

@alexandrustana
Copy link
Contributor Author

@djspiewak @vasilmkd, can someone publish a snapshot, please, so I can try and write the scalafix migration? (if it is still needed I mean 😄 )

@vasilmkd
Copy link
Member

I will right now.

@vasilmkd
Copy link
Member

"io.vasilev" %%% "cats-effect" % "3.3-208-b352672"

@alexandrustana

Copy link
Member

@vasilmkd vasilmkd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did the build.sbt changes get pulled in? That PR is not merged yet.

@rossabaker
Copy link
Member

This one is tough: a marginally better API for what is probably 100% of users, or a landmine for an unanticipated use case. We chose the Scala users in http4s, but it makes me queasy, and the risk is higher on this project.

A third option is bytecode postprocessing. I haven't done it in the Scala Era, but I'm pretty sure it would be possible.

@vasilmkd
Copy link
Member

With a repeated word of caution and previous experience, maybe it is worth reconsidering the extent of this change.

@alexandrustana
Copy link
Contributor Author

Having these in mind, should I close this PR?

@vasilmkd
Copy link
Member

vasilmkd commented Oct 5, 2021

No, not yet, let's have a discussion with Daniel, whenever he's back.

We'll definitely add interruptibleOnce and interruptibleMany if this doesn't go through, and the scalafix rules will still be good (with minor changes).

@djspiewak
Copy link
Member

I'm back ish. :-) To @rossabaker's point… There's another option here, which is that we can keep the current interruptible overload public and mark it as @deprecated. That's what's visible currently in the PR, and thus there is no binary breakage at all. It's kind of ugly, but it's not really the end of the world, and it's probably less fraught than bytecode shenanigans in the build.

Either way, I think that we can shift the discussion a bit. We agree the changes here are good (I think), and they're binary-compatible as currently written. I'm in favor of merging this PR and having a follow-up if we want to get fancy with hiding the old interruptible overload.

Copy link
Member

@djspiewak djspiewak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work, @alexandrustana! Thanks for shepherding this through the process, and sorry for handing you such a thorny change.

.in(file("v3_3_0/output"))
.settings(
libraryDependencies ++= Seq(
"io.vasilev" %% "cats-effect" % "3.3-208-b352672"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remember to update this to the mainline 3.3.0 once released

@djspiewak djspiewak merged commit b117e63 into typelevel:series/3.x Oct 10, 2021
@fthomas
Copy link
Member

fthomas commented Oct 11, 2021

FYI: scala-steward-org/scala-steward#2270 adds the v3_3_0 Scalafix to Scala Steward.

@alexandrustana
Copy link
Contributor Author

@djspiewak, @vasilmkd thank you for all the help and support 🙂

@vasilmkd
Copy link
Member

Thank you @fthomas!

@alexandrustana Thank you for the outstanding work and your patience.

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

Successfully merging this pull request may close these issues.

Deprecate interruptible(boolean) and replace with interruptible/interruptibleMany
7 participants