Fix Def cannot be used inside a taskDyn #3110

The illegalReference check did not actually validate whether the illegal
reference actually referred to an M[_] (which is pretty much always
Initialize[_]]). The means by which this failure was induces were fairly
obscure and go through multiple levels of macro transformations that I
attempt to explain in the comment in IllegalReferenceSpec.

Fixes #3110

build.sbt

@@ -621,6 +621,7 @@ lazy val sbtProj = (project in file("sbt"))
       version,
       // WORKAROUND https://github.com/sbt/sbt-buildinfo/issues/117
       BuildInfoKey.map((fullClasspath in Compile).taskValue) { case (ident, cp) => ident -> cp.files },
+      BuildInfoKey.map((dependencyClasspath in Compile).taskValue) { case (ident, cp) => ident -> cp.files },
       classDirectory in Compile,
       classDirectory in Test,
     ),

core-macros/src/main/scala/sbt/internal/util/appmacro/ContextUtil.scala

@@ -114,12 +114,24 @@ final class ContextUtil[C <: blackbox.Context](val ctx: C) {
     defs
   }
 
+  /**
+   * A reference is illegal if it is to an M instance defined within the scope of the macro call.
+   * As an approximation, disallow referenced to any local definitions `defs`.
+   */
+  def illegalReference(defs: collection.Set[Symbol], sym: Symbol, mType: Type): Boolean =
+    sym != null && sym != NoSymbol && defs.contains(sym) && {
+      sym match {
+        case m: MethodSymbol => m.returnType.erasure <:< mType
+        case _               => sym.typeSignature <:< mType
+      }
+    }
+
   /**
    * A reference is illegal if it is to an M instance defined within the scope of the macro call.
    * As an approximation, disallow referenced to any local definitions `defs`.
    */
   def illegalReference(defs: collection.Set[Symbol], sym: Symbol): Boolean =
-    sym != null && sym != NoSymbol && defs.contains(sym)
+    illegalReference(defs, sym, weakTypeOf[Any])
 
   type PropertyChecker = (String, Type, Tree) => Boolean
 
@@ -127,15 +139,28 @@ final class ContextUtil[C <: blackbox.Context](val ctx: C) {
    * A function that checks the provided tree for illegal references to M instances defined in the
    *  expression passed to the macro and for illegal dereferencing of M instances.
    */
-  def checkReferences(defs: collection.Set[Symbol], isWrapper: PropertyChecker): Tree => Unit = {
+  def checkReferences(
+      defs: collection.Set[Symbol],
+      isWrapper: PropertyChecker,
+      mType: Type
+  ): Tree => Unit = {
     case s @ ApplyTree(TypeApply(Select(_, nme), tpe :: Nil), qual :: Nil) =>
-      if (isWrapper(nme.decodedName.toString, tpe.tpe, qual))
+      if (isWrapper(nme.decodedName.toString, tpe.tpe, qual)) {
         ctx.error(s.pos, DynamicDependencyError)
-    case id @ Ident(name) if illegalReference(defs, id.symbol) =>
+      }
+    case id @ Ident(name) if illegalReference(defs, id.symbol, mType) =>
       ctx.error(id.pos, DynamicReferenceError + ": " + name)
     case _ => ()
   }
 
+  @deprecated("1.3.0", "Use that variant that specifies the M instance types to exclude")
+  /**
+   * A function that checks the provided tree for illegal references to M instances defined in the
+   *  expression passed to the macro and for illegal dereferencing of M instances.
+   */
+  def checkReferences(defs: collection.Set[Symbol], isWrapper: PropertyChecker): Tree => Unit =
+    checkReferences(defs, isWrapper, weakTypeOf[Any])
+
   /** Constructs a ValDef with a parameter modifier, a unique name, with the provided Type and with an empty rhs. */
   def freshMethodParameter(tpe: Type): ValDef =
     ValDef(parameterModifiers, freshTermName("p"), TypeTree(tpe), EmptyTree)

core-macros/src/main/scala/sbt/internal/util/appmacro/Instance.scala

@@ -124,7 +124,7 @@ object Instance {
     // Local definitions `defs` in the macro.  This is used to ensure references are to M instances defined outside of the macro call.
     // Also `refCount` is the number of references, which is used to create the private, synthetic method containing the body
     val defs = util.collectDefs(tree, isWrapper)
-    val checkQual: Tree => Unit = util.checkReferences(defs, isWrapper)
+    val checkQual: Tree => Unit = util.checkReferences(defs, isWrapper, mttpe.erasure)
 
     type In = Input[c.universe.type]
     var inputs = List[In]()

main-settings/src/main/scala/sbt/std/TaskMacro.scala

@@ -493,7 +493,7 @@ object TaskMacro {
       isParserWrapper(n, tpe, tr) || isTaskWrapper(n, tpe, tr)
     val ttree = t.tree
     val defs = util.collectDefs(ttree, isAnyWrapper)
-    val checkQual = util.checkReferences(defs, isAnyWrapper)
+    val checkQual = util.checkReferences(defs, isAnyWrapper, weakTypeOf[Initialize[InputTask[Any]]])
 
     // the Symbol for the anonymous function passed to the appropriate Instance.map/flatMap/pure method
     // this Symbol needs to be known up front so that it can be used as the owner of synthetic vals

sbt/src/test/scala/sbt/IllegalReferenceSpec.scala

@@ -0,0 +1,90 @@
+/*
+ * sbt
+ * Copyright 2011 - 2018, Lightbend, Inc.
+ * Copyright 2008 - 2010, Mark Harrah
+ * Licensed under Apache License 2.0 (see LICENSE)
+ */
+
+package sbt
+
+import org.scalatest.FunSuite
+
+import scala.tools.reflect.{ FrontEnd, ToolBoxError }
+
+class IllegalReferenceSpec extends FunSuite {
+  lazy val toolboxClasspath: String = {
+    val mainClassesDir = buildinfo.TestBuildInfo.classDirectory
+    val testClassesDir = buildinfo.TestBuildInfo.test_classDirectory
+    val depsClasspath = buildinfo.TestBuildInfo.dependencyClasspath
+    mainClassesDir +: testClassesDir +: depsClasspath mkString java.io.File.pathSeparator
+  }
+  def eval(code: String, compileOptions: String = ""): Any = {
+    val m = scala.reflect.runtime.currentMirror
+    import scala.tools.reflect.ToolBox
+    val tb = m.mkToolBox(options = compileOptions)
+    tb.eval(tb.parse(code))
+  }
+  private def expectError(
+      errorSnippet: String,
+      compileOptions: String = "",
+      baseCompileOptions: String = s"-cp $toolboxClasspath",
+  )(code: String) = {
+    val errorMessage = intercept[ToolBoxError] {
+      eval(code, s"$compileOptions $baseCompileOptions")
+      println(s"Test failed -- compilation was successful! Expected:\n$errorSnippet")
+    }.getMessage
+    val userMessage =
+      s"""
+         |FOUND: $errorMessage
+         |EXPECTED: $errorSnippet
+      """.stripMargin
+    assert(errorMessage.contains(errorSnippet), userMessage)
+  }
+  private class CachingToolbox {
+    private[this] val m = scala.reflect.runtime.currentMirror
+    private[this] var _infos: List[FrontEnd#Info] = Nil
+    private[this] val frontEnd = new FrontEnd {
+      override def display(info: Info): Unit = _infos ::= info
+      override def interactive(): Unit = {}
+    }
+
+    import scala.tools.reflect.ToolBox
+    val toolbox = m.mkToolBox(frontEnd, options = s"-cp $toolboxClasspath")
+    def eval(code: String): Any = toolbox.eval(toolbox.parse(code))
+    def infos: List[FrontEnd#Info] = _infos
+  }
+
+  test("Def.sequential should be legal within Def.taskDyn") {
+    val toolbox = new CachingToolbox
+    // This example was taken from @dos65 in https://github.com/sbt/sbt/issues/3110
+    val build =
+      s"""
+         |import sbt._
+         |import Keys._
+         |
+         |Def.taskDyn[Int] {
+         |  // Calling baseDirectory.value will cause the task macros to add a reference to
+         |  // `Def.toITask(sbt.Keys.baseDirectory)`. This, in turn, causes `Def` to be added
+         |  // to a list of local definitions. Later on, we dereference `Def` with
+         |  // `Def.sequential` which used to erroneously cause an illegal dynamic reference.
+         |  baseDirectory.value
+         |  Def.sequential(Def.task(42))
+         |}.dependencies.headOption.map(_.key.label)
+       """.stripMargin
+    assert(toolbox.eval(build) == Some("baseDirectory"))
+    assert(toolbox.infos.isEmpty)
+  }
+  test("Local task defs should be illegal within Def.task") {
+    val build =
+      s"""
+         |import sbt._
+         |import Keys._
+         |
+         |Def.task[Int] {
+         |  def foo = Def.task(5)
+         |  foo.value
+         |}.dependencies.headOption.map(_.key.label)
+       """.stripMargin
+    expectError("Illegal dynamic reference: foo")(build)
+  }
+}