Merge pull request #270 from mccartney/mccartney-masking-exceptions

New (sub-)inspection: Masking exceptions
scapegoat-scala · Dec 14, 2019 · fecc962 · fecc962
2 parents c4c1ebb + 87f2c7c
commit fecc962
Show file tree

Hide file tree

Showing 2 changed files with 65 additions and 7 deletions.
diff --git a/src/main/scala/com/sksamuel/scapegoat/inspections/exception/SwallowedException.scala b/src/main/scala/com/sksamuel/scapegoat/inspections/exception/SwallowedException.scala
@@ -10,23 +10,39 @@ class SwallowedException extends Inspection("Empty catch block", Levels.Warning)
 
       import context.global._
 
-      private def warn(cd: CaseDef): Unit = {
-        if (cd.body.toString == "()")
-          context.warn(cd.pos, self, "Empty catch block " + cd.toString().take(100))
+      private def containsMaskingThrow(expectedCauseException: Name, trees: Seq[Tree]): Boolean = {
+        trees.exists(tree => {
+          val thisResult = tree match {
+            case Throw(Apply(Select(New(_), _), args))
+              if args.collect { case Ident(i: Name) if i == expectedCauseException => true }.isEmpty =>
+              true
+            case _ => false
+          }
+          thisResult || containsMaskingThrow(expectedCauseException, tree.children)
+        })
       }
 
       private def checkCatches(defs: List[CaseDef]) = defs.foreach {
         case CaseDef(Bind(TermName("ignored") | TermName("ignore"), _), _, _) =>
-        case cdef @ CaseDef(_, _, Literal(Constant(()))) => warn(cdef)
+
+        case cdef @ CaseDef(_, _, Literal(Constant(())))
+          if cdef.body.toString == "()" =>
+            context.warn(cdef.pos, self, "Empty catch block " + cdef.toString().take(100))
+
+        case cdef @ CaseDef(Bind(caughtException, _), _, subtree)
+          if containsMaskingThrow(caughtException, Seq(subtree)) =>
+            context.warn(cdef.pos, self,
+              "Masking exception by not passing the original exception as cause: " + cdef.toString().take(100))
+
         case _ =>
       }
 
       override def inspect(tree: Tree): Unit = {
         tree match {
           case Try(_, catches, _) => checkCatches(catches)
-          case _                             => continue(tree)
+          case _                  => continue(tree)
         }
       }
     }
   }
-}
+}
diff --git a/src/test/scala/com/sksamuel/scapegoat/inspections/exception/SwallowedExceptionTest.scala b/src/test/scala/com/sksamuel/scapegoat/inspections/exception/SwallowedExceptionTest.scala
@@ -53,9 +53,38 @@ class SwallowedExceptionTest
         compileCodeSnippet(code1)
         compiler.scapegoat.feedback.warnings.size shouldBe 1
       }
+      "when exception is masked" in {
+        val code1 = """object Test {
+                         try {
+                           println(Integer.valueOf("asdf"))
+                         } catch {
+                           case nfe: NumberFormatException =>
+                             throw new IllegalArgumentException("not a number")
+                         }
+                       }""".stripMargin
+
+        compileCodeSnippet(code1)
+        compiler.scapegoat.feedback.warnings.size shouldBe 1
+      }
+      "when original exception is logged but not re-thrown" in {
+        val code1 = s"""object Test {
+                         def error(e: Exception, str: String) = println("Log ERROR " + str + " " + e)
+                         try {
+                           println(Integer.valueOf("asdf"))
+                         } catch {
+                           case nfe2: NumberFormatException =>
+                             error(nfe2, "Invalid format")
+                             throw new IllegalStateException("it's not a number")
+                         }
+                       }""".stripMargin
+
+        compileCodeSnippet(code1)
+        compiler.scapegoat.feedback.warnings.size shouldBe 1
+      }
     }
+
     "should not report warning" - {
-      "for exceptions all handled" in {
+      "for all exceptions handled" in {
         val code = """object Test {
                         try {
                         } catch {
@@ -77,6 +106,19 @@ class SwallowedExceptionTest
         compileCodeSnippet(code)
         compiler.scapegoat.feedback.warnings.size shouldBe 0
       }
+      "when exception is re-thrown with proper root cause" in {
+        val code1 = """object Test {
+                         try {
+                           println(Integer.valueOf("asdf"))
+                         } catch {
+                           case nfe: NumberFormatException =>
+                             throw new IllegalArgumentException("not a number", nfe)
+                         }
+                       }""".stripMargin
+
+        compileCodeSnippet(code1)
+        compiler.scapegoat.feedback.warnings.size shouldBe 0
+      }
     }
   }
 }