Merge pull request #262 from mccartney/mccartney-prefer-empty-2

False positives: not suggesting .empty() for mutable collections
scapegoat-scala · Nov 14, 2019 · 342b0df · 342b0df
2 parents 2190306 + 32ea1ab
commit 342b0df
Show file tree

Hide file tree

Showing 5 changed files with 38 additions and 25 deletions.
diff --git a/src/main/scala/com/sksamuel/scapegoat/inspections/collections/PreferMapEmpty.scala b/src/main/scala/com/sksamuel/scapegoat/inspections/collections/PreferMapEmpty.scala
@@ -14,10 +14,11 @@ class PreferMapEmpty extends Inspection("Prefer Map.empty", Levels.Info) {
 
       override def inspect(tree: Tree): Unit = {
         tree match {
-          case Apply(TypeApply(Select(Select(_, MapTerm), ApplyTerm), _), List()) =>
-            context.warn(tree.pos, self,
-              "Map[K,V]() creates a new instance. Consider Map.empty which does not allocate a new object. " +
-                tree.toString().take(500))
+          case a@Apply(TypeApply(Select(Select(_, MapTerm), ApplyTerm), _), List())
+            if a.tpe.toString.startsWith("scala.collection.immutable.") =>
+              context.warn(tree.pos, self,
+                "Map[K,V]() allocates an intermediate object. Consider Map.empty which returns a singleton instance without creating a new object." +
+                  tree.toString().take(500))
           case _ => continue(tree)
         }
       }

diff --git a/src/main/scala/com/sksamuel/scapegoat/inspections/collections/PreferSeqEmpty.scala b/src/main/scala/com/sksamuel/scapegoat/inspections/collections/PreferSeqEmpty.scala
@@ -15,14 +15,16 @@ class PreferSeqEmpty extends Inspection("Prefer Seq.empty", Levels.Info) {
 
       override def inspect(tree: Tree): Unit = {
         tree match {
-          case Apply(TypeApply(Select(Select(_, SeqTerm), ApplyTerm), _), List()) => warn(tree)
+          case a@Apply(TypeApply(Select(Select(_, SeqTerm), ApplyTerm), _), List())
+            if (!a.tpe.toString.startsWith("scala.collection.mutable.")) =>
+              warn(tree)
           case _ => continue(tree)
         }
       }
 
       private def warn(tree: Tree): Unit = {
         context.warn(tree.pos, self,
-          "Seq[T]() creates a new instance. Consider Seq.empty which does not allocate a new object. " +
+          "Seq[T]() allocates an intermediate object. Consider Seq.empty wich returns a singleton instance without creating a new object. " +
             tree.toString().take(500))
       }
     }

diff --git a/src/main/scala/com/sksamuel/scapegoat/inspections/collections/PreferSetEmpty.scala b/src/main/scala/com/sksamuel/scapegoat/inspections/collections/PreferSetEmpty.scala
@@ -18,7 +18,7 @@ class PreferSetEmpty extends Inspection("Prefer Set.empty", Levels.Info) {
           case a@Apply(TypeApply(Select(Select(_, SetTerm), ApplyTerm), _), List())
             if a.tpe.toString.startsWith("scala.collection.immutable.") =>
               context.warn(tree.pos, self,
-                "Set[T]() creates a new instance. Consider Set.empty which does not allocate a new object. " +
+                "Set[T]() allocates an intermediate object. Consider Set.empty which returns a singleton instance without creating a new object. " +
                   tree.toString().take(500))
           case _ => continue(tree)
         }

diff --git a/src/test/scala/com/sksamuel/scapegoat/inspections/collections/PreferMapEmptyTest.scala b/src/test/scala/com/sksamuel/scapegoat/inspections/collections/PreferMapEmptyTest.scala
@@ -7,8 +7,8 @@ class PreferMapEmptyTest extends FreeSpec with Matchers with PluginRunner with O
 
   override val inspections = Seq(new PreferMapEmpty)
 
-  "map apply" - {
-    "should report warning" in {
+  "should report warning" - {
+    "with map apply" in {
 
       val code = """object Test {
                       val a = Map[String, String]()
@@ -19,26 +19,31 @@ class PreferMapEmptyTest extends FreeSpec with Matchers with PluginRunner with O
       compiler.scapegoat.feedback.warnings.size shouldBe 1
     }
   }
-  "non empty map apply" - {
-    "should not report warning" in {
-
+  "should not report warning" - {
+    "with non empty Map apply" in {
       val code = """object Test {
                       val a = Map(1 -> 2)
                     } """.stripMargin
 
       compileCodeSnippet(code)
       compiler.scapegoat.feedback.warnings.size shouldBe 0
     }
-  }
-  "Map.empty" - {
-    "should not report warning" in {
-
+    "with Map.empty" in {
       val code = """object Test {
                       val b = Map.empty
                     } """.stripMargin
 
       compileCodeSnippet(code)
       compiler.scapegoat.feedback.warnings.size shouldBe 0
     }
+    "with mutable.Map" in {
+      val code = """import scala.collection.mutable.Map
+                    object Test {
+                      val b = Map()
+                    }""".stripMargin
+
+      compileCodeSnippet(code)
+      compiler.scapegoat.feedback.warnings.size shouldBe 0
+    }
   }
 }
diff --git a/src/test/scala/com/sksamuel/scapegoat/inspections/collections/PreferSeqEmptyTest.scala b/src/test/scala/com/sksamuel/scapegoat/inspections/collections/PreferSeqEmptyTest.scala
@@ -8,8 +8,8 @@ class PreferSeqEmptyTest extends FreeSpec with Matchers with PluginRunner with O
 
   override val inspections = Seq(new PreferSeqEmpty)
 
-  "empty seq apply" - {
-    "should report warning" in {
+  "should report warning" - {
+    "with empty seq apply" in {
 
       val code = """object Test {
                       val a = Seq[String]()
@@ -20,26 +20,31 @@ class PreferSeqEmptyTest extends FreeSpec with Matchers with PluginRunner with O
       compiler.scapegoat.feedback.warnings.size shouldBe 1
     }
   }
-  "non empty seq apply" - {
-    "should not report warning" in {
-
+  "should not report warning" - {
+    "with non empty seq apply" in {
       val code = """object Test {
                       val a = Seq("sam")
                     } """.stripMargin
 
       compileCodeSnippet(code)
       compiler.scapegoat.feedback.warnings.size shouldBe 0
     }
-  }
-  "Set.empty" - {
-    "should not report warning" in {
+    "with Seq.empty" in {
+      val code = """object Test {
+                      val b = Seq.empty
+                    } """.stripMargin
 
+      compileCodeSnippet(code)
+      compiler.scapegoat.feedback.warnings.size shouldBe 0
+    }
+    "with mutable.Seq" in {
       val code = """object Test {
-                      val b = Set.empty
+                      val b = scala.collection.mutable.Seq()
                     } """.stripMargin
 
       compileCodeSnippet(code)
       compiler.scapegoat.feedback.warnings.size shouldBe 0
     }
+
   }
 }