From a578ad6a4e94e9e14de7aa3b19e3b3013fe8e2ab Mon Sep 17 00:00:00 2001 From: James Yoo Date: Sun, 22 May 2022 14:13:50 -0700 Subject: [PATCH] Adding inspection for immediate rethrows (#652) * Adding inspection for immediate rethrows * Inspections now available when throwables/exceptions are immediately rethrown in a catch block implementation. * Adding inspection * Updating README.md --- README.md | 3 +- .../com/sksamuel/scapegoat/Inspections.scala | 1 + .../CatchExceptionImmediatelyRethrown.scala | 45 ++++++++++++++ ...atchExceptionImmediatelyRethrownTest.scala | 62 +++++++++++++++++++ 4 files changed, 110 insertions(+), 1 deletion(-) create mode 100644 src/main/scala/com/sksamuel/scapegoat/inspections/exception/CatchExceptionImmediatelyRethrown.scala create mode 100644 src/test/scala/com/sksamuel/scapegoat/inspections/exception/CatchExceptionImmediatelyRethrownTest.scala diff --git a/README.md b/README.md index f48c5e34..966e9764 100644 --- a/README.md +++ b/README.md @@ -149,7 +149,7 @@ For instructions on suppressing warnings by file, by inspection or by line see [ ### Inspections -There are currently 119 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 120 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 | Default Level | |---------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------| @@ -167,6 +167,7 @@ There are currently 119 inspections. An overview list is given, followed by a mo | BoundedByFinalType | Looks for types with upper bounds of a final type | Warning | | BrokenOddness | checks for a % 2 == 1 for oddness because this fails on negative numbers | Warning | | CatchException | Checks for try blocks that catch Exception | Warning | +| CatchExceptionImmediatelyRethrown | Checks for try-catch blocks that immediately rethrow caught exceptions. | Warning | | CatchFatal | Checks for try blocks that catch fatal exceptions: VirtualMachineError, ThreadDeath, InterruptedException, LinkageError, ControlThrowable | Warning | | CatchNpe | Checks for try blocks that catch null pointer exceptions | Error | | CatchThrowable | Checks for try blocks that catch Throwable | Warning | diff --git a/src/main/scala/com/sksamuel/scapegoat/Inspections.scala b/src/main/scala/com/sksamuel/scapegoat/Inspections.scala index 5adbaba5..faa8c466 100644 --- a/src/main/scala/com/sksamuel/scapegoat/Inspections.scala +++ b/src/main/scala/com/sksamuel/scapegoat/Inspections.scala @@ -40,6 +40,7 @@ object Inspections extends App { new BoundedByFinalType, new BrokenOddness, new CatchException, + new CatchExceptionImmediatelyRethrown, new CatchFatal, new CatchNpe, new CatchThrowable, diff --git a/src/main/scala/com/sksamuel/scapegoat/inspections/exception/CatchExceptionImmediatelyRethrown.scala b/src/main/scala/com/sksamuel/scapegoat/inspections/exception/CatchExceptionImmediatelyRethrown.scala new file mode 100644 index 00000000..8f571dd2 --- /dev/null +++ b/src/main/scala/com/sksamuel/scapegoat/inspections/exception/CatchExceptionImmediatelyRethrown.scala @@ -0,0 +1,45 @@ +package com.sksamuel.scapegoat.inspections.exception + +import com.sksamuel.scapegoat._ + +class CatchExceptionImmediatelyRethrown + extends Inspection( + text = "Caught Exception Immediately Rethrown", + defaultLevel = Levels.Warning, + description = "Checks for try-catch blocks that immediately rethrow caught exceptions.", + explanation = "Immediately re-throwing a caught exception is equivalent to not catching it at all." + ) { + + def inspector(context: InspectionContext): Inspector = + new Inspector(context) { + + override def postTyperTraverser: context.Traverser = + new context.Traverser { + import context.global._ + + private def exceptionHandlers(cases: List[CaseDef]): List[(String, Tree)] = { + cases.collect { + // matches t : Exception + case CaseDef(Bind(name, Typed(_, tpt)), _, body) if tpt.tpe =:= typeOf[Exception] => + (name.toString(), body) + // matches t : Throwable + case CaseDef(Bind(name, Typed(_, tpt)), _, body) if tpt.tpe =:= typeOf[Throwable] => + (name.toString(), body) + } + } + + override def inspect(tree: Tree): Unit = { + tree match { + case Try(_, catches, _) => + val exceptionBodies = exceptionHandlers(catches) + exceptionBodies.collect { + case (exceptionName, Throw(Ident(name))) if name.toString() == exceptionName => + context.warn(tree.pos, self) + } + case _ => + continue(tree) + } + } + } + } +} diff --git a/src/test/scala/com/sksamuel/scapegoat/inspections/exception/CatchExceptionImmediatelyRethrownTest.scala b/src/test/scala/com/sksamuel/scapegoat/inspections/exception/CatchExceptionImmediatelyRethrownTest.scala new file mode 100644 index 00000000..9a753a2b --- /dev/null +++ b/src/test/scala/com/sksamuel/scapegoat/inspections/exception/CatchExceptionImmediatelyRethrownTest.scala @@ -0,0 +1,62 @@ +package com.sksamuel.scapegoat.inspections.exception + +import com.sksamuel.scapegoat.InspectionTest + +class CatchExceptionImmediatelyRethrownTest extends InspectionTest { + + override val inspections = Seq(new CatchExceptionImmediatelyRethrown) + + "catch exception immediately throw" - { + "should report warning" in { + + val testCode = """object Test { + try { + val x = 1 + } catch { + case foo : Exception => + throw foo + } + } """.stripMargin + + compileCodeSnippet(testCode) + compiler.scapegoat.feedback.warnings.size shouldBe 1 + } + } + + "catch throwable immediately throw" - { + "should report warning" in { + + val testCode = """object Test { + try { + val x = 1 + } catch { + case foo : Throwable => + throw foo + } + } """.stripMargin + + compileCodeSnippet(testCode) + compiler.scapegoat.feedback.warnings.size shouldBe 1 + } + + } + + "catch throwable and exception immediately throw" - { + "should report all warnings" in { + + val testCode = """object Test { + try { + val x = 1 + } catch { + case foo : Throwable => + throw foo + case bar : Exception => + throw bar + } + } """.stripMargin + + compileCodeSnippet(testCode) + compiler.scapegoat.feedback.warnings.size shouldBe 2 + } + } +}