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.

Sign up for GitHub

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

Jump to bottom

Add reflect ClassDef.apply and Symbol.newClass #14124

Merged
merged 1 commit into from
Mar 24, 2022
Merged

Add reflect ClassDef.apply and Symbol.newClass #14124

merged 1 commit into from
Mar 24, 2022

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Dec 17, 2021 •
edited
Loading

We also add

  • SymbolMethods.typeRef
  • SymbolMethods.termRef
  • TypeTreeModule.ref

@nicolasstucki nicolasstucki self-assigned this Jan 13, 2022
@nicolasstucki nicolasstucki force-pushed the add-reflect-ClassDef-apply branch 2 times, most recently from 8bc0c20 to 1fbda34 Compare February 18, 2022 09:08
@nicolasstucki nicolasstucki mentioned this pull request Feb 18, 2022
Merged
@nicolasstucki nicolasstucki force-pushed the add-reflect-ClassDef-apply branch 3 times, most recently from 647629e to 24586e1 Compare February 22, 2022 11:09
@nicolasstucki nicolasstucki force-pushed the add-reflect-ClassDef-apply branch 2 times, most recently from 7def96e to d32d4af Compare February 28, 2022 08:23
@nicolasstucki nicolasstucki marked this pull request as ready for review February 28, 2022 12:07
smarter
smarter requested changes Feb 28, 2022
View reviewed changes
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.

compiler/src/scala/quoted/runtime/impl/QuotesImpl.scala Outdated Show resolved Hide resolved
library/src/scala/quoted/Quotes.scala Outdated Show resolved Hide resolved
library/src/scala/quoted/Quotes.scala Show resolved Hide resolved
@smarter smarter assigned nicolasstucki and unassigned smarter Feb 28, 2022
@nicolasstucki nicolasstucki changed the title Add reflect class def apply Add reflect ClassDef.apply and Symbol.newClass Mar 2, 2022
@nicolasstucki nicolasstucki added the release-notes Should be mentioned in the release notes label Mar 2, 2022
@nicolasstucki nicolasstucki force-pushed the add-reflect-ClassDef-apply branch 2 times, most recently from 2254fa4 to 4845353 Compare March 2, 2022 08:13
@nicolasstucki nicolasstucki added release-notes Should be mentioned in the release notes and removed release-notes Should be mentioned in the release notes labels Mar 2, 2022
smarter
smarter requested changes Mar 2, 2022
View reviewed changes
library/src/scala/quoted/Quotes.scala Show resolved Hide resolved
tests/neg-macros/newClassExtendsNoParents/Macro_1.scala

val cls = Symbol.newClass(Symbol.spliceOwner, name, parents = Nil, decls, selfInfo = None)
val clsDef = ClassDef(cls, parents, body = List())
val newCls = Typed(Apply(Select(New(TypeIdent(cls)), cls.primaryConstructor), Nil), TypeTree.of[Object])
Copy link
Member

Choose a reason for hiding this comment

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

I guess this is orthogonal but in tpd we have a New overload that takes constructor arguments this could be useful here too

@nicolasstucki nicolasstucki force-pushed the add-reflect-ClassDef-apply branch 2 times, most recently from 072c154 to 4800912 Compare March 16, 2022 12:19
smarter
smarter requested changes Mar 16, 2022
View reviewed changes
library/src/scala/quoted/Quotes.scala Outdated Show resolved Hide resolved
tests/neg-macros/newClassExtendsNoParents.check Outdated Show resolved Hide resolved
library/src/scala/quoted/Quotes.scala
* @pre symbol.isType returns true
*/
@experimental
def typeRef: TypeRef
Copy link
Member

Choose a reason for hiding this comment

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

It seems that we're missing an API to create a TypeRef/TermRef from a specific prefix: with foo.typeRef I can get a TypeRef this.foo and with pre.memberType(foo) I can get the info corresponding to pre.foo, but is there a way to generate the TypeRef pre.foo itself?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doesn't TypeRepr.select provide that?

Copy link
Member

