From 225e585e53e8bad582c6f7f98b25024a4ac6a30c Mon Sep 17 00:00:00 2001 From: "tom.vaughan" Date: Sat, 28 Jul 2012 18:47:00 -0400 Subject: [PATCH 01/11] Using com.typesafe.config for externalizing some of the configuration of Linter. As of this check-in, the check for any wildcard imports now benefits from an on/off switch along with placeholders for whitelist and blacklist respect. Also adding support for the concept of "severity" of the Lint failures (just "warn" or "error" right now) Still to do: * implement the whitelist & blacklist package regex checking * convert the existing unit.warn(...) calls to use the annotateUnit( ) function * decrease the nastiness of my Java-verbose val names for the config properties --- build.sbt | 6 ++-- src/main/resources/reference.conf | 32 +++++++++++++++++++ src/main/scala/LinterPlugin.scala | 51 +++++++++++++++++++++++++++++-- 3 files changed, 84 insertions(+), 5 deletions(-) create mode 100644 src/main/resources/reference.conf diff --git a/build.sbt b/build.sbt index 1a00093..2db8381 100644 --- a/build.sbt +++ b/build.sbt @@ -1,12 +1,14 @@ libraryDependencies <++= (scalaVersion) { (scalaVer) => Seq( "org.scala-lang" % "scala-compiler" % scalaVer, + "com.typesafe" % "config" % "0.5.0", "org.scala-tools.testing" %% "specs" % "1.6.9" % "test" withSources(), "junit" % "junit" % "4.8.2" % "test" withSources(), "com.novocode" % "junit-interface" % "0.7" % "test" ) } -scalacOptions in console in Compile <+= (packageBin in Compile) map { pluginJar => - "-Xplugin:"+pluginJar +scalacOptions in console in Compile <++= (packageBin in Compile, managedClasspath in Compile) map { (pluginJar, cp) => + val configJar = cp.map(_.data).find(_.toString.contains("config-0.5.0.jar")) + Seq("-Xplugin:"+pluginJar) ++ configJar.map("-Xplugin:"+_) } diff --git a/src/main/resources/reference.conf b/src/main/resources/reference.conf new file mode 100644 index 0000000..99cd94b --- /dev/null +++ b/src/main/resources/reference.conf @@ -0,0 +1,32 @@ +# +# HOCON-standard Linter configuration to control which Lint checks to run +# For syntax help, @see https://github.com/typesafehub/config +# +linter { + + # + # Wildcard package import control - controls warnings related to importing foo.bar._ + # + package.wildcard { + + whitelist { + # Set to true to have linter check and squawk on the use of wildcard imports + checkEnabled=true + + # Comma-separated list of packages to allow for wildcard importing + packages=[] + + # What level of severity to apply to a failed Lint check for this test + severity="error" + } + + # === TODO === respect me in the morning (these conf settings are ignored by the plugin) + blacklist { + # Set to true to check for a blacklist wildcard import, regardless of the "disallow" setting + disallowBlacklist=true + + # Comma-separated list of packages in the blacklist + blacklist=["scala.collections.JavaConversions"] + } + } +} diff --git a/src/main/scala/LinterPlugin.scala b/src/main/scala/LinterPlugin.scala index 8132819..1bd104b 100644 --- a/src/main/scala/LinterPlugin.scala +++ b/src/main/scala/LinterPlugin.scala @@ -19,6 +19,7 @@ package com.foursquare.lint import scala.reflect.generic.Flags import scala.tools.nsc.{Global, Phase} import scala.tools.nsc.plugins.{Plugin, PluginComponent} +import com.typesafe.config.ConfigFactory class LinterPlugin(val global: Global) extends Plugin { import global._ @@ -26,7 +27,7 @@ class LinterPlugin(val global: Global) extends Plugin { val name = "linter" val description = "" val components = List[PluginComponent](LinterComponent) - + private object LinterComponent extends PluginComponent { import global._ @@ -44,6 +45,7 @@ class LinterPlugin(val global: Global) extends Plugin { class LinterTraverser(unit: CompilationUnit) extends Traverser { import definitions.{AnyClass, ObjectClass, Object_==, OptionClass, SeqClass} + import LinterSeverity._ val JavaConversionsModule: Symbol = definitions.getModule("scala.collection.JavaConversions") val SeqLikeClass: Symbol = definitions.getClass("scala.collection.SeqLike") @@ -66,28 +68,41 @@ class LinterPlugin(val global: Global) extends Plugin { selector.name == nme.WILDCARD && selector.renamePos == -1 } + /** + * Apply a warning or an error to some position in the code being linted + */ + def annotateUnit(pos: Position, message: String, severity: LinterSeverity.Value): Unit = severity match { + case WARN => unit.warning(pos, message) + case ERROR => unit.error(pos, message) + } + 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)) + // TODO - use annotateUnit right here + // TODO - replace this with an implementation of the blacklist from our Config case Import(pkg, selectors) if pkg.symbol == JavaConversionsModule && selectors.exists(isGlobalImport) => unit.warning(pkg.pos, "Conversions in scala.collection.JavaConversions._ are dangerous.") + // TODO - respect the whitelist exceptions case Import(pkg, selectors) - if selectors.exists(isGlobalImport) => - unit.warning(pkg.pos, "Wildcard imports should be avoided. Favor import selector clauses.") + if LinterConfig.packageWildcardWhitelistCheckEnabled && selectors.exists(isGlobalImport) => + annotateUnit(pkg.pos, "Wildcard imports should be avoided. Favor import selector clauses.", LinterConfig.packageWildcardWhitelistSeverity) 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)) + // TODO - use annotateUnit right here 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.") + // TODO - use annotateUnit right here } case _ => @@ -96,3 +111,33 @@ class LinterPlugin(val global: Global) extends Plugin { } } } + + +/** + *

Configuration class for running (or not) different lint checks

+ *

As a library, linter defaults to including the src/main/resources/reference.conf settings + * which are override-able by an including application.

+ */ +object LinterConfig { + + val config = ConfigFactory.load("linter") + config.checkValid(ConfigFactory.defaultReference(), "linter") + + val packageWildcardWhitelistCheckEnabled = config.getBoolean("linter.package.wildcard.whitelist.checkEnabled") + val packageWildcardWhitelistPackages = config.getStringList("linter.package.wildcard.whitelist.packages") + val packageWildcardWhitelistSeverity = LinterSeverity.withName(config.getString("linter.package.wildcard.whitelist.severity")) +} + +/** + * Enumeration that models the different compile-time notifications Linter + * can alert the developer to based on configuration of the different + * types of lint checks. + * + * These severities are a subset of the available notification functions on + * the @scala.tools.nsc.CompilationUnits$CompilationUnit + */ +object LinterSeverity extends Enumeration { + type LinterSeverity = Value + val WARN = Value("warn") + val ERROR = Value("error") +} From aeaf20ce820b6e1285189f0c20e4f34820c8b745 Mon Sep 17 00:00:00 2001 From: "tom.vaughan" Date: Sun, 29 Jul 2012 09:42:31 -0400 Subject: [PATCH 02/11] Moving the configuration cruft out of the LinterPlugin code --- src/main/scala/LinterConfig.scala | 18 ++++++++++++++++++ src/main/scala/LinterPlugin.scala | 15 --------------- 2 files changed, 18 insertions(+), 15 deletions(-) create mode 100644 src/main/scala/LinterConfig.scala diff --git a/src/main/scala/LinterConfig.scala b/src/main/scala/LinterConfig.scala new file mode 100644 index 0000000..beac242 --- /dev/null +++ b/src/main/scala/LinterConfig.scala @@ -0,0 +1,18 @@ +package com.foursquare.lint + +import com.typesafe.config.ConfigFactory + +/** + *

Configuration class for running (or not) different lint checks

+ *

As a library, linter defaults to including the src/main/resources/reference.conf settings + * which are override-able by an including application.

+ */ +object LinterConfig { + + val config = ConfigFactory.load("linter") + config.checkValid(ConfigFactory.defaultReference(), "linter") + + val packageWildcardWhitelistCheckEnabled = config.getBoolean("linter.package.wildcard.whitelist.checkEnabled") + val packageWildcardWhitelistPackages = config.getStringList("linter.package.wildcard.whitelist.packages") + val packageWildcardWhitelistSeverity = LinterSeverity.withName(config.getString("linter.package.wildcard.whitelist.severity")) +} diff --git a/src/main/scala/LinterPlugin.scala b/src/main/scala/LinterPlugin.scala index 1bd104b..bd302e2 100644 --- a/src/main/scala/LinterPlugin.scala +++ b/src/main/scala/LinterPlugin.scala @@ -19,7 +19,6 @@ package com.foursquare.lint import scala.reflect.generic.Flags import scala.tools.nsc.{Global, Phase} import scala.tools.nsc.plugins.{Plugin, PluginComponent} -import com.typesafe.config.ConfigFactory class LinterPlugin(val global: Global) extends Plugin { import global._ @@ -113,20 +112,6 @@ class LinterPlugin(val global: Global) extends Plugin { } -/** - *

Configuration class for running (or not) different lint checks

- *

As a library, linter defaults to including the src/main/resources/reference.conf settings - * which are override-able by an including application.

- */ -object LinterConfig { - - val config = ConfigFactory.load("linter") - config.checkValid(ConfigFactory.defaultReference(), "linter") - - val packageWildcardWhitelistCheckEnabled = config.getBoolean("linter.package.wildcard.whitelist.checkEnabled") - val packageWildcardWhitelistPackages = config.getStringList("linter.package.wildcard.whitelist.packages") - val packageWildcardWhitelistSeverity = LinterSeverity.withName(config.getString("linter.package.wildcard.whitelist.severity")) -} /** * Enumeration that models the different compile-time notifications Linter From bb9e7f8f7e093be52811da84ac209f0a3225b96c Mon Sep 17 00:00:00 2001 From: "tom.vaughan" Date: Sun, 29 Jul 2012 09:51:12 -0400 Subject: [PATCH 03/11] Moving default wildcard import severity down to warn --- src/main/resources/reference.conf | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/resources/reference.conf b/src/main/resources/reference.conf index 99cd94b..31767fb 100644 --- a/src/main/resources/reference.conf +++ b/src/main/resources/reference.conf @@ -16,8 +16,8 @@ linter { # Comma-separated list of packages to allow for wildcard importing packages=[] - # What level of severity to apply to a failed Lint check for this test - severity="error" + # What level of severity to apply to a failed Lint check for this test (one of "warn" or "error") + severity="warn" } # === TODO === respect me in the morning (these conf settings are ignored by the plugin) From b9777fd4350bb1fda3d81ddb6a4b1f0f1b16b269 Mon Sep 17 00:00:00 2001 From: "tom.vaughan" Date: Sun, 29 Jul 2012 09:51:36 -0400 Subject: [PATCH 04/11] Need to import the LinterConfig._ attributes to avoid compile errors --- src/main/scala/LinterPlugin.scala | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/scala/LinterPlugin.scala b/src/main/scala/LinterPlugin.scala index bd302e2..7350c96 100644 --- a/src/main/scala/LinterPlugin.scala +++ b/src/main/scala/LinterPlugin.scala @@ -19,6 +19,7 @@ package com.foursquare.lint import scala.reflect.generic.Flags import scala.tools.nsc.{Global, Phase} import scala.tools.nsc.plugins.{Plugin, PluginComponent} +import com.foursquare.lint.LinterConfig._ class LinterPlugin(val global: Global) extends Plugin { import global._ From 6fa4d65cc6be370b12689873b98d0fa74d2a839f Mon Sep 17 00:00:00 2001 From: "tom.vaughan" Date: Sun, 29 Jul 2012 10:01:40 -0400 Subject: [PATCH 05/11] Adding configuration for the eqeq check --- src/main/resources/reference.conf | 11 +++++++++++ src/main/scala/LinterConfig.scala | 3 +++ src/main/scala/LinterPlugin.scala | 12 +++++++----- 3 files changed, 21 insertions(+), 5 deletions(-) diff --git a/src/main/resources/reference.conf b/src/main/resources/reference.conf index 31767fb..1e00eeb 100644 --- a/src/main/resources/reference.conf +++ b/src/main/resources/reference.conf @@ -4,6 +4,17 @@ # linter { + # + # EqualsEquals comparison check on objects of different types + # + eqeq { + # Set to true to have linter check for using "==" on different types + checkEnabled=true + + # What level of severity to apply to a failed Lint check (one of "warn" or "error") + severity="error" + } + # # Wildcard package import control - controls warnings related to importing foo.bar._ # diff --git a/src/main/scala/LinterConfig.scala b/src/main/scala/LinterConfig.scala index beac242..4a26a5d 100644 --- a/src/main/scala/LinterConfig.scala +++ b/src/main/scala/LinterConfig.scala @@ -12,6 +12,9 @@ object LinterConfig { val config = ConfigFactory.load("linter") config.checkValid(ConfigFactory.defaultReference(), "linter") + val eqeqCheckEnabled = config.getBoolean("linter.eqeq.checkEnabled") + val eqeqSeverity = LinterSeverity.withName(config.getString("linter.eqeq.severity")) + val packageWildcardWhitelistCheckEnabled = config.getBoolean("linter.package.wildcard.whitelist.checkEnabled") val packageWildcardWhitelistPackages = config.getStringList("linter.package.wildcard.whitelist.packages") val packageWildcardWhitelistSeverity = LinterSeverity.withName(config.getString("linter.package.wildcard.whitelist.severity")) diff --git a/src/main/scala/LinterPlugin.scala b/src/main/scala/LinterPlugin.scala index 7350c96..6457115 100644 --- a/src/main/scala/LinterPlugin.scala +++ b/src/main/scala/LinterPlugin.scala @@ -77,11 +77,13 @@ class LinterPlugin(val global: Global) extends Plugin { } override def traverse(tree: Tree): Unit = tree match { + // eqeq check case Apply(eqeq @ Select(lhs, nme.EQ), List(rhs)) - if methodImplements(eqeq.symbol, Object_==) && !(isSubtype(lhs, rhs) || isSubtype(rhs, lhs)) => + if eqeqCheckEnabled + && 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)) - // TODO - use annotateUnit right here + annotateUnit(eqeq.pos, warnMsg.format(lhs.tpe.widen, rhs.tpe.widen), eqeqSeverity) // TODO - replace this with an implementation of the blacklist from our Config case Import(pkg, selectors) @@ -90,8 +92,8 @@ class LinterPlugin(val global: Global) extends Plugin { // TODO - respect the whitelist exceptions case Import(pkg, selectors) - if LinterConfig.packageWildcardWhitelistCheckEnabled && selectors.exists(isGlobalImport) => - annotateUnit(pkg.pos, "Wildcard imports should be avoided. Favor import selector clauses.", LinterConfig.packageWildcardWhitelistSeverity) + if packageWildcardWhitelistCheckEnabled && selectors.exists(isGlobalImport) => + annotateUnit(pkg.pos, "Wildcard imports should be avoided. Favor import selector clauses.", packageWildcardWhitelistSeverity) case Apply(contains @ Select(seq, _), List(target)) if methodImplements(contains.symbol, SeqLikeContains) && !(target.tpe <:< SeqMemberType(seq.tpe)) => From 6ae9fb98c792f62101526cb6da70d228bf8366ae Mon Sep 17 00:00:00 2001 From: "tom.vaughan" Date: Sun, 29 Jul 2012 10:07:27 -0400 Subject: [PATCH 06/11] Moving LinterSeverity enumeration out of LinerConfig and cleaning up some import statements --- src/main/scala/LinterConfig.scala | 2 +- src/main/scala/LinterPlugin.scala | 17 +---------------- src/main/scala/LinterSeverity.scala | 15 +++++++++++++++ 3 files changed, 17 insertions(+), 17 deletions(-) create mode 100644 src/main/scala/LinterSeverity.scala diff --git a/src/main/scala/LinterConfig.scala b/src/main/scala/LinterConfig.scala index 4a26a5d..a5ccd6d 100644 --- a/src/main/scala/LinterConfig.scala +++ b/src/main/scala/LinterConfig.scala @@ -8,7 +8,7 @@ import com.typesafe.config.ConfigFactory * which are override-able by an including application.

*/ object LinterConfig { - + val config = ConfigFactory.load("linter") config.checkValid(ConfigFactory.defaultReference(), "linter") diff --git a/src/main/scala/LinterPlugin.scala b/src/main/scala/LinterPlugin.scala index 6457115..ff3b180 100644 --- a/src/main/scala/LinterPlugin.scala +++ b/src/main/scala/LinterPlugin.scala @@ -19,7 +19,6 @@ package com.foursquare.lint import scala.reflect.generic.Flags import scala.tools.nsc.{Global, Phase} import scala.tools.nsc.plugins.{Plugin, PluginComponent} -import com.foursquare.lint.LinterConfig._ class LinterPlugin(val global: Global) extends Plugin { import global._ @@ -46,6 +45,7 @@ class LinterPlugin(val global: Global) extends Plugin { class LinterTraverser(unit: CompilationUnit) extends Traverser { import definitions.{AnyClass, ObjectClass, Object_==, OptionClass, SeqClass} import LinterSeverity._ + import LinterConfig._ val JavaConversionsModule: Symbol = definitions.getModule("scala.collection.JavaConversions") val SeqLikeClass: Symbol = definitions.getClass("scala.collection.SeqLike") @@ -114,18 +114,3 @@ class LinterPlugin(val global: Global) extends Plugin { } } - - -/** - * Enumeration that models the different compile-time notifications Linter - * can alert the developer to based on configuration of the different - * types of lint checks. - * - * These severities are a subset of the available notification functions on - * the @scala.tools.nsc.CompilationUnits$CompilationUnit - */ -object LinterSeverity extends Enumeration { - type LinterSeverity = Value - val WARN = Value("warn") - val ERROR = Value("error") -} diff --git a/src/main/scala/LinterSeverity.scala b/src/main/scala/LinterSeverity.scala new file mode 100644 index 0000000..79b0ee9 --- /dev/null +++ b/src/main/scala/LinterSeverity.scala @@ -0,0 +1,15 @@ +package com.foursquare.lint + +/** + * Enumeration that models the different compile-time notifications Linter + * can alert the developer to based on configuration of the different + * types of lint checks. + * + * These severities are a subset of the available notification functions on + * the @scala.tools.nsc.CompilationUnits$CompilationUnit + */ +object LinterSeverity extends Enumeration { + type LinterSeverity = Value + val WARN = Value("warn") + val ERROR = Value("error") +} From e48c6a66694bd3ad19ab478b968a92d9f6c2134d Mon Sep 17 00:00:00 2001 From: "tom.vaughan" Date: Sun, 29 Jul 2012 10:15:47 -0400 Subject: [PATCH 07/11] Adding configuration for checking sequence[A].contains(B) where A and B are different types --- src/main/resources/reference.conf | 14 ++++++++++++++ src/main/scala/LinterConfig.scala | 3 +++ src/main/scala/LinterPlugin.scala | 7 ++++--- 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/src/main/resources/reference.conf b/src/main/resources/reference.conf index 1e00eeb..7ad1098 100644 --- a/src/main/resources/reference.conf +++ b/src/main/resources/reference.conf @@ -40,4 +40,18 @@ linter { blacklist=["scala.collections.JavaConversions"] } } + + # + # Sequence/Collection related checks + # + seq { + # Checks for Seq.contains calls with different types + contains { + # Set to true to check for use of a Seq[A].contains(B) call where A & B are different types + checkEnabled=true + + # What level of severity to apply to a failed check (one of "warn" or "error") + severity=warn + } + } } diff --git a/src/main/scala/LinterConfig.scala b/src/main/scala/LinterConfig.scala index a5ccd6d..9509c2e 100644 --- a/src/main/scala/LinterConfig.scala +++ b/src/main/scala/LinterConfig.scala @@ -18,4 +18,7 @@ object LinterConfig { val packageWildcardWhitelistCheckEnabled = config.getBoolean("linter.package.wildcard.whitelist.checkEnabled") val packageWildcardWhitelistPackages = config.getStringList("linter.package.wildcard.whitelist.packages") val packageWildcardWhitelistSeverity = LinterSeverity.withName(config.getString("linter.package.wildcard.whitelist.severity")) + + val seqContainsCheckEnabled = config.getBoolean("linter.seq.contains.checkEnabled") + val seqContainsSeverity = LinterSeverity.withName(config.getString("linter.seq.contains.severity")) } diff --git a/src/main/scala/LinterPlugin.scala b/src/main/scala/LinterPlugin.scala index ff3b180..08b1c04 100644 --- a/src/main/scala/LinterPlugin.scala +++ b/src/main/scala/LinterPlugin.scala @@ -96,10 +96,11 @@ class LinterPlugin(val global: Global) extends Plugin { annotateUnit(pkg.pos, "Wildcard imports should be avoided. Favor import selector clauses.", packageWildcardWhitelistSeverity) case Apply(contains @ Select(seq, _), List(target)) - if methodImplements(contains.symbol, SeqLikeContains) && !(target.tpe <:< SeqMemberType(seq.tpe)) => + if seqContainsCheckEnabled + && 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)) - // TODO - use annotateUnit right here + annotateUnit(contains.pos, warnMsg.format(SeqMemberType(seq.tpe), target.tpe.widen), seqContainsSeverity) case get @ Select(_, nme.get) if methodImplements(get.symbol, OptionGet) => if (!get.pos.source.path.contains("src/test")) { From 0134ad4b1aa188f7ba104eaadbd4bb02f078a2e5 Mon Sep 17 00:00:00 2001 From: "tom.vaughan" Date: Sun, 29 Jul 2012 10:41:23 -0400 Subject: [PATCH 08/11] Adding configuration for the Option.get check and making the configuration documentation more concise --- src/main/resources/reference.conf | 28 +++++++++++++++------------- src/main/scala/LinterConfig.scala | 3 +++ src/main/scala/LinterPlugin.scala | 10 +++++----- 3 files changed, 23 insertions(+), 18 deletions(-) diff --git a/src/main/resources/reference.conf b/src/main/resources/reference.conf index 7ad1098..eac60c6 100644 --- a/src/main/resources/reference.conf +++ b/src/main/resources/reference.conf @@ -2,16 +2,16 @@ # HOCON-standard Linter configuration to control which Lint checks to run # For syntax help, @see https://github.com/typesafehub/config # +# Unless otherwise stated, any "checkEnabled" configuration is one of {"true", "false"} +# Unless otherwise stated, any "severity" configuration is one of {"warn", "error"} +# linter { # # EqualsEquals comparison check on objects of different types # eqeq { - # Set to true to have linter check for using "==" on different types checkEnabled=true - - # What level of severity to apply to a failed Lint check (one of "warn" or "error") severity="error" } @@ -21,20 +21,17 @@ linter { package.wildcard { whitelist { - # Set to true to have linter check and squawk on the use of wildcard imports checkEnabled=true - + severity="warn" + # Comma-separated list of packages to allow for wildcard importing packages=[] - - # What level of severity to apply to a failed Lint check for this test (one of "warn" or "error") - severity="warn" } # === TODO === respect me in the morning (these conf settings are ignored by the plugin) blacklist { - # Set to true to check for a blacklist wildcard import, regardless of the "disallow" setting - disallowBlacklist=true + # Set to true to check for a blacklist wildcard import, regardless of the "whitelist" setting + checkEnabled=true # Comma-separated list of packages in the blacklist blacklist=["scala.collections.JavaConversions"] @@ -47,11 +44,16 @@ linter { seq { # Checks for Seq.contains calls with different types contains { - # Set to true to check for use of a Seq[A].contains(B) call where A & B are different types checkEnabled=true - - # What level of severity to apply to a failed check (one of "warn" or "error") severity=warn } } + + # + # Option.get check (TODO - is there a broader classification that makes for better organization?) + # + option.get { + checkEnabled=true + severity=warn + } } diff --git a/src/main/scala/LinterConfig.scala b/src/main/scala/LinterConfig.scala index 9509c2e..ecc809c 100644 --- a/src/main/scala/LinterConfig.scala +++ b/src/main/scala/LinterConfig.scala @@ -21,4 +21,7 @@ object LinterConfig { val seqContainsCheckEnabled = config.getBoolean("linter.seq.contains.checkEnabled") val seqContainsSeverity = LinterSeverity.withName(config.getString("linter.seq.contains.severity")) + + val optionGetCheckEnabled = config.getBoolean("linter.option.get.checkEnabled") + val optionGetSeverity = LinterSeverity.withName(config.getString("linter.option.get.severity")) } diff --git a/src/main/scala/LinterPlugin.scala b/src/main/scala/LinterPlugin.scala index 08b1c04..df854f1 100644 --- a/src/main/scala/LinterPlugin.scala +++ b/src/main/scala/LinterPlugin.scala @@ -102,11 +102,11 @@ class LinterPlugin(val global: Global) extends Plugin { val warnMsg = "SeqLike[%s].contains(%s) will probably return false." annotateUnit(contains.pos, warnMsg.format(SeqMemberType(seq.tpe), target.tpe.widen), seqContainsSeverity) - 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.") - // TODO - use annotateUnit right here - } + case get @ Select(_, nme.get) + if optionGetCheckEnabled + && methodImplements(get.symbol, OptionGet) + && !(get.pos.source.path.contains("src/test")) => + annotateUnit(get.pos, "Calling .get on Option will throw an exception if the Option is None.", optionGetSeverity) case _ => super.traverse(tree) From 41a1cb2dc12b2b06e0ba881f5bf9ada91381b7a3 Mon Sep 17 00:00:00 2001 From: "tom.vaughan" Date: Sun, 29 Jul 2012 10:43:26 -0400 Subject: [PATCH 09/11] Re-organizing the option.get configuration to parallel the seq.contains configuration --- src/main/resources/reference.conf | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/main/resources/reference.conf b/src/main/resources/reference.conf index eac60c6..75d84b9 100644 --- a/src/main/resources/reference.conf +++ b/src/main/resources/reference.conf @@ -50,10 +50,13 @@ linter { } # - # Option.get check (TODO - is there a broader classification that makes for better organization?) + # Option related checks (TODO - is there a broader classification that makes for better organization?) # - option.get { - checkEnabled=true - severity=warn + option { + # Check for calling Option.get + get { + checkEnabled=true + severity=warn + } } } From 5155d2422074cd24278b96e8a370e76e52f0f382 Mon Sep 17 00:00:00 2001 From: "tom.vaughan" Date: Sun, 29 Jul 2012 10:53:02 -0400 Subject: [PATCH 10/11] Adding configuration documentation to the README and updating the TODOs and Future Work section --- README.md | 39 ++++++++++++++++++++++++++++++++++++--- 1 file changed, 36 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index a9fcec1..aeaa052 100644 --- a/README.md +++ b/README.md @@ -17,7 +17,11 @@ Or, until published: Optionally, run `sbt console` in this project to see it in action. -## Currently suported warnings +## Currently supported warnings + +Note: for configuration documentation, unless otherwise stated, any "checkEnabled" configuration option +is one of {"true", "false"} and any severity configuration option is one of {"warn", "error"}. +Configuration documentation examples show the default settings that ship with this linter library. ### Unsafe `==` @@ -26,6 +30,10 @@ Optionally, run `sbt console` in this project to see it in action. Nil == None ^ + Configuration: + linter.eqeq.checkEnabled=true + linter.eqeq.severity=error + ### Unsafe `contains` scala> List(1, 2, 3).contains("4") @@ -33,6 +41,10 @@ Optionally, run `sbt console` in this project to see it in action. List(1, 2, 3).contains("4") ^ + Configuration: + linter.seq.contains.checkEnabled=true + linter.seq.contains.severity=warn + ### Wildcard import from `scala.collection.JavaConversions` @@ -41,6 +53,9 @@ Optionally, run `sbt console` in this project to see it in action. import scala.collection.JavaConversions._ ^ + Configuration: + None at present + ### Any and all wildcard imports scala> import scala.collection.JavaConversions._ @@ -48,6 +63,11 @@ Optionally, run `sbt console` in this project to see it in action. import scala.reflect.generic._ + Configuration: + linter.package.wildcard.whitelist.checkEnabled=true + linter.package.wildcard.whitelist.severity=warn + linter.package.wildcard.whitelist.packages=[] + ### Calling `Option#get` scala> Option(1).get @@ -55,11 +75,24 @@ Optionally, run `sbt console` in this project to see it in action. Option(1).get ^ + Configuration: + linter.option.get.checkEnabled=true + linter.option.get.severity=warn + +## Configuring Linter + +Linter is configured with the com.typesafe.config library (https://github.com/typesafehub/config) + +In your application, you can override any of the configuration options from these locations (in priority-respecting order): +* system properties +* application.conf available on the classpath +* application.json available on the classpath +* application.properties available on the classpath + ## Future Work * Add more warnings -* Pick and choose which warnings you want -* Choose whether they should be warnings or errors +* Support "blacklist"ing imports and re-write the scala.collection.JavaCollections check with that mechanism ### Ideas for new warnings From 089c33abcf6d2d4186e7ffe8abf49ff6bb4e8c29 Mon Sep 17 00:00:00 2001 From: "tom.vaughan" Date: Sun, 29 Jul 2012 12:57:11 -0400 Subject: [PATCH 11/11] Checking in a commented-out version of my attempt to respect the whitelist list. The problem I ran in to is that the "case Import(pkg, selectors)" has the pkg of type Tree, which when queried for the pkg.symbol only returns "util" (when you're importing java.util._ for example). We need to figure out how to grab the ancestors (?) of the pkg to get the entire "java.util" package so that we can match on specific whitelist exclusions. --- src/main/scala/LinterPlugin.scala | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/main/scala/LinterPlugin.scala b/src/main/scala/LinterPlugin.scala index df854f1..3bcf4ae 100644 --- a/src/main/scala/LinterPlugin.scala +++ b/src/main/scala/LinterPlugin.scala @@ -92,7 +92,16 @@ class LinterPlugin(val global: Global) extends Plugin { // TODO - respect the whitelist exceptions case Import(pkg, selectors) - if packageWildcardWhitelistCheckEnabled && selectors.exists(isGlobalImport) => + if packageWildcardWhitelistCheckEnabled + && selectors.exists(isGlobalImport) + + /* For a statement like "import java.util._", + pkg.symbol = "util" + pkg.class = class scala.reflect.generic.Trees$Select + We need to figure out how to get the "ancestor symbol(s)" of pkg to do string matching on the whole package name + + && !(packageWildcardWhitelistPackages.contains(pkg.symbol)) */ => + println("The class of pkg is " + pkg.getClass) annotateUnit(pkg.pos, "Wildcard imports should be avoided. Favor import selector clauses.", packageWildcardWhitelistSeverity) case Apply(contains @ Select(seq, _), List(target))