Configuring the action to perform on encountering a warn-condition

README.md

@@ -9,17 +9,29 @@ It's a work in progress.
 
 Add it as a compiler plugin in your project, or run `sbt console` in this project to see it in action.
 
+The plugin can optionally use a properties file (provided with `-P
+linter:config:/path/to/it` on the scalac command line) that allows
+controlling the action taken by the plugin either globally or on a
+per-warning basis. The default action is `warn` and may be changed to
+`no_action` or `error` by setting the `default_action` value in the
+properties file.  The default can be overridden on a per-check basis
+with any of those three options.
+
 ## Currently suported warnings
 
 ### Unsafe `==`
 
+The configuration property is `check.unsafe_equals`.
+
     scala> Nil == None
     <console>:29: warning: Comparing with == on instances of different types (object Nil, object None) will probably return false.
                   Nil == None
                       ^
 
 ### Unsafe `contains`
 
+The configuration property is `check.unsafe_contains`.
+
     scala> List(1, 2, 3).contains("4")
     <console>:29: warning: SeqLike[Int].contains(java.lang.String) will probably return false.
                   List(1, 2, 3).contains("4")
@@ -28,13 +40,17 @@ Add it as a compiler plugin in your project, or run `sbt console` in this projec
 
 ### Wildcard import from `scala.collection.JavaConversions`
 
+The configuration property is `check.java_conversions`.
+
     scala> import scala.collection.JavaConversions._
     <console>:29: warning: Conversions in scala.collection.JavaConversions._ are dangerous.
            import scala.collection.JavaConversions._
                                    ^
 
 ### Calling `Option#get`
 
+The configuration property is `check.option_get`.
+
     scala> Option(1).get
     <console>:29: warning: Calling .get on Option will throw an exception if the Option is None.
                   Option(1).get
@@ -43,8 +59,6 @@ Add it as a compiler plugin in your project, or run `sbt console` in this projec
 ## Future Work
 
 * Add more warnings
-* Pick and choose which warnings you want
-* Choose whether they should be warnings or errors
 
 ### Ideas for new warnings
 

src/main/scala/JavaConversions.scala

@@ -0,0 +1,19 @@
+package com.foursquare.lint
+
+import scala.tools.nsc.Global
+
+class JavaConversions(g: Global) extends LinterAction(g, Warnings.JavaConversions) {
+  import global._
+
+  val JavaConversionsModule: Symbol = definitions.getModule("scala.collection.JavaConversions")
+
+  def isGlobalImport(selector: ImportSelector): Boolean = {
+    selector.name == nme.WILDCARD && selector.renamePos == -1
+  }
+
+  val action: PartialFunction[Tree, (Position, String)] = {
+    case Import(pkg, selectors)
+        if pkg.symbol == JavaConversionsModule && selectors.exists(isGlobalImport) =>
+      (pkg.pos, "Conversions in scala.collection.JavaConversions._ are dangerous.")
+  }
+}

src/main/scala/LinterAction.scala

@@ -0,0 +1,15 @@
+package com.foursquare.lint
+
+import scala.tools.nsc.Global
+
+abstract class LinterAction(val global: Global, val warning: Warnings.Warning) {
+  import global._
+
+  def action: PartialFunction[Tree, (Position, String)]
+
+  // Utility predicates that are used by multiple warning types
+
+  def methodImplements(method: Symbol, target: Symbol): Boolean = {
+    method == target || method.allOverriddenSymbols.contains(target)
+  }
+}

src/main/scala/LinterPlugin.scala

@@ -20,74 +20,116 @@ import scala.reflect.generic.Flags
 import scala.tools.nsc.{Global, Phase}
 import scala.tools.nsc.plugins.{Plugin, PluginComponent}
 
+import java.io.{InputStream, FileInputStream, IOException}
+
 class LinterPlugin(val global: Global) extends Plugin {
   import global._
 
   val name = "linter"
   val description = ""
   val components = List[PluginComponent](LinterComponent)
+  var warningActions = (_: Warnings.Warning) => Actions.Warn
 
-  private object LinterComponent extends PluginComponent {
-    import global._
-
-    val global = LinterPlugin.this.global
+  def warningEnabled(warning: Warnings.Warning) = warningActions(warning) != Actions.NoAction
 
-    override val runsAfter = List("typer")
+  object Actions extends Enumeration {
+    type Action = Value
+    val NoAction,
+        Warn,
+        Error = Value
+  }
 
-    val phaseName = "linter"
+  val OptionConfig = "config:"
 
-    override def newPhase(prev: Phase): StdPhase = new StdPhase(prev) {
-      override def apply(unit: global.CompilationUnit): Unit = {
-        new LinterTraverser(unit).traverse(unit.body)
-      }
+  override def processOptions(options: List[String], error: String => Unit) {
+    for(option <- options) {
+      if(option.startsWith(OptionConfig)) loadConfiguration(option.drop(OptionConfig.length), error)
+      else error("Unknown option: " + option)
     }
+  }
 
-    class LinterTraverser(unit: CompilationUnit) extends Traverser {
-      import definitions.{AnyClass, ObjectClass, Object_==, OptionClass, SeqClass}
+  private def underscoreify(camelCase: String): String = // not super-smart, but sufficient unto the purpose
+    camelCase.replaceAll("([A-Z])","_$1").toLowerCase.dropWhile(_ == '_')
 
-      val JavaConversionsModule: Symbol = definitions.getModule("scala.collection.JavaConversions")
-      val SeqLikeClass: Symbol = definitions.getClass("scala.collection.SeqLike")
-      val SeqLikeContains: Symbol = SeqLikeClass.info.member(newTermName("contains"))
-      val OptionGet: Symbol = OptionClass.info.member(nme.get)
+  private def loadConfiguration(filename: String, error: String => Unit) {
+    import java.util.Properties
+    import scala.collection.JavaConverters._
 
-      def SeqMemberType(seenFrom: Type): Type = {
-        SeqLikeClass.tpe.typeArgs.head.asSeenFrom(seenFrom, SeqLikeClass)
-      }
+    val props = new Properties
 
-      def isSubtype(x: Tree, y: Tree): Boolean = {
-        x.tpe.widen <:< y.tpe.widen
+    try {
+      val stream = openPropertiesFile(filename)
+      try {
+        props.load(stream)
+      } finally {
+        stream.close()
       }
+    } catch {
+      case e: IOException =>
+        error("Exception occurred while loading properties file: " + e.getMessage)
+        return
+    }
 
-      def methodImplements(method: Symbol, target: Symbol): Boolean = {
-        method == target || method.allOverriddenSymbols.contains(target)
-      }
+    val NoAction = underscoreify(Actions.NoAction.toString)
+    val Warn = underscoreify(Actions.Warn.toString)
+    val Error = underscoreify(Actions.Error.toString)
 
-      def isGlobalImport(selector: ImportSelector): Boolean = {
-        selector.name == nme.WILDCARD && selector.renamePos == -1
+    val defaultAction = props.getProperty("default_action", Warn) match {
+      case action@(NoAction|Warn|Error) => action
+      case other =>
+        error("Unknown action for default_action: " + other)
+        return
+    }
+
+    warningActions =
+      Warnings.values.foldLeft(Map.empty[Warnings.Warning, Actions.Action]) { (warningConfig, warning) =>
+        val action = props.getProperty("check." + underscoreify(warning.toString), defaultAction) match {
+          case NoAction => Actions.NoAction
+          case Warn => Actions.Warn
+          case Error => Actions.Error
+          case other =>
+            error("Unknown action for warning " + warning + ": " + other)
+            return
+        }
+        warningConfig + (warning -> action)
       }
+  }
+
+  protected def openPropertiesFile(filename: String): InputStream =
+    new FileInputStream(filename)
 
-      override def traverse(tree: Tree): Unit = tree match {
-        case Apply(eqeq @ Select(lhs, nme.EQ), List(rhs))
-            if methodImplements(eqeq.symbol, Object_==) && !(isSubtype(lhs, rhs) || isSubtype(rhs, lhs)) =>
-          val warnMsg = "Comparing with == on instances of different types (%s, %s) will probably return false."
-          unit.warning(eqeq.pos, warnMsg.format(lhs.tpe.widen, rhs.tpe.widen))
+  private object LinterComponent extends PluginComponent {
+    import global._
+
+    val global = LinterPlugin.this.global
 
-        case Import(pkg, selectors)
-            if pkg.symbol == JavaConversionsModule && selectors.exists(isGlobalImport) =>
-          unit.warning(pkg.pos, "Conversions in scala.collection.JavaConversions._ are dangerous.")
+    override val runsAfter = List("typer")
 
-        case Apply(contains @ Select(seq, _), List(target))
-            if methodImplements(contains.symbol, SeqLikeContains) && !(target.tpe <:< SeqMemberType(seq.tpe)) =>
-          val warnMsg = "SeqLike[%s].contains(%s) will probably return false."
-          unit.warning(contains.pos, warnMsg.format(SeqMemberType(seq.tpe), target.tpe.widen))
+    val phaseName = "linter"
 
-        case get @ Select(_, nme.get) if methodImplements(get.symbol, OptionGet) =>
-          if (!get.pos.source.path.contains("src/test")) {
-            unit.warning(get.pos, "Calling .get on Option will throw an exception if the Option is None.")
-          }
+    override def newPhase(prev: Phase): StdPhase = new StdPhase(prev) {
+      override def apply(unit: global.CompilationUnit): Unit = {
+        new LinterTraverser(unit).traverse(unit.body)
+      }
+    }
 
-        case _ =>
-          super.traverse(tree)
+    class LinterTraverser(unit: CompilationUnit) extends Traverser {
+      val actions = List(new UnsafeEquals(global),
+                         new UnsafeContains(global),
+                         new OptionGet(global),
+                         new JavaConversions(global)).filter(a => warningEnabled(a.warning))
+
+      override def traverse(tree: Tree): Unit = {
+        // I hate thes .asInstanceOfs.  But I cannot convince the
+        // compiler to thread the dependent types through!
+        actions.find(a => a.action.isDefinedAt(tree.asInstanceOf[a.global.Tree])) match {
+          case Some(action) =>
+            val (pos, msg) = action.action(tree.asInstanceOf[action.global.Tree])
+            if(warningActions(action.warning) == Actions.Error) unit.error(pos, msg)
+            else unit.warning(pos, msg)
+          case None =>
+            super.traverse(tree)
+        }
       }
     }
   }

src/main/scala/OptionGet.scala

@@ -0,0 +1,16 @@
+package com.foursquare.lint
+
+import scala.tools.nsc.Global
+
+class OptionGet(g: Global) extends LinterAction(g, Warnings.OptionGet) {
+  import global._
+
+  import definitions.OptionClass
+  val OptionGet: Symbol = OptionClass.info.member(nme.get)
+
+  val action: PartialFunction[Tree, (Position, String)] = {
+    case get @ Select(_, nme.get)
+        if methodImplements(get.symbol, OptionGet) && !get.pos.source.path.contains("src/test") =>
+      (get.pos, "Calling .get on Option will throw an exception if the Option is None.")
+  }
+}

src/main/scala/UnsafeContains.scala

@@ -0,0 +1,21 @@
+package com.foursquare.lint
+
+import scala.tools.nsc.Global
+
+class UnsafeContains(g: Global) extends LinterAction(g, Warnings.UnsafeContains) {
+  import global._
+
+  val SeqLikeClass: Symbol = definitions.getClass("scala.collection.SeqLike")
+  val SeqLikeContains: Symbol = SeqLikeClass.info.member(newTermName("contains"))
+
+  def SeqMemberType(seenFrom: Type): Type = {
+    SeqLikeClass.tpe.typeArgs.head.asSeenFrom(seenFrom, SeqLikeClass)
+  }
+
+  val action: PartialFunction[Tree, (Position, String)] = {
+    case Apply(contains @ Select(seq, _), List(target))
+        if methodImplements(contains.symbol, SeqLikeContains) && !(target.tpe <:< SeqMemberType(seq.tpe)) =>
+      val warnMsg = "SeqLike[%s].contains(%s) will probably return false."
+      (contains.pos, warnMsg.format(SeqMemberType(seq.tpe), target.tpe.widen))
+  }
+}

src/main/scala/UnsafeEquals.scala

@@ -0,0 +1,20 @@
+package com.foursquare.lint
+
+import scala.tools.nsc.Global
+
+class UnsafeEquals(g: Global) extends LinterAction(g, Warnings.UnsafeEquals) {
+  import global._
+  import definitions.Object_==
+
+  def isSubtype(x: Tree, y: Tree): Boolean = {
+    x.tpe.widen <:< y.tpe.widen
+  }
+
+  val action: PartialFunction[Tree, (Position, String)] = {
+    case Apply(eqeq @ Select(lhs, nme.EQ), List(rhs))
+        if methodImplements(eqeq.symbol, Object_==) && !(isSubtype(lhs, rhs) || isSubtype(rhs, lhs)) =>
+      val warnMsg = "Comparing with == on instances of different types (%s, %s) will probably return false."
+      (eqeq.pos, warnMsg.format(lhs.tpe.widen, rhs.tpe.widen))
+  }
+}
+

src/main/scala/Warnings.scala

@@ -0,0 +1,9 @@
+package com.foursquare.lint
+
+object Warnings extends Enumeration {
+  type Warning = Value
+  val JavaConversions,
+  OptionGet,
+  UnsafeContains,
+  UnsafeEquals = Value
+}

src/test/resources/testprops/allbutjavaconversionserror.properties

@@ -0,0 +1,3 @@
+check.option_get = error
+check.unsafe_contains = error
+check.unsafe_equals = error

src/test/resources/testprops/allbutjavaconversionsoff.properties

@@ -0,0 +1,3 @@
+check.option_get = no_action
+check.unsafe_contains = no_action
+check.unsafe_equals = no_action

src/test/resources/testprops/allbutoptiongeterror.properties

@@ -0,0 +1,3 @@
+check.java_conversions = error
+check.unsafe_contains = error
+check.unsafe_equals = error

src/test/resources/testprops/allbutoptiongetoff.properties

@@ -0,0 +1,3 @@
+check.java_conversions = no_action
+check.unsafe_contains = no_action
+check.unsafe_equals = no_action

src/test/resources/testprops/allbutunsafecontainserror.properties

@@ -0,0 +1,3 @@
+check.java_conversions = error
+check.option_get = error
+check.unsafe_equals = error

src/test/resources/testprops/allbutunsafecontainsoff.properties

@@ -0,0 +1,3 @@
+check.java_conversions = no_action
+check.option_get = no_action
+check.unsafe_equals = no_action

src/test/resources/testprops/allbutunsafeequalserror.properties

@@ -0,0 +1,3 @@
+check.java_conversions = error
+check.option_get = error
+check.unsafe_contains = error

src/test/resources/testprops/allbutunsafeequalsoff.properties

@@ -0,0 +1,3 @@
+check.java_conversions = no_action
+check.option_get = no_action
+check.unsafe_contains = no_action

src/test/resources/testprops/allerror.properties

@@ -0,0 +1,4 @@
+check.java_conversions = error
+check.option_get = error
+check.unsafe_contains = error
+check.unsafe_equals = error

src/test/resources/testprops/alloff.properties

@@ -0,0 +1,4 @@
+check.java_conversions = no_action
+check.option_get = no_action
+check.unsafe_contains = no_action
+check.unsafe_equals = no_action

src/test/resources/testprops/defaulterror.properties

@@ -0,0 +1 @@
+default_action = error

src/test/resources/testprops/defaultnoaction.properties

@@ -0,0 +1 @@
+default_action = no_action

src/test/resources/testprops/javaconversionserror.properties

@@ -0,0 +1 @@
+check.java_conversions = error

src/test/resources/testprops/javaconversionsoff.properties

@@ -0,0 +1 @@
+check.java_conversions = no_action

src/test/resources/testprops/optiongeterror.properties

@@ -0,0 +1 @@
+check.option_get = error

src/test/resources/testprops/optiongetoff.properties

@@ -0,0 +1 @@
+check.option_get = no_action

src/test/resources/testprops/unsafecontainserror.properties

@@ -0,0 +1 @@
+check.unsafe_contains = error

src/test/resources/testprops/unsafecontainsoff.properties

@@ -0,0 +1 @@
+check.unsafe_contains = no_action

src/test/resources/testprops/unsafeequalserror.properties

@@ -0,0 +1 @@
+check.unsafe_equals = error

src/test/resources/testprops/unsafeequalsoff.properties

@@ -0,0 +1 @@
+check.unsafe_equals = no_action