search

optics-dev / Monocly

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

Upgrade to Scala 3.0.0-M3 + major refactor of GenLens/Focus #15

Closed kenbot closed 3 years ago

kenbot commented 3 years ago

Bonne année and 新年快乐 @yilinwei and @julien-truffaut!

I had intended to submit just a bare upgrade to M3 before doing anything else, but unfortunately a straight translation of the macros proved difficult - the TASTys didn't match up and poring through multi-page pretty-prints of the AST proved less appetising than just doing the refactor.

Here's what I've done:

1. Compiler-style separation of parsing from code-gen

Focus now has separate parsing and code-gen phases, which I believe simplifies the code and should make it easier to extend. It will also allow us to evolve by splitting the different parts ("domain", "parsing", "codegen", "execution") into different files or packages when the time comes.

2. Explicit model

There are now explicit model classes which define the DSL, which are the output of the parsing phase.

3. Declarative errors

All possible error conditions are now defined as data types. Error messages are only defined at the end in a separate step. This allows for possible future work, where we might explicitly test all possible error conditions in the macro, as well as the compiler-approved happy paths.

4. Eliminate reuse of GenLens by Focus

Generally speaking, while a "big thing" can of course be built out of "little things", the "little things" should never know about or carry water for the "big thing". I believe that the reuse of GenLens by Focus was the source of most of the complexity in GenLens, and that the reuse did not offer much value, so I took it out. You can see that the resulting repetition is quite small in the end.

(To be honest, I am not sure why both macros exist, considering that GenLens' behaviour is entirely subsumed by the Focus macro; I would just rename Focus to GenLens and just have that)

5. Temporary retraction of ? and idx

Of course these are still required features, but I wanted to hold off adding them back because:

In summary, I think we will find this to be a very beneficial reworking of Yilin's great macro prototype, which should allow us to more easily extend, modify and maintain Focus & GenLens. Looking forward to your thoughts.

julien-truffaut commented 3 years ago

Amazing work @kenbot. I don't know much about macro but the code seems to be much more readable, and as you said, easier to extend. I guess there might be a tiny cost at compile time but the code is going to get more complex as we add more features, so it is better to optimise for readability and extensibility at the moment.

Pinging @asjad02 who is working on adding Focus to the Monocle main repo.

yilinwei commented 3 years ago

Thank you @kenbot for your hard work; I will take a proper look this weekend but it looks good!

Some initial comments from your points:

  1. Agree we should fix #11 first before adding new features
  2. GenLens can be discarded; originally the design I was hoping to use was quite different due to the TASTY API and differences between Scala 2/3 macros.
  3. I'll spend some time this weekend knocking up the encoding I proposed in #14 to see how it fits.

Again; thank you for the hard work - the moving APIs, lack of documentation/edge-cases in TASTY and reliance on informal behaviour are not easy to deal with. I am massively appreciative of the effort that you've put in to uplift the old code.

kenbot commented 3 years ago

Thanks for the reviews guys! I'm going to merge it now, since everything else we could hope to do with the codebase depends on it, and I think you are both supportive. If you have any more suggestions, I can incorporate them in a separate PR.