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

Enable interop for scala.js #28

Merged
merged 19 commits into from
Aug 29, 2019
Merged

Enable interop for scala.js #28

merged 19 commits into from
Aug 29, 2019

Conversation

joroKr21
Copy link
Contributor

@joroKr21 joroKr21 commented Jul 31, 2019

Revive PR zio/zio#777

  • Test more thoroughly
  • Delete Alternative instance (unlawful)
  • Simplify some method implementations

There are two incompatibilities that need to be fixed (possibly in ZIO itself):

  • bracket release is called on Completed or Error

  • runCancelable is synchronous

I don't think we can claim compatibility with cats-effect without fixing those.

Edit: I don't have the bandwidth to investigate the failures so I disabled the tests.
We can open separate issues for them.

This fixes #20

@jdegoes
Copy link
Member

jdegoes commented Aug 5, 2019

There are two incompatibilities that need to be fixed (possibly in ZIO itself):

Can you open two issues in ZIO for these, ideally with the tests restated using ZIO, rather than Cats Effect?

jdegoes
jdegoes previously approved these changes Aug 5, 2019
Copy link
Member

@jdegoes jdegoes left a comment

Choose a reason for hiding this comment

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

Looks good to me but should get feedback from @Kaishh or others.

@ghostdogpr
Copy link
Member

ghostdogpr commented Aug 5, 2019

Can you rebase to latest master? There was a lot of renaming due to upgrading to the latest ZIO (in particular, TaskR => RIO)

@joroKr21
Copy link
Contributor Author

joroKr21 commented Aug 6, 2019

Can you open two issues in ZIO for these, ideally with the tests restated using ZIO, rather than Cats Effect?

I'm not sure how to do that, the laws rely on classes from cats-effect. I will give it a try though.

Can you rebase to latest master? There was a lot of renaming due to upgrading to the latest ZIO (in particular, TaskR => RIO)

Rebased

  * Test more thoroughly
  * Delete `Alternative` instance (unlawful)
  * Simplify some method implementations
  * `bracketReleaseIsCalledOnCompletedOrError`
  * `runCancelableIsSynchronous`
@neko-kai
Copy link
Member

runCancelable is synchronous is just the lack of latch in upstream laws typelevel/cats-effect#611

@joroKr21
Copy link
Contributor Author

@neko-kai nice!

@neko-kai
Copy link
Member

Seems like the same applies to bracketReleaseIsCalledOnCompletedOrError typelevel/cats-effect#612

@neko-kai
Copy link
Member

I've also made a change to stop using unsafeRunAsyncAndForget since it logs failures (in addition to RTS doing that anyway and unlike cats IO implementations of the same)

@neko-kai
Copy link
Member

@joroKr21
Is this PR good to merge now?

@neko-kai
Copy link
Member

neko-kai commented Aug 28, 2019

[info] - Concurrent[Task].concurrent.uncancelable prevents Cancelled case *** FAILED ***
[info]   GeneratorDrivenPropertyCheckFailedException was thrown during property evaluation.
[info]    (Discipline.scala:12)
[info]     Falsified after 2 successful property evaluations.
[info]     Location: (Discipline.scala:12)
[info]     Occurred when passed generated values (
[info]       arg0 = <function1>,
[info]       arg1 = zio.ZIO$FlatMap@daeb5e4,
[info]       arg2 = zio.ZIO$FlatMap@2615c453
[info]     )
[info]     Label of failing property:
[info]       Expected: zio.ZIO$InterruptStatus@66e2b1e4
[info]   Received: zio.ZIO$InterruptStatus@70488c06
[info] - ApplicativeLocal[ZIO[Ctx, Error, ?]].applicativeLocal.local distributes over ap *** FAILED ***
[info]   GeneratorDrivenPropertyCheckFailedException was thrown during property evaluation.
[info]    (Discipline.scala:12)
[info]     Falsified after 1 successful property evaluations.
[info]     Location: (Discipline.scala:12)
[info]     Occurred when passed generated values (
[info]       arg0 = zio.ZIO$FlatMap@63f20abb,
[info]       arg1 = zio.ZIO$FlatMap@655029c5,
[info]       arg2 = <function1>
[info]     )
[info]     Label of failing property:
[info]       Expected: zio.ZIO$FlatMap@140e882f
[info]   Received: zio.ZIO$Read@434132d

https://circleci.com/gh/zio/interop-cats/582
https://circleci.com/gh/zio/interop-cats/585

Non-deterministic failures on CI? 🤔
Or maybe these are also equalities broken by genRace and genParallel changing TestContext-based equality?

…nParallel by ZManaged.fork-based version; split out shared test implicits; re-add repeat tests for ApplicativeLocal & ConcurrentEffect
@ghostdogpr
Copy link
Member

I started to get weird diverging implicit issues after updating a project to cats 2.0.0-RC2:

diverging implicit expansion for type cats.Applicative[[+A]zio.ZIO[Any,Nothing,A]]
[error] starting with method catsKernelStdOrderForSortedSet in trait LowPrioritySortedSetInstancesBinCompat1
[error]   val test: UIO[Option[Unit]] = Option(ZIO.unit).sequence

I narrowed it down to ioMonoidInstances : by removing it, the diverging implicit error disappear. And I just saw that you removed it in this PR for other reasons but anyway that fixes the problem (tried with this PR branch).

@joroKr21
Copy link
Contributor Author

@neko-kai that's great work. Thank you so much for getting to the bottom of these tricky laws!

I've also made a change to stop using unsafeRunAsyncAndForget since it logs failures (in addition to RTS doing that anyway and unlike cats IO implementations of the same)

Great, I hadn't thought of that 👍

Non-deterministic failures on CI? thinking
Or maybe these are also equalities broken by genRace and genParallel changing TestContext-based equality?

Did you figure this out?

It's a bit crazy how much stack and heap we need, but it's not enough for scala.js
Is sbt not picking the .jvmopts up? Or something could be done in CircleCI config.

@neko-kai
Copy link
Member

@joroKr21

Did you figure this out?

Yeah, io.zipPar(parIO) in genParallel is non-deterministic wrt TestContext tasks, so I worked around it by replacing with ZManaged(parIO).fork.use_(io)...

OOM build failures on JS still remain though...

@neko-kai
Copy link
Member

Is sbt not picking the .jvmopts up?

Yeah, sounds like they were ignored, moving options to .sbtopts worked

@neko-kai neko-kai merged commit 0f97463 into zio:master Aug 29, 2019
@neko-kai
Copy link
Member

Merged! 🎉 @joroKr21 Thank you!

@joroKr21 joroKr21 deleted the js-interop branch August 31, 2019 14:47
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.

Diverging implicit extension for UIO when importing cats instances
4 participants