diff --git a/main-settings/src/main/scala/sbt/std/TaskLinterDSL.scala b/main-settings/src/main/scala/sbt/std/TaskLinterDSL.scala index 6798d8144..69cc100e3 100644 --- a/main-settings/src/main/scala/sbt/std/TaskLinterDSL.scala +++ b/main-settings/src/main/scala/sbt/std/TaskLinterDSL.scala @@ -5,7 +5,6 @@ import sbt.internal.util.appmacro.{ Convert, Converted, LinterDSL } import scala.collection.mutable.{ HashSet => MutableSet } import scala.io.AnsiColor -import scala.reflect.internal.util.Position import scala.reflect.macros.blackbox abstract class BaseTaskLinterDSL extends LinterDSL { @@ -17,9 +16,13 @@ abstract class BaseTaskLinterDSL extends LinterDSL { val isTask = convert.asPredicate(ctx) class traverser extends Traverser { private val unchecked = symbolOf[sbt.sbtUnchecked].asClass + private val taskKeyType = typeOf[sbt.TaskKey[_]] + private val settingKeyType = typeOf[sbt.SettingKey[_]] + private val inputKeyType = typeOf[sbt.InputKey[_]] private val uncheckedWrappers = MutableSet.empty[Tree] var insideIf: Boolean = false var insideAnon: Boolean = false + var disableNoValueReport: Boolean = false def handleUncheckedAnnotation(exprAtUseSite: Tree, tt: TypeTree): Unit = { tt.original match { @@ -44,42 +47,75 @@ abstract class BaseTaskLinterDSL extends LinterDSL { } } + @inline def isKey(tpe: Type): Boolean = + tpe <:< taskKeyType || tpe <:< settingKeyType || tpe <:< inputKeyType + + def detectAndErrorOnKeyMissingValue(i: Ident): Unit = { + if (isKey(i.tpe)) { + val keyName = i.name.decodedName.toString + ctx.error(i.pos, TaskLinterDSLFeedback.missingValueForKey(keyName)) + } else () + } + override def traverse(tree: ctx.universe.Tree): Unit = { tree match { - case s @ Apply(TypeApply(Select(selectQual, nme), tpe :: Nil), qual :: Nil) => - val shouldIgnore = uncheckedWrappers.contains(s) + case ap @ Apply(TypeApply(Select(_, nme), tpe :: Nil), qual :: Nil) => + val shouldIgnore = uncheckedWrappers.contains(ap) val wrapperName = nme.decodedName.toString if (!shouldIgnore && isTask(wrapperName, tpe.tpe, qual)) { val qualName = if (qual.symbol != null) qual.symbol.name.decodedName.toString - else s.pos.lineContent + else ap.pos.lineContent if (insideIf && !isDynamicTask) { // Error on the use of value inside the if of a regular task (dyn task is ok) - ctx.error(s.pos, TaskLinterDSLFeedback.useOfValueInsideIfExpression(qualName)) + ctx.error(ap.pos, TaskLinterDSLFeedback.useOfValueInsideIfExpression(qualName)) } if (insideAnon) { // Error on the use of anonymous functions in any task or dynamic task - ctx.error(s.pos, TaskLinterDSLFeedback.useOfValueInsideAnon(qualName)) + ctx.error(ap.pos, TaskLinterDSLFeedback.useOfValueInsideAnon(qualName)) } - } else traverse(selectQual) + } traverse(qual) case If(condition, thenp, elsep) => traverse(condition) + val previousInsideIf = insideIf insideIf = true traverse(thenp) traverse(elsep) - insideIf = false + insideIf = previousInsideIf case Typed(expr, tpt: TypeTree) if tpt.original != null => handleUncheckedAnnotation(expr, tpt) traverse(expr) traverse(tpt) - case f @ Function(vparams, body) => - super.traverseTrees(vparams) + case Function(vparams, body) => + traverseTrees(vparams) if (!vparams.exists(_.mods.hasFlag(Flag.SYNTHETIC))) { + val previousInsideAnon = insideAnon insideAnon = true traverse(body) - insideAnon = false + insideAnon = previousInsideAnon } else traverse(body) + case Block(stmts, expr) => + if (!isDynamicTask) { + /* The missing .value analysis is dumb on purpose because it's expensive. + * Detecting valid use cases of idents whose type is an sbt key is difficult + * and dangerous because we may miss some corner cases. Instead, we report + * on the easiest cases in which we are certain that the user does not want + * to have a stale key reference. Those are idents in the rhs of a val definition + * whose name is `_` and those idents that are in statement position inside blocks. */ + stmts.foreach { + // TODO: Consider using unused names analysis to be able to report on more cases + case ValDef(_, valName, _, rhs) if valName == termNames.WILDCARD => + rhs match { + case i: Ident => detectAndErrorOnKeyMissingValue(i) + case _ => () + } + case i: Ident => detectAndErrorOnKeyMissingValue(i) + case _ => () + } + } + traverseTrees(stmts) + traverse(expr) case _ => super.traverse(tree) } } @@ -138,69 +174,11 @@ object TaskLinterDSLFeedback { | 2. Otherwise, make the static evaluation explicit by evaluating `$task` outside the if expression. """.stripMargin - /* If( - Ident(TermName("condition")), - Typed( - Typed(Apply(TypeApply(Select(Ident(sbt.std.InputWrapper), - TermName("wrapInitTask_$u2603$u2603")), - List(TypeTree())), - List(Ident(TermName("foo")))), - TypeTree()), - TypeTree().setOriginal( - Annotated( - Apply(Select(New(Ident(TypeName("unchecked"))), termNames.CONSTRUCTOR), List()), - Typed(Apply(TypeApply(Select(Ident(sbt.std.InputWrapper), - TermName("wrapInitTask_$u2603$u2603")), - List(TypeTree())), - List(Ident(TermName("foo")))), - TypeTree()) - )) - ), - Typed( - Typed(Apply(TypeApply(Select(Ident(sbt.std.InputWrapper), - TermName("wrapInitTask_$u2603$u2603")), - List(TypeTree())), - List(Ident(TermName("bar")))), - TypeTree()), - TypeTree().setOriginal( - Annotated( - Apply(Select(New(Ident(TypeName("unchecked"))), termNames.CONSTRUCTOR), List()), - Typed(Apply(TypeApply(Select(Ident(sbt.std.InputWrapper), - TermName("wrapInitTask_$u2603$u2603")), - List(TypeTree())), - List(Ident(TermName("bar")))), - TypeTree()) - )) - ) - )*/ - - /* Block( - List( - ValDef( - Modifiers(), - TermName("anon"), - TypeTree(), - Function( - List(), - Apply( - Select( - Typed( - Apply(TypeApply(Select(Ident(sbt.std.InputWrapper), - TermName("wrapInitTask_$u2603$u2603")), - List(TypeTree())), - List(Ident(TermName("fooNeg")))), - TypeTree() - ), - TermName("$plus") - ), - List(Literal(Constant(""))) - ) - ) - )), - If( - Ident(TermName("condition")), - Apply(Select(Ident(TermName("anon")), TermName("apply")), List()), - Apply(Select(Ident(TermName("anon")), TermName("apply")), List()) - ) - )*/ + def missingValueForKey(key: String) = + s"""${startBold}The key `$key` is not being invoked inside the task definition.$reset + | + |${ProblemHeader}: Keys missing `.value` are not initialized and their dependency is not registered. + | + |${SolutionHeader}: Replace `$key` by `$key.value` or remove it if unused. + """.stripMargin } diff --git a/main-settings/src/test/scala/sbt/std/TaskPosSpec.scala b/main-settings/src/test/scala/sbt/std/TaskPosSpec.scala index 3cd8eed74..17bc85a9f 100644 --- a/main-settings/src/test/scala/sbt/std/TaskPosSpec.scala +++ b/main-settings/src/test/scala/sbt/std/TaskPosSpec.scala @@ -27,6 +27,30 @@ class TaskPosSpec { } } + locally { + import sbt._ + import sbt.Def._ + val foo = taskKey[String]("") + var condition = true + val baz = Def.task[String] { + val fooAnon = () => foo.value: @sbtUnchecked + if (condition) fooAnon() + else fooAnon() + } + } + + locally { + import sbt._ + import sbt.Def._ + val foo = taskKey[String]("") + var condition = true + val baz = Def.task[String] { + val fooAnon = () => (foo.value: @sbtUnchecked) + "" + if (condition) fooAnon() + else fooAnon() + } + } + locally { import sbt._ import sbt.Def._ @@ -66,4 +90,64 @@ class TaskPosSpec { } else Def.task("") } } + + locally { + // missing .value error should not happen inside task dyn + import sbt._ + import sbt.Def._ + val foo = taskKey[String]("") + val baz = Def.taskDyn[String] { + foo + } + } + + locally { + // missing .value error should not happen inside task dyn + import sbt._ + import sbt.Def._ + val foo = taskKey[String]("") + val avoidDCE = "" + val baz = Def.task[String] { + foo: @sbtUnchecked + avoidDCE + } + } + + locally { + import sbt._ + import sbt.Def._ + val foo = taskKey[String]("") + val baz = Def.task[String] { + def inner(s: KeyedInitialize[_]) = println(s) + inner(foo) + "" + } + } + + locally { + // In theory, this should be reported, but missing .value analysis is dumb at the cost of speed + import sbt._ + import sbt.Def._ + val foo = taskKey[String]("") + def avoidDCE = { println(""); "" } + val baz = Def.task[String] { + val (_, _) = "" match { + case _ => (foo, 1 + 2) + } + avoidDCE + } + } + + locally { + import sbt._ + import sbt.Def._ + val foo = taskKey[String]("") + def avoidDCE = { println(""); "" } + val baz = Def.task[String] { + val hehe = foo + // We do not detect `hehe` because guessing that the user did the wrong thing would require + // us to run the unused name traverser defined in Typer (and hence proxy it from context util) + avoidDCE + } + } } diff --git a/main-settings/src/test/scala/sbt/std/neg/TaskNegSpec.scala b/main-settings/src/test/scala/sbt/std/neg/TaskNegSpec.scala index d63d8cb5b..ff9447502 100644 --- a/main-settings/src/test/scala/sbt/std/neg/TaskNegSpec.scala +++ b/main-settings/src/test/scala/sbt/std/neg/TaskNegSpec.scala @@ -7,19 +7,17 @@ import sbt.std.TestUtil._ class TaskNegSpec extends FunSuite { import tools.reflect.ToolBoxError def expectError(errorSnippet: String, - compileOptions: String = "-Xfatal-warnings", + compileOptions: String = "", baseCompileOptions: String = s"-cp $toolboxClasspath")(code: String) = { val errorMessage = intercept[ToolBoxError] { eval(code, s"$compileOptions $baseCompileOptions") - println("SUCCESS") + println(s"Test failed -- compilation was successful! Expected:\n$errorSnippet") }.getMessage - println(errorMessage) val userMessage = s""" |FOUND: $errorMessage |EXPECTED: $errorSnippet """.stripMargin - println(userMessage) assert(errorMessage.contains(errorSnippet), userMessage) } @@ -64,7 +62,7 @@ class TaskNegSpec extends FunSuite { } } - test("Fail on task invocation inside inside `if` of task returned by dynamic task") { + test("Fail on task invocation inside `if` of task returned by dynamic task") { expectError(TaskLinterDSLFeedback.useOfValueInsideIfExpression("fooNeg")) { """ |import sbt._ @@ -87,6 +85,37 @@ class TaskNegSpec extends FunSuite { } } + test("Fail on task invocation inside nested `if` of task returned by dynamic task") { + val fooNegCatch = TaskLinterDSLFeedback.useOfValueInsideIfExpression("fooNeg") + val barNegCatch = TaskLinterDSLFeedback.useOfValueInsideIfExpression("barNeg") + expectError(List(fooNegCatch, barNegCatch).mkString("\n")) { + """ + |import sbt._ + |import sbt.Def._ + | + |val fooNeg = taskKey[String]("") + |val barNeg = taskKey[String]("") + |var condition = true + | + |val bazNeg = Def.taskDyn[String] { + | if (condition) { + | Def.task { + | if (condition) { + | val first = if (!condition && condition) { + | fooNeg.value + | } else "" + | if ("true".toBoolean) first + | else { + | barNeg.value + | } + | } else "" + | } + | } else Def.task("") + |} + """.stripMargin + } + } + test("Fail on task invocation inside else of task returned by dynamic task") { expectError(TaskLinterDSLFeedback.useOfValueInsideIfExpression("barNeg")) { """ @@ -129,6 +158,27 @@ class TaskNegSpec extends FunSuite { } } + test("Fail on task invocation inside nested anonymous function returned by regular task") { + val fooNegError = TaskLinterDSLFeedback.useOfValueInsideAnon("fooNeg") + val barNegError = TaskLinterDSLFeedback.useOfValueInsideAnon("barNeg") + expectError(List(fooNegError, barNegError).mkString("\n")) { + """ + |import sbt._ + |import sbt.Def._ + | + |val fooNeg = taskKey[String]("") + |val barNeg = taskKey[String]("") + |var condition = true + | + |val bazNeg = Def.task[String] { + | val anon = () => { val _ = () => fooNeg.value; barNeg.value} + | if (condition) anon() + | else anon() + |} + """.stripMargin + } + } + test("Fail on task invocation inside complex anonymous function returned by regular task") { val fooNegError = TaskLinterDSLFeedback.useOfValueInsideAnon("fooNeg") expectError(fooNegError) { @@ -168,4 +218,131 @@ class TaskNegSpec extends FunSuite { """.stripMargin } } + + test("Detect a missing `.value` inside a task") { + expectError(TaskLinterDSLFeedback.missingValueForKey("fooNeg")) { + """ + |import sbt._ + |import sbt.Def._ + | + |val fooNeg = taskKey[String]("") + | + |def avoidDCE = {println(""); ""} + |val bazNeg = Def.task[String] { + | fooNeg + | avoidDCE + |} + """.stripMargin + } + } + + test("Detect a missing `.value` inside a val definition of a task") { + expectError(TaskLinterDSLFeedback.missingValueForKey("fooNeg2")) { + """ + |import sbt._ + |import sbt.Def._ + | + |val fooNeg2 = taskKey[String]("") + | + |def avoidDCE = {println(""); ""} + |val bazNeg = Def.task[String] { + | val _ = fooNeg2 + | avoidDCE + |} + """.stripMargin + } + } + + test("Detect a missing `.value` inside a val definition of an inner method of a task") { + expectError(TaskLinterDSLFeedback.missingValueForKey("fooNeg2")) { + """ + |import sbt._ + |import sbt.Def._ + | + |val fooNeg2 = taskKey[String]("") + | + |def avoidDCE = {println(""); ""} + |val bazNeg = Def.task[String] { + | def inner = { + | val _ = fooNeg2 + | avoidDCE + | } + | inner + |} + """.stripMargin + } + } + + test("Detect a missing `.value` inside an inner method of a task") { + expectError(TaskLinterDSLFeedback.missingValueForKey("fooNeg3")) { + """ + |import sbt._ + |import sbt.Def._ + | + |val fooNeg3 = taskKey[String]("") + |def avoidDCE = {println(""); ""} + |val bazNeg = Def.task[String] { + | def inner: String = { + | fooNeg3 + | avoidDCE + | } + | inner + |} + """.stripMargin + } + } + + test("Detect a missing `.value` inside a task whose return type is Unit") { + expectError(TaskLinterDSLFeedback.missingValueForKey("fooNeg4")) { + """ + |import sbt._ + |import sbt.Def._ + | + |val fooNeg4 = taskKey[String]("") + | + |val bazNeg = Def.task[Unit] { + | fooNeg4 + |} + """.stripMargin + } + } + + // Enable these tests when https://github.com/scala/bug/issues/10340 is fixed + + /* + test("Detect a missing `.value` inside a val of an inner method of a task returning a literal") { + expectError(TaskLinterDSLFeedback.missingValueForKey("fooNeg3")) { + """ + |import sbt._ + |import sbt.Def._ + | + |val fooNeg3 = taskKey[String]("") + | + |val bazNeg = Def.task[String] { + | def inner: String = { + | val _ = fooNeg3 + | "" + | } + | inner + |} + """.stripMargin + } + } + + test("Detect a missing `.value` inside a val of a task returning a literal") { + expectError(TaskLinterDSLFeedback.missingValueForKey("fooNeg3")) { + """ + |import sbt._ + |import sbt.Def._ + | + |val fooNeg3 = taskKey[String]("") + | + |val bazNeg = Def.task[String] { + | val _ = fooNeg3 + | "" + |} + """.stripMargin + } + } + */ } diff --git a/main/src/main/scala/sbt/Defaults.scala b/main/src/main/scala/sbt/Defaults.scala index 7933200be..d6a1b4657 100755 --- a/main/src/main/scala/sbt/Defaults.scala +++ b/main/src/main/scala/sbt/Defaults.scala @@ -1186,6 +1186,7 @@ object Defaults extends BuildCommon { val s = streams.value val opts = forkOptions.value val options = javaOptions.value + val trap = trapExit.value if (fork.value) { s.log.debug(s"javaOptions: $options") new ForkRun(opts) @@ -1200,7 +1201,7 @@ object Defaults extends BuildCommon { mask) s.log.warn(s"$showJavaOptions will be ignored, $showFork is set to false") } - new Run(si, trapExit.value, tmp) + new Run(si, trap, tmp) } }