Homogenize Reflect Type constructors #9744
Conversation
nicolasstucki
changed the title
nicolasstucki
force-pushed
the
improve-macro-implementations
branch
from
ac6fc35 to
553330b
Compare
nicolasstucki
changed the title
nicolasstucki
force-pushed
the
improve-macro-implementations
branch
3 times, most recently
from
389b550 to
d78262c
Compare
nicolasstucki
force-pushed
the
improve-macro-implementations
branch
from
b819291 to
f5d1bf4
Compare
| qtype.asInstanceOf[scala.internal.quoted.Type[TypeTree]].typeTree.tpe | ||
|
|
||
| /** Returns the type or kind of the class of the type T */ | ||
| def ofClass[T](using ct: scala.reflect.ClassTag[T]): Type = |
There was a problem hiding this comment.
The existence of both Type.of and Type.ofClass can cause confusion. What about change Type.of to Type.apply. We can also change return type of Type.of to Type[T]:
def apply[T <: AnyKind](using qtype: scala.quoted.Type[T]): Type[T] = qtype
// usage
Type[T]I see in the test code we write Type.of[T].seal in many places. Without the change above, it's simpler.
Then we can rename Type.ofClass to Type.of.
There was a problem hiding this comment.
Can Type.of subsumes all usage of Type.ofClass?
There was a problem hiding this comment.
I did not notice the Type.of[T].seal. That is just bad code, they should be replaced with '[T] unless that are tests on the seal operation. I will check all those occurrences.
Type.of does subsume most cases of Type.ofClass. The only case it does not is if you get a Class[_] at runtime and want to recover the Type from it. The second is also slightly more performant.
The apply is not good because it leads to easy mistakes as quoted.Type[T] and Reflect.Type[T] dot not return the same thing. Knowing that you need to use Type.of helps users find the correct version and autocompletion may help to find the other variants that are more specific/performant.
There was a problem hiding this comment.
Actually Type.of[T].seal is equivalent to '[T].asInstanceOf[Type[?]] which is relly bad code.
There was a problem hiding this comment.
I only see 2 tests files with Type.of[T].seal where we explicitly test the seal operation.
There was a problem hiding this comment.
If it takes an explicit argument no one will use it as they would have to write much more code for something that they should be encouraged to do. Type.ofClass[Foo] would become Type.ofClass(classOf[Foo]). Additionally, that would introduce extra redundancy.
There was a problem hiding this comment.
This concept is in the middle between Type.of and classOf and it makes sense to have a name that intuitively makes allusion to both those concepts.
There was a problem hiding this comment.
I think I have exhausted my arguments ;)
I'm happy to hear what others think about it.
I'm open to any agreed decision in order to move on.
There was a problem hiding this comment.
I don't mind the name but I am not sure what the benefit is except to conveniently have an unapplied polymorphic type (when generating ClassTag still requires a statically known type)
There was a problem hiding this comment.
That is unfortunately unavoidable for any ClassTag or classOf.
* Rename Reflect.typeOf to Reflect.Type.of * Rename Reflect.Type.apply to Reflect.Type.ofClass * Make Reflect.Type.of AnyKinded * Improve some other type uses in community build
nicolasstucki
force-pushed
the
improve-macro-implementations
branch
from
f16b39d to
b32ae8f
Compare
Uh oh!
There was an error while loading. Please reload this page.