Implement Reflection API directly #9818

nicolasstucki · 2020-09-17T17:10:36Z

Revert to the original design with abstract modules that are implemented directly.
We had to move the implementations to the compiler interface with one of the iterations of extension methods.
The current extension methods are again compatible with this design.

CompilerInterface is kept for the internal methods that are only available in the stdlib.

Now Reflection contains the full user-facing API
CompilerInterface only contains internal methods used in the stdlib
tasty.refelct.Types was merged into Reflection
QuoteContextImpl implements every member directly
All members of tasty are lazily initialized using object
Fix defn.NullClass
Fix typo in Flags.Accessor
Rename Scala2X to Scala2x to follow dotty conventions

nicolasstucki · 2020-09-23T09:36:54Z

There are further improvements possible on the definitions and implementation of the CompilerInterface. These are left for a future PR.

Revert to the original design with abstract modules that are implemented directly. We had to move the implementations to the compiler interface with one of the iterations of extension methods. The current extension methods are again compatible with this design. CompilerInterface is kept for the internal methods that are only available in the stdlib. * Now Reflection contains the full user-facing API * CompilerInterface only contains internal methods used in the stdlib * tasty.refelct.Types was merged into Reflection * QuoteContextImpl implements every member directly * All members of tasty are lazily initialized using object * Fix defn.NullClass * Fix typo in Flags.Accessor * Rename Scala2X to Scala2x to follow dotty conventions

nicolasstucki · 2020-09-24T08:11:25Z

This change is great. With this change, I am managing to quickly take care of the TODOs which were a pain to fix before. See #9857, #9858, #9861, #9862, #9863, #9864, ...

b-studios

I am generally in favor of the architectural changes. However, I have a few questions and remarks:

I am not sure 100% sure whether it is a good idea to inline Types into Reflection. Maybe it is, since this is a very fine grained interface, only suitable for Reflection anyways.
Why is val XXX: XXXModule defined as a val? Defining it as a val has the disadvantage that implementors cannot easily stub aspects they do not care about (i.e., def XXX = ???). Is the reason that you want to use it as a self-type? If yes: I couldn't immediately find a reason why the self-type is necessary. Same reasoning applies to XXXMethods and XXXTypeTest.

compiler/src/dotty/tools/dotc/quoted/QuoteContextImpl.scala

b-studios · 2020-09-24T07:32:50Z

compiler/src/dotty/tools/dotc/quoted/QuoteContextImpl.scala

+            case _ => false
+      end extension
+
+      extension [T](tree: Tree)


This has nothing to do with this PR, but it is sad that you are forced to open a second extension block, here.

compiler/src/dotty/tools/dotc/transform/TailRec.scala

library/src/scala/tasty/Reflection.scala

b-studios · 2020-09-24T07:49:56Z

library/src/scala/tasty/Reflection.scala

-      Some((cdef.name, cdef.constructor, cdef.parents, cdef.derived, cdef.self, cdef.body))
-  end ClassDef
+    def copy(original: Tree)(name: String, constr: DefDef, parents: List[Tree /* Term | TypeTree */], derived: List[TypeTree], selfOpt: Option[ValDef], body: List[Statement]): ClassDef
+    def unapply(cdef: ClassDef): Option[(String, DefDef, List[Tree /* Term | TypeTree */], List[TypeTree], Option[ValDef], List[Statement])]


As discussed already, the apply is still missing here.

There is a TODO to remind us to do it just above.

b-studios · 2020-09-24T07:51:30Z

library/src/scala/tasty/Reflection.scala


-    /** Class name of the current CompilationUnit */
-    def compilationUnitClassname: String = reflectSelf.Source_compilationUnitClassname
+  trait TreeModule { this: Tree.type => }


Do you plan to add methods here?

Possibly. If we do, it is better to have it already defined to avoid breaking user code if we add it later. This happened in the past with the Position module.

liufengyun · 2020-09-24T09:03:23Z

It would be good to spell out the design trade-offs here. I try to reproduce the main points of an offline discussion with @nicolasstucki below.

The reflection library has two sets of APIs:

contract APIs with the compiler to get the primitive capabilities for reflection
friendly APIs to meta-programmers

Conceptually, the two contracts have different stakeholders and have different requirements. For compiler contracts, the APIs need to be minimal and powerful. Coarse-grained APIs are ideal, friendliness and type safety are not needed. For meta-programming APIs, friendliness is a concern, and some-level of type safety can prevent meta-programmers from accidental mistakes. Fine-grained APIs seem to be a good fit for the latter use case.

Seen from that perspective, there are two reasonable design points:

Use the same unified interface for the two contracts
Use two different interfaces corresponding to the two contracts

Both the two design points are arguably local optima, but design points between them are not. The first design is simpler in implementation efforts, as there is only one interface. It has the disadvantage of complicating/polluting the contract with the compiler. If there is going to be another compiler to support the Reflection API, it will be a nightmare.

The second design allows the two interfaces to evolve differently, which might help with maintainability, in particular, when there are multiple compilers to support the Reflect library. But it has the disadvantage of creating another abstraction layer, which might cause a maintenance problem.

However, if the reflection library always evolves together with the Dotty compiler and only with the Dotty compiler, the 2nd approach seems to be an over-design. That seems to be the assumption of this PR, which sounds reasonable.

