luke
/
sbt
mirror of https://github.com/sbt/sbt.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Make the task linter configurable 

The user should be able to configure whether or not the task linting is
strictly enforced. In my opinion, the linter is generally pretty good
about warning users that a particular definition may not behave the way
the user expects. Unfortunately, it is fairly common that the user
explicitly wants this behavior and making the linter abort compilation
is a frustrating user experience.

To fix this, I add the LinterLevel trait to a new sbt.dsl package in the
core-macros project. The user can configure the linter to:
1) abort when an issue is detected. This is the current default behavior.
2) print a warning when an issues is detected
3) skip linting all together

Linter configuration is managed by importing the corresponding implicit
object from the LinterLevel companion object.

Fixes #3266
This commit is contained in:
Ethan Atkins 2018-12-11 11:52:06 -08:00
parent 2e027640d3
commit 32792f2b9d
4 changed files with 179 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
core-macros/src/main/scala/sbt/internal/util/appmacro/Instance.scala
View File

@ -8,8 +8,8 @@
package sbt.internal.util
package appmacro
import Classes.Applicative
import Types.Id
import sbt.internal.util.Classes.Applicative
import sbt.internal.util.Types.Id
/**
* The separate hierarchy from Applicative/Monad is for two reasons.
@ -28,8 +28,7 @@ trait MonadInstance extends Instance {
def flatten[T](in: M[M[T]]): M[T]
}
import scala.reflect._
import macros._
import scala.reflect.macros._
object Instance {
type Aux[M0[_]] = Instance { type M[x] = M0[x] }

68
main-settings/src/main/scala/sbt/dsl/LinterLevel.scala Normal file
View File

@ -0,0 +1,68 @@
/*
* sbt
* Copyright 2011 - 2018, Lightbend, Inc.
* Copyright 2008 - 2010, Mark Harrah
* Licensed under Apache License 2.0 (see LICENSE)
*/
package sbt.dsl
/**
* Controls the kind of linting performed during task macro expansion. The linting level is
* controlled by implicitly bringing an instance of LinterLevel into scope:* {{{
* val foo = taskKey[Unit]("")
* val bar = taskKey[Unit]("")
* // This compiles because of the import in the block
* val fooTask = {
* import sbt.dsl.LinterLevel.Ignore
* Def.task {
* if (true) bar.value
* else bar.value
* }
* }
* // because the import above was in a local scope, it does not apply here, so this won't
* // compile:
* //val barTask = Def.task {
* // if (true) foo.value
* // else foo.value
* //}
*
* import sbt.dsl.LinterLevel.Ignore
* // Both defs compile because the Ignore LinterLevel is implicitly brought into scope and
* // picked up by both task defs underneath
* val newFooTask = Def.task {
* if (true) bar.value
* else bar.value
* }
* val barTask = Def.task {
* if (true) foo.value
* else foo.value
* }
* }}}
* To make this work, the instances are all defined as implicit case objects. Moreover, the
* the [[LinterLevel.Abort]] setting is made default by placing [[LinterLevel.Warn]] and
* [[LinterLevel.Ignore]] in the [[LinterLevelLowPriority]] trait that the [[LinterLevel]]
* companion object extends.
*/
sealed trait LinterLevel
object LinterLevel extends LinterLevelLowPriority {
/**
* Abort the macro expansion if any linter check fails.
*/
implicit case object Abort extends LinterLevel
}
private[dsl] trait LinterLevelLowPriority {
/**
* Do not perform any linting.
*/
implicit case object Ignore extends LinterLevel
/**
* Apply the linter but print warnings instead of aborting macro expansion when linter violations
* are found.
*/
implicit case object Warn extends LinterLevel
}

41
main-settings/src/main/scala/sbt/std/TaskLinterDSL.scala
View File