Choose a reason for hiding this comment

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

Oh indeed, I missed that one, maybe each of their documentation should @see the other one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

tests/run-macros/newClassSelf/Macro_1.scala Outdated
*/
def makeFooBar(using Quotes)(name: String, fooCls: quotes.reflect.Symbol): quotes.reflect.ClassDef = {
import quotes.reflect.*
val parents = List(Inferred(fooCls.typeRef), TypeTree.of[Bar])
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps instead of the Inferred(foo.typeRef) pattern we could have an overload of TypeTree.apply (or TypeTree.of? Not sure why we use of there) that takes a Symbol. This would be similar to the compiler-internal tpd.ref (in fact alternatively we could expose ref directly?)

Copy link
Contributor Author

@nicolasstucki nicolasstucki Mar 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Added TypeTree.apply Added TypeTree.ref.

We should probably add TypeTree.apply as an alias of Inferred.apply. We cannot overload this method.

@nicolasstucki nicolasstucki force-pushed the add-reflect-ClassDef-apply branch 5 times, most recently from d0bb8b6 to cf0b09e Compare March 22, 2022 14:53
@nicolasstucki nicolasstucki force-pushed the add-reflect-ClassDef-apply branch 3 times, most recently from a0d7004 to 26bb201 Compare March 23, 2022 15:48
smarter
smarter approved these changes Mar 23, 2022
View reviewed changes
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!

library/src/scala/quoted/Quotes.scala Outdated Show resolved Hide resolved
@smarter smarter assigned nicolasstucki and unassigned smarter Mar 23, 2022
@nicolasstucki nicolasstucki force-pushed the add-reflect-ClassDef-apply branch from 26bb201 to 36853ff Compare March 24, 2022 07:16
@nicolasstucki nicolasstucki enabled auto-merge March 24, 2022 07:17
@nicolasstucki nicolasstucki merged commit a5d5fe5 into scala:main Mar 24, 2022
@nicolasstucki nicolasstucki deleted the add-reflect-ClassDef-apply branch March 24, 2022 08:39
@smarter smarter mentioned this pull request Apr 20, 2022
Closed
This was referenced May 27, 2022
Merged
Closed
@neko-kai neko-kai mentioned this pull request May 30, 2022
Closed
@evbo evbo mentioned this pull request Jul 22, 2022
Open
@evbo
Copy link

evbo commented Jul 22, 2022 •
edited
Loading

@nicolasstucki thank you for this!!! I am noticing a class cast exception if I try pattern matching typeRef you added in this PR:

java.lang.ClassCastException: class dotty.tools.dotc.core.Names$SimpleName cannot be cast to class dotty.tools.dotc.core.Names$TypeName (dotty.tool
s.dotc.core.Names$SimpleName and dotty.tools.dotc.cor
e.Names$TypeName are in unnamed module of loader sbt.
internal.classpath.ClassLoaderCache$Key$CachedClassLo
ader @2c470fec)

e.g. if I try to check the type of a case class field like:

// scala 3.2.0-RC2
declaredField.companionClass.caseFields.map { ccField =>
  val tt = ccField.typeRef match {
    case TypeRef(typeRepr, args) => s"args are: ${args}"
  }
}

I wasn't sure if this was a separate bug or not, happy to open a new one. It does seem odd seeing that exception; feels like a class cast exception happening internally so thought it might be important.

Update: I've found this and while I'm still not sure about that class cast exception above, it does seem to be the case that matching on AppliedType will yield the type args

@Kordyjan Kordyjan added this to the 3.1.3 milestone Aug 1, 2023
This was referenced Sep 5, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smarter smarter smarter approved these changes

Assignees

@nicolasstucki nicolasstucki

Labels
release-notes Should be mentioned in the release notes
Projects
None yet
Milestone
3.1.3
Development

Successfully merging this pull request may close these issues.

No way to get a TypeRepr directly from a Symbol Quotes: generate arbitrary trait or class implementations
4 participants
@nicolasstucki @evbo @smarter @Kordyjan