Fix compilation of lenses with context bounds in Scala 3

[NTPape]

Implement support of focusing into case classes with implicit parameters. Introduce more specific error cases when trying to focus onto non-case fields, or when trying to focus on a case class with multiple (non-implicit) parameter sets. Fix compilation exception when trying to focus onto a case field which has a method with the same name in the same case class.

Fixes #1259

[NTPape]

I did exactly what the error message said: try to eta expand the Term if it's not an Expr yet. This should have no influence on working code because for all code where it already compiles, i.e. asExpr succeeds, isExpr is already true and hence eta expanding would be skipped. Truth be told, this was my first time working with Scala 3 metaprogramming, so the solution might not be optimal.

[NTPape]

I had to move the S case class out of the companion in the test due to a bug in the dotty compiler having trouble finding the (correct) reference to the companionModule of locally defined classes. I created an issue for it https://github.com/lampepfl/dotty/issues/14574

[julien-truffaut]

Thanks @NTPape ! @kenbot would you mind to review this PR when you have time?

[kenbot]

👀

On Mon, 28 Feb 2022, 11:20 pm Julien Truffaut, @.***> wrote:

Thanks @NTPape https://github.com/NTPape ! @kenbot https://github.com/kenbot would you mind to review this PR when you have time?

— Reply to this email directly, view it on GitHub https://github.com/optics-dev/Monocle/pull/1267#issuecomment-1054201047, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACNIYZE2MG4LGQ4SXXCYJLU5NSB5ANCNFSM5PLQK4HA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

[kenbot]

Great work @NTPape, thanks!

I'll need more time to understand the nuts & bolts, I'll write up some more tonight. Tbh I assume the internal bits are fine, but it interacts with the architecture in an interesting way, which I'll explain here.

First up, sorry I haven't documented the Focus architecture at all, so this isn't terribly obvious and makes it harder to contribute.

Each feature is split into a "Parser", which interprets the user expression into list of FocusAction objects, and a "Generator", which turns the list of FocusActions into Monocle optics code.

The Parser side never directly fails the compiler; it returns FocusResult objects, which might contain FocusErrors, containing relevant information about the failure. The user text is considered a user interface matter, so lives in ErrorHandling.scala instead of being scattered throughout the code.

The Generator side has no concept of an error; all the decisions & validation & vetting is supposed to happen on the Parser side, and the FocusAction should be completely bulletproof by the time the Generator gets it. This makes the code cleaner and easier to maintain.

Now your code has compiler errors directly thrown by the Generator side; if all the implicit searching can be done on the Parser side, then that's easy, we should move it there, and package failure data in a FocusError, and move the text to ErrorHandling. I'm pretty sure this will be the case, I can't imagine why it couldn't be done in the Parser.

However, if for whatever reason it's impossible or unwise to do the checks in the Parser and it has to be done by the Generator, then the current architecture is broken, and we need to rethink how the whole Monocle Focus code is structured. I'm pretty sure this won't be the case though.

Good luck - feel free to have a crack at the refactor, but if you like I'm happy to do it when I have time if it feels a bit daunting, especially given the dearth of documentation.

[NTPape]

Thanks for the detailed explanation, @kenbot!

I gave it a quick go and the issue I'm running into is lifting the FocusResult of a splice into the surrounding expression. I.e. etaExpandIfNecessary now returns a FocusResult[Term] and now I'm trying to resolve the setter in the parser like so:

(fromType.asType, toType.asType) match {
  case ('[f], '[t]) =>
    '{
      (to: t) => ${
        etaExpandIfNecessary(
          Select.overloaded(companion, "apply", fromTypeArgs, List('{ to }.asTerm))
        ).asExprOf[f]
      }
    }.asTerm)
}

