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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

CE3 Laws upgrade #3807

Merged
merged 4 commits into from Nov 9, 2020
Merged

CE3 Laws upgrade #3807

merged 4 commits into from Nov 9, 2020

Conversation

domaspoliakas
Copy link
Contributor

As per the title. I spent a good chunk of time reading docs and sources and wasn't getting anywhere, so I though I'll take Fabio's advice and open a draft PR for guidance.

My troubles come from 2 places:

  • EntityCodecTests
  • ArbitraryInstances

I'll start with EntityCodecTests. To refresh your memory it currently looks like this

    def entityCodec(implicit
        arbitraryA: Arbitrary[A],
        shrinkA: Shrink[A],
        eqA: Eq[A],
        eqFBoolean: Eq[F[Boolean]],
        testContext: TestContext): RuleSet =
      new DefaultRuleSet(
        name = "EntityCodec",
        parent = Some(entityEncoder),
        "roundTrip" -> Prop.forAll { (a: A) =>
          laws.entityCodecRoundTrip(a)
        }
      )

Because I changed Effect for Concurrent in EntityCodecLaws now entityCodecRoundTrip is a IsEq[F[Either[Decodefailure, A]]. And that is failing to be converted to a prop, and I wasn't sure what a good way forward would be.

Admittedly I'm not very well practiced with IsEq. But my understanding is that for IsEq[A] to be converted to a prop it needs an Eq[A] and a A => Pretty. I could of course add those as implicits, but that doesn't seem like the right solution as it would shift a significant burden to the caller of this method that was previously handled internally. However, I'm not sure how to keep it inside this method either. In cats-effect tests the Eq[IO[A]] instance is defined by running the IO. As such, one option would be to add a dispatcher implicit parameter and similarly define the Eq[F[A]] instance in terms of running the F using the dispatcher. However, Daniel previously mentioned that passing a dispatcher around using implicits does not seem like a good idea. One could add an explicit parameter list with Dispatcher to remedy that, but that feels like it could have downstream implications that would result in requiring a not insignificant rewrite to accommodate the new Dispatcher requirement.

Writing this all down the last option seems like it could be the most viable, but at this point my confidence of delivering a quality replacement here is pretty low, so I'd appreciate some advice.

Anyway, on to number 2: ArbitraryInstances. The problem here stems from Cogen[EntityBody[F]]. Previously it required F[_] to be Effect and then defined the cogen in terms of Cogen[IO[A]] imported from cats' own arbitraries. There is no more Cogen[IO[A]] (and of course no more Effect) and as such this poses a problem.

The problem here is very similar to EntityCodecTests. Now that cats-effect-laws (or the new testkit) do not provide the Cogen[IO[A]], one has to introduce a Cogen[F[A]]. This can similarly be remedied with the help of Dispatcher, however the question of where to source the dispatcher pops up once more. Here I thought about 4 potential solutions.

First option is similar to that of EntityCodecTests: add an implicit Dispatcher, but as I mentioned - I have some reservations as per Daniel's advice.

Second option is to define an abstract Dispatcher val on the whole trait and use that to define the cogens in terms of running unsafely. This has the huge downside of one not being able to just import arbitraries - one would have to mix ArbitraryInstances in to provide the dispatcher or create per-test instances of some concrete implementation of ArbitraryInstances which seems like it'd introduce significant boilerplate.

Option no3 is the same as 2 except I'd extract cogens into their own trait.

Option no4 is basically 2 and 3 except the abstract val is a Resource[Dispatcher[F]] and is acquired in the cogen every time.

Here I'm leaning towards the implicit since it is the least intrusive, but I am wary that it is the wrong choice.

Nonetheless, I implore you for guidance. This has been rattling around in my brain not going anywhere for a number of days now and I'm anxious about moving this forward. If it'd be too much time guiding me through getting this remedied I am perfectly fine handing this off to someone else too.

Thanks for sticking through to the end of this very long read 馃槄

@domaspoliakas domaspoliakas changed the title Small steps forward [WIP] CE3 Laws upgrade - Small steps forward Nov 4, 2020
@SystemFw
Copy link
Member

SystemFw commented Nov 5, 2020

On EntityCodecTests, it seems to be only used at IO, so one simple solution would be to just put IO there, and remove the parameterisation. For the general problem, I don't really see an issue with requiring an implicit Dispatcher, and then provide one in the concrete tests through allocated

@rossabaker
Copy link
Member

I don't think there are many external consumers of these laws, and I'm not terribly concerned about breaking changes (i.e., taking an explicit dispatcher) if it otherwise seems the right thing to do.

Theoretically, it's nice to be able to test in an abstract F, particularly in a public project. But it making it work for IO is the path of least resistance, it won't hurt us.

I'd say whichever of those gets it compiling and gets us unblocked is great for now. Along with testing, a bunch of other things will open up after this, and we can refactor later.

@domaspoliakas
Copy link
Contributor Author

domaspoliakas commented Nov 5, 2020

Thanks for both of your input. I've started implementing it as Fabio suggested, shouldn't take too long from here.

@domaspoliakas
Copy link
Contributor Author

domaspoliakas commented Nov 6, 2020

I ended up fixing both issues by adding Dispatcher, so I left EntityCodecTest as F. I think one odd thing to me that remains here is that e.g. in arbitraries now you have to have both Dispatcher and TestContext as an implicit parameter to a few methods, which I think raises my eyebrow because they both kind of do the same thing; in real tests you'd probably want a Dispatcher that uses TestContext under the hood anyway, so it kind of seems redundant to need both.

I'm not sure that's a real problem or not though. Should I try to eliminate one in favour of the other, or should I leave it as is?

@RaasAhsan
Copy link
Member

RaasAhsan commented Nov 6, 2020

I think it makes sense to have a reference to both if you're running in the ticked executor. You need Dispatcher to request the evaluation of effects in an unsafe region and TestContext to drive the execution of fibers. In other words, if your Dispatcher is running on a TestContext, dispatched effects won't actually run unless you tick. But I imagine the Dispatcher in these tests is already using the TestContext?

@domaspoliakas
Copy link
Contributor Author

Atm the dispatcher is provided by the caller, so technically one could supply a dispatcher that isn't backed by TestContext. This only affects cogen instances though, so I'm not sure if that's an actual problem or not.

@RaasAhsan
Copy link
Member

Got it. I'm not completely sure how this stuff interacts with Cogen yet, but having a Dispatcher that isn't backed by the TestContext defeats the purpose of using TestContext if it loses determinism. I think what you have here looks good though.

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

I think we're all set here, then?

@@ -9,12 +9,11 @@ package laws
package discipline

import cats.Eq
import cats.implicits._
// import cats.implicits._
Copy link
Member

Choose a reason for hiding this comment

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

Can delete the comment

@domaspoliakas domaspoliakas marked this pull request as ready for review November 8, 2020 21:42
@domaspoliakas domaspoliakas changed the title [WIP] CE3 Laws upgrade - Small steps forward CE3 Laws upgrade Nov 9, 2020
@domaspoliakas
Copy link
Contributor Author

I think it should be all good. Similarly as Raas I'm not certain about how Cogen and TestContext mix, but any problems should be cought when fixing futher modules.

@rossabaker rossabaker merged commit 9e01a41 into http4s:cats-effect-3 Nov 9, 2020
@rossabaker rossabaker added this to To do in Cats Effect 3 via automation Nov 9, 2020
@rossabaker rossabaker moved this from To do to Done in Cats Effect 3 Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants