Skip to content

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

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

Merged
merged 1 commit into from
Aug 13, 2025

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
Contributor

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
Contributor

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

@Kalin-Rudnicki
Copy link
Contributor

Hello, what is the status of this issue?

As I mentioned in the previous comments, I had to disable -Yretain-trees in order to get my project to compile. I now am trying to pull my macro library into another project which requires -Yretain-trees. Im really hoping to get some information on the timeline for this fix, because this issue is now a complete blocker.

@jchyb jchyb requested a review from a team as a code owner August 13, 2025 09:56
@jchyb jchyb enabled auto-merge (squash) August 13, 2025 09:58
@Kalin-Rudnicki
Copy link
Contributor

❤️ thank you for getting on this!! any idea what compiler version this will go out in?

@jchyb
Copy link
Contributor Author

jchyb commented Aug 13, 2025

@Kalin-Rudnicki sorry for sitting on this PR for all this time. It will most likely be included in 3.8.0

@jchyb jchyb merged commit 4788641 into scala:main Aug 13, 2025
45 checks passed
@jchyb jchyb deleted the fix-i22584 branch August 13, 2025 12:56
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