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

Quotes reflect: Allow to return DefDef from a val symbol tree #22603

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jchyb
Copy link
Contributor

@jchyb jchyb commented Feb 14, 2025

Fixes #22584
During the Getters phase, the compiler replaces class member vals with defs. Meanwhile .tree returns a tree most recently associated with a symbol (if currently compiled), or one read from tasty (if compiled before). An interesting thing happens in the issue - since all of the files are tried being compiled in one compilation run, the file with a macro call is suspended, and then the macro and the MyClass class definition are compiled to the end (also including the Getters phase, where the tree stops being a ValDef).

Initially I thought about manually replacing that returned DefDef with a ValDef in quotes reflection (like what happens when the tree is not available), but that would have been pointless - there would be little use of calling .tree on a symbol if we cannot read the rhs of the definition. Returning raw DefDef seems like a less bad choice, especially .tree is meant to be for power users anyway, and is full of other, more dangerous warts.

@jchyb jchyb requested a review from prolativ February 14, 2025 14:25
@jchyb
Copy link
Contributor Author

jchyb commented Feb 14, 2025

@prolativ Since you already looked at the issue, I though you might have been interested, but feel free to not do a review or unassign yourself if you feel uncomfortable with that/ don't have time.

Copy link
Contributor

@prolativ prolativ left a comment

Choose a reason for hiding this comment

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

Would one now get different results for .tree with and without -Yretain-trees (with the difference other than some tree vs no/empty tree)?

BTW as this area of code is changed we could fix the doc string, which doesn't render well in scala-doc currently

@@ -4060,9 +4060,10 @@ trait Quotes { self: runtime.QuoteUnpickler & runtime.QuoteMatching =>
*
* If this symbol `isClassDef` it will return `a `ClassDef`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* If this symbol `isClassDef` it will return `a `ClassDef`,
* If this symbol `isClassDef` it will return a `ClassDef`,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good catch!

@@ -4060,9 +4060,10 @@ trait Quotes { self: runtime.QuoteUnpickler & runtime.QuoteMatching =>
*
* If this symbol `isClassDef` it will return `a `ClassDef`,
* if this symbol `isTypeDef` it will return `a `TypeDef`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* if this symbol `isTypeDef` it will return `a `TypeDef`,
* if this symbol `isTypeDef` it will return a `TypeDef`,

@@ -4060,9 +4060,10 @@ trait Quotes { self: runtime.QuoteUnpickler & runtime.QuoteMatching =>
*
* If this symbol `isClassDef` it will return `a `ClassDef`,
* if this symbol `isTypeDef` it will return `a `TypeDef`,
* if this symbol `isValDef` it will return `a `ValDef`,
* if this symbol `isDefDef` it will return `a `DefDef`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* if this symbol `isDefDef` it will return `a `DefDef`
* if this symbol `isDefDef` it will return a `DefDef`

@@ -4060,9 +4060,10 @@ trait Quotes { self: runtime.QuoteUnpickler & runtime.QuoteMatching =>
*
* If this symbol `isClassDef` it will return `a `ClassDef`,
* if this symbol `isTypeDef` it will return `a `TypeDef`,
* if this symbol `isValDef` it will return `a `ValDef`,
* if this symbol `isDefDef` it will return `a `DefDef`
* if this symbol `isBind` it will return `a `Bind`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* if this symbol `isBind` it will return `a `Bind`,
* if this symbol `isBind` it will return a `Bind`,

* if this symbol `isDefDef` it will return `a `DefDef`
* if this symbol `isBind` it will return `a `Bind`,
* if this symbol `isValDef` it will return `a `ValDef`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* if this symbol `isValDef` it will return `a `ValDef`,
* if this symbol `isValDef` it will return a `ValDef`,

@jchyb
Copy link
Contributor Author

jchyb commented Feb 14, 2025

Yes, they would be different:

  • no -Yretain-trees - we get a ValDef without a rhs (manually constructed in the .tree implementation).
  • we have -Yretain-trees, and we compile the macro call together with the macro method and the type (macro call gets suspended, after which everything else finishes compilation before calling macro) - we get a DefDef, potentially with a rhs.
  • we have -Yretain-trees, we already have macro implementation compiled, we compile macro call along with the inspected symbols - we get a ValDef, potentially with a rhs
  • we have -Yretain-trees, we already have macro implementation compiled and the symbol definition compiled, we compile the macro call along with the inspected symbols - we get a ValDef, potentially with a rhs (read from tasty)

This means that the .tree can even return different things in different incremental compilation runs of the same code.

@jchyb
Copy link
Contributor Author

jchyb commented Feb 14, 2025

Ideally we would be able to turn back time and always be able to see what the tree looked like in Pickler and use that, but in general even then there would be inconsistencies like these when transparent inline were used since we cannot look into the future in that case, and the inspected symbol may or may not have already been compiled.
I am not a fan of .tree, I admit.

@Kalin-Rudnicki
Copy link

I had to remove -Yretain-trees for the time being in order to make progress. Is this what is causing annotations on my fields to disappear? Or a separate issue entirely?

source code:

  @oxygen.meta.annotation.showDerivation
  final case class CaseClass1(
      @ExampleTypeClasses.fieldName("my-int") int: Int,
      @ExampleTypeClasses.misc string: String,
      boolean: Boolean,
  )

rendered tree:

@oxygen.meta.annotation.showDerivation final case class CaseClass1(int: scala.Int, string: scala.Predef.String, boolean: scala.Boolean) extends java.lang.Object with _root_.scala.Product with java.io.Serializable {
  override def hashCode(): scala.Int
  override def equals(x$0: scala.Any): scala.Boolean
  override def toString(): java.lang.String
  override def canEqual(that: scala.Any): scala.Boolean
  override def productArity: scala.Int
  override def productPrefix: scala.Predef.String
  override def productElement(n: scala.Int): scala.Any
}

@soronpo
Copy link
Contributor

soronpo commented Feb 14, 2025

Would one now get different results for .tree with and without -Yretain-trees (with the difference other than some tree vs no/empty tree)?

This is expected and explicitly documented for the tree method.

@Kalin-Rudnicki
Copy link

seems annotations are only available on the constructor symbols, not the case fields or declared fields, interesting

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quoted API has broken impl for fields (case fields / declared fields / ...)
4 participants