There is a minor issue with the current PR. While it approaches the 2nd design point, it still keeps a tiny compiler interface. Is it possible to eliminate CompilerInterface and protect its APIs in Reflection with private[scala]?

b-studios · 2020-09-24T09:13:39Z

There is a minor issue with the current PR. While it approaches the 2nd design point, it still keeps a tiny compiler interface. Is it possible to eliminate CompilerInterface and protect its APIs in Reflection with private[scala]?

@liufengyun I prefer separating the user facing reflection API from compiler internals. For other potential implementors of a reflection API having compiler internals in the trait would be a blocker.

liufengyun · 2020-09-24T09:29:23Z

There is a minor issue with the current PR. While it approaches the 2nd design point, it still keeps a tiny compiler interface. Is it possible to eliminate CompilerInterface and protect its APIs in Reflection with private[scala]?

@liufengyun I prefer separating the user facing reflection API from compiler internals. For other potential implementors of a reflection API having compiler internals in the trait would be a blocker.

If I were correct, it is impossible for another compiler to only implement reflection API but not CompilerInterface. Both are necessary contracts with compilers for meta-programming to work.

b-studios · 2020-09-24T09:38:46Z

If I were correct, it is impossible for another compiler to only implement reflection API but not CompilerInterface. Both are necessary contracts with compilers for meta-programming to work.

You are probably right and in the current design compilers would also need to implement for example unpickleXXX.

You say "meta-programming" but eventually, it might be relevant to separate different notions of meta-programming (quoting, quoted pattern matching, reflection). Compilers (or DSL authors) might implement quoting, but not reflection. But I'd leave that for another PR / discussion

b-studios · 2020-09-24T09:54:42Z

Regarding tradeoffs:

Previously, Reflection and CompilerInterface could evolve somewhat independently. However, I am not sure whether divergence between the two interfaces would be desirable.
Previously, Reflection consisted of many concrete implementation details (often forwarding to CompilerInterface, but not exclusively). The current PR goes into the direction of turning Reflection into an abstract API. In the long run this might allow different implementations (in particular in combination with a change of QuoteContext and Expr and Type as in Experiment with static quote scope safety #8940).

liufengyun

LGTM

As user-facing API keeps the same, I think it's fine to delay the further refactoring of CompilerInterface to a different PR.

nicolasstucki · 2020-09-24T12:24:18Z

As a final note for users of the Reflection API. This PR does not change the user-facing API (modulo couple of small fixes). The changes in the way we implement this interface internally.

nicolasstucki force-pushed the use-direct-implementation-style-for-reflection branch 19 times, most recently from 34e1f18 to bf2ac88 Compare September 23, 2020 09:35

nicolasstucki force-pushed the use-direct-implementation-style-for-reflection branch from bf2ac88 to d2400b4 Compare September 23, 2020 14:39

nicolasstucki marked this pull request as ready for review September 23, 2020 16:05

nicolasstucki requested review from liufengyun and b-studios September 23, 2020 16:05

nicolasstucki assigned liufengyun and b-studios Sep 23, 2020

nicolasstucki force-pushed the use-direct-implementation-style-for-reflection branch from d2400b4 to 9b9c9f8 Compare September 23, 2020 16:26

This was referenced Sep 23, 2020

Expose ofErasedClass in Reflection #9851

Merged

Remove Reflection.Id #9857

Merged

nicolasstucki mentioned this pull request Sep 23, 2020

Rename reflection comment to documentation #9858

Merged

nicolasstucki added the prio:blocker label Sep 24, 2020

nicolasstucki mentioned this pull request Sep 24, 2020

Remove Reflection.LambdaType #9861

Closed

b-studios reviewed Sep 24, 2020

View reviewed changes

liufengyun approved these changes Sep 24, 2020

View reviewed changes

Review

5cd1cce

b-studios approved these changes Sep 24, 2020

View reviewed changes

nicolasstucki merged commit 01fc4a1 into scala:master Sep 24, 2020

nicolasstucki deleted the use-direct-implementation-style-for-reflection branch September 24, 2020 14:10

liufengyun mentioned this pull request Nov 6, 2020

Simplify CompilerInterface abstraction #10189

Merged

Kordyjan added this to the 3.0.0 milestone Aug 2, 2023

Implement Reflection API directly #9818

Implement Reflection API directly #9818

Uh oh!

Conversation

nicolasstucki commented Sep 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolasstucki commented Sep 23, 2020

Uh oh!

nicolasstucki commented Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

b-studios left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

b-studios Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

b-studios Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

nicolasstucki Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

b-studios Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

nicolasstucki Sep 24, 2020

Choose a reason for hiding this comment

Uh oh!

liufengyun commented Sep 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

b-studios commented Sep 24, 2020

Uh oh!

liufengyun commented Sep 24, 2020

Uh oh!

b-studios commented Sep 24, 2020

Uh oh!

b-studios commented Sep 24, 2020

Uh oh!

liufengyun left a comment

Choose a reason for hiding this comment

Uh oh!

nicolasstucki commented Sep 24, 2020

Uh oh!

Uh oh!

nicolasstucki commented Sep 17, 2020 •

edited

Loading

nicolasstucki commented Sep 24, 2020 •

edited

Loading

liufengyun commented Sep 24, 2020 •

edited

Loading