Add the possibility to create a typeSymbol in the Quotes API #20347

hamzaremmal · 2024-05-06T19:01:35Z

Closes #19448

hamzaremmal · 2024-05-06T20:12:07Z

Note that I didn't add a test for TypeBounds because calling tree.show will not print the bounds that were generated by TypeRepr.of[...]. Nevertheless, if we comment out the following two lines, the bounds will be printed too:

scala3/compiler/src/scala/quoted/runtime/impl/printers/SourceCode.scala

Line 1369 in 6af2bcf

case Inferred() =>

scala3/compiler/src/scala/quoted/runtime/impl/printers/SourceCode.scala

Line 1375 in 6af2bcf

case Inferred() => this

This is due to the fact that when a user writes the following code: type X >: Int, the compiler will infer the upper bound to Any. The following user written code (type X >: Int) will be represented as such in the Quotes API:

TypeDef(X,TypeBoundsTree(Ident(Int),TypeTree[TypeRef(ThisType(TypeRef(NoPrefix,module class scala)),class Any)],EmptyTree))

The same type tree generated with the Quotes API will the represented as this:

TypeDef(X,TypeTree[TypeBounds(TypeRef(ThisType(TypeRef(NoPrefix,module class scala)),class Int),TypeRef(TermRef(ThisType(TypeRef(NoPrefix,module class <root>)),object scala),Any))])

Both representations are equivalent but due to how the printer is designed, we cannot show the bounds of the second representation at the moment

smarter · 2024-05-06T21:07:01Z

tests/pos-macros/quoted-sym-newtype/Macro_1.scala