Is there a nice way to lift the FocusResult into the surrounding expression, or is there another way how to get a parameter for the overload resolution? (I haven't tried it yet but I guess one hacky solution would be to throw an exception, catch it, and convert it back into a FocusResult.)

[kenbot]

@NTPape Check my comment on #1259 - if you don't end up needing the use case maybe all we do is intercept the context-bounds-constructor-case-class and make a nicer error message.

If you're still keen, here's what it looks like:

The feature we're dealing with is SelectOnlyField; this handles plain method selection where the case class only has one field; this is so that instead of a Lens, it can create the strongest possible optic, an Iso, by constructing a reverseGet method by reconstructing the case class from the companion object, calling apply with the single argument.

Have a look at the SelectOnlyFieldParser getFieldAction code: https://github.com/optics-dev/Monocle/blob/21a37a05ccc7cdba541448c01b458190364dfaf0/core/shared/src/main/scala-3.x/monocle/internal/focus/features/selectonlyfield/SelectOnlyFieldParser.scala#L25

• It collects a bunch of stuff hidden in the FocusResult monad, including the companion object
• It puts it back together again into a FocusAction.SelectOnlyField object to funnel all the booty to the Generator

In your case, we need to funnel a bit more stuff through, so that reverseGet can put the implicit value into a second apply parameter list.

• The general case would potentially involve many context bound parameters with arbitrary kinds, but let's keep it simple and just solve for 1 argument. We can leave an open issue for the fully general solution.
• Firstly, you'll need to add fields to the FocusAction.SelectOnlyField; it might be an Option[Term] for the implicit value that you need to fish out with the Implicit.search.
• In the Parser file, you'll need to create something like private def expectsContextBounds(companion: Term): Boolean and private def getContextValue(tpe: TypeRepr): FocusResult[Option[Term]] or something.
• You might need to thread through more stuff in the FocusAction.SelectOnlyField, depending on what the generator needs. eg a TypeRepr for the implicit parameter type
• The Generator side should just pattern match on the option, with None doing the current thing, and Some(term) doing new code that slots the value into the second parameter list of apply.
• See if you can squish all the error cases into the Parser side, possible adding new FocusError cases, and ErrorHandling code to generate the messages. The FocusResults should all get chewed up by the for comprehension in getFieldAction.

Lmk if you get stuck

[NTPape]

Sorry, my last comment was a bit too concise and without having had uploaded the code yet, it was unnecessarily difficult to understand my question and to follow the current state of my attempt. I have now uploaded a working version.

As far as I understand, it is necessary to move the setter creation completely into the parser then, because only in the expression (to: To) => fromCompanion.apply(to) the type parameters of from become initialized in a way which allows for implicit search for values which needs these type parameters. (One cannot search for Foo[T] without knowing what T is, but in the expression above it becomes A.this.T in the spec for example, which can be found.)

I also used the hacky FocusResult lifting via throwing exceptions for now because I'm not aware of a nicer way.

Is this going in the right direction or am I on the wrong track here? Thanks for your help!

[kenbot]

Yep, we're not there yet, but much closer & heading in the right direction! I'll take a closer look tonight.

On Fri, 4 Mar 2022 at 09:10, NTPape @.***> wrote:

Sorry, my last comment was a bit too concise and without having had uploaded the code yet, it was unnecessarily difficult to understand my question and to follow the current state of my attempt. I have now uploaded a working version.

As far as I understand, it is necessary to move the setter creation completely into the parser then, because only in the expression (to: To) => fromCompanion.apply(to) the type parameters of from become initialized in a way which allows for implicit search for values which needs these type parameters. (One cannot search for Foo[T] without knowing what T is, but in the expression above it becomes A.this.T in the spec for example, which can be found.)

I also used the hacky FocusResult lifting via throwing exceptions for now because I'm not aware of a nicer way.

Is this going in the right direction or am I on the wrong track here? Thanks for your help!

— Reply to this email directly, view it on GitHub https://github.com/optics-dev/Monocle/pull/1267#issuecomment-1058544853, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACNIY3GQGJ3FVJTHVLVX7LU6E2D3ANCNFSM5PLQK4HA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

[kenbot]

One thing that occurred to me is that it might be better to make this use case an entirely new feature, say SelectOnlyFieldWithContext or something like that, in the same way that SelectOnlyField is a special case of SelectField. That way we can leave all the working code unmolested and put all the complexity for this edge case in it's own bucket.

What do you think?

Btw, there should be no need to modify the SelectField feature - it doesn't need to create a reverseGet, so it doesn't need the companion object and doesn't need to care in the slightest about context bounds.

On Fri, 4 Mar 2022 at 09:13, Ken Scambler @.***> wrote:

Yep, we're not there yet, but much closer & heading in the right direction! I'll take a closer look tonight.

On Fri, 4 Mar 2022 at 09:10, NTPape @.***> wrote:

Sorry, my last comment was a bit too concise and without having had uploaded the code yet, it was unnecessarily difficult to understand my question and to follow the current state of my attempt. I have now uploaded a working version.

As far as I understand, it is necessary to move the setter creation completely into the parser then, because only in the expression (to: To) => fromCompanion.apply(to) the type parameters of from become initialized in a way which allows for implicit search for values which needs these type parameters. (One cannot search for Foo[T] without knowing what T is, but in the expression above it becomes A.this.T in the spec for example, which can be found.)

I also used the hacky FocusResult lifting via throwing exceptions for now because I'm not aware of a nicer way.

Is this going in the right direction or am I on the wrong track here? Thanks for your help!

— Reply to this email directly, view it on GitHub https://github.com/optics-dev/Monocle/pull/1267#issuecomment-1058544853, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACNIY3GQGJ3FVJTHVLVX7LU6E2D3ANCNFSM5PLQK4HA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you were mentioned.Message ID: @.***>

[NTPape]

Yes, we can split it up into Select(Only)FieldWithImplicits and without implicits to not touch the current implementation, good point! I will do that tomorrow.

We need it for both SelectField and SelectOnlyField because both companion.apply as well as the copy method have the same type parameters as the case class including context bounds and including additional parameter sets for the case class which might contain implicit value parameters. If you call copy from within the case class itself, you will mostly not notice this because the implicit values from the case class will automatically be used (unless the copy changes type parameters and thus potentially implicits too.) But in the lens we call it from the outside, so we still need all of them explicitly.

[NTPape]

I did the split now and merged the Select(Only)Field parsers and generators because they are so similar.

Can you have a look again?

[kenbot]

Hey thanks for all this @NTPape - sorry I've been a bit busy, I'll try to get some time to take a look soon.