@ -8,6 +8,8 @@
package sbt.std
import sbt.SettingKey
import sbt.dsl.LinterLevel
import sbt.dsl.LinterLevel.{ Abort, Warn }
import sbt.internal.util.ConsoleAppender
import sbt.internal.util.appmacro.{ Convert, LinterDSL }
@ -18,7 +20,9 @@ abstract class BaseTaskLinterDSL extends LinterDSL {
def isDynamicTask: Boolean
def convert: Convert
override def runLinter(ctx: blackbox.Context)(tree: ctx.Tree): Unit = {
private def impl(
ctx: blackbox.Context
)(tree: ctx.Tree, lint: (ctx.Position, String) => Unit): Unit = {
import ctx.universe._
val isTask = convert.asPredicate(ctx)
val unchecked = symbolOf[sbt.sbtUnchecked].asClass
@ -26,6 +30,7 @@ abstract class BaseTaskLinterDSL extends LinterDSL {
/**
* Lints a task tree.
*
* @param insideIf indicates whether or not the current tree is enclosed in an if statement.
* It is generally illegal to call `.value` on a task within such a tree unless
* the tree has been annotated with `@sbtUnchecked`.
@ -68,21 +73,21 @@ abstract class BaseTaskLinterDSL extends LinterDSL {
def detectAndErrorOnKeyMissingValue(i: Ident): Unit = {
if (isKey(i.tpe)) {
val keyName = i.name.decodedName.toString
ctx.error(i.pos, TaskLinterDSLFeedback.missingValueForKey(keyName))
lint(i.pos, TaskLinterDSLFeedback.missingValueForKey(keyName))
} else ()
}
def detectAndErrorOnKeyMissingValue(s: Select): Unit = {
if (isKey(s.tpe)) {
val keyName = s.name.decodedName.toString
ctx.error(s.pos, TaskLinterDSLFeedback.missingValueForKey(keyName))
lint(s.pos, TaskLinterDSLFeedback.missingValueForKey(keyName))
} else ()
}
def detectAndErrorOnKeyMissingValue(a: Apply): Unit = {
if (isInitialize(a.tpe)) {
val expr = "X / y"
ctx.error(a.pos, TaskLinterDSLFeedback.missingValueForInitialize(expr))
lint(a.pos, TaskLinterDSLFeedback.missingValueForInitialize(expr))
} else ()
}
@ -99,11 +104,11 @@ abstract class BaseTaskLinterDSL extends LinterDSL {
if (!isSettingKey && !shouldIgnore && isTask(wrapperName, tpe.tpe, qual)) {
if (insideIf && !isDynamicTask) {
// Error on the use of value inside the if of a regular task (dyn task is ok)
ctx.error(ap.pos, TaskLinterDSLFeedback.useOfValueInsideIfExpression(qualName))
lint(ap.pos, TaskLinterDSLFeedback.useOfValueInsideIfExpression(qualName))
}
if (insideAnon) {
// Error on the use of anonymous functions in any task or dynamic task
ctx.error(ap.pos, TaskLinterDSLFeedback.useOfValueInsideAnon(qualName))
lint(ap.pos, TaskLinterDSLFeedback.useOfValueInsideAnon(qualName))
}
}
traverse(qual)
@ -127,7 +132,8 @@ abstract class BaseTaskLinterDSL extends LinterDSL {
* 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. */
* 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 =>
@ -151,6 +157,17 @@ abstract class BaseTaskLinterDSL extends LinterDSL {
}
new traverser(insideIf = false, insideAnon = false, uncheckedWrapper = None).traverse(tree)
}
override def runLinter(ctx: blackbox.Context)(tree: ctx.Tree): Unit = {
import ctx.universe._
ctx.inferImplicitValue(weakTypeOf[LinterLevel]) match {
case t if t.tpe =:= weakTypeOf[Abort.type] =>
impl(ctx)(tree, (pos, msg) => ctx.error(pos, msg))
case t if t.tpe =:= weakTypeOf[Warn.type] =>
impl(ctx)(tree, (pos, msg) => ctx.warning(pos, msg))
case _ => ()
}
}
}
object TaskLinterDSL extends BaseTaskLinterDSL {
@ -190,6 +207,7 @@ object TaskLinterDSLFeedback {
| 1. Make `$task` evaluation explicit outside of the function body if you don't care about its evaluation.
| 2. Use a dynamic task to evaluate `$task` and pass that value as a parameter to an anonymous function.
| 3. Annotate the `$task` evaluation with `@sbtUnchecked`, e.g. `($task.value: @sbtUnchecked)`.
| 4. Add `import sbt.dsl.LinterLevel.Ignore` to your build file to disable all task linting.
""".stripMargin
def useOfValueInsideIfExpression(task: String): String =
@ -201,19 +219,24 @@ object TaskLinterDSLFeedback {
| 1. If you only want to evaluate it when the if predicate is true or false, use a dynamic task.
| 2. Make the static evaluation explicit by evaluating `$task` outside the if expression.
| 3. If you still want to force the static evaluation, you may annotate the task evaluation with `@sbtUnchecked`, e.g. `($task.value: @sbtUnchecked)`.
| 4. Add `import sbt.dsl.LinterLevel.Ignore` to your build file to disable all task linting.
""".stripMargin
def missingValueForKey(key: String): 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.
|$SolutionHeader:
| 1. Replace `$key` by `$key.value` or remove it if unused
| 2. Add `import sbt.dsl.LinterLevel.Ignore` to your build file to disable all task linting.
""".stripMargin
def missingValueForInitialize(expr: String): String =
s"""${startBold}The setting/task `$expr` is not being invoked inside the task definition.$reset
|
|$ProblemHeader: Settings/tasks missing `.value` are not initialized and their dependency is not registered.
|$SolutionHeader: Replace `$expr` by `($expr).value` or remove it if unused.
|$SolutionHeader:
| 1. Replace `$expr` by `($expr).value` or remove it if unused.
| 2. Add `import sbt.dsl.LinterLevel.Ignore` to your build file to disable all task linting.
""".stripMargin
}

76
main-settings/src/test/scala/sbt/std/TaskConfigSpec.scala Normal file
View File

@ -0,0 +1,76 @@
/*
* sbt
* Copyright 2011 - 2018, Lightbend, Inc.
* Copyright 2008 - 2010, Mark Harrah
* Licensed under Apache License 2.0 (see LICENSE)
*/
package sbt.std
import org.scalatest.FunSuite
import sbt.std.TestUtil._
import scala.tools.reflect.{ FrontEnd, ToolBoxError }
class TaskConfigSpec extends FunSuite {
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
}
private def taskDef(linterLevelImport: Option[String]): String =
s"""
|import sbt._
|import sbt.Def._
|${linterLevelImport.getOrElse("")}
|
|val fooNeg = taskKey[String]("")
|var condition = true
|
|val barNeg = Def.task[String] {
| if (condition) fooNeg.value
| else ""
|}
""".stripMargin
private val fooNegError = TaskLinterDSLFeedback.useOfValueInsideIfExpression("fooNeg")
test("Fail on task invocation inside if it is used inside a regular task") {
val fooNegError = TaskLinterDSLFeedback.useOfValueInsideIfExpression("fooNeg")
expectError(List(fooNegError).mkString("\n"))(taskDef(Some("import sbt.dsl.LinterLevel.Abort")))
}
test("Succeed if the linter level is set to warn") {
val toolbox = new CachingToolbox
assert(toolbox.eval(taskDef(None)) == ((): Unit))
assert(toolbox.infos.exists(i => i.severity.count == 1 && i.msg.contains(fooNegError)))
}
test("Succeed if the linter level is set to ignore") {
val toolbox = new CachingToolbox
assert(toolbox.eval(taskDef(Some("import sbt.dsl.LinterLevel.Ignore"))) == ((): Unit))
assert(toolbox.infos.isEmpty)
}
}