8000 Fixing bug in implementation of makeSome without repeting fresh layers by pablf · Pull Request #8850 · zio/zio · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fixing bug in implementation of makeSome without repeting fresh layers #8850

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

Fixing bug in implementation of makeSome without repeting fresh layers #8850

wants to merge 5 commits into from

Conversation

pablf
Copy link
Member
@pablf pablf commented May 12, 2024

Reimplements #8678 fixing #8767 and adds new tests with different combination of layers to check behaviour.

@pablf
Copy link
Member Author
pablf commented May 12, 2024 •
edited
Loading

@ghostdogpr Would you mind taking a look? Do you think the testing is enough or are there some other use cases that you think should be covered? The provideSome issue should be fixed as it uses the same algorithm. Thanks!

@ghostdogpr
Copy link
Member

When I get time, I will try to test that one on my work codebase. Would be nice if others did too @kyri-petrou maybe?

@ghostdogpr
Copy link
Member

I am getting false positive warnings like this:

[error] ──── ZLAYER WARNING ──────────────────────────────────────────────────
[error] 
[error]  You have provided more arguments to provideSome than is required.
[error]  You may remove the following type:
[error]    
[error]    1. com.devsisters.ck.dependencies.package.TransactorForCK
[error] 
[error]   
[error] ──────────────────────────────────────────────────────────────────────

when doing

type BaseDependencies = Config with TransactorForCK
trait TransactorForCK { def transactor: Transactor[Task] }

ZLayer.makeSome[BaseDependencies, SomeService]

where I am not supplying TransactorForCK and some layers need it.

8000
@pablf
Copy link
Member Author
pablf commented May 14, 2024 •
edited
Loading

I am not able to create a similar warning. Are there other type of incorrect warnings or are all the same? Would you mind giving a minimal example or some more information? The layers and its In, Out should suffice.
These examples are currently compiling without warning, but it's only up to three layers.

{

  def test1[I1, O1, I2](
    a: ZLayer[I1, Nothing, O1],
    b: ZLayer[I2, Nothing, Int]
  ): ZLayer[I1 & I2, Nothing, O1 & Int] =
    ZLayer.makeSome[I1 & I2, O1 & Int](a, b)

  def test2[I1, O1](a: ZLayer[I1, Nothing, O1], b: ZLayer[O1, Nothing, Int]): ZLayer[I1, Nothing, O1 & Int] =
    ZLayer.makeSome[I1, O1 & Int](a, b)

  def test3[I1, O1](a: ZLayer[I1, Nothing, O1], b: ZLayer[I1, Nothing, Int]): ZLayer[I1, Nothing, O1 & Int] =
    ZLayer.makeSome[I1, O1 & Int](a, b)

}
{

  def test1[R, R1](a: ZLayer[R1 & Int, Nothing, R], b: ZLayer[Int, Nothing, R1]): ZLayer[Int, Nothing, R] =
    ZLayer.makeSome[Int, R](a, b)

  def test2[I1, I3, O1, O2](
    a: ZLayer[I1 & Double, Nothing, O1 & O2],
    b: ZLayer[I3 & Float, Nothing, Int]
  ): ZLayer[I1 & Double & I3 & Float, Nothing, O1 & O2 & Int] =
    ZLayer.makeSome[I1 & Double & I3 & Float, O1 & O2 & Int](a, b)

  def test3[I1, I4, O1, O2](
    a: ZLayer[I1 & Double, Nothing, O1 & O2],
    b: ZLayer[I1 & Float, Nothing, Int]
  ): ZLayer[I1 & Double & Float, Nothing, O1 & O2 & Int] =
    ZLayer.makeSome[I1 & Double & Float, O1 & O2 & Int](a, b)

  def test4[I1, O1](
    a: ZLayer[I1 & Double, Nothing, O1 & String],
    b: ZLayer[O1 & Float, Nothing, Int]
  ): ZLayer[I1 & Double & Float, Nothing, O1 & String & Int] =
    ZLayer.makeSome[I1 & Double & Float, O1 & String & Int](a, b)


}
{
  def test1[A1, A2, B1, C1](a: ZLayer[A1, Nothing, A2], b: ZLayer[B1, Nothing, Int], c: ZLayer[C1, Nothing, Float]): ZLayer[A1 & B1 & C1, Nothing, A2 & Int & Float] =
    ZLayer.makeSome[A1 & B1 & C1, A2 & Int & Float](a, b, c)

  def test2[A1, A2, B1, C1](a: ZLayer[A1, Nothing, A2], b: ZLayer[B1, Nothing, Int], c: ZLayer[B1 & C1, Nothing, Float]): ZLayer[A1 & B1 & C1, Nothing, A2 & Int & Float] =
    ZLayer.makeSome[A1 & B1 & C1, A2 & Int & Float](a, b, c)

  def test3[A1, A2, B1, C2](a: ZLayer[A1, Nothing, A2], b: ZLayer[B1, Nothing, Int], c: ZLayer[A2 & Int, Nothing, C2]): ZLayer[A1 & B1, Nothing, C2] =
    ZLayer.makeSome[A1 & B1, C2](a, b, c)

  def test4[A1, A2, B1, C2](a: ZLayer[A1, Nothing, A2], b: ZLayer[B1, Nothing, Int], c: ZLayer[A2 & Int & Float, Nothing, C2]): ZLayer[A1 & B1 & Float, Nothing, C2] =
    ZLayer.makeSome[A1 & B1 & Float, C2](a, b, c)

  def test5[A1, A2, C2](a: ZLayer[A1, Nothing, A2], b: ZLayer[A2 & Int, Nothing, Double], c: ZLayer[A2 & Double & Float, Nothing, C2]): ZLayer[A1 & Int & Float, Nothing, C2] =
    ZLayer.makeSome[A1 & Int & Float, C2](a, b, c)

  def test6[A1, A2, B1, C2](a: ZLayer[A1, Nothing, A2], b: ZLayer[A1 & B1, Nothing, Int], c: ZLayer[A2 & Int, Nothing, C2]): ZLayer[A1 & B1, Nothing, C2] =
    ZLayer.makeSome[A1 & B1, C2](a, b, c)

  def test7[A2, C2](a: ZLayer[Int, Nothing, A2], b: ZLayer[Int & Float & A2, Nothing, Double], c: ZLayer[A2 & Double, Nothing, C2]): ZLayer[Int & Float, Nothing, C2] =
    ZLayer.makeSome[Int & Float, C2](a, b, c)
}

