From ded281b5c260b8373a17a20403174ee8568f3908 Mon Sep 17 00:00:00 2001 From: jvican Date: Sun, 28 May 2017 12:45:47 +0200 Subject: [PATCH 1/4] 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 + } + } + */ } From b017eaee394e196149e76f756f18f4b9dec39272 Mon Sep 17 00:00:00 2001 From: jvican Date: Sun, 28 May 2017 18:15:47 +0200 Subject: [PATCH 2/4] Support DSL detection for nested ifs and anons Before, we were not preserving the value `insideXXX`. This commit makes sure that we handle much more complex scenarios and we report them successfully. Have a look at the tests. --- .../main/scala/sbt/std/TaskLinterDSL.scala | 8 +-- .../test/scala/sbt/std/neg/TaskNegSpec.scala | 54 ++++++++++++++++++- main/src/main/scala/sbt/Defaults.scala | 3 +- 3 files changed, 60 insertions(+), 5 deletions(-) diff --git a/main-settings/src/main/scala/sbt/std/TaskLinterDSL.scala b/main-settings/src/main/scala/sbt/std/TaskLinterDSL.scala index 7dcaf2758..9390cee21 100644 --- a/main-settings/src/main/scala/sbt/std/TaskLinterDSL.scala +++ b/main-settings/src/main/scala/sbt/std/TaskLinterDSL.scala @@ -84,20 +84,22 @@ abstract class BaseTaskLinterDSL extends LinterDSL { 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 Function(vparams, body) => - super.traverseTrees(vparams) + 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) { 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 5c0c07b7f..ff9447502 100644 --- a/main-settings/src/test/scala/sbt/std/neg/TaskNegSpec.scala +++ b/main-settings/src/test/scala/sbt/std/neg/TaskNegSpec.scala @@ -62,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._ @@ -85,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")) { """ @@ -127,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) { 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) } } From 6a31251a01685525c637f4db53fd2b1092fb0e37 Mon Sep 17 00:00:00 2001 From: jvican Date: Sun, 28 May 2017 18:26:58 +0200 Subject: [PATCH 3/4] Add tests that check `sbtUnchecked` is respected --- .../src/test/scala/sbt/std/TaskPosSpec.scala | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/main-settings/src/test/scala/sbt/std/TaskPosSpec.scala b/main-settings/src/test/scala/sbt/std/TaskPosSpec.scala index 9206ff749..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._ @@ -77,6 +101,18 @@ class TaskPosSpec { } } + 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._ From c85fb95851599d324623167a1b10a71d20612f1e Mon Sep 17 00:00:00 2001 From: jvican Date: Sun, 28 May 2017 18:36:14 +0200 Subject: [PATCH 4/4] Remove unnecessary infra for .value DSL check --- main-settings/src/main/scala/sbt/std/TaskLinterDSL.scala | 6 ------ 1 file changed, 6 deletions(-) diff --git a/main-settings/src/main/scala/sbt/std/TaskLinterDSL.scala b/main-settings/src/main/scala/sbt/std/TaskLinterDSL.scala index 9390cee21..69cc100e3 100644 --- a/main-settings/src/main/scala/sbt/std/TaskLinterDSL.scala +++ b/main-settings/src/main/scala/sbt/std/TaskLinterDSL.scala @@ -20,7 +20,6 @@ abstract class BaseTaskLinterDSL extends LinterDSL { 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 @@ -61,11 +60,6 @@ abstract class BaseTaskLinterDSL extends LinterDSL { override def traverse(tree: ctx.universe.Tree): Unit = { tree match { 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)) {