Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Oct 8, 2024

Conversation

hamzaremmal
Copy link
Member

Closes #19448

@hamzaremmal hamzaremmal self-assigned this May 6, 2024
@hamzaremmal
Copy link
Member Author

hamzaremmal commented May 6, 2024

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:

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

@hamzaremmal hamzaremmal requested review from smarter and jchyb May 6, 2024 20:13
@hamzaremmal hamzaremmal marked this pull request as ready for review May 6, 2024 20:13
@hamzaremmal hamzaremmal force-pushed the i19448 branch 4 times, most recently from 38eab54 to 429ac32 Compare May 6, 2024 20:50

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jchyb jchyb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

jchyb commented May 7, 2024

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)


inline def testMacro = ${ testImpl }

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

@jchyb jchyb May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing @experimental here and in def test I believe

@jchyb jchyb added the needs-minor-release This PR cannot be merged until the next minor release label May 31, 2024
@Gedochao
Copy link
Contributor

Gedochao commented Jul 4, 2024

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

@jchyb
Copy link
Contributor

jchyb commented Aug 20, 2024

(@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
Copy link
Member Author

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.

@jchyb jchyb force-pushed the i19448 branch 5 times, most recently from efcdf18 to 2aee139 Compare August 30, 2024 14:05
*/
@experimental
// Keep: `flags` doc aligned with QuotesImpl's `validTypeFlags`
def newTypeAlias(parent: Symbol, name: String, flags: Flags, tpe: TypeRepr, privateWithin: Symbol): Symbol
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if someone calls this method with a TypeBounds ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* @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.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

* @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.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@smarter smarter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

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 =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest calling it newAbstractType instead.


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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jchyb I think you removed Deferred from the wrong function when you addressed my comments. (thus @smarter's comment too)

import quotes.reflect.*

def makeType(owner: Symbol): Symbol =
Symbol.newBoundedType(owner, "mytype", Flags.EmptyFlags, TypeBounds.lower(TypeRepr.of[String]), Symbol.noSymbol)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@jchyb
Copy link
Contributor

jchyb commented Oct 8, 2024

@smarter Thank you for the review!
@hamzaremmal The point about adding the TypeLambda tests highlighted some issues. When we make an illegal TypeBound we do get an error, but only for the transparent macro (which makes sense, but I imagine we should also add something to at least -Xcheck-macros. This, combined with your point about adding TypeAlias, leads me to believe we should add those and merge for 3.7... I apologize for taking over and messing this up like this!

@smarter
Copy link
Member

smarter commented Oct 8, 2024

Since the API is @experimental, adding it now with some missing checks seems fine to me.

@hamzaremmal
Copy link
Member Author

Since the API is @experimental, adding it now with some missing checks seems fine to me.

Agreed !

@hamzaremmal hamzaremmal merged commit a3015f4 into scala:main Oct 8, 2024
28 checks passed
@hamzaremmal hamzaremmal deleted the i19448 branch October 8, 2024 13:44
@WojciechMazur WojciechMazur added this to the 3.6.0 milestone Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-minor-release This PR cannot be merged until the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a method to create a new type symbol
5 participants