Adding inspection for unnecessary value store before return (#653)

* Adding inspection for unnecessary value store

  * Now checking for a case where a value is stored in a block, and then
    immediately returned in the next line.
  * Tests for both implicit and explicit scope returns.

* Adding test-case for mutually-recursive fns

* 121 inspections

Co-authored-by: Greg Oledzki <mccartney@users.noreply.github.com>

README.md

@@ -149,7 +149,7 @@ For instructions on suppressing warnings by file, by inspection or by line see [
 
 ### Inspections
 
-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)
+There are currently 121 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 |
 |---------------------------------------|--------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------|
@@ -248,6 +248,7 @@ There are currently 120 inspections. An overview list is given, followed by a mo
 | ReverseTailReverse                    | `.reverse.tail.reverse` can be replaced with `init`                                                                                                          | Info          |
 | ReverseTakeReverse                    | `.reverse.take(...).reverse` can be replaced with `takeRight`                                                                                                | Info          |
 | SimplifyBooleanExpression             | `b == false` can be simplified to `!b`                                                                                                                       | Info          |
+| StoreBeforeReturn                     | Checks for storing a value in a block, and immediately returning the value                                                                                   | Info          |
 | StripMarginOnRegex                    | Checks for .stripMargin on regex strings that contain a pipe                                                                                                 | Error         |
 | SubstringZero                         | Checks for `String.substring(0)`                                                                                                                             | Info          |
 | SuspiciousMatchOnClassObject          | Finds code where matching is taking place on class literals                                                                                                  | Warning       |

src/main/scala/com/sksamuel/scapegoat/Inspections.scala

@@ -121,6 +121,7 @@ object Inspections extends App {
       new ReverseTailReverse,
       new ReverseTakeReverse,
       new SimplifyBooleanExpression,
+      new StoreBeforeReturn,
       new StripMarginOnRegex,
       new SubstringZero,
       new SuspiciousMatchOnClassObject,

src/main/scala/com/sksamuel/scapegoat/inspections/unneccesary/StoreBeforeReturn.scala

@@ -0,0 +1,41 @@
+package com.sksamuel.scapegoat.inspections.unneccesary
+
+import com.sksamuel.scapegoat._
+
+class StoreBeforeReturn
+    extends Inspection(
+      text = "Unnecessary store before return.",
+      defaultLevel = Levels.Info,
+      description = "Checks for storing a value in a block, and immediately returning the value.",
+      explanation =
+        "Storing a value and then immediately returning it is equivalent to returning the raw value itself."
+    ) {
+
+  override def inspector(context: InspectionContext): Inspector =
+    new Inspector(context) {
+      override def postTyperTraverser: context.Traverser =
+        new context.Traverser {
+          import context.global._
+
+          private def lastExprName(expr: Tree): Option[String] =
+            expr match {
+              case Return(Ident(name)) => Some(name.toString())
+              case Ident(name)         => Some(name.toString())
+              case _                   => None
+            }
+
+          override def inspect(tree: context.global.Tree): Unit =
+            tree match {
+              case DefDef(_, _, _, _, _, Block(stmts, lastExprInBody)) =>
+                val maybeLastExprName = lastExprName(lastExprInBody)
+                stmts.lastOption.foreach {
+                  case defn @ ValDef(_, assignmentName, _, _)
+                      if maybeLastExprName.contains(assignmentName.toString()) =>
+                    context.warn(defn.pos, self)
+                  case _ => stmts.foreach(inspect)
+                }
+              case _ => continue(tree)
+            }
+        }
+    }
+}

src/test/scala/com/sksamuel/scapegoat/inspections/unnecessary/StoreBeforeReturnTest.scala

@@ -0,0 +1,112 @@
+package com.sksamuel.scapegoat.inspections.unnecessary
+
+import com.sksamuel.scapegoat.InspectionTest
+import com.sksamuel.scapegoat.inspections.unneccesary.StoreBeforeReturn
+
+class StoreBeforeReturnTest extends InspectionTest {
+
+  override val inspections = Seq(new StoreBeforeReturn)
+
+  "store value before explicit return" - {
+    "should report warning" in {
+      val code =
+        """
+          |object Test {
+          |  def hello(): String = {
+          |    var s = "sammy"
+          |    return s
+          |  }
+          |}
+          |""".stripMargin
+      compileCodeSnippet(code)
+      compiler.scapegoat.feedback.warnings.size shouldBe 1
+    }
+
+    "should report warning in a nested function" in {
+      val code =
+        """
+          |object Test {
+          |  def foo(): Int = {
+          |    def bar(): Int = {
+          |      val x = 1
+          |      return x
+          |    }
+          |    return bar()
+          |  }
+          |}
+          |""".stripMargin
+      compileCodeSnippet(code)
+      compiler.scapegoat.feedback.warnings.size shouldBe 1
+    }
+  }
+
+  "store value before implicit return" - {
+    "should report warning" in {
+      val code =
+        """
+          |object Test {
+          |  def hello(): String = {
+          |    var s = "sammy"
+          |    s
+          |  }
+          |}
+          |""".stripMargin
+      compileCodeSnippet(code)
+      compiler.scapegoat.feedback.warnings.size shouldBe 1
+    }
+
+    "should report warning in a nested function" in {
+      val code =
+        """
+          |object Test {
+          |  def foo(): Int = {
+          |    def bar(): Int = {
+          |      val x = 1
+          |      x
+          |    }
+          |    bar()
+          |  }
+          |}
+          |""".stripMargin
+
+      compileCodeSnippet(code)
+      compiler.scapegoat.feedback.warnings.size shouldBe 1
+    }
+  }
+  "store value and modify before return" - {
+    "should not report warning" in {
+      val code =
+        """
+          |object Test {
+          |  def hello(): Int = {
+          |    var x = 1
+          |    x = x + 1
+          |    return  x
+          |  }
+          |}
+          |""".stripMargin
+      compileCodeSnippet(code)
+      compiler.scapegoat.feedback.warnings.size shouldBe 0
+    }
+  }
+
+  "store value and return for mutually-recusive functions" - {
+    "should report a warning, and terminate" in {
+      val code =
+        """
+          |object Test {
+          |  def foo(): Int = {
+          |    var x = bar()
+          |    return  x
+          |  }
+          |  def bar(): Int = {
+          |    val x = foo()
+          |    return x
+          |  }
+          |}
+          |""".stripMargin
+      compileCodeSnippet(code)
+      compiler.scapegoat.feedback.warnings.size shouldBe 2
+    }
+  }
+}