From ed8b8cc3ecd5b46b0a2f4a4b37f1d3b36350b761 Mon Sep 17 00:00:00 2001 From: "Robert J. Macomber" Date: Sun, 18 Mar 2012 10:19:10 -0700 Subject: [PATCH 1/5] Config file framework and per-check disabling It should be pretty trivial to enhance this to become "choose whether you want an error or warning". --- src/main/scala/LinterPlugin.scala | 61 +++++++- .../allbutjavaconversionsoff.properties | 3 + .../testprops/allbutoptiongetoff.properties | 3 + .../allbutunsafecontainsoff.properties | 3 + .../allbutunsafeequalsoff.properties | 3 + .../resources/testprops/alloff.properties | 4 + .../testprops/javaconversionsoff.properties | 1 + .../testprops/optiongetoff.properties | 1 + .../testprops/unsafecontainsoff.properties | 1 + .../testprops/unsafeequalsoff.properties | 1 + src/test/scala/LinterPluginTest.scala | 136 +++++++++++------- 11 files changed, 164 insertions(+), 53 deletions(-) create mode 100644 src/test/resources/testprops/allbutjavaconversionsoff.properties create mode 100644 src/test/resources/testprops/allbutoptiongetoff.properties create mode 100644 src/test/resources/testprops/allbutunsafecontainsoff.properties create mode 100644 src/test/resources/testprops/allbutunsafeequalsoff.properties create mode 100644 src/test/resources/testprops/alloff.properties create mode 100644 src/test/resources/testprops/javaconversionsoff.properties create mode 100644 src/test/resources/testprops/optiongetoff.properties create mode 100644 src/test/resources/testprops/unsafecontainsoff.properties create mode 100644 src/test/resources/testprops/unsafeequalsoff.properties diff --git a/src/main/scala/LinterPlugin.scala b/src/main/scala/LinterPlugin.scala index ec73b4c..68a7ade 100644 --- a/src/main/scala/LinterPlugin.scala +++ b/src/main/scala/LinterPlugin.scala @@ -20,12 +20,64 @@ 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 warningEnabled = (_: Warnings.Warning) => true + + object Warnings extends Enumeration { + type Warning = Value + val JavaConversions, + OptionGet, + UnsafeContains, + UnsafeEquals = Value + } + + val OptionConfig = "config:" + + 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) + } + } + + private def underscoreify(camelCase: String): String = // not super-smart, but sufficient unto the purpose + camelCase.replaceAll("([A-Z])","_$1").toLowerCase.dropWhile(_ == '_') + + private def loadConfiguration(filename: String, error: String => Unit) { + import java.util.Properties + import scala.collection.JavaConverters._ + + val props = new Properties + + 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 + } + + warningEnabled = + Warnings.values.foldLeft(Map.empty[Warnings.Warning, Boolean]) { (warningConfig, warning) => + // Anything other than "false" is treated as enabling the warning + warningConfig + (warning -> !(props.getProperty("check." + underscoreify(warning.toString), "true") == "false")) + } + } + + protected def openPropertiesFile(filename: String): InputStream = + new FileInputStream(filename) private object LinterComponent extends PluginComponent { import global._ @@ -68,20 +120,21 @@ class LinterPlugin(val global: Global) extends Plugin { 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)) => + if warningEnabled(Warnings.UnsafeEquals) && 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)) case Import(pkg, selectors) - if pkg.symbol == JavaConversionsModule && selectors.exists(isGlobalImport) => + if warningEnabled(Warnings.JavaConversions) && pkg.symbol == JavaConversionsModule && selectors.exists(isGlobalImport) => unit.warning(pkg.pos, "Conversions in scala.collection.JavaConversions._ are dangerous.") case Apply(contains @ Select(seq, _), List(target)) - if methodImplements(contains.symbol, SeqLikeContains) && !(target.tpe <:< SeqMemberType(seq.tpe)) => + if warningEnabled(Warnings.UnsafeContains) && 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)) - case get @ Select(_, nme.get) if methodImplements(get.symbol, OptionGet) => + case get @ Select(_, nme.get) + if warningEnabled(Warnings.OptionGet) && 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.") } diff --git a/src/test/resources/testprops/allbutjavaconversionsoff.properties b/src/test/resources/testprops/allbutjavaconversionsoff.properties new file mode 100644 index 0000000..2e56f2c --- /dev/null +++ b/src/test/resources/testprops/allbutjavaconversionsoff.properties @@ -0,0 +1,3 @@ +check.option_get = false +check.unsafe_contains = false +check.unsafe_equals = false diff --git a/src/test/resources/testprops/allbutoptiongetoff.properties b/src/test/resources/testprops/allbutoptiongetoff.properties new file mode 100644 index 0000000..052229a --- /dev/null +++ b/src/test/resources/testprops/allbutoptiongetoff.properties @@ -0,0 +1,3 @@ +check.java_conversions = false +check.unsafe_contains = false +check.unsafe_equals = false diff --git a/src/test/resources/testprops/allbutunsafecontainsoff.properties b/src/test/resources/testprops/allbutunsafecontainsoff.properties new file mode 100644 index 0000000..6f4fbfc --- /dev/null +++ b/src/test/resources/testprops/allbutunsafecontainsoff.properties @@ -0,0 +1,3 @@ +check.java_conversions = false +check.option_get = false +check.unsafe_equals = false diff --git a/src/test/resources/testprops/allbutunsafeequalsoff.properties b/src/test/resources/testprops/allbutunsafeequalsoff.properties new file mode 100644 index 0000000..55b7442 --- /dev/null +++ b/src/test/resources/testprops/allbutunsafeequalsoff.properties @@ -0,0 +1,3 @@ +check.java_conversions = false +check.option_get = false +check.unsafe_contains = false diff --git a/src/test/resources/testprops/alloff.properties b/src/test/resources/testprops/alloff.properties new file mode 100644 index 0000000..a8610ae --- /dev/null +++ b/src/test/resources/testprops/alloff.properties @@ -0,0 +1,4 @@ +check.java_conversions = false +check.option_get = false +check.unsafe_contains = false +check.unsafe_equals = false diff --git a/src/test/resources/testprops/javaconversionsoff.properties b/src/test/resources/testprops/javaconversionsoff.properties new file mode 100644 index 0000000..aed563d --- /dev/null +++ b/src/test/resources/testprops/javaconversionsoff.properties @@ -0,0 +1 @@ +check.java_conversions = false diff --git a/src/test/resources/testprops/optiongetoff.properties b/src/test/resources/testprops/optiongetoff.properties new file mode 100644 index 0000000..33b13b2 --- /dev/null +++ b/src/test/resources/testprops/optiongetoff.properties @@ -0,0 +1 @@ +check.option_get = false diff --git a/src/test/resources/testprops/unsafecontainsoff.properties b/src/test/resources/testprops/unsafecontainsoff.properties new file mode 100644 index 0000000..8634b6b --- /dev/null +++ b/src/test/resources/testprops/unsafecontainsoff.properties @@ -0,0 +1 @@ +check.unsafe_contains = false diff --git a/src/test/resources/testprops/unsafeequalsoff.properties b/src/test/resources/testprops/unsafeequalsoff.properties new file mode 100644 index 0000000..8b47ad5 --- /dev/null +++ b/src/test/resources/testprops/unsafeequalsoff.properties @@ -0,0 +1 @@ +check.unsafe_equals = false diff --git a/src/test/scala/LinterPluginTest.scala b/src/test/scala/LinterPluginTest.scala index 20b0a00..d709f9d 100644 --- a/src/test/scala/LinterPluginTest.scala +++ b/src/test/scala/LinterPluginTest.scala @@ -22,7 +22,9 @@ import org.specs.SpecsMatchers class LinterPluginTest extends SpecsMatchers { var linterPlugin: LinterPlugin = null - class Compiler { + class InitializationException(message: String) extends Exception(message) + + class Compiler(pluginOptions: List[String]) { import java.io.{PrintWriter, StringWriter} import scala.io.Source import scala.tools.nsc.{Global, Settings} @@ -46,7 +48,16 @@ class LinterPluginTest extends SpecsMatchers { new Global(settings, reporter) { override protected def computeInternalPhases () { super.computeInternalPhases - linterPlugin = new LinterPlugin(this) + linterPlugin = new LinterPlugin(this) { + override def openPropertiesFile(filename: String) = + Option(getClass.getClassLoader.getResourceAsStream(filename)).getOrElse { + throw new java.io.FileNotFoundException("No such file " + filename) + } + } + + // is this the right place? + linterPlugin.processOptions(pluginOptions, msg => throw new InitializationException(msg)) + for (phase <- linterPlugin.components) phasesSet += phase } @@ -65,11 +76,22 @@ class LinterPluginTest extends SpecsMatchers { } } - val compiler = new Compiler - def check(code: String, expectedError: Option[String] = None) { + val compilerCache = new scala.collection.mutable.HashMap[List[String], Compiler] + def compilerFor(options: List[String]): Compiler = { + compilerCache.get(options) match { + case Some(compiler) => compiler + case None => + val compiler = new Compiler(options) + compilerCache += options -> compiler + compiler + } + } + + def check(code: String, expectedError: Option[String] = None, options: List[String] = Nil) { // Either they should both be None or the expected error should be a // substring of the actual error. - (expectedError, compiler.compileAndLint(code)) must beLike { + + (expectedError, compilerFor(options).compileAndLint(code)) must beLike { case (None, None) => true case (Some(exp), Some(act)) => act.contains(exp) } @@ -81,64 +103,80 @@ class LinterPluginTest extends SpecsMatchers { } @Test - def testHasVersusContains(): Unit = { - val msg = Some("SeqLike[Int].contains(java.lang.String) will probably return false.") + def nonExistantFileErrors(): Unit = { + check("""1 + 1""", None, List("config:thisfiledoesnotexistimsure")) must throwA[InitializationException] + } - check("""val x = List(4); x.contains("foo")""", msg) + @Test + def nonBadOptionFileErrors(): Unit = { + check("""1 + 1""", None, List("notavalidconfigoption")) must throwA[InitializationException] + } - // Set and Map have type-safe contains methods so we don't want to warn on - // those. - check("""val x = Set(4); x.contains(3)""", None) - check("""val x = Map(4 -> 5); x.contains(3)""", None) + def tripleTest(message: String, basename: String)(impltest: (Option[String], List[String]) => Unit): Unit = { + impltest(Some(message), Nil) + impltest(None, List("config:testprops/" + basename + "off.properties")) + impltest(Some(message), List("config:testprops/allbut" + basename + "off.properties")) } @Test - def testNoOptionGet(): Unit = { - val msg = Some("Calling .get on Option will throw an exception if the Option is None.") + def testHasVersusContains(): Unit = { + tripleTest("SeqLike[Int].contains(java.lang.String) will probably return false.", "unsafecontains") { (msg, options) => + check("""val x = List(4); x.contains("foo")""", msg, options) - check("""Option(10).get""", msg) - check("""val x: Option[Int] = None ; x.get""", msg) - check("""val x: Option[Int] = Some(3); x.get""", msg) - check("""val x = None ; x.get""", msg) - check("""val x = Some(3) ; x.get""", msg) + // Set and Map have type-safe contains methods so we don't want to warn on + // those. + check("""val x = Set(4); x.contains(3)""", options = options) + check("""val x = Map(4 -> 5); x.contains(3)""", options = options) + } + } - check("""Map(1 -> "1", 2 -> "2").get(1)""") + @Test + def testNoOptionGet(): Unit = { + tripleTest("Calling .get on Option will throw an exception if the Option is None.", "optionget") { (msg, options) => + check("""Option(10).get""", msg, options) + check("""val x: Option[Int] = None ; x.get""", msg, options) + check("""val x: Option[Int] = Some(3); x.get""", msg, options) + check("""val x = None ; x.get""", msg, options) + check("""val x = Some(3) ; x.get""", msg, options) + + check("""Map(1 -> "1", 2 -> "2").get(1)""", options = options) + } } @Test def testJavaConversionsImport(): Unit = { - val msg = Some("Conversions in scala.collection.JavaConversions._ are dangerous.") - - check("import scala.collection.JavaConversions._;", msg) + tripleTest("Conversions in scala.collection.JavaConversions._ are dangerous.", "javaconversions") { (msg, options) => + check("import scala.collection.JavaConversions._;", msg, options) + } } @Test def testUnsafeEquals(): Unit = { - val msg = Some("Comparing with ==") - - // Should warn - check("Nil == None", msg) - check("""{ - val x: List[Int] = Nil - val y: List[String] = Nil - x == y - }""", msg) - - // Should compile - check(""" "foo" == "bar" """) - check("""{ - val x: List[Int] = Nil - val y: List[Int] = Nil - x == y - }""") - check("""{ - val x: String = "foo" - val y: String = "bar" - x == y - }""") - check("""{ - val x: String = "foo" - x == "bar" - }""") + tripleTest("Comparing with ==", "unsafeequals") { (msg, options) => + // Should warn + check("Nil == None", msg, options) + check("""{ + val x: List[Int] = Nil + val y: List[String] = Nil + x == y + }""", msg, options) + + // Should compile + check(""" "foo" == "bar" """, options = options) + check("""{ + val x: List[Int] = Nil + val y: List[Int] = Nil + x == y + }""", options = options) + check("""{ + val x: String = "foo" + val y: String = "bar" + x == y + }""", options = options) + check("""{ + val x: String = "foo" + x == "bar" + }""", options = options) + } } } From d531759c657078c720757d3516a6d7c1e01aff06 Mon Sep 17 00:00:00 2001 From: "Robert J. Macomber" Date: Sun, 18 Mar 2012 11:05:28 -0700 Subject: [PATCH 2/5] Make errors warnings on a per-check basis --- src/main/scala/LinterPlugin.scala | 43 +++++++++++++++---- .../allbutjavaconversionserror.properties | 3 ++ .../allbutjavaconversionsoff.properties | 6 +-- .../testprops/allbutoptiongeterror.properties | 3 ++ .../testprops/allbutoptiongetoff.properties | 6 +-- .../allbutunsafecontainserror.properties | 3 ++ .../allbutunsafecontainsoff.properties | 6 +-- .../allbutunsafeequalserror.properties | 3 ++ .../allbutunsafeequalsoff.properties | 6 +-- .../resources/testprops/allerror.properties | 4 ++ .../resources/testprops/alloff.properties | 8 ++-- .../testprops/javaconversionserror.properties | 1 + .../testprops/javaconversionsoff.properties | 2 +- .../testprops/optiongeterror.properties | 1 + .../testprops/optiongetoff.properties | 2 +- .../testprops/unsafecontainserror.properties | 1 + .../testprops/unsafecontainsoff.properties | 2 +- .../testprops/unsafeequalserror.properties | 1 + .../testprops/unsafeequalsoff.properties | 2 +- src/test/scala/LinterPluginTest.scala | 22 +++++----- 20 files changed, 86 insertions(+), 39 deletions(-) create mode 100644 src/test/resources/testprops/allbutjavaconversionserror.properties create mode 100644 src/test/resources/testprops/allbutoptiongeterror.properties create mode 100644 src/test/resources/testprops/allbutunsafecontainserror.properties create mode 100644 src/test/resources/testprops/allbutunsafeequalserror.properties create mode 100644 src/test/resources/testprops/allerror.properties create mode 100644 src/test/resources/testprops/javaconversionserror.properties create mode 100644 src/test/resources/testprops/optiongeterror.properties create mode 100644 src/test/resources/testprops/unsafecontainserror.properties create mode 100644 src/test/resources/testprops/unsafeequalserror.properties diff --git a/src/main/scala/LinterPlugin.scala b/src/main/scala/LinterPlugin.scala index 68a7ade..0a8340d 100644 --- a/src/main/scala/LinterPlugin.scala +++ b/src/main/scala/LinterPlugin.scala @@ -28,7 +28,9 @@ class LinterPlugin(val global: Global) extends Plugin { val name = "linter" val description = "" val components = List[PluginComponent](LinterComponent) - var warningEnabled = (_: Warnings.Warning) => true + var warningActions = (_: Warnings.Warning) => Actions.Warn + + def warningEnabled(warning: Warnings.Warning) = warningActions(warning) != Actions.NoAction object Warnings extends Enumeration { type Warning = Value @@ -38,6 +40,13 @@ class LinterPlugin(val global: Global) extends Plugin { UnsafeEquals = Value } + object Actions extends Enumeration { + type Action = Value + val NoAction, + Warn, + Error = Value + } + val OptionConfig = "config:" override def processOptions(options: List[String], error: String => Unit) { @@ -69,10 +78,21 @@ class LinterPlugin(val global: Global) extends Plugin { return } - warningEnabled = - Warnings.values.foldLeft(Map.empty[Warnings.Warning, Boolean]) { (warningConfig, warning) => - // Anything other than "false" is treated as enabling the warning - warningConfig + (warning -> !(props.getProperty("check." + underscoreify(warning.toString), "true") == "false")) + val NoAction = underscoreify(Actions.NoAction.toString) + val Warn = underscoreify(Actions.Warn.toString) + val Error = underscoreify(Actions.Error.toString) + + warningActions = + Warnings.values.foldLeft(Map.empty[Warnings.Warning, Actions.Action]) { (warningConfig, warning) => + val action = props.getProperty("check." + underscoreify(warning.toString), Warn) 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) } } @@ -102,6 +122,11 @@ class LinterPlugin(val global: Global) extends Plugin { val SeqLikeContains: Symbol = SeqLikeClass.info.member(newTermName("contains")) val OptionGet: Symbol = OptionClass.info.member(nme.get) + def onWarn(w: Warnings.Warning) = { + if(warningActions(w) == Actions.Error) unit.error _ + else unit.warning _ + } + def SeqMemberType(seenFrom: Type): Type = { SeqLikeClass.tpe.typeArgs.head.asSeenFrom(seenFrom, SeqLikeClass) } @@ -122,21 +147,21 @@ class LinterPlugin(val global: Global) extends Plugin { case Apply(eqeq @ Select(lhs, nme.EQ), List(rhs)) if warningEnabled(Warnings.UnsafeEquals) && 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)) + onWarn(Warnings.UnsafeEquals)(eqeq.pos, warnMsg.format(lhs.tpe.widen, rhs.tpe.widen)) case Import(pkg, selectors) if warningEnabled(Warnings.JavaConversions) && pkg.symbol == JavaConversionsModule && selectors.exists(isGlobalImport) => - unit.warning(pkg.pos, "Conversions in scala.collection.JavaConversions._ are dangerous.") + onWarn(Warnings.JavaConversions)(pkg.pos, "Conversions in scala.collection.JavaConversions._ are dangerous.") case Apply(contains @ Select(seq, _), List(target)) if warningEnabled(Warnings.UnsafeContains) && 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)) + onWarn(Warnings.UnsafeContains)(contains.pos, warnMsg.format(SeqMemberType(seq.tpe), target.tpe.widen)) case get @ Select(_, nme.get) if warningEnabled(Warnings.OptionGet) && 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.") + onWarn(Warnings.OptionGet)(get.pos, "Calling .get on Option will throw an exception if the Option is None.") } case _ => diff --git a/src/test/resources/testprops/allbutjavaconversionserror.properties b/src/test/resources/testprops/allbutjavaconversionserror.properties new file mode 100644 index 0000000..98a038a --- /dev/null +++ b/src/test/resources/testprops/allbutjavaconversionserror.properties @@ -0,0 +1,3 @@ +check.option_get = error +check.unsafe_contains = error +check.unsafe_equals = error diff --git a/src/test/resources/testprops/allbutjavaconversionsoff.properties b/src/test/resources/testprops/allbutjavaconversionsoff.properties index 2e56f2c..153bf52 100644 --- a/src/test/resources/testprops/allbutjavaconversionsoff.properties +++ b/src/test/resources/testprops/allbutjavaconversionsoff.properties @@ -1,3 +1,3 @@ -check.option_get = false -check.unsafe_contains = false -check.unsafe_equals = false +check.option_get = no_action +check.unsafe_contains = no_action +check.unsafe_equals = no_action diff --git a/src/test/resources/testprops/allbutoptiongeterror.properties b/src/test/resources/testprops/allbutoptiongeterror.properties new file mode 100644 index 0000000..f109939 --- /dev/null +++ b/src/test/resources/testprops/allbutoptiongeterror.properties @@ -0,0 +1,3 @@ +check.java_conversions = error +check.unsafe_contains = error +check.unsafe_equals = error diff --git a/src/test/resources/testprops/allbutoptiongetoff.properties b/src/test/resources/testprops/allbutoptiongetoff.properties index 052229a..3902ad9 100644 --- a/src/test/resources/testprops/allbutoptiongetoff.properties +++ b/src/test/resources/testprops/allbutoptiongetoff.properties @@ -1,3 +1,3 @@ -check.java_conversions = false -check.unsafe_contains = false -check.unsafe_equals = false +check.java_conversions = no_action +check.unsafe_contains = no_action +check.unsafe_equals = no_action diff --git a/src/test/resources/testprops/allbutunsafecontainserror.properties b/src/test/resources/testprops/allbutunsafecontainserror.properties new file mode 100644 index 0000000..0293136 --- /dev/null +++ b/src/test/resources/testprops/allbutunsafecontainserror.properties @@ -0,0 +1,3 @@ +check.java_conversions = error +check.option_get = error +check.unsafe_equals = error diff --git a/src/test/resources/testprops/allbutunsafecontainsoff.properties b/src/test/resources/testprops/allbutunsafecontainsoff.properties index 6f4fbfc..90d0c10 100644 --- a/src/test/resources/testprops/allbutunsafecontainsoff.properties +++ b/src/test/resources/testprops/allbutunsafecontainsoff.properties @@ -1,3 +1,3 @@ -check.java_conversions = false -check.option_get = false -check.unsafe_equals = false +check.java_conversions = no_action +check.option_get = no_action +check.unsafe_equals = no_action diff --git a/src/test/resources/testprops/allbutunsafeequalserror.properties b/src/test/resources/testprops/allbutunsafeequalserror.properties new file mode 100644 index 0000000..e9bb745 --- /dev/null +++ b/src/test/resources/testprops/allbutunsafeequalserror.properties @@ -0,0 +1,3 @@ +check.java_conversions = error +check.option_get = error +check.unsafe_contains = error diff --git a/src/test/resources/testprops/allbutunsafeequalsoff.properties b/src/test/resources/testprops/allbutunsafeequalsoff.properties index 55b7442..a005828 100644 --- a/src/test/resources/testprops/allbutunsafeequalsoff.properties +++ b/src/test/resources/testprops/allbutunsafeequalsoff.properties @@ -1,3 +1,3 @@ -check.java_conversions = false -check.option_get = false -check.unsafe_contains = false +check.java_conversions = no_action +check.option_get = no_action +check.unsafe_contains = no_action diff --git a/src/test/resources/testprops/allerror.properties b/src/test/resources/testprops/allerror.properties new file mode 100644 index 0000000..3471c9c --- /dev/null +++ b/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 diff --git a/src/test/resources/testprops/alloff.properties b/src/test/resources/testprops/alloff.properties index a8610ae..f5a78d1 100644 --- a/src/test/resources/testprops/alloff.properties +++ b/src/test/resources/testprops/alloff.properties @@ -1,4 +1,4 @@ -check.java_conversions = false -check.option_get = false -check.unsafe_contains = false -check.unsafe_equals = false +check.java_conversions = no_action +check.option_get = no_action +check.unsafe_contains = no_action +check.unsafe_equals = no_action diff --git a/src/test/resources/testprops/javaconversionserror.properties b/src/test/resources/testprops/javaconversionserror.properties new file mode 100644 index 0000000..dcd92e5 --- /dev/null +++ b/src/test/resources/testprops/javaconversionserror.properties @@ -0,0 +1 @@ +check.java_conversions = error diff --git a/src/test/resources/testprops/javaconversionsoff.properties b/src/test/resources/testprops/javaconversionsoff.properties index aed563d..9524f53 100644 --- a/src/test/resources/testprops/javaconversionsoff.properties +++ b/src/test/resources/testprops/javaconversionsoff.properties @@ -1 +1 @@ -check.java_conversions = false +check.java_conversions = no_action diff --git a/src/test/resources/testprops/optiongeterror.properties b/src/test/resources/testprops/optiongeterror.properties new file mode 100644 index 0000000..c0f035a --- /dev/null +++ b/src/test/resources/testprops/optiongeterror.properties @@ -0,0 +1 @@ +check.option_get = error diff --git a/src/test/resources/testprops/optiongetoff.properties b/src/test/resources/testprops/optiongetoff.properties index 33b13b2..74c3c7f 100644 --- a/src/test/resources/testprops/optiongetoff.properties +++ b/src/test/resources/testprops/optiongetoff.properties @@ -1 +1 @@ -check.option_get = false +check.option_get = no_action diff --git a/src/test/resources/testprops/unsafecontainserror.properties b/src/test/resources/testprops/unsafecontainserror.properties new file mode 100644 index 0000000..9c0cb10 --- /dev/null +++ b/src/test/resources/testprops/unsafecontainserror.properties @@ -0,0 +1 @@ +check.unsafe_contains = error diff --git a/src/test/resources/testprops/unsafecontainsoff.properties b/src/test/resources/testprops/unsafecontainsoff.properties index 8634b6b..3e36be8 100644 --- a/src/test/resources/testprops/unsafecontainsoff.properties +++ b/src/test/resources/testprops/unsafecontainsoff.properties @@ -1 +1 @@ -check.unsafe_contains = false +check.unsafe_contains = no_action diff --git a/src/test/resources/testprops/unsafeequalserror.properties b/src/test/resources/testprops/unsafeequalserror.properties new file mode 100644 index 0000000..8f4f815 --- /dev/null +++ b/src/test/resources/testprops/unsafeequalserror.properties @@ -0,0 +1 @@ +check.unsafe_equals = error diff --git a/src/test/resources/testprops/unsafeequalsoff.properties b/src/test/resources/testprops/unsafeequalsoff.properties index 8b47ad5..6089438 100644 --- a/src/test/resources/testprops/unsafeequalsoff.properties +++ b/src/test/resources/testprops/unsafeequalsoff.properties @@ -1 +1 @@ -check.unsafe_equals = false +check.unsafe_equals = no_action diff --git a/src/test/scala/LinterPluginTest.scala b/src/test/scala/LinterPluginTest.scala index d709f9d..c6a0188 100644 --- a/src/test/scala/LinterPluginTest.scala +++ b/src/test/scala/LinterPluginTest.scala @@ -37,7 +37,7 @@ class LinterPluginTest extends SpecsMatchers { settings.bootclasspath.append(Source.fromURL(loader.getResource("boot.class.path")).mkString) settings.deprecation.value = true // enable detailed deprecation warnings settings.unchecked.value = true // enable detailed unchecked warnings - settings.Xwarnfatal.value = true // warnings cause compile failures too + settings.Xwarnfatal.value = false // but not this because we're testing the difference between error and warning val stringWriter = new StringWriter() @@ -69,8 +69,8 @@ class LinterPluginTest extends SpecsMatchers { stringWriter.getBuffer.delete(0, stringWriter.getBuffer.length) val thunked = "() => { %s }".format(code) interpreter.interpret(thunked) match { - case Results.Success => None - case Results.Error => Some(stringWriter.toString) + case Results.Success if stringWriter.toString.indexOf("warning") == -1 => None + case Results.Success | Results.Error => Some(stringWriter.toString) case Results.Incomplete => throw new Exception("Incomplete code snippet") } } @@ -112,15 +112,17 @@ class LinterPluginTest extends SpecsMatchers { check("""1 + 1""", None, List("notavalidconfigoption")) must throwA[InitializationException] } - def tripleTest(message: String, basename: String)(impltest: (Option[String], List[String]) => Unit): Unit = { - impltest(Some(message), Nil) + def multiTest(message: String, basename: String)(impltest: (Option[String], List[String]) => Unit): Unit = { + impltest(Some("warning: " + message), Nil) impltest(None, List("config:testprops/" + basename + "off.properties")) - impltest(Some(message), List("config:testprops/allbut" + basename + "off.properties")) + impltest(Some("error: " + message), List("config:testprops/" + basename + "error.properties")) + impltest(Some("warning: " + message), List("config:testprops/allbut" + basename + "off.properties")) + impltest(Some("warning: " + message), List("config:testprops/allbut" + basename + "error.properties")) } @Test def testHasVersusContains(): Unit = { - tripleTest("SeqLike[Int].contains(java.lang.String) will probably return false.", "unsafecontains") { (msg, options) => + multiTest("SeqLike[Int].contains(java.lang.String) will probably return false.", "unsafecontains") { (msg, options) => check("""val x = List(4); x.contains("foo")""", msg, options) // Set and Map have type-safe contains methods so we don't want to warn on @@ -132,7 +134,7 @@ class LinterPluginTest extends SpecsMatchers { @Test def testNoOptionGet(): Unit = { - tripleTest("Calling .get on Option will throw an exception if the Option is None.", "optionget") { (msg, options) => + multiTest("Calling .get on Option will throw an exception if the Option is None.", "optionget") { (msg, options) => check("""Option(10).get""", msg, options) check("""val x: Option[Int] = None ; x.get""", msg, options) check("""val x: Option[Int] = Some(3); x.get""", msg, options) @@ -145,14 +147,14 @@ class LinterPluginTest extends SpecsMatchers { @Test def testJavaConversionsImport(): Unit = { - tripleTest("Conversions in scala.collection.JavaConversions._ are dangerous.", "javaconversions") { (msg, options) => + multiTest("Conversions in scala.collection.JavaConversions._ are dangerous.", "javaconversions") { (msg, options) => check("import scala.collection.JavaConversions._;", msg, options) } } @Test def testUnsafeEquals(): Unit = { - tripleTest("Comparing with ==", "unsafeequals") { (msg, options) => + multiTest("Comparing with ==", "unsafeequals") { (msg, options) => // Should warn check("Nil == None", msg, options) check("""{ From 89c2db9e6be1154bf3c016c9d2a64154e09f4fab Mon Sep 17 00:00:00 2001 From: "Robert J. Macomber" Date: Sun, 18 Mar 2012 11:09:12 -0700 Subject: [PATCH 3/5] Allow setting default action --- src/main/scala/LinterPlugin.scala | 9 ++++++++- src/test/resources/testprops/defaulterror.properties | 1 + .../resources/testprops/defaultnoaction.properties | 1 + src/test/scala/LinterPluginTest.scala | 10 ++++++++++ 4 files changed, 20 insertions(+), 1 deletion(-) create mode 100644 src/test/resources/testprops/defaulterror.properties create mode 100644 src/test/resources/testprops/defaultnoaction.properties diff --git a/src/main/scala/LinterPlugin.scala b/src/main/scala/LinterPlugin.scala index 0a8340d..2fc3f0b 100644 --- a/src/main/scala/LinterPlugin.scala +++ b/src/main/scala/LinterPlugin.scala @@ -82,9 +82,16 @@ class LinterPlugin(val global: Global) extends Plugin { val Warn = underscoreify(Actions.Warn.toString) val Error = underscoreify(Actions.Error.toString) + 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), Warn) match { + val action = props.getProperty("check." + underscoreify(warning.toString), defaultAction) match { case NoAction => Actions.NoAction case Warn => Actions.Warn case Error => Actions.Error diff --git a/src/test/resources/testprops/defaulterror.properties b/src/test/resources/testprops/defaulterror.properties new file mode 100644 index 0000000..73a3c79 --- /dev/null +++ b/src/test/resources/testprops/defaulterror.properties @@ -0,0 +1 @@ +default_action = error diff --git a/src/test/resources/testprops/defaultnoaction.properties b/src/test/resources/testprops/defaultnoaction.properties new file mode 100644 index 0000000..d27dc35 --- /dev/null +++ b/src/test/resources/testprops/defaultnoaction.properties @@ -0,0 +1 @@ +default_action = no_action diff --git a/src/test/scala/LinterPluginTest.scala b/src/test/scala/LinterPluginTest.scala index c6a0188..bcaa3e4 100644 --- a/src/test/scala/LinterPluginTest.scala +++ b/src/test/scala/LinterPluginTest.scala @@ -181,4 +181,14 @@ class LinterPluginTest extends SpecsMatchers { }""", options = options) } } + + @Test + def testDefaultNoAction(): Unit = { + check("Nil == None", None, List("config:testprops/defaultnoaction.properties")) + } + + @Test + def testDefaultError(): Unit = { + check("Nil == None", Some("error: Comparing with =="), List("config:testprops/defaulterror.properties")) + } } From dd94cde655aa6262f9e09c23d143a34e7aed1b92 Mon Sep 17 00:00:00 2001 From: "Robert J. Macomber" Date: Sun, 18 Mar 2012 11:41:24 -0700 Subject: [PATCH 4/5] Document the properties file. --- README.md | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 6c4ef15..bad24b2 100644 --- a/README.md +++ b/README.md @@ -9,10 +9,20 @@ 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 :29: warning: Comparing with == on instances of different types (object Nil, object None) will probably return false. Nil == None @@ -20,6 +30,8 @@ Add it as a compiler plugin in your project, or run `sbt console` in this projec ### Unsafe `contains` +The configuration property is `check.unsafe_contains`. + scala> List(1, 2, 3).contains("4") :29: warning: SeqLike[Int].contains(java.lang.String) will probably return false. List(1, 2, 3).contains("4") @@ -28,6 +40,8 @@ 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._ :29: warning: Conversions in scala.collection.JavaConversions._ are dangerous. import scala.collection.JavaConversions._ @@ -35,6 +49,8 @@ Add it as a compiler plugin in your project, or run `sbt console` in this projec ### Calling `Option#get` +The configuration property is `check.option_get`. + scala> Option(1).get :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 From d99f3356319ae64e4bf26f74dc0c6b2a0ed5b852 Mon Sep 17 00:00:00 2001 From: "Robert J. Macomber" Date: Sun, 18 Mar 2012 15:42:35 -0700 Subject: [PATCH 5/5] Refactor warning-case detection out Presumably this plugin will end up with many more warnings. This will prevent LinterPlugin.scala from getting unweildly huge. Also it removes some boilerplate of the form case ... if warningEnable(Warnings.ThisWarning) && ... => onWarn(Warnings.ThisWarning)... That double "Warnings.ThisWarning" was setting my teeth on edge. Unfortunately, it introduces a pair of typecasts! I cannot convince scalac that my references to a Global are all references to the *same* Global. --- src/main/scala/JavaConversions.scala | 19 +++++++ src/main/scala/LinterAction.scala | 15 ++++++ src/main/scala/LinterPlugin.scala | 75 ++++++---------------------- src/main/scala/OptionGet.scala | 16 ++++++ src/main/scala/UnsafeContains.scala | 21 ++++++++ src/main/scala/UnsafeEquals.scala | 20 ++++++++ src/main/scala/Warnings.scala | 9 ++++ 7 files changed, 116 insertions(+), 59 deletions(-) create mode 100644 src/main/scala/JavaConversions.scala create mode 100644 src/main/scala/LinterAction.scala create mode 100644 src/main/scala/OptionGet.scala create mode 100644 src/main/scala/UnsafeContains.scala create mode 100644 src/main/scala/UnsafeEquals.scala create mode 100644 src/main/scala/Warnings.scala diff --git a/src/main/scala/JavaConversions.scala b/src/main/scala/JavaConversions.scala new file mode 100644 index 0000000..d4a80c2 --- /dev/null +++ b/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.") + } +} diff --git a/src/main/scala/LinterAction.scala b/src/main/scala/LinterAction.scala new file mode 100644 index 0000000..e5416ae --- /dev/null +++ b/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) + } +} diff --git a/src/main/scala/LinterPlugin.scala b/src/main/scala/LinterPlugin.scala index 2fc3f0b..eb08a78 100644 --- a/src/main/scala/LinterPlugin.scala +++ b/src/main/scala/LinterPlugin.scala @@ -32,14 +32,6 @@ class LinterPlugin(val global: Global) extends Plugin { def warningEnabled(warning: Warnings.Warning) = warningActions(warning) != Actions.NoAction - object Warnings extends Enumeration { - type Warning = Value - val JavaConversions, - OptionGet, - UnsafeContains, - UnsafeEquals = Value - } - object Actions extends Enumeration { type Action = Value val NoAction, @@ -122,57 +114,22 @@ class LinterPlugin(val global: Global) extends Plugin { } class LinterTraverser(unit: CompilationUnit) extends Traverser { - import definitions.{AnyClass, ObjectClass, Object_==, OptionClass, SeqClass} - - 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) - - def onWarn(w: Warnings.Warning) = { - if(warningActions(w) == Actions.Error) unit.error _ - else unit.warning _ - } - - def SeqMemberType(seenFrom: Type): Type = { - SeqLikeClass.tpe.typeArgs.head.asSeenFrom(seenFrom, SeqLikeClass) - } - - def isSubtype(x: Tree, y: Tree): Boolean = { - x.tpe.widen <:< y.tpe.widen - } - - def methodImplements(method: Symbol, target: Symbol): Boolean = { - method == target || method.allOverriddenSymbols.contains(target) - } - - def isGlobalImport(selector: ImportSelector): Boolean = { - selector.name == nme.WILDCARD && selector.renamePos == -1 - } - - override def traverse(tree: Tree): Unit = tree match { - case Apply(eqeq @ Select(lhs, nme.EQ), List(rhs)) - if warningEnabled(Warnings.UnsafeEquals) && 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." - onWarn(Warnings.UnsafeEquals)(eqeq.pos, warnMsg.format(lhs.tpe.widen, rhs.tpe.widen)) - - case Import(pkg, selectors) - if warningEnabled(Warnings.JavaConversions) && pkg.symbol == JavaConversionsModule && selectors.exists(isGlobalImport) => - onWarn(Warnings.JavaConversions)(pkg.pos, "Conversions in scala.collection.JavaConversions._ are dangerous.") - - case Apply(contains @ Select(seq, _), List(target)) - if warningEnabled(Warnings.UnsafeContains) && methodImplements(contains.symbol, SeqLikeContains) && !(target.tpe <:< SeqMemberType(seq.tpe)) => - val warnMsg = "SeqLike[%s].contains(%s) will probably return false." - onWarn(Warnings.UnsafeContains)(contains.pos, warnMsg.format(SeqMemberType(seq.tpe), target.tpe.widen)) - - case get @ Select(_, nme.get) - if warningEnabled(Warnings.OptionGet) && methodImplements(get.symbol, OptionGet) => - if (!get.pos.source.path.contains("src/test")) { - onWarn(Warnings.OptionGet)(get.pos, "Calling .get on Option will throw an exception if the Option is None.") - } - - case _ => - super.traverse(tree) + 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) + } } } } diff --git a/src/main/scala/OptionGet.scala b/src/main/scala/OptionGet.scala new file mode 100644 index 0000000..e06210b --- /dev/null +++ b/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.") + } +} diff --git a/src/main/scala/UnsafeContains.scala b/src/main/scala/UnsafeContains.scala new file mode 100644 index 0000000..bda17a2 --- /dev/null +++ b/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)) + } +} diff --git a/src/main/scala/UnsafeEquals.scala b/src/main/scala/UnsafeEquals.scala new file mode 100644 index 0000000..c06f77a --- /dev/null +++ b/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)) + } +} + diff --git a/src/main/scala/Warnings.scala b/src/main/scala/Warnings.scala new file mode 100644 index 0000000..9610d91 --- /dev/null +++ b/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 +}