From ded281b5c260b8373a17a20403174ee8568f3908 Mon Sep 17 00:00:00 2001 From: jvican Date: Sun, 28 May 2017 12:45:47 +0200 Subject: [PATCH] Add missing .value DSL check 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 val definitions with `_` as name and idents in statement position inside blocks. In the future, we can cover all val definitions no matter what their name is. Unfortunately, doing so will come at the cost of speed: we have to run the unused name analysis in `TypeDiagnostics` and expose it from the power context in `ContextUtil`. This is good enough for now. If users express interest in having a smarter analysis, we can always consider trying the unused name analysis. I am not sure how slow it will be -- hopefully it won't be that much. --- .../main/scala/sbt/std/TaskLinterDSL.scala | 128 ++++++++--------- .../src/test/scala/sbt/std/TaskPosSpec.scala | 48 +++++++ .../test/scala/sbt/std/neg/TaskNegSpec.scala | 133 +++++++++++++++++- 3 files changed, 232 insertions(+), 77 deletions(-) diff --git a/main-settings/src/main/scala/sbt/std/TaskLinterDSL.scala b/main-settings/src/main/scala/sbt/std/TaskLinterDSL.scala index 6798d8144..7dcaf2758 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,14 @@ 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] + private val identsWithValue = MutableSet.empty[Ident] var insideIf: Boolean = false var insideAnon: Boolean = false + var disableNoValueReport: Boolean = false def handleUncheckedAnnotation(exprAtUseSite: Tree, tt: TypeTree): Unit = { tt.original match { @@ -44,24 +48,39 @@ 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) => + // Keep track of wrapped idents to detect missing `.value` + qual match { + case i: Ident => identsWithValue.add(i) + case _ => () + } + 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) @@ -73,13 +92,34 @@ abstract class BaseTaskLinterDSL extends LinterDSL { handleUncheckedAnnotation(expr, tpt) traverse(expr) traverse(tpt) - case f @ Function(vparams, body) => + case Function(vparams, body) => super.traverseTrees(vparams) if (!vparams.exists(_.mods.hasFlag(Flag.SYNTHETIC))) { insideAnon = true traverse(body) insideAnon = false } 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 +178,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..9206ff749 100644 --- a/main-settings/src/test/scala/sbt/std/TaskPosSpec.scala +++ b/main-settings/src/test/scala/sbt/std/TaskPosSpec.scala @@ -66,4 +66,52 @@ 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 { + 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..5c0c07b7f 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) } @@ -168,4 +166,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 + } + } + */ }