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

Compilation error with context bounds #1259

Open
NTPape opened this issue Feb 5, 2022 · 8 comments · May be fixed by #1267
Open

Compilation error with context bounds #1259

NTPape opened this issue Feb 5, 2022 · 8 comments · May be fixed by #1267
Labels

Comments

@NTPape
Copy link
Contributor

NTPape commented Feb 5, 2022

Hello there!

Compilation of the following code will fail

import monocle.Lens
import monocle.macros.GenLens

trait Foo[T]
trait Bar[T]

case class A[T: Foo](s: A.S[T]) {
  private val lens: Lens[A.S[T], Bar[T]] = GenLens[A.S[T]](_.bar)
}

object A {
  case class S[T: Foo](bar: Bar[T])
}

in Scala 3 with the following error

[error] 8 |  private val lens: Lens[A.S[T], Bar[T]] = GenLens[A.S[T]](_.bar)
[error]   |                                           ^^^^^^^^^^^^^^^^^^^^^^
[error]   |Exception occurred while executing macro expansion.
[error]   |java.lang.Exception: Expected an expression. This is a partially applied Term. Try eta-expanding the term first.

and in Scala 2.13 with

[error] could not find implicit value for evidence parameter of type Foo[T]
[error]   private val lens: Lens[A.S[T], Bar[T]] = GenLens[A.S[T]](_.bar)
[error]                                                           ^

Interestingly, the last version that appears to work is 1.5.1-cats on Scala 2.11 (but not on Scala 2.12.)

@julien-truffaut
Copy link
Member

Thanks for the bug report. I am not sure when we will have time to investigate.

@NTPape
Copy link
Contributor Author

NTPape commented Feb 19, 2022

I had a look into the issue in Scala 2.13 and as far as I can see the issue is located in the lens's modifyF method because it has a context bound for cats.Functor. Due to macro magic, the Functor evidence is named like it were the first evidence in the source file, i.e. like the evidence for Foo of the enclosing class A, so the evidence for Foo is shadowed and hence unavailable in modifyF's function body where it is needed for copy.

That means one workaround for the issue in Scala 2.13 is putting another class with a context bound above the case class to rename the evidences below it, like this:

import monocle.Lens
import monocle.macros.GenLens

trait Foo[T]
trait Bar[T]

private class IncrementAllEvidencesBelow[T: Foo]()

case class A[T: Foo](s: A.S[T]) {
  private val lens: Lens[A.S[T], Bar[T]] = GenLens[A.S[T]](_.bar)
}

object A {
  case class S[T: Foo](bar: Bar[T])
}

or by explicitly naming the implicit parameter:

import monocle.Lens
import monocle.macros.GenLens

trait Foo[T]
trait Bar[T]

case class A[T](s: A.S[T])(implicit foo: Foo[T]) {
  private val lens: Lens[A.S[T], Bar[T]] = GenLens[A.S[T]](_.bar)
}

object A {
  case class S[T: Foo](bar: Bar[T])
}

@julien-truffaut
Copy link
Member

Thanks for investigating. Do you think it will solve the problem if we change https://github.com/optics-dev/Monocle/blob/master/macro/src/main/scala-2.x/monocle/macros/internal/Macro.scala#L100

from:

override def modifyF[$F[_]: _root_.cats.Functor](f: $aTpe => $F[$bTpe])(s: $sTpe): $F[$tTpe] =
          _root_.cats.Functor[$F].map(f(s.$fieldMethod))(a => s.copy($field = a))

to:

override def modifyF[$F[_[](f: $aTpe => $F[$bTpe])(s: $sTpe)(implicit evFunctor: _root_.cats.Functor[F]): $F[$tTpe] =
          _root_.cats.Functor[$F].map(f(s.$fieldMethod))(a => s.copy($field = a))

NTPape added a commit to NTPape/Monocle that referenced this issue Feb 20, 2022
NTPape added a commit to NTPape/Monocle that referenced this issue Feb 20, 2022
Will (mostly) solve optics-dev#1259 for Scala 2.13.
@NTPape
Copy link
Contributor Author

NTPape commented Feb 20, 2022

I suggest using a freshName too, to make a shadowing conflict less likely. I created a (naive) PR accordingly.

julien-truffaut pushed a commit that referenced this issue Feb 21, 2022
Will (mostly) solve #1259 for Scala 2.13.
@julien-truffaut
Copy link
Member

Good call, I completely forgot about freshName. Do you want me to cut a release with this fix?

@NTPape
Copy link
Contributor Author

NTPape commented Feb 22, 2022

Thanks for the quick merge! A release for this fix is not needed from my end.
I'm also trying to look into the issue in Scala 3.

NTPape added a commit to NTPape/Monocle that referenced this issue Feb 25, 2022
NTPape added a commit to NTPape/Monocle that referenced this issue Feb 25, 2022
@kenbot
Copy link
Collaborator

kenbot commented Mar 3, 2022

This use case is a bit unusual @NTPape - usually typeclass bounds are declared at the methods where they are required, rather that at the class constructor level.

YAGNI is one reason, there's no physical reason why the A.S object construction needs to be held up for the lack of a Foo[T].

Another reason is that creating an instance of A.S[T: Foo](bar: Bar[T]) captures the Foo[T] instance and stores it as a field in the object. The created A.S object can wander far away from the initial scope into other scopes where A.Ss are being created with entirely different Foo[T] instances, which is hard to reason about and could cause subtle bugs when they interact.

Are you sure really need us to support this use case?

@NTPape
Copy link
Contributor Author

NTPape commented Mar 15, 2022

Oh, I didn't notice that the discussion also moved back here from the PR.

The minimal example was somewhat artificially constructed to produce the implicit parameter name shadowing of the context bounds that was observed in Scala 2.13. It just so happened that it also fails in Scala 3 but for totally different reasons as we know now.

For Scala 3 a minimal example would just be a lens to a case field of a case class with any (positive) number of implicit parameters (including context bounds.)

These will both fail to compile:

import monocle.macros.GenLens

case class Test1(i: Int)(implicit j: Int)
GenLens[Test1](_.i)

trait Foo[T]

case class Test2[T: Foo](t: T)
GenLens[Test2[Int]](_.t)

Is that really too unusual?

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

Successfully merging a pull request may close this issue.

3 participants