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

Support for zio layers #49

Merged
merged 1 commit into from
Mar 11, 2021
Merged

Support for zio layers #49

merged 1 commit into from
Mar 11, 2021

Conversation

danilbykov
Copy link
Contributor

Using ZLayers from zio leads to unreadable compilation messages when environment misses some layer. This plugin can greatly improve error messages. RefinedFormatter already does most of work but small changes are needed.

RefinedFormatter collects traits only from top level while zlayers can have nested RefinedTypes. Here is example from my test

RefinedType(List(
  RefinedType(List(
    TypeRef(ThisType(zio), zio.Has, List(
      RefinedType(List(TypeRef(ThisType(scala), TypeName("AnyRef"), List()), TypeRef(ThisType(layers), layers.Service1, List())), Scope())
    )),
    TypeRef(ThisType(zio), zio.Has, List(
      RefinedType(List(TypeRef(ThisType(scala), TypeName("AnyRef"), List()), TypeRef(ThisType(layers), layers.Service2, List())), Scope())
    ))
  ), Scope()),
  TypeRef(ThisType(zio), zio.Has, List(
    RefinedType(List(TypeRef(ThisType(scala), TypeName("AnyRef"), List()), TypeRef(ThisType(layers), layers.Service3, List())), Scope())
  ))
), Scope())

It has several levels of RefinedTypes and services are on bottom level. This is why I added class DeepRefined.

There is another issue with this syntax tree which breaks error reporting. Compiler defines type of services as zio.Has[AnyRef with layers.Service1] which is equivalent to zio.Has[layers.Service1]. So I was needed to handle this case inside DeepRefined and remove AnyRef inside zio.Has.

What do you think about these changes? Can I add some new option for plugin which allows to replace default Refined by DeepRefined?

@tek
Copy link
Owner

tek commented Feb 21, 2021

looks good! what would be the problem with making this the default?

@tribbloid
Copy link
Collaborator

Is AnyRef with layers.Service1 the actual type signature in the source code?

I probably encountered a similar case before, if you have a small test case I should be able to help

@danilbykov
Copy link
Contributor Author

danilbykov commented Feb 22, 2021

what would be the problem with making this the default?

I'm afraid that my changes may lead to not equivalent types in common case. It must work for zlayers from zio but I'm not sure about common case. I don't know about other cases when compiler can generate such nested RefinedTypes. But if you are OK we can make this the default. :)

Is AnyRef with layers.Service1 the actual type signature in the source code?
I probably encountered a similar case before, if you have a small test case I should be able to help

My test already includes this example. For example, if I specify type explicitly

  val service1: ULayer[Has[Service1]] = ZLayer.succeed(new Service1 {})
  val service2: ULayer[Has[Service2]] = ZLayer.succeed(new Service2 {})
  val service3: URLayer[Has[Service1], Has[Service3]] = ZLayer.fromService { (_: Service1) => new Service3 {} }
  val service4: ULayer[Has[Service4]] = ZLayer.succeed(new Service4 {})

then compiler generates following tree for service1 ++ service2 >+> service3

RefinedType(List(
  RefinedType(List(
    TypeRef(ThisType(zio), zio.Has, List(TypeRef(ThisType(ru.tinkoff.mvno.callerid.layers), ru.tinkoff.mvno.callerid.layers.Service1, List()))),
    TypeRef(ThisType(zio), zio.Has, List(TypeRef(ThisType(ru.tinkoff.mvno.callerid.layers), ru.tinkoff.mvno.callerid.layers.Service2, List())))
  ), Scope()),
  TypeRef(ThisType(zio), zio.Has, List(TypeRef(ThisType(ru.tinkoff.mvno.callerid.layers), ru.tinkoff.mvno.callerid.layers.Service3, List())))
), Scope())

Comparing this tree with tree from my initial comment we can see that depth of RefinedTypes is two and services don't have with AnyRef.

But it requires explicit types which we may try to avoid.

@tek
Copy link
Owner

tek commented Feb 22, 2021

well since you're checking for the presence of the zio type before the analysis I don't see what the harm is

@danilbykov
Copy link
Contributor Author

Hmm, I was going to remove DeepRefined and move this functionality into Refined. But I noted that simple Refined is used inside RefinedFormatter.apply. Should I use DeepRefined in this method too?

@tek
Copy link
Owner

tek commented Feb 22, 2021

if the tests are green, ship it!

@tek
Copy link
Owner

tek commented Mar 9, 2021

@danilbykov Am I right to assume that you wanted to change that thing before merging?

@danilbykov
Copy link
Contributor Author

Sorry, I thought that it means I should not change current version.

if the tests are green, ship it!

I removed DeepRefined and put all changes in Refined.

@tek
Copy link
Owner

tek commented Mar 11, 2021

Oh, I can see how that might have been ambiguous. sorry!

@tek tek merged commit 2f7e4f0 into tek:master Mar 11, 2021
@tribbloid tribbloid mentioned this pull request May 6, 2022
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.

None yet

3 participants