@ghostdogpr
Copy link
Member

It's a little tricky to minimize but I was able to reproduce with this configuration:
In one file:

trait B

val reproducer = ZLayer.makeSome[B, Reproducer.A](Reproducer.live)

In another file:

object Reproducer {

  case class A()

  val live: ZLayer[B, Nothing, A] = ???

}

Both are under different packages. If I put them together it seems to work.

@jdegoes
Copy link
Member
jdegoes commented Jun 4, 2024

@pablf Would love to get this in. Let me know when ready for a final review after fixing @ghostdogpr's reproducer.

@pablf
Copy link
Member Author
pablf commented Jun 4, 2024 •
edited
Loading

@jdegoes Puzzlingly, I haven't been able to reproduce it yet. I would like to try again this or the following week. @ghostdogpr I don't want to make you lose too much time on this, but if you had already an isolated project with this problem, would you mind sharing it? In the worst case (not being able to reproduce it) I can try to rewrite how the algorithm compares classes so that maybe somehow the bug disappears (I think something like this should be the problem), but it will be tiresome for @ghostdogpr to check it so many times... Sorry for the inconveniences! Let's see if we can get this merged!

@ghostdogpr
Copy link
Member

@jdegoes Puzzlingly, I haven't been able to reproduce it yet. I would like to try again this or the following week. @ghostdogpr I don't want to make you lose too much time on this, but if you had already an isolated project with this problem, would you mind sharing it? In the worst case (not being able to reproduce it) I can try to rewrite how the algorithm compares classes so that maybe somehow the bug disappears (I think something like this should be the problem), but it will be tiresome for @ghostdogpr to check it so many times... Sorry for the inconveniences! Let's see if we can get this merged!

I can't share it, it's my work project 😄 But I don't mind testing again if you have a new version.

@pablf
Copy link
Member Author
pablf commented Jun 13, 2024

@ghostdogpr I understand. Would you be able to isolate the reproducer in a standalone project apart from your work project? I've tried to reproduce it with different configuration (under different packages) with no luck.

If not, can you share build details? Scala version, maybe java version and maybe dependencies or other macro used that could modify something during compilation. If using some version in Scala, the bug doesn't appear, that is also helpful to know.

Also, are you able to check if the layer construction works? Because from what I see it triggers a warning, not an error. So if you are able to try it without converting warnings to errors and it creates the appropriate layer, at least we do know that the problem is that some type is expressed by the scala compiler in different ways in different places, which I don't see how to reproduce.

@pablf
Copy link
Member Author
pablf commented Jun 13, 2024

@ghostdogpr If you want, you can also try this last version.

@ghostdogpr
Copy link
Member

Your last commit solves the compile error!

@pablf
Copy link
Member Author
pablf commented Jun 14, 2024

@ghostdogpr Glad to hear that, thanks!
@jdegoes Now it's ready for review. I've changed also a test that assumed fresh layers treated as not fresh by the provide algorithm, which is now incorrect. It is equivalent to the test of #8870.

@jdegoes
Copy link
Member
jdegoes commented Jun 23, 2024

@pablf Thank you! Good to merge when conflicts are fixed.

@jdegoes
Copy link
Member
jdegoes commented Jul 30, 2024

@pablf Please re-open if you get a chance to fix conflicts! 🙏

@jdegoes
Copy link
Member
jdegoes commented Aug 5, 2024

@pablf I will close this for now, just to clean up the PR pool, but please re-open when you get a chance to fix the conflicts in Graph.scala. Thank you!

@jdegoes jdegoes closed this Aug 5, 2024
@pablf pablf mentioned this pull request Aug 5, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jdegoes jdegoes jdegoes approved these changes

@ghostdogpr ghostdogpr ghostdogpr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pablf @ghostdogpr @jdegoes
0