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 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 ec73b4c..eb08a78 100644 --- a/src/main/scala/LinterPlugin.scala +++ b/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) + } } } } 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 +} 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 new file mode 100644 index 0000000..153bf52 --- /dev/null +++ b/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 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 new file mode 100644 index 0000000..3902ad9 --- /dev/null +++ b/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 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 new file mode 100644 index 0000000..90d0c10 --- /dev/null +++ b/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 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 new file mode 100644 index 0000000..a005828 --- /dev/null +++ b/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 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 new file mode 100644 index 0000000..f5a78d1 --- /dev/null +++ b/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 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/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 new file mode 100644 index 0000000..9524f53 --- /dev/null +++ b/src/test/resources/testprops/javaconversionsoff.properties @@ -0,0 +1 @@ +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 new file mode 100644 index 0000000..74c3c7f --- /dev/null +++ b/src/test/resources/testprops/optiongetoff.properties @@ -0,0 +1 @@ +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 new file mode 100644 index 0000000..3e36be8 --- /dev/null +++ b/src/test/resources/testprops/unsafecontainsoff.properties @@ -0,0 +1 @@ +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 new file mode 100644 index 0000000..6089438 --- /dev/null +++ b/src/test/resources/testprops/unsafeequalsoff.properties @@ -0,0 +1 @@ +check.unsafe_equals = no_action diff --git a/src/test/scala/LinterPluginTest.scala b/src/test/scala/LinterPluginTest.scala index 20b0a00..bcaa3e4 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} @@ -35,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() @@ -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 } @@ -58,18 +69,29 @@ 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") } } } - 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,92 @@ 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 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("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 testNoOptionGet(): Unit = { - val msg = Some("Calling .get on Option will throw an exception if the Option is None.") + def testHasVersusContains(): Unit = { + multiTest("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 = { + 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) + 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) + multiTest("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" - }""") + multiTest("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) + } + } + + @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")) } }