Skip to content

macros: remove or restrict symbol.tree to avoid CyclicReference exception #7025

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

Closed
liufengyun opened this issue Aug 12, 2019 · 9 comments · Fixed by #9984
Closed

macros: remove or restrict symbol.tree to avoid CyclicReference exception #7025

liufengyun opened this issue Aug 12, 2019 · 9 comments · Fixed by #9984
Milestone

Comments

@liufengyun
Copy link
Contributor

liufengyun commented Aug 12, 2019

Current macro system exposes symbol.tree to get the tree of a symbol. However, its usage in macros easily leads to CyclicReference exception.

minimized code

Macro_1.scala:

  import scala.quoted._
  import scala.tasty._

  inline def debug: Unit = ${debugImpl}

  def debugImpl given (qctx: QuoteContext): Expr[Unit] = {
    import qctx.tasty._

    def nearestEnclosingDef(owner: Symbol): Symbol =
      owner match {
        case IsDefDefSymbol(x)   => x
        case IsClassDefSymbol(x) => x
        case _                   => nearestEnclosingDef(owner.owner)
      }

    nearestEnclosingDef(rootContext.owner) match {
        case IsDefDefSymbol(x) =>
          val code = x.tree.show
          '{ println(${code.toExpr}) }
        case _ =>
          '{()}
      }
  }
}

Test_2.scala:

object Test {
  def main(args: Array[String]): Unit = {
    bar("world", 100, true)
  }

  def bar(a1: String, a2: Long, a3: Boolean) = {  // works fine with explicit return type
    debug
  }
}

expectation

The code above leads to CyclicReference exception:

stack trace
-- Error: Test_2.scala:7:4 ---------------------------------------------------------------------
7 |    debug
  |    ^^^^^
  |Exception occurred while executing macro expansion.
  |dotty.tools.dotc.core.CyclicReference:
  |	at dotty.tools.dotc.core.CyclicReference$.apply(TypeErrors.scala:157)
  |	at dotty.tools.dotc.core.SymDenotations$SymDenotation.completeFrom(SymDenotations.scala:249)
  |	at dotty.tools.dotc.core.Denotations$Denotation.completeInfo$1(Denotations.scala:180)
  |	at dotty.tools.dotc.core.Denotations$Denotation.info(Denotations.scala:182)
  |	at dotty.tools.dotc.ast.tpd$.polyDefDef(tpd.scala:224)
  |	at dotty.tools.dotc.ast.tpd$.DefDef(tpd.scala:221)
  |	at dotty.tools.dotc.ast.tpd$.DefDef(tpd.scala:218)
  |	at dotty.tools.dotc.tastyreflect.FromSymbol$.defDefFromSym(FromSymbol.scala:46)
  |	at dotty.tools.dotc.tastyreflect.ReflectionCompilerInterface.DefDefSymbol_tree(ReflectionCompilerInterface.scala:1644)
  |	at dotty.tools.dotc.tastyreflect.ReflectionCompilerInterface.DefDefSymbol_tree(ReflectionCompilerInterface.scala:1643)
  |	at scala.tasty.reflect.SymbolOps$DefDefSymbolAPI.tree(SymbolOps.scala:217)
  |	at Macros$.debugImpl(Macro_1.scala:24)
  |
  | This location is in code that was inlined at Test_2.scala:7

To avoid such crashes with symbol.tree, we may either:

  • remove symbol.tree in the API (scala2 macros doesn't have such an API)
  • only expose the API in black-box macros, and make black-box macros expand in a later phase
@milessabin
Copy link
Contributor

shapeless 3's Typeable uses Symbol#tree here. I'd be happy to change that if there's an alternative.

@milessabin
Copy link
Contributor

Typeable is blackbox, so the second option might be viable.

@LPTK
Copy link
Contributor

LPTK commented Aug 12, 2019

How about having symbol.tree return Either[CyclicReference,Tree]? This way, macro authors are forced to be careful with this feature and have to handle the cases when it's not applicable — they can for example fall back to some alternative macro implementation when it happens, or return an error to the user such as "please provide an explicit type for ...".

Having the ability to examine the trees of symbols sounds extremely useful from a macro author perspective. Scala 2 macros often go through very ugly hacks because they don't have it (including casting their way through to expose internal compiler APIs so they can look at the surrounding trees). As another example, IIRC the quill library has been stashing trees in annotations in order to recover them later, but that's a hack and it has plenty of drawbacks.

@liufengyun
Copy link
Contributor Author

Thanks for sharing the use cases @LPTK @milessabin .

It seems CyclicReference is not a good argument to remove symbol.tree, as symbol.signature may also cause CyclicReference exceptions.

inline def debug: Unit = ${Macros.debugImpl}

object Macros {
  import scala.quoted._
  import scala.tasty._

  def debugImpl given (qctx: QuoteContext): Expr[Unit] = {
    import qctx.tasty._

    def nearestEnclosingDef(owner: Symbol): Symbol =
      owner match {
        case IsDefDefSymbol(x)   => x
        case IsClassDefSymbol(x) => x
        case _                   => nearestEnclosingDef(owner.owner)
      }

    nearestEnclosingDef(rootContext.owner) match {
        case IsDefDefSymbol(x) =>
          val code = x.signature.toString
          '{ println(${code.toExpr}) }
        case _ =>
          '{()}
      }
  }
}
object Test {
  def main(args: Array[String]): Unit = {
    bar("world", 100, true)
  }

  def bar(a1: String, a2: Long, a3: Boolean) = {
    debug
  }
}

@nicolasstucki
Copy link
Contributor

Another approach could be to just not set the trese when expanding white box macros.

liufengyun added a commit that referenced this issue Jan 29, 2020
@liufengyun
Copy link
Contributor Author

Reopen: the original test is not added.

@liufengyun liufengyun reopened this Jan 29, 2020
@nicolasstucki
Copy link
Contributor

@liufengyun the original test is incomplete

@liufengyun
Copy link
Contributor Author

@nicolasstucki Updated.

@LPTK
Copy link
Contributor

LPTK commented Jan 30, 2020

Out of curiosity, what does the current test print? Or does it take the other branch and not print anything?

@nicolasstucki nicolasstucki linked a pull request Jan 14, 2021 that will close this issue
@Kordyjan Kordyjan added this to the 3.0.0 milestone Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants