Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Extern config #8

Open
wants to merge 11 commits into from

1 participant

@tvaughan77

This branch contains the integration with com.typesafe.config to externalize the on/off switches of the different checks we might want to run and to introduce a "Severity" level of the check failure messages.

I ran in to a bit of a wall when actually implementing the whitelist because when the compiler plugin's traverse() method gets handed an "import java.util._" statement (for example), the "pkg" paremeter passed to the Import(pkg, selectors) case statement is actually just the Tree "util", so we gotta figure out how to walk up that Tree and get the complete "java.util" statement so that we can check for its string existing in our List of configured whitelists.

tvaughan77 added some commits
@tvaughan77 tvaughan77 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
225e585
@tvaughan77 tvaughan77 Moving the configuration cruft out of the LinterPlugin code aeaf20c
@tvaughan77 tvaughan77 Moving default wildcard import severity down to warn bb9e7f8
@tvaughan77 tvaughan77 Need to import the LinterConfig._ attributes to avoid compile errors b9777fd
@tvaughan77 tvaughan77 Adding configuration for the eqeq check 6fa4d65
@tvaughan77 tvaughan77 Moving LinterSeverity enumeration out of LinerConfig and cleaning up …
…some import statements
6ae9fb9
@tvaughan77 tvaughan77 Adding configuration for checking sequence[A].contains(B) where A and…
… B are different types
e48c6a6
@tvaughan77 tvaughan77 Adding configuration for the Option.get check and making the configur…
…ation documentation more concise
0134ad4
@tvaughan77 tvaughan77 Re-organizing the option.get configuration to parallel the seq.contai…
…ns configuration
41a1cb2
@tvaughan77 tvaughan77 Adding configuration documentation to the README and updating the TOD…
…Os and Future Work section
5155d24
@tvaughan77 tvaughan77 Checking in a commented-out version of my attempt to respect the whit…
…elist 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.
089c33a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 28, 2012
  1. @tvaughan77

    Using com.typesafe.config for externalizing some of the configuration…

    tvaughan77 authored
    … 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
Commits on Jul 29, 2012
  1. @tvaughan77
  2. @tvaughan77
  3. @tvaughan77
  4. @tvaughan77
  5. @tvaughan77
  6. @tvaughan77
  7. @tvaughan77

    Adding configuration for the Option.get check and making the configur…

    tvaughan77 authored
    …ation documentation more concise
  8. @tvaughan77
  9. @tvaughan77
  10. @tvaughan77

    Checking in a commented-out version of my attempt to respect the whit…

    tvaughan77 authored
    …elist 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.
This page is out of date. Refresh to see the latest.
View
39 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
View
6 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:"+_)
}
View
62 src/main/resources/reference.conf
@@ -0,0 +1,62 @@
+#
+# 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 {
+ checkEnabled=true
+ severity="error"
+ }
+
+ #
+ # Wildcard package import control - controls warnings related to importing foo.bar._
+ #
+ package.wildcard {
+
+ whitelist {
+ checkEnabled=true
+ severity="warn"
+
+ # Comma-separated list of packages to allow for wildcard importing
+ packages=[]
+ }
+
+ # === 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 "whitelist" setting
+ checkEnabled=true
+
+ # Comma-separated list of packages in the blacklist
+ blacklist=["scala.collections.JavaConversions"]
+ }
+ }
+
+ #
+ # Sequence/Collection related checks
+ #
+ seq {
+ # Checks for Seq.contains calls with different types
+ contains {
+ checkEnabled=true
+ severity=warn
+ }
+ }
+
+ #
+ # Option related checks (TODO - is there a broader classification that makes for better organization?)
+ #
+ option {
+ # Check for calling Option.get
+ get {
+ checkEnabled=true
+ severity=warn
+ }
+ }
+}
View
27 src/main/scala/LinterConfig.scala
@@ -0,0 +1,27 @@
+package com.foursquare.lint
+
+import com.typesafe.config.ConfigFactory
+
+/**
+ * <p>Configuration class for running (or not) different lint checks</p>
+ * <p>As a library, linter defaults to including the src/main/resources/reference.conf settings
+ * which are override-able by an including application.</p>
+ */
+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"))
+
+ 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"))
+}
View
50 src/main/scala/LinterPlugin.scala
@@ -26,7 +26,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 +44,8 @@ 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")
@@ -66,29 +68,54 @@ 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 {
+ // 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))
+ 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)
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 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))
- 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))
+ 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.")
- }
+ 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)
@@ -96,3 +123,4 @@ class LinterPlugin(val global: Global) extends Plugin {
}
}
}
+
View
15 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")
+}
Something went wrong with that request. Please try again.