Merge pull request #4083 from retronym/ticket/8947

SI-8947 Avoid cross talk between tag materializers and reify

6 files changed, 64 insertions, 3 deletions

diff --git a/src/compiler/scala/reflect/reify/Taggers.scala b/src/compiler/scala/reflect/reify/Taggers.scala
index 093c2bee22..0863ee38f9 100644
--- a/src/compiler/scala/reflect/reify/Taggers.scala
+++ b/src/compiler/scala/reflect/reify/Taggers.scala
@@ -79,8 +79,7 @@ abstract class Taggers {
     try materializer
     catch {
       case ReificationException(pos, msg) =>
-        c.error(pos.asInstanceOf[c.Position], msg) // this cast is a very small price for the sanity of exception handling
-        EmptyTree
+        c.abort(pos.asInstanceOf[c.Position], msg) // this cast is a very small price for the sanity of exception handling
       case UnexpectedReificationException(pos, err, cause) if cause != null =>
         throw cause
     }
diff --git a/src/compiler/scala/tools/nsc/typechecker/StdAttachments.scala b/src/compiler/scala/tools/nsc/typechecker/StdAttachments.scala
index 57f27a05fd..ea44b9dc39 100644
--- a/src/compiler/scala/tools/nsc/typechecker/StdAttachments.scala
+++ b/src/compiler/scala/tools/nsc/typechecker/StdAttachments.scala
@@ -61,7 +61,7 @@ trait StdAttachments {
     val metadata = MacroExpansionAttachment(expandee, expanded)
     expandee updateAttachment metadata
     expanded match {
-      case expanded: Tree => expanded updateAttachment metadata
+      case expanded: Tree if !expanded.isEmpty => expanded updateAttachment metadata
       case _ => // do nothing
     }
   }
diff --git a/src/reflect/scala/reflect/internal/Trees.scala b/src/reflect/scala/reflect/internal/Trees.scala
index 2f07cef315..35de3adff6 100644
--- a/src/reflect/scala/reflect/internal/Trees.scala
+++ b/src/reflect/scala/reflect/internal/Trees.scala
@@ -9,6 +9,7 @@ package internal
 
 import Flags._
 import scala.collection.{ mutable, immutable }
+import scala.reflect.macros.Attachments
 import util.Statistics
 
 trait Trees extends api.Trees {
@@ -1074,6 +1075,13 @@ trait Trees extends api.Trees {
     override def setType(t: Type) = { requireLegal(t, NoType, "tpe"); this }
     override def tpe_=(t: Type) = setType(t)
 
+    // We silently ignore attempts to add attachments to `EmptyTree`. See SI-8947 for an
+    // example of a bug in macro expansion that this solves.
+    override def setAttachments(attachments: Attachments {type Pos = Position}): this.type = attachmentWarning()
+    override def updateAttachment[T: ClassTag](attachment: T): this.type = attachmentWarning()
+    override def removeAttachment[T: ClassTag]: this.type = attachmentWarning()
+    private def attachmentWarning(): this.type = {devWarning(s"Attempt to mutate attachments on $self ignored"); this}
+
     private def requireLegal(value: Any, allowed: Any, what: String) = (
       if (value != allowed) {
         log(s"can't set $what for $self to value other than $allowed")
diff --git a/test/files/pos/t8947/Client_2.scala b/test/files/pos/t8947/Client_2.scala
new file mode 100644
index 0000000000..1a5082a2f9
--- /dev/null
+++ b/test/files/pos/t8947/Client_2.scala
@@ -0,0 +1 @@
+object Test { X.extractor }
\ No newline at end of file
diff --git a/test/files/pos/t8947/Macro_1.scala b/test/files/pos/t8947/Macro_1.scala
new file mode 100644
index 0000000000..4a5de3decb
--- /dev/null
+++ b/test/files/pos/t8947/Macro_1.scala
@@ -0,0 +1,41 @@
+import language.experimental.macros
+import scala.reflect.macros._
+import blackbox.Context
+
+object X {
+
+  def classTagOrNull[T](implicit t: reflect.ClassTag[T] = null) = t
+  // the failed search for ClassTag[T] does not issue a visible
+  // error as we fall back to the default argument. But, the
+  // macro engine things we have expanded the macro `materializeClassTag[D]()`
+  // to `EmptyTree`, and then attaches a backreference from the expansion
+  // to the expandee. This is the `MacroExpansionAttachment` tree attachment.
+  def foo[D] = classTagOrNull[D]
+
+  def extractor: Any = macro X.extractorMacro
+  def extractorMacro(c: Context): c.Expr[Any] = {
+    // Later, in reify, an unrelated use of `EmptyTree` in the AST representing
+    // the argument is now treated as a macro expansion which should be rolled
+    // back in the tree we reify! This ends up generating a call to `implicitly`
+    // which leads to an ambiguous error.
+    //
+    // Any macro call that expands to EmptyTree could have triggered this problem.
+    c.universe.reify(new { def something(data: Any) = ??? })
+  }
+
+  // Workarounds:
+  //
+  // 1. Use quasiquotes rather than `reify`. (But, beware to fully qualify all references, e.g. `_root_.scala.Predef.???`)
+  // 2. Avoid failed ClassTag lookups (e.g. in the original bug report, annotate the type argument to `map`)
+  // 3. In the macro implementation, just before calling the `reify` macro, you could call another macro
+  //
+  //      def prepareReify = macro prepareReifyImpl
+  //      def prepareReifyImpl(c: Context) = {
+  //        val symtab = c.universe.asInstanceOf[reflect.internal.SymbolTable]
+  //        symtab.EmptyTree.setAttachments(symtab.NoPosition)
+  //      }
+  //
+  //   To make this visible to the macro implementaiton, it will need to be compiled in an earlier stage,
+  //   e.g a separate SBT sub-project.
+
+}
diff --git a/test/junit/scala/tools/nsc/symtab/CannotHaveAttrsTest.scala b/test/junit/scala/tools/nsc/symtab/CannotHaveAttrsTest.scala
index d424f12710..69931c9e24 100644
--- a/test/junit/scala/tools/nsc/symtab/CannotHaveAttrsTest.scala
+++ b/test/junit/scala/tools/nsc/symtab/CannotHaveAttrsTest.scala
@@ -64,4 +64,16 @@ class CannotHaveAttrsTest {
       assertThrows[IllegalArgumentException] { t.setType(tpe) }
     }
   }
+
+  class Attach
+  @Test
+  def attachmentsAreIgnored = {
+    attrlessTrees.foreach { t =>
+      t.setAttachments(NoPosition.update(new Attach))
+      assert(t.attachments == NoPosition)
+      t.updateAttachment(new Attach)
+      assert(t.attachments == NoPosition)
+      t.removeAttachment[Attach] // no exception
+    }
+  }
 }