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.

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

Fix compilation of lenses with context bounds in Scala 3 #1267

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fix compilation of lenses with context bounds in Scala 3 #1267

wants to merge 6 commits into from

Conversation

NTPape
Copy link
Contributor

@NTPape NTPape commented Feb 25, 2022 •
edited
Loading

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
Copy link
Contributor Author

NTPape commented Feb 25, 2022 •
edited
Loading

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
Copy link
Contributor Author

NTPape commented Feb 26, 2022 •
edited
Loading

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 scala/scala3#14574

@julien-truffaut
Copy link
Member

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

@kenbot
Copy link
Collaborator

kenbot commented Feb 28, 2022 via email

@kenbot
Copy link
Collaborator

kenbot commented Mar 1, 2022

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
Copy link
Contributor Author

NTPape commented Mar 2, 2022

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
Copy link
Collaborator

kenbot commented Mar 3, 2022 •
edited
Loading

@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:

Monocle/core/shared/src/main/scala-3.x/monocle/internal/focus/features/selectonlyfield/SelectOnlyFieldParser.scala

Line 25 in 21a37a0

private def getFieldAction(fromType: TypeRepr, fieldName: String): FocusResult[FocusAction] =

  • 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 NTPape force-pushed the master branch 2 times, most recently from b911eca to 5d0d6cc Compare March 3, 2022 20:24
@NTPape
Copy link
Contributor Author

NTPape commented Mar 3, 2022

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
Copy link
Collaborator

kenbot commented Mar 3, 2022 via email

@kenbot
Copy link
Collaborator

kenbot commented Mar 3, 2022 via email •
edited
Loading

@NTPape
Copy link
Contributor Author

NTPape commented Mar 3, 2022

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
Copy link
Contributor Author

NTPape commented Mar 9, 2022

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?

@NTPape NTPape force-pushed the master branch 2 times, most recently from c3eff55 to 4ba0472 Compare March 11, 2022 20:34
@NTPape
Introduce explicit error for non-implicit non-case parameters
293e6c5 
Fix compilation error when case class has a method named like a case field
@kenbot
Copy link
Collaborator

kenbot commented Mar 17, 2022 •
edited
Loading

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julien-truffaut julien-truffaut Awaiting requested review from julien-truffaut julien-truffaut is a code owner

@cquiroz cquiroz Awaiting requested review from cquiroz cquiroz is a code owner

@xuwei-k xuwei-k Awaiting requested review from xuwei-k xuwei-k is a code owner

@kenbot kenbot Awaiting requested review from kenbot

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

Successfully merging this pull request may close these issues.

Compilation error with context bounds
3 participants
@NTPape @julien-truffaut @kenbot