diff --git a/README.md b/README.md index e28b77bd..b648ef33 100644 --- a/README.md +++ b/README.md @@ -80,7 +80,7 @@ Please note that scapegoat is a new project. While it's been tested on some comm ### Inspections -There are currently 107 inspections. An overview list is given, followed by a more detailed description of each inspection after the list (todo: finish rest of detailed descriptions) +There are currently 108 inspections. An overview list is given, followed by a more detailed description of each inspection after the list (todo: finish rest of detailed descriptions) | Name | Brief Description | |----|-----------| @@ -101,6 +101,7 @@ There are currently 107 inspections. An overview list is given, followed by a mo | CatchFatal | Checks for try blocks that catch fatal exceptions: VirtualMachineError, ThreadDeath, InterruptedException, LinkageError, ControlThrowable | | CatchThrowable | Checks for try blocks that catch Throwable | | ClassNames | Ensures class names adhere to the style guidelines | +| CollectionIndexOnNonIndexedSeq | Checks for indexing on a Seq which is not an IndexedSeq | | CollectionNamingConfusion | Checks for variables that are confusingly named | | CollectionNegativeIndex | Checks for negative access on a sequence eg `list.get(-1)` | | CollectionPromotionToAny | Checks for collection operations that promote the collection to `Any` | @@ -202,6 +203,12 @@ There are currently 107 inspections. An overview list is given, followed by a mo Checks for explicit toString calls on arrays. Since toString on an array does not perform a deep toString, like say scala's List, this is usually a mistake. +##### CollectionIndexOnNonIndexedSeq + +Checks for calls of `.apply(idx)` on a `Seq` where the index is not a literal and the `Seq` is not an `IndexedSeq`. + +*Rationale* If code which expects O(1) positional access to a Seq is given a non-IndexedSeq (such as a List, where indexing is O(n)) then this may cause poor performance. + ##### ComparingUnrelatedTypes Checks for equality comparisons that cannot succeed because the types are unrelated. Eg `"string" == BigDecimal(1.0)`. The scala compiler has a less strict version of this inspection. diff --git a/src/main/scala/com/sksamuel/scapegoat/ScapegoatConfig.scala b/src/main/scala/com/sksamuel/scapegoat/ScapegoatConfig.scala index 4f0ddcdb..a09efad6 100644 --- a/src/main/scala/com/sksamuel/scapegoat/ScapegoatConfig.scala +++ b/src/main/scala/com/sksamuel/scapegoat/ScapegoatConfig.scala @@ -39,6 +39,7 @@ object ScapegoatConfig extends App { new CatchFatal, new CatchThrowable, new ClassNames, + new CollectionIndexOnNonIndexedSeq, new CollectionNamingConfusion, new CollectionNegativeIndex, new CollectionPromotionToAny, diff --git a/src/main/scala/com/sksamuel/scapegoat/inspections/collections/CollectionIndexOnNonIndexedSeq.scala b/src/main/scala/com/sksamuel/scapegoat/inspections/collections/CollectionIndexOnNonIndexedSeq.scala new file mode 100644 index 00000000..c04a344a --- /dev/null +++ b/src/main/scala/com/sksamuel/scapegoat/inspections/collections/CollectionIndexOnNonIndexedSeq.scala @@ -0,0 +1,33 @@ +package com.sksamuel.scapegoat.inspections.collections + +import com.sksamuel.scapegoat._ + +/** @author Josh Rosen */ +class CollectionIndexOnNonIndexedSeq extends Inspection { + + def inspector(context: InspectionContext): Inspector = new Inspector(context) { + override def postTyperTraverser = Some apply new context.Traverser { + + import context.global._ + + private def isSeq(t: Tree) = t.tpe <:< typeOf[Seq[_]] + private def isIndexedSeq(t: Tree) = t.tpe <:< typeOf[IndexedSeq[_]] + private def isLiteral(t: Tree) = t match { + case Literal(_) => true + case _ => false + } + + override def inspect(tree: Tree): Unit = { + tree match { + case Apply(Select(lhs, TermName("apply")), List(idx)) if isSeq(lhs) && !isIndexedSeq(lhs) && !isLiteral(idx)=> + context.warn("Seq.apply() on a non-IndexedSeq may cause performance problems", + tree.pos, + Levels.Warning, + tree.toString().take(100), + CollectionIndexOnNonIndexedSeq.this) + case _ => continue(tree) + } + } + } + } +} diff --git a/src/test/scala/com/sksamuel/scapegoat/inspections/collections/CollectionIndexOnNonIndexedSeqTest.scala b/src/test/scala/com/sksamuel/scapegoat/inspections/collections/CollectionIndexOnNonIndexedSeqTest.scala new file mode 100644 index 00000000..42c190f8 --- /dev/null +++ b/src/test/scala/com/sksamuel/scapegoat/inspections/collections/CollectionIndexOnNonIndexedSeqTest.scala @@ -0,0 +1,57 @@ +package com.sksamuel.scapegoat.inspections.collections + +import com.sksamuel.scapegoat.PluginRunner +import org.scalatest.{ FreeSpec, Matchers, OneInstancePerTest } + +/** @author Josh Rosen */ +class CollectionIndexOnNonIndexedSeqTest + extends FreeSpec + with Matchers + with PluginRunner + with OneInstancePerTest { + + override val inspections = Seq(new CollectionIndexOnNonIndexedSeq) + + "collection index on non indexed seq" - { + "should report warning" in { + val code = + """ + | object Test { + | val idx = 2 + | List(1,2,3)(idx) + | Seq(1,2,3)(idx) + | val s: Seq[Int] = Array(1,2,3) + | s(idx) + | } + """.stripMargin + compileCodeSnippet(code) + compiler.scapegoat.feedback.warnings.size shouldBe 3 + } + "should not report warning" - { + "for literal index" in { + val code = + """ + | object Test { + | List(1,2,3)(1) + | Seq(1,2,3)(2) + | } + """.stripMargin + compileCodeSnippet(code) + compiler.scapegoat.feedback.warnings.size shouldBe 0 + } + "for IndexedSeq" in { + val code = + """ + | object Test { + | val idx = 2 + | IndexedSeq(1,2,3)(idx) + | Array(1,2,3)(idx) + | Vector(1,2,3)(idx) + | } + """.stripMargin + compileCodeSnippet(code) + compiler.scapegoat.feedback.warnings.size shouldBe 0 + } + } + } +}