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

resetLocalAttrs does not work with {existential types, unapplySeq, local case classes} #8500

Open
scabug opened this issue Apr 14, 2014 · 6 comments

Comments

@scabug
Copy link

commented Apr 14, 2014

Welcome to Scala version 2.10.3 (Java HotSpot(TM) 64-Bit Server VM, Java 1.7.0_45).
Type in expressions to have them evaluated.
Type :help for more information.

scala> object Mover {
     |   import scala.reflect.macros.Context
     |   def moveTreeMacro(c: Context)(e: c.Expr[Any]): c.Expr[Any] = {
     |     import c._
     |     import c.universe._
     |     val ee = c.Expr(c.resetLocalAttrs(e.tree))
     |     reify {
     |       new AnyRef {
     |         def moved = {
     |           ee.splice
     |         }
     |       }
     |     }
     |   }
     |   import scala.language.experimental.macros
     |   def moveTree(e: Any): Any = macro moveTreeMacro
     | }
defined module Mover

scala> trait Container[+A]
defined trait Container

scala> case class ContainerImpl[A](value: A) extends Container[A]
defined class ContainerImpl

scala> Mover.moveTree {
     |   val buf: Seq[_] = null
     |   val c = ContainerImpl(buf(0))
     | }
<console>:14: error: type mismatch;
 found   : (some other)_$1(in value buf)
 required: _$1(in value buf)
                val c = ContainerImpl(buf(0))
                                         ^
@scabug

This comment has been minimized.

Copy link
Author

commented Apr 14, 2014

Imported From: https://issues.scala-lang.org/browse/SI-8500?orig=1
Reporter: @Atry
Affected Versions: 2.10.3, 2.10.4
See #5464

@scabug

This comment has been minimized.

Copy link
Author

commented Apr 14, 2014

@scabug

This comment has been minimized.

Copy link
Author

commented Apr 14, 2014

@Atry said (edited on Apr 15, 2014 1:39:47 AM UTC):
Reply to your comments under the gist:

I considered that the public API in scala-reflect are too complex and too dangerous. The public API exposes too many details in compilers. The right way to resolve these problem is to reduce the API, instead of providing more and more scala.reflect.internal hacks.

For example, there are two types of macros, untyped macro and typed macro. The typed macro accepts typed trees while the untyped macro accepts untyped tree. But what about the tree the macros produce? Should the output trees be typed or untyped?
In my opinion, I prefer to remove resetLocalAttrs in the public API. Instead, all trees return from macro must be treated as untyped, and the resetLocalAttrs should be invoked internally by the compiler after every macro calls.

Furthermore, the public scala-reflect API should be read-only. A macro writer should never setType or setSymbol.
I think what a macro does is simulating a man typing source code. A man can type expression trees, but he cannot type symbols or types directly, so the macro should only generate plain expression trees, too.

@scabug

This comment has been minimized.

Copy link
Author

commented Apr 15, 2014

@Atry said (edited on Apr 17, 2014 10:22:35 AM UTC):
I wonder why the compiler re-type-check every tree returned from macros, while the compiler does not clear the old types before re-type-check. What does the compiler expect from macros?
The mixed untyped and typed trees returned from macros are very dangerous, cause compiler crash nonsensically everywhere.

@scabug

This comment has been minimized.

Copy link
Author

commented Apr 15, 2014

@xeno-by said:
You're right. Current reflection API is too tightly coupled with compiler internals, which in certain situations makes it hard to understand (because compiler is huge) and hard to get right (because certain compiler internals are quite messed up).

That's why, as Jason has mentioned in gist comments, we're working on Project Palladium (http://scalareflect.org/), which will provide a metaprogramming foundation independent of scalac with a focus on usability. In Palladium there are no separate notions of typed or untyped trees - they are all just trees and Palladium's macro engine figures out the rest.

In the meanwhile, as again Jason has mentioned, the only robust way of dealing with the problem of typed vs untyped trees is the hardcore way of creating symbols and assigning types, i.e. manually doing the work of the typechecker. In 2.10, this requires casts from the public API to compiler internals (scala.reflect.internal.SymbolTable). In 2.11, there's a new c.internal API that exposes required idioms as part of the public API.

As for the critique of scala.reflect.internal hacks leaking into the public API, consider that the automatic resetLocalAttrs/typecheck approach (which is exactly what we planned: https://groups.google.com/forum/#!msg/scala-internals/rIyJ4yHdPDU/qS-7DreEbCwJ) is very hard to implement, because typer was never designed with resetAttrs in mind. You've probably already figured this out yourself while submitting the flurry of "resetLocalAttrs doesn't work for this and for that" bugs.

Therefore in December 2013, which is when we got to discussing this topic, I had just two options: 1) rush for an automatic untypecheck fix, which given the complexity of the task would provide no guarantees that I would ever make it in time for 2.11.0 code freeze or that I would be able to ensure no regressions in existing macros, 2) codify the hacks in the public API. Option #2 is surely less sexy, but it definitely scales better - it immediately stops people from casting to SymbolTable, which is useful in many ways beyond just the typed/untyped problem + it doesn't prevent us from pursuing and delivering the fix over the course of 2.11.x.

Hope this provides some perspective and shows that we do take the issues with the reflection API as seriously as possible within the limits of our resources and deadlines.

@SethTisue

This comment has been minimized.

Copy link
Member

commented Mar 3, 2018

consolidating here:

  • #8825 (same, but for unapplySeq)
  • #8505 (same, but for local case classes)
@SethTisue SethTisue changed the title resetLocalAttrs does not work with existential types resetLocalAttrs does not work with {existential types, unapplySeq, local case classes} Mar 3, 2018
@SethTisue SethTisue added this to the Backlog milestone Mar 3, 2018
@SethTisue SethTisue removed the existential label Mar 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.