Skip to content

TASTY: PackageDef#unapply is unsafe #8521

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
pshirshov opened this issue Mar 11, 2020 · 4 comments
Closed

TASTY: PackageDef#unapply is unsafe #8521

pshirshov opened this issue Mar 11, 2020 · 4 comments
Assignees
Labels

Comments

@pshirshov
Copy link
Contributor

pshirshov commented Mar 11, 2020

The following code would fail:

  private def packageToName(tree: Tree): List[String] = {
    tree match {
      case PackageDef(name, owner) =>
        packageToName(owner) ++ List(name)
      case o =>
        List.empty
    }
  }

with exception like

Error:(19, 33) Exception occurred while executing macro expansion.
java.lang.AssertionError: NoDenotation.owner
	at dotty.tools.dotc.core.SymDenotations$NoDenotation$.owner(SymDenotations.scala:2253)
	at dotty.tools.dotc.tastyreflect.ReflectionCompilerInterface.PackageDef_owner(ReflectionCompilerInterface.scala:177)
	at dotty.tools.dotc.tastyreflect.ReflectionCompilerInterface.PackageDef_owner(ReflectionCompilerInterface.scala:177)
	at scala.tasty.Reflection$PackageDefOps$.owner(Reflection.scala:615)
	at scala.tasty.Reflection$PackageDef$.unapply(Reflection.scala:621)
	at izumi.reflect.dottyreflection.Inspector.packageToName(Inspector.scala:147)
	at izumi.reflect.dottyreflection.Inspector.packageToName(Inspector.scala:148)
	at izumi.reflect.dottyreflection.Inspector.packageToName(Inspector.scala:148)
	at izumi.reflect.dottyreflection.Inspector.inspectSymbol(Inspector.scala:135)
	at izumi.reflect.dottyreflection.Inspector.prefixOf(Inspector.scala:158)
	at izumi.reflect.dottyreflection.Inspector.inspectSymbol(Inspector.scala:127)
	at izumi.reflect.dottyreflection.Inspector.inspectTree(Inspector.scala:119)
	at izumi.reflect.dottyreflection.Inspector.inspectSymbol(Inspector.scala:133)
	at izumi.reflect.dottyreflection.Inspector.prefixOf(Inspector.scala:158)
	at izumi.reflect.dottyreflection.Inspector.inspectSymbol(Inspector.scala:127)
	at izumi.reflect.dottyreflection.Inspector.inspectTree(Inspector.scala:119)
	at izumi.reflect.dottyreflection.Inspector.buildTypeRef(Inspector.scala:33)
	at izumi.reflect.dottyreflection.TypeInspections$.apply(TypeInspections.scala:10)
	at izumi.reflect.dottyreflection.Inspect$.inspectAny(LightTypeTagMaterializer.scala:21)
    val bazTag = Inspect.inspect[Baz]

Seems like it fails on root package which has no owner.

@nicolasstucki
Copy link
Contributor

@pshirshov could you provide a self-contained code to reproduce this issue.

@griggt
Copy link
Contributor

griggt commented Sep 28, 2020

I was able to create a standalone reproducer for this issue:

// Main.scala
import scala.quoted._

object Foo {
  inline def foo[T <: AnyKind]: String = ${ bar[T] }

  def bar[T <: AnyKind : Type](using qctx: QuoteContext): Expr[String] = {
    import qctx.tasty.{Type => _, given _, _}

    def packageToName(tree: Tree): Unit = tree match {
      case PackageDef(_, owner) =>
        packageToName(owner)
    }

    val sym = implicitly[Type[T]].unseal.symbol
    if (!sym.isNoSymbol) {
      sym.tree match {
        case c: ClassDef =>
          if (!sym.maybeOwner.isNoSymbol) {
            sym.maybeOwner.tree match {
              case _: PackageDef =>
                packageToName(sym.maybeOwner.tree)
            }
          }
      }
    }

    ???
  }
}
// Test.scala
class A