+
+def testImpl(using Quotes): Expr[Unit] = { 
+    import quotes.reflect.*
+    val sym = Symbol.newType(Symbol.spliceOwner, "mytype", Flags.EmptyFlags, TypeRepr.of[String], Symbol.noSymbol)


In the compiler, the info of a type symbol can either be a ClassInfo (if we're defining a class) or a TypeBounds (if we're defining a type parameter, a type alias, an abstract type member or an opaque type). So String is not a valid info, it should be TypeAlias(String), but giving the user higher-level methods like newTypeAlias and newTypeBounds would be better.

jchyb

We should also have a test of the use case from the original discussion:
Two macro methods: both a transparent inline and an inline macro method returning a trait with a bounded type member and a type alias type member, so the incorrect stuff we generate there can be caught by -Ycheck and -Xcheck-macros

jchyb · 2024-05-07T10:55:59Z

But great work so far! Especially with investigating the printer issue with the type bounds. I personally think showing the inferred stuff would be better than showing nothing, but this is something we can change in the future.

The TypeAlias issue makes me wonder whether the previous uses of TypeDef.apply were correct (I thought the only way to use it was to get the symbol from elsewhere and effectively use it as a type alias, but I don't think we wrap it with TypeAlias anywhere)

jchyb · 2024-05-07T10:58:55Z

tests/pos-macros/quoted-sym-newtype/Macro_1.scala

+
+inline def testMacro = ${ testImpl }
+
+def testImpl(using Quotes): Expr[Unit] = { 


Missing @experimental here and in def test I believe

Gedochao · 2024-07-04T08:02:11Z

This has been decided to be worked on in the 3.6.0 release.

jchyb · 2024-08-20T15:33:39Z

(@hamzaremmal, I know you are away temporarily so feel free to ignore this post for now - apologies for the notification)

I remember mentioning that I would help with this and I never ended up getting around to it, until now (sorry about that). I wanted to add some more tests so that we can check if we fail -Xcheck-macro in outputted type symbol and check the use case from the original issue (constructing a trait with a type alias member).

While adding the -Xcheck-macro one, I discovered that the lack of TypeAlias in info is indeed a problem, like @smarter said - we can get a "Malformed tree" error we do not get when using TypeAlias. Since we do not have anything like that in the reflect API (and I do not really want to add that), let's go with the suggestion of splitting the functionality into newTypeAlias and newTypeBounds, which should be simple thankfully.

Can I push onto this PR? I have the necessary changes basically done on my machine already, so there is no need to worry about any added work or anything like that, especially at the end of the current cycle.

hamzaremmal · 2024-08-27T12:46:59Z

Can I push onto this PR? I have the necessary changes basically done on my machine already, so there is no need to worry about any added work or anything like that, especially at the end of the current cycle.

Hi @jchyb, sorry I was bit overwhelmed by work and forgot about this PR. Nevertheless, I added it in my TODO for next week but if you have a working version, feel free to push here and I would be happy to review it.

hamzaremmal · 2024-09-11T09:49:28Z

library/src/scala/quoted/Quotes.scala

+        */
+      @experimental
+      // Keep: `flags` doc aligned with QuotesImpl's `validTypeFlags`
+      def newTypeAlias(parent: Symbol, name: String, flags: Flags, tpe: TypeRepr, privateWithin: Symbol): Symbol


What if someone calls this method with a TypeBounds ?

Should we represent TypeAlias in the Quotes API as substype of TypeRepr instead ?

Then we would have the same problem, with being able to put TypeBounds into TypeAlias. I think it's better to add an assertion to the method (but thank you for catching this!)

Wait, I guess we are still able to put TypeBounds into TypeBounds, so it would not be that different... Still, I am hesitant to changing the reflection api types hierarchy, if we have alternatives

hamzaremmal · 2024-10-03T09:52:49Z

compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala

+
+      def newTypeAlias(owner: Symbol, name: String, flags: Flags, tpe: TypeRepr, privateWithin: Symbol): Symbol =
+        checkValidFlags(flags.toTypeFlags, Flags.validTypeFlags)
+        dotc.core.Symbols.newSymbol(owner, name.toTypeName, flags | dotc.core.Flags.Deferred, dotc.core.Types.TypeAlias(tpe), privateWithin)


TypeAlias are not Deferred and cannot be Deferred since they have a defined member (rhs)

This means we should also change the Flags.validTypeFlags to reflect this

hamzaremmal · 2024-10-03T09:54:19Z

library/src/scala/quoted/Quotes.scala

+        *  @param name The name of the type
+        *  @param flags extra flags to with which symbol can be constructed. `Deferred` flag will be added. Can be `Private` | `Protected` | `Override` | `Deferred` | `Final` | `Infix` | `Local`
+        *  @param tpe The rhs the type alias
+        *  @param privateWithin the symbol within which this new method symbol should be private. May be noSymbol.


Suggested change

* @param privateWithin the symbol within which this new method symbol should be private. May be noSymbol.

* @param privateWithin the symbol within which this new type symbol should be private. May be noSymbol.

hamzaremmal · 2024-10-03T09:55:07Z

library/src/scala/quoted/Quotes.scala

+        *  @param name The name of the type
+        *  @param flags extra flags to with which symbol can be constructed. `Deferred` flag will be added. Can be `Private` | `Protected` | `Override` | `Deferred` | `Final` | `Infix` | `Local`
+        *  @param tpe The bounds of the type
+        *  @param privateWithin the symbol within which this new method symbol should be private. May be noSymbol.


Suggested change

* @param privateWithin the symbol within which this new method symbol should be private. May be noSymbol.

* @param privateWithin the symbol within which this new type symbol should be private. May be noSymbol.

smarter

Otherwise LGTM.

smarter · 2024-10-04T13:46:06Z

compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala

+      def newTypeAlias(owner: Symbol, name: String, flags: Flags, tpe: TypeRepr, privateWithin: Symbol): Symbol =
+        checkValidFlags(flags.toTypeFlags, Flags.validTypeAliasFlags)
+        assert(!tpe.isInstanceOf[Types.TypeBounds], "Passed `tpe` into newTypeAlias should not represent TypeBounds")
+        dotc.core.Symbols.newSymbol(owner, name.toTypeName, flags | dotc.core.Flags.Deferred, dotc.core.Types.TypeAlias(tpe), privateWithin)


As mentioned by hamza:

Suggested change

dotc.core.Symbols.newSymbol(owner, name.toTypeName, flags | dotc.core.Flags.Deferred, dotc.core.Types.TypeAlias(tpe), privateWithin)

dotc.core.Symbols.newSymbol(owner, name.toTypeName, flags, dotc.core.Types.TypeAlias(tpe), privateWithin)

smarter · 2024-10-04T13:46:45Z

compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala

+        assert(!tpe.isInstanceOf[Types.TypeBounds], "Passed `tpe` into newTypeAlias should not represent TypeBounds")
+        dotc.core.Symbols.newSymbol(owner, name.toTypeName, flags | dotc.core.Flags.Deferred, dotc.core.Types.TypeAlias(tpe), privateWithin)
+
+      def newBoundedType(owner: Symbol, name: String, flags: Flags, tpe: TypeBounds, privateWithin: Symbol): Symbol =


I suggest calling it newAbstractType instead.

smarter · 2024-10-04T13:50:58Z

compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala

+
+      def newBoundedType(owner: Symbol, name: String, flags: Flags, tpe: TypeBounds, privateWithin: Symbol): Symbol =
+        checkValidFlags(flags.toTypeFlags, Flags.validBoundedTypeFlags)
+        dotc.core.Symbols.newSymbol(owner, name.toTypeName, flags, tpe, privateWithin)


This one should be Deferred:

Suggested change

dotc.core.Symbols.newSymbol(owner, name.toTypeName, flags, tpe, privateWithin)

dotc.core.Symbols.newSymbol(owner, name.toTypeName, flags | dotc.core.Flags.Deferred, tpe, privateWithin)

smarter · 2024-10-04T13:54:45Z

tests/pos-macros/quote-sym-newboundedtype/Macro_1.scala

+    import quotes.reflect.*
+
+    def makeType(owner: Symbol): Symbol =
+      Symbol.newBoundedType(owner, "mytype", Flags.EmptyFlags, TypeBounds.lower(TypeRepr.of[String]), Symbol.noSymbol)


Would be good to have some tests with type lambdas as bounds, that is the equivalent of:

type Foo = [X] =>> Int type Bla >: Nothing <: [X] =>> Int type Bar >: [X] =>> Int <: [X] =>> Int

It's also important that error-checking runs the same bound-checks than regular code:

type Foo1 >: Int <: String // error type Foo2 >: [X] =>> Int <: Any // error: kind mismatch

smarter · 2024-10-04T13:56:59Z

tests/pos-macros/quote-sym-newboundedtype/Macro_1.scala

+      Symbol.newBoundedType(owner, "mytype", Flags.EmptyFlags, TypeBounds.lower(TypeRepr.of[String]), Symbol.noSymbol)
+
+    val typeDef = TypeDef(makeType(Symbol.spliceOwner))
+    // Expr printer does not work here, see comment:


If this cannot be fixed right now, we should open an issue for it. (IMO we should just use the regular compiler type printer and get rid of the custom one used for quotes, it creates more problems than it solves, and we can make the regular type printer more flexible if needed).

hamzaremmal self-assigned this May 6, 2024

hamzaremmal requested review from smarter and jchyb May 6, 2024 20:13

hamzaremmal marked this pull request as ready for review May 6, 2024 20:13

hamzaremmal force-pushed the i19448 branch 4 times, most recently from 38eab54 to 429ac32 Compare May 6, 2024 20:50

smarter requested changes May 6, 2024

View reviewed changes

jchyb requested changes May 7, 2024

View reviewed changes

jchyb reviewed May 7, 2024

View reviewed changes

jchyb added the needs-minor-release This PR cannot be merged until the next minor release label May 31, 2024

Gedochao assigned jchyb Jul 4, 2024

pomadchin mentioned this pull request Jul 11, 2024

Scala 3 feature parity typelevel/cats-tagless#170

Open

24 tasks

jchyb force-pushed the i19448 branch 5 times, most recently from efcdf18 to 2aee139 Compare August 30, 2024 14:05

hamzaremmal commented Oct 3, 2024

View reviewed changes

hamzaremmal requested a review from smarter October 3, 2024 10:02

hamzaremmal assigned smarter Oct 3, 2024

hamzaremmal and others added 2 commits October 3, 2024 13:20

Add the possibility to create a typeSymbol in the Quotes API

61764dd

Split Symbol.newType into 2 methods and add tests

56893de

jchyb force-pushed the i19448 branch from 2aee139 to 23d01e5 Compare October 3, 2024 12:11

Address review comments

9db0a00

jchyb force-pushed the i19448 branch from 23d01e5 to 9db0a00 Compare October 3, 2024 12:16

smarter requested changes Oct 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the possibility to create a typeSymbol in the Quotes API #20347

Add the possibility to create a typeSymbol in the Quotes API #20347

hamzaremmal commented May 6, 2024

hamzaremmal commented May 6, 2024 •

edited

Loading

smarter May 6, 2024

jchyb left a comment

jchyb commented May 7, 2024

jchyb May 7, 2024 •

edited

Loading

Gedochao commented Jul 4, 2024

jchyb commented Aug 20, 2024

hamzaremmal commented Aug 27, 2024

hamzaremmal Sep 11, 2024

hamzaremmal Oct 3, 2024

jchyb Oct 3, 2024

jchyb Oct 3, 2024

hamzaremmal Oct 3, 2024

hamzaremmal Oct 3, 2024

hamzaremmal Oct 3, 2024

hamzaremmal Oct 3, 2024

smarter left a comment

smarter Oct 4, 2024

smarter Oct 4, 2024

smarter Oct 4, 2024

smarter Oct 4, 2024

smarter Oct 4, 2024


		inline def testMacro = ${ testImpl }

		def testImpl(using Quotes): Expr[Unit] = {

	* @param privateWithin the symbol within which this new method symbol should be private. May be noSymbol.
	* @param privateWithin the symbol within which this new type symbol should be private. May be noSymbol.

	dotc.core.Symbols.newSymbol(owner, name.toTypeName, flags \| dotc.core.Flags.Deferred, dotc.core.Types.TypeAlias(tpe), privateWithin)
	dotc.core.Symbols.newSymbol(owner, name.toTypeName, flags, dotc.core.Types.TypeAlias(tpe), privateWithin)

Add the possibility to create a typeSymbol in the Quotes API #20347

Are you sure you want to change the base?

Add the possibility to create a typeSymbol in the Quotes API #20347

Conversation

hamzaremmal commented May 6, 2024

hamzaremmal commented May 6, 2024 • edited Loading

Choose a reason for hiding this comment

jchyb left a comment

Choose a reason for hiding this comment

jchyb commented May 7, 2024

jchyb May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Gedochao commented Jul 4, 2024

jchyb commented Aug 20, 2024

hamzaremmal commented Aug 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamzaremmal commented May 6, 2024 •

edited

Loading

jchyb May 7, 2024 •

edited

Loading