Toward a Safer Scala

Detect errors earlier with �static analysis

Gallatin Peak

​

Thanks,

Thomas!

Gallatin Peak

@leifwickland

tinyurl.com/

pnwslint

Why Static Analysis?

• scalac enables some error-prone code
• Some Scala features demo well, work poorly
• Coding standards
• Without enforcement, only suggestions
• Executable best practices, rather than Markdown
• Code review
• Automate it
• Works when expert or OSS project leader is out

What You Deserve

• Better compiler
• Inferring Any is the root of much WTF
• Invalid string interpolation references

What You Deserve

• Better compiler
• More hygienic code
• Two spaces, no tabs, no discussion
• Limits on line, function, and file length

What You Deserve

• Better compiler
• More hygienic code
• Higher quality code
• When I said no nulls, I meant no nulls
• Regret every var

What You Deserve

• Better compiler
• More hygienic code
• Higher quality code
• Fewer bugs
• Runtime exploding String.format()
• What is the head of an empty List?

Static Analysis Options

• IDE-based analysis
• scalac switches
• FindBugs
• Scalastyle
• Abide
• WartRemover
• Linter
• Scapegoat

IDE-based Options

If not in release build process, it doesn’t exist.

​

If you build your releases with your IDE, I’m very sorry for you.

Criteria

• Signal to Noise
• ROI for errors avoided
• False positives
• Suppression
• Possible
• Limited Scope
• Learning curve
• Build integration
• Documentation

Source

???

Source

Reference uninitialized value

Good: Scala 2.11 warns

​

Bad: Only warns on change or clean

​

Ugly: Can still ship it

​

​

Seth Tisue says:

“I believe almost any well-managed project should be using �-Xfatal-warnings”�

in SI-7529

​

I agree with Seth.�Is your project well-managed?

scalac switches

Recommendation: Enable fatal warnings

scalacOptions ++= Seq(

"-Xfatal-warnings"

)

Set(1).equals()

Adapted Arguments

Set(1).equals()

Compiles�Always false

List(1,2,3).toSet()

From Daniel Worthington-Bodart’s blog

Adapted Arguments

List(1,2,3).toSet()

Compiles�Always false

Adapted Arguments

Good: Deprecated around 2.10

​

Bad: Deprecation is just a warning

​

​

Adapted Arguments

Ugly: Compiler reports�"re-run with -deprecation for details"

​

Uglier: Enabling -deprecation and -Xfatal-warnings means you can’t use deprecated dependencies as stopgap

​

scalac switches

Recommend: Strive to enable -deprecation

​

​

​

​

Caveat: Hassle when upgrading dependencies.

scalacOptions ++= Seq(

"-deprecation",

"-Xfatal-warnings"

)

logger.error(

"Failed to open %s. Error: %d"� .format(file)�)

Wrong number of args to format()

logger.error(

"Failed to open %s. Error: %d"� .format(file)�)

Runtime Exception

logger.error(

"Failed to open $file." + � "Error: $IoError"�)

Oh, you wanted to interpolate that?

logger.error(

"Failed to open $file." + � "Error: $IoError"�)

Mo’ money. Mo’ problems.

String Interpolation With -Xlint

logger.error(

"Failed to open $file." + � "Error: $IoError"�)

Missing interpolator

scalac switches

Recommendation: Enable -Xlint

​

​

​

​

Bonus: 2.11.4 allows selective -Xlint enabling

scalacOptions ++= Seq(

"-Xlint”,

"-deprecation",

"-Xfatal-warnings"

)

sealed trait ADT

object A extends ADT

object B extends ADT

final val C = 3

val adt = Seq(A,B,C)

sealed trait ADT

object A extends ADT

object B extends ADT

final val C = 3

val adt = Seq(A,B,C)

Inferred Seq[Any]

final val As = Seq(1, 2)

final val Bs = Seq("3", "4")

val crossProduct = As.flatMap { a =>

Bs.flatMap { b => Seq(a, b) }

}

final val As = Seq(1, 2)

final val Bs = Seq("3", "4")

val crossProduct = As.flatMap { a =>

Bs.flatMap { b => Seq(a, b) }

}

Inferred Seq[Any]

Inferring Any With -Xlint

You’re right, scalac! Thank you!

A type was inferred to be `Any`; this may indicate a programming error.

scalac switches

Recommendation: No seriously enable -Xlint

​

​

​

​

​

scalacOptions ++= Seq(

"-Xlint”,

"-deprecation",

"-Xfatal-warnings"

)

scalac switches

Signal/Noise

Suppression

Learning Curve

FindBugs

• Venerable Java static analysis tool
• findbugs4sbt plugs it into sbt
• High false positive rate for Scala
• Baffled by synthetic code

FindBugs

?

?

Signal/Noise

Suppression

Learning Curve

Scalastyle

• Oriented toward “style”
• Uses scalariform’s parser
• Not a scalac plugin
• sbt plugin for easy integration
• Executed via scalastyle command
• Can be tied to compile or test commands
• Used by Martin O’s Coursera classes
• Mature, fairly well-documented project

Incrementally configured via XML

<scalastyle>

<check level="error" class="NullChecker" enabled="false">

<check level="error" class="FileLengthChecker" enabled="true">

<parameters>

<parameter name="maxFileLength"><![CDATA[1500]]></parameter>

</parameters>

</check>

</scalastyle>

Incrementally configured via XML

<scalastyle>

<check level="error" class="NullChecker" enabled="true">

<check level="error" class="FileLengthChecker" enabled="true">

<parameters>

<parameter name="maxFileLength"><![CDATA[1500]]></parameter>

</parameters>

</check>

</scalastyle>

Incrementally configured via XML

<scalastyle>

<check level="error" class="NullChecker" enabled="true">

<check level="error" class="FileLengthChecker" enabled="true">

<parameters>

<parameter name="maxFileLength"><![CDATA[750]]></parameter>

</parameters>

</check>

</scalastyle>

Suppress via comments

Consistent with Rule 0.1

// scalastyle:off null

TerribleJavaApi.madeMe(null)

// scalastyle:on null

Suppress via comments

Bad.api(null) // scalastyle:ignore null

1. Executable coding standard
2. Ban “Better Java”
3. Encourage Good Practices

Executable coding standard

• Line, file, and function length
• Cyclomatic complexity
• Argument list length
• Functions per type
• Indentation
• Magic numbers
• Identifier names match regex
• Require/ban space near punctuation

Ban “Better Java”

• Ban null
• Ban var
• Ban return
• Ban while
• Ban clone() method

Encourage Good Practices

• Explicit return type for public methods
• Nested methods require suppression
• Don’t ship ??? and println
• Require equals/hashCode if other defined
• Any equals must override Object.equals
• Disallow structural types
• Type lambdas require suppression
• Simplify boolean expressions

Scalastyle Extensibility

• Simple: Regexes to find banned patterns
• Complex: Custom rules are possible but, cumbersome

Scalastyle

• Rule set feels broad and unfocused
• Implements rules for both sides of controversies

Scalastyle

Signal/Noise

Suppression

Learning Curve

If your coding standard isn’t automatically and uniformly applied, you don’t have a coding standard.

Abide

• Very new project
• 0.1-SNAPSHOT
• Must build from source. Artifacts not online.
• github.com/scala/scala-abide
• Driven by Typesafe
• Iulian Dragos
• Adriaan Moors
• Documentation is work in progress

Rules have mainstream flavor

• Only bans var in public context
• Detects when a var instead of val
• Unused locals detection. WANT!
• Local vars, defs
• Private class members
• Method arguments
• Akka specific rules
• Avoid capturing unintended context

Abide

?

Signal/Noise

Suppression

Learning Curve

WartRemover

• Brian McKenna
• Moving forward fast
• Accurately and well documented
• Easy sbt integration
• github.com/typelevel/wartremover
• Extensibility is reasonably well documented

val testUsers = Seq(

("Flintstone, Fred", 42),

("Rubble", "Betty", 35)

)

val testUsers = Seq(

("Flintstone, Fred", 42),

("Rubble", "Betty", 35)

)

Inferred Seq[Product]

val testUsers = Seq(

("Flintstone, Fred", 42),

("Rubble", "Betty", 35)

)

Inferred Seq[Product with Serializable]

• Avoid Problematic Inference
• Ban “Better Java”
• Partial Methods which throw

Avoid Problematic Inference

• Product
• Serializable
• Nothing
• Any

Ban “Better Java”

• Ban null
• Ban var
• Ban return

Partial Methods which throw

• List.head, .tail, .last
• Option.get
• scala.util.Either.get

wartremoverErrors ++= Warts.all

​

​

wartremoverErrors ++= Seq(

Wart.Any,

Wart.Nothing,

Wart.Serializable,

Wart.Product

)

wartremoverExcluded ++= Seq(

"com.bad.package",

"com.good.DirtyClass"

)

WartRemover

Signal/Noise

Suppression

Learning Curve

WartRemover

• Focused and opinionated
• Recommendation: Best suited to
1. Greenfield, or
2. Already clean projects, or
3. Writing the typelevel-y-est Scala

def f(s: String) = {

s.split(':') match {

case Array(k,v) => k -> v

case a => error("bad: " + a)

}

}

def f(s: String) = {

s.split(':') match {

case Array(k,v) => k -> v

case a => error("bad: " + a)

}

}

Array.toString()�bad: [LString;@6f89341e

def g(l: List) = {

if (l.length == 0) 0

else l.reduce(...)

}

def g(l: List) = {

if (l.length == 0) 0

else l.reduce(...)

}

List.length is O(n)

Use isEmpty()

Linter

• Many years of many forks
• Jorge Ortiz: com.foursquare.lint in 2012
• Matic Potočnik (@HairyFotr): current fork
• Documentation is spotty
• scalac plugin without sbt plugin
• Configured via scalac switches
• No suppression mechanism

Implements 50% more rules than Scalastyle

• Guess what they do based on name
• Tend toward more traditional linter-style

​

Linter found

• String.format() call with mismatched args
• Unused function argument
• aList.length == 0 <- use .isEmpty
• Using string interpolation unnecessarily
• Calling .toString on Array
• Close scala.io.Source
• Inferring Nothing

Linter Also Found

• Casting instead of .toByte
• Calling .toSeq on a Seq
• Calling .toString on a String
• Calling .toList on a List
• Calling .toMap on a Map
• Use JavaConversions, not JavaConverters

Linter

• I really like it but it’s iffy to recommend
• Incomplete docs
• 0.1-SNAPSHOT
• Definitely has value; identified real bugs
• No false positives
• Wish: short list of highest ROI checks
• Wish: suppression
• Wish: sbt plugin

Linter

Signal/Noise

Suppression

Learning Curve

seq.find(_ > 42).isDefined

​

seq.find(_ > 42).isDefined

seq.exists(_ > 42)

seq.exists(_ == "the one")

seq.exists(_ == "the one")

seq.contains("the one")

Scapegoat

• Stephen Samuel
• Less than a year old
• Releasing prolifically
• Churning a fair bit
• sbt plugin makes it easy to get started
• Fairly good docs. A little behind the code.
• File level, path regex based suppression

​

WartRemover �++ �Linter

++�Scalastyle

.find(...).isDefined -> .exists(...)

Lonely sealed class

Match instead of partial function

Duplicated import

.length == 0

var could be a val

​

Unused method parameter

Use .contains(y) instead of� .exists(_ == y)

​

​

What I dislike

• Overwhelming. Enables 100+ rules by default
• Somewhat unclear how to disable rule
• Bans final case class
• Bans wildcard imports. Bedazzling?
• At least 5 different classes of false positive

Scapegoat

Signal/Noise

Suppression

Learning Curve

S/N

Sup

LC

?

?

scalac switches

FindBugs

Scalastyle

Abide

WartRemover

Linter

Scapegoat

?

Greenfield

1. scalac switches
1. Adapted Rob Norris’ post
2. Scalastyle coding standard
• Such as alexandru’s list
3. WartRemover for inference and partial

Protip: Skeleton Project

Existing Project

Start small; tighten screws

1. Start with all rules disabled
2. Make linter gate in CI
3. Pick a rule team agrees on
4. Clean up code. Suppress.
5. Enable rule as error
6. Goto 3

TL;DR

scalacOptions ++= Seq(

"-Xlint”,

"-deprecation",

"-Xfatal-warnings"

)

Questions?