object Test {
  def test = Foo.foo[A]
}
[info] Compiling 2 Scala sources to /src/dotty-issues/i8521/standalone/target/scala-0.22/classes ...
[error] -- Error: /src/dotty-issues/i8521/standalone/src/main/scala/Test.scala:5:20 ----
[error] 5 |  def test = Foo.foo[A]
[error]   |             ^^^^^^^^^^
[error]   |Exception occurred while executing macro expansion.
[error]   |java.lang.AssertionError: NoDenotation.owner
[error]   |     at dotty.tools.dotc.core.SymDenotations$NoDenotation$.owner(SymDenotations.scala:2253)
[error]   |     at dotty.tools.dotc.tastyreflect.ReflectionCompilerInterface.PackageDef_owner(ReflectionCompilerInterface.scala:177)
[error]   |     at dotty.tools.dotc.tastyreflect.ReflectionCompilerInterface.PackageDef_owner(ReflectionCompilerInterface.scala:177)
[error]   |     at scala.tasty.Reflection$PackageDefOps$.owner(Reflection.scala:615)
[error]   |     at scala.tasty.Reflection$PackageDef$.unapply(Reflection.scala:621)
[error]   |     at Foo$.packageToName$1(Main.scala:11)
[error]   |     at Foo$.bar(Main.scala:22)
[error]   |
[error]   | This location contains code that was inlined from Test.scala:5
[error] one error found

The original bug appears to have been filed against 0.22.0-RC1. It still crashes in 0.28.0-bin-20200927-d084b8b-NIGHTLY:

[info] Compiling 2 Scala sources to /src/dotty-issues/i8521/standalone/target/scala-0.28/classes ...
[error] -- Error: /src/dotty-issues/i8521/standalone/src/main/scala/Test.scala:5:20 ----
[error] 5 |  def test = Foo.foo[A]
[error]   |             ^^^^^^^^^^
[error]   |Exception occurred while executing macro expansion.
[error]   |java.lang.AssertionError: NoDenotation.owner
[error]   |     at dotty.tools.dotc.core.SymDenotations$NoDenotation$.owner(SymDenotations.scala:2326)
[error]   |     at dotty.tools.dotc.quoted.QuoteContextImpl$tasty$SymbolMethodsImpl$.extension_owner(QuoteContextImpl.scala:2214)
[error]   |     at dotty.tools.dotc.quoted.QuoteContextImpl$tasty$SymbolMethodsImpl$.extension_owner(QuoteContextImpl.scala:2214)
[error]   |     at dotty.tools.dotc.quoted.QuoteContextImpl$tasty$PackageDefMethodsImpl$.extension_owner(QuoteContextImpl.scala:303)
[error]   |     at dotty.tools.dotc.quoted.QuoteContextImpl$tasty$PackageDefMethodsImpl$.extension_owner(QuoteContextImpl.scala:303)
[error]   |     at dotty.tools.dotc.quoted.QuoteContextImpl$tasty$PackageDef$.unapply(QuoteContextImpl.scala:298)
[error]   |     at dotty.tools.dotc.quoted.QuoteContextImpl$tasty$PackageDef$.unapply(QuoteContextImpl.scala:297)
[error]   |     at Foo$.packageToName$1(Main.scala:11)
[error]   |     at Foo$.bar(Main.scala:22)
[error]   |
[error]   | This location contains code that was inlined from Test.scala:5
[error] one error found

@nicolasstucki
Copy link
Contributor

I see it now. We are trying to access the owner of the <empty package> or _root_ package which do not exist.

PackageDef is never a useful tree (PackageClause is the useful one), it was added to wrap around a package symbol to provide a package symbol like thing back when we tried to not have symbols. Now we can do all these operations on Symbol directly. It seems that the sensible thing to do is to remove the methods of PacakgeDef which basically leaves it a tree that does only contains a symbol reference. If we have that we should go all the way and just remove PackageDef . We would also need to add def members on Symbol.

@nicolasstucki
Copy link
Contributor

The solution for the original code is to use the Symbol of the package instead and use maybeOwner

nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Sep 29, 2020
PackageDef was originally added to try to encode trees in the API without the use of symbols. Now that we have symbols all the package def logic is redundant.
At the time we assumed that we would be able to have a tree for all Symbol, but this has not been the case since the introduction of NoSymbol.
Now we handle the `tree` of a package def the same way we did the one of a NoSymbol.

* Remove `PackageDef`
* Add `members` method on `Symbol`
* Disallow package symbols in `Symbol.tree` as done with `NoSymbol`
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Sep 29, 2020
PackageDef was originally added to try to encode trees in the API without the use of symbols. Now that we have symbols all the package def logic is redundant.
At the time we assumed that we would be able to have a tree for all Symbol, but this has not been the case since the introduction of NoSymbol.
Now we handle the `tree` of a package def the same way we did the one of a NoSymbol.

* Remove `PackageDef`
* Add `members` method on `Symbol`
* Disallow package symbols in `Symbol.tree` as done with `NoSymbol`
nicolasstucki added a commit that referenced this issue Sep 29, 2020
Fix #8521: Remove PackageDef and use Symbol directly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants