From 01b2e86a544038bfaaec403283d04e2d51b7d9b5 Mon Sep 17 00:00:00 2001 From: Ethan Atkins Date: Tue, 11 Dec 2018 16:24:30 -0800 Subject: [PATCH] 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 | 1 + .../internal/util/appmacro/ContextUtil.scala | 33 ++++++- .../sbt/internal/util/appmacro/Instance.scala | 2 +- .../src/main/scala/sbt/std/TaskMacro.scala | 2 +- .../test/scala/sbt/IllegalReferenceSpec.scala | 90 +++++++++++++++++++ 5 files changed, 122 insertions(+), 6 deletions(-) create mode 100644 sbt/src/test/scala/sbt/IllegalReferenceSpec.scala diff --git a/build.sbt b/build.sbt index 5f3ee5142..50b5f97ec 100644 --- a/build.sbt +++ b/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, ), diff --git a/core-macros/src/main/scala/sbt/internal/util/appmacro/ContextUtil.scala b/core-macros/src/main/scala/sbt/internal/util/appmacro/ContextUtil.scala index 1f92c8824..e40cdeac9 100644 --- a/core-macros/src/main/scala/sbt/internal/util/appmacro/ContextUtil.scala +++ b/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) diff --git a/core-macros/src/main/scala/sbt/internal/util/appmacro/Instance.scala b/core-macros/src/main/scala/sbt/internal/util/appmacro/Instance.scala index 99e8b2ab7..3f5066d66 100644 --- a/core-macros/src/main/scala/sbt/internal/util/appmacro/Instance.scala +++ b/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]() diff --git a/main-settings/src/main/scala/sbt/std/TaskMacro.scala b/main-settings/src/main/scala/sbt/std/TaskMacro.scala index bb5cc46fc..650914c7a 100644 --- a/main-settings/src/main/scala/sbt/std/TaskMacro.scala +++ b/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 diff --git a/sbt/src/test/scala/sbt/IllegalReferenceSpec.scala b/sbt/src/test/scala/sbt/IllegalReferenceSpec.scala new file mode 100644 index 000000000..b4a0c17d2 --- /dev/null +++ b/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) + } +}