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-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) =>
+          case ap @ Apply(TypeApply(Select(_, nme), tpe :: Nil), qual :: Nil) =>
-            val shouldIgnore = uncheckedWrappers.contains(s)
+            // 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(
+  def missingValueForKey(key: String) =
-    Ident(TermName("condition")),
+    s"""${startBold}The key `$key` is not being invoked inside the task definition.$reset
-    Typed(
+       |
-      Typed(Apply(TypeApply(Select(Ident(sbt.std.InputWrapper),
+       |${ProblemHeader}: Keys missing `.value` are not initialized and their dependency is not registered.
-                                   TermName("wrapInitTask_$u2603$u2603")),
+       |
-                            List(TypeTree())),
+       |${SolutionHeader}: Replace `$key` by `$key.value` or remove it if unused.
-                  List(Ident(TermName("foo")))),
+    """.stripMargin
-            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())
-    )
-  )*/
 }

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
+    }
+  }
 }

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
+    }
+  }
+ */
 }