optics-dev / Monocly

Optics experimentation for Dotty
MIT License
32 stars 5 forks source link

Get macros to work with generic classes #11

Closed yilinwei closed 3 years ago

yilinwei commented 4 years ago

copy seems to be defined slightly differently; I'll need to see what actually gets generated for case classes.

yilinwei commented 4 years ago

Raised an issue here

doofin commented 4 years ago

Hi,does the macro syntax like x.lens().modify() currently works in dotty ? would like to help.

yilinwei commented 4 years ago

TLDR; No it doesn't currently work inside dotty.

I don't really use the postfix operator syntax very much (typically I reuse the lenses); but feel free to give it a go!

There are some examples of how the feature should work — the underlying macro already exists and so it should be relatively straightforward. It would probably need to be an inline macro extension method due to how macro's work. I don't think there's an example of that in the current codebase.

I think we were also waiting on dotty support in the various IDE's back in May to see how it would be supported; but that's not a good reason not to see whether it works.

kenbot commented 3 years ago

I failed to do this in Goggles, but that was because of limitations in the string format I was stuck in. It should be fairly straightforward (by macro standards) to do it here, because the copy method is naturally polymorphic, as described in the Goggles issue:

https://github.com/kenbot/goggles/issues/17

(Assuming the copy behaviour is no less capable in Scala 3)

kenbot commented 3 years ago

OK I think this is thoroughly feasible. Here's what I understand so far, assuming #15 gets merged:

Example use cases:

case class Box[A](a: A) // def copy[Z](a: Z): Box[Z] case class Two[A,B](a: A, b: B) // def copy[Y,Z](a: Y, b: Z): Two[Y,Z] class class Higher[F[_], A](fa: F[A]) // def copy[G[_], Z](fa: G[Z]): Higher[G, Z] class class Union[A,B](ab: A | B) // def copy[Y,Z](ab: Y | Z): Union[Y,Z]

We could try to confuse polymorphic Focus further with type bounds and variance annotations, etc.

kenbot commented 3 years ago

Status report:

Heh, this is not actually straightforward at all.

Firstly, even the underlying lenses don't really give us what we need. For instance, for a class case class Box[A](a: A), we would want to be able to do something like this:

Focus[Box[String]](_.a).replace(5) // Box[String] => Box[Int]

Where our choice of an integer in the replace method determines the result type of Box[Int] somehow. This is how case class copy methods work, but it's not how optics work. The Int parameter needs to be baked into the optic type signature at the time the optic is constructed, in advance of any uses; the replace method does not really take a type parameter.

Currently there's no real theory for how Focus would do this, tbh. There's no information in the code expression inside Focus(...) that could make us pick anything other than a monomorphic PLens[S,S,A,A]. Even if we could, it would be incredibly irksome for users to have to create a new optic for every kind of type they might want to swap into the Box. So actually allowing polymorphic operations on the optic we create will need more thought.

Secondly, let's say don't want to change the types around, we just want to get/set Strings with a Box[String] type. This should be technically possible, but it's not easy.

As per the previous comment, the generated copy method in case classes has a type argument for each type argument the case class has. When we call copy, we are using this TASTy code: Select.overloaded(from, "copy", Nil, NamedArg(field, to) :: Nil) This calls the copy method on the "from" type, with a Nil list of type arguments, and setting the argument with the given name to the "to" value.

If we want this to work with classes like Box[String], then we need to provide a list of TypeReprs in place of Nil. However, I haven't been able to make this work yet.

So that's where I'm up to.

yilinwei commented 3 years ago

Currently there's no real theory for how Focus would do this, tbh. There's no information in the code expression inside Focus(...)

Regarding this particular point - if Focus is inlined I seem to remember you can do a trick (using the owner on the context) to see the rest of the context (i.e. outside of Focus(...)); which theoretically would mean we could inspect the next expression; and if it's replacement is not the same type on a generic parameter, infer the intent that it's meant to be a polymorphic lens.

On the one hand this is definitely MAGIC™ :sparkles: . On the other, it's not too different to type-checking and Scala has never had the property that if you split your code onto 2 lines, you get the same compilation output.

kenbot commented 3 years ago

Haha oh my. That's amazing and horrifying. If the mechanism works, it looks like it might solve the problem, with a lot of work. I'll suggest we keep this in the tank as a last resort though, on the basis that:

kenbot commented 3 years ago

I will split off the existential "Polymorphic lenses don't work properly with case classes" thing as a separate ticket (#28), as there are some deep things we need to think through there, we can't hack around this.

I will leave this ticket #11 open, but constrained to solving monomorphic lens generation for case classes with type parameters, which Focus does not currently support.

kenbot commented 3 years ago

Ok I've solved this, but the implementation sucks, so I'll clean it up a bit over the weekend before sending a PR.

You might find this interesting @yilinwei - there were two things that I had to change from the code I copied from your version.

First, we are currently generating the setter with this line:

Select.overloaded(from, "copy", Nil, NamedArg(field, to) :: Nil)

The Nil argument there is type arguments. Unfortunately we need to supply all of them by hand. It is properly Nil for monomorphic case classes of course, but for polymorphic ones we need to fish them out from somewhere. This is the secret sauce to fish them out of the "from" TypeRepr:

fromType match {
  case AppliedType(_, argTypeReprs) => argTypeReprs 
  case _ => Nil
}

Secondly, we are using something like this to get the field type: fromType.classSymbol.get.memberField(x).tree. Using the classSymbol works fine for concrete field types like String. However, when the field type is a class-level type parameter (eg, A), it only ever wants to treat it as this disembodied abstract type A and doesn't want to unify it with the actual type parameters we are using, like say scala.Predef.String. So I'm doing some crazy thing to manually map the A back to the real type we got from the pattern match earlier.

Anyway, it's a piece of crap, I'll tidy it up, but see what you reckon when I get the PR up, you can probably do better.

kenbot commented 3 years ago

I have cleaned up the code and I am pretty happy with it now.

There are some things that still don't work; case class HigherBox[F[_], A](fa: F[A]) doesn't work, because let's say for Focus[HigherBox[Option, Int]], I can't just swap out A for Int, I need to dig down into Option[A] and ferret out all the type variables. Currently this doesn't work, because while I can pattern match on an AppliedType, there is no dual constructor; I can't build it back up again. I have started asking some questions in EPFL/Dotty Gitter, but it might take a while to figure it out. I suspect it's an oversight that might need to wait for the next release of Dotty.

Guillaume Martres has confirmed that there is no way to infer the types we give the copy method, so we appear to be going about this the right way.

Given all this, I will submit a PR containing monomorphic getting and setting for polymorphic case classes, except where the type variables are hidden in type constructors or type refinements. I think I will call that done for this issue, and I'll add another issue to track the higher-kinded stuff we're missing that we need help with.

After the PR, I think we can charge ahead with the full implementation of polymorphic optic support that we are tracking in #28; it shouldn't be too hard to graft the solution for the higher-kinded stuff back over the top of that, and I'd rather keep up the momentum.