Add CollectionIndexOnNonIndexedSeq rule to Scapegoat

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.

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,

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)
+        }
+      }
+    }
+  }
+}

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
+      }
+    }
+  }
+}