Analyzer enhancements

analyzer/src/main/scala/com/avsystem/commons/analyzer/AnalyzerPlugin.scala

@@ -59,6 +59,12 @@ final class AnalyzerPlugin(val global: Global) extends Plugin { plugin =>
     new BadSingletonComponent(global),
     new ConstantDeclarations(global),
     new BasePackage(global),
+    new ImplicitValueClasses(global),
+    new FinalValueClasses(global),
+    new FinalCaseClasses(global),
+    new ImplicitParamDefaults(global),
+    new CatchThrowable(global),
+    new ImplicitFunctionParams(global),
   )
 
   private lazy val rulesByName = rules.map(r => (r.name, r)).toMap

analyzer/src/main/scala/com/avsystem/commons/analyzer/AnalyzerRule.scala

@@ -32,7 +32,8 @@ abstract class AnalyzerRule(val global: Global, val name: String, defaultLevel:
     pos: Position,
     message: String,
     category: WarningCategory = WarningCategory.Lint,
-    site: Symbol = NoSymbol
+    site: Symbol = NoSymbol,
+    level: Level = this.level
   ): Unit =
     level match {
       case Level.Off =>

analyzer/src/main/scala/com/avsystem/commons/analyzer/CatchThrowable.scala

@@ -0,0 +1,35 @@
+package com.avsystem.commons
+package analyzer
+
+import scala.tools.nsc.Global
+import scala.util.control.NonFatal
+
+class CatchThrowable(g: Global) extends AnalyzerRule(g, "catchThrowable", Level.Warn) {
+
+  import global._
+
+  private lazy val throwableTpe = typeOf[Throwable]
+
+  private lazy val nonFatalSymbols = {
+    val nonFatal = typeOf[NonFatal.type].termSymbol
+    val nonFatalAlias = classType("com.avsystem.commons.CommonAliases").member(TermName("NonFatal"))
+
+    Set(nonFatal, nonFatalAlias)
+  }
+
+  private def isNonFatalPattern(tree: Tree): Boolean = tree match {
+    case UnApply(Apply(Select(qualifier, TermName("unapply")), _), _) if nonFatalSymbols contains qualifier.symbol => true
+    case _ => false
+  }
+
+  def analyze(unit: CompilationUnit): Unit =
+    unit.body.foreach {
+      case t: Try =>
+        t.catches.foreach { case cas@CaseDef(pat, _, _) =>
+          if (pat.tpe != null && pat.tpe =:= throwableTpe && !isNonFatalPattern(pat)) {
+            report(cas.pos, "Catching Throwable is discouraged, catch specific exceptions instead")
+          }
+        }
+      case _ =>
+    }
+}

analyzer/src/main/scala/com/avsystem/commons/analyzer/FinalCaseClasses.scala

@@ -0,0 +1,25 @@
+package com.avsystem.commons
+package analyzer
+
+import com.avsystem.commons.analyzer.Level.Info
+
+import scala.tools.nsc.Global
+
+class FinalCaseClasses(g: Global) extends AnalyzerRule(g, "finalCaseClasses", Level.Warn) {
+
+  import global.*
+
+  def analyze(unit: CompilationUnit): Unit = unit.body.foreach {
+    case cd: ClassDef if !cd.mods.hasFlag(Flag.FINAL) && cd.mods.hasFlag(Flag.CASE) =>
+      // Skip case classes defined inside traits (SI-4440)
+
+      val isInner = cd.symbol.isStatic
+
+      if (isInner) {
+        report(cd.pos, "Case classes should be marked as final")
+      } else {
+        report(cd.pos, "Case classes should be marked as final. Due to the SI-4440 bug, it cannot be done here. Consider moving the case class to the companion object", level = Info)
+      }
+    case _ =>
+  }
+}

analyzer/src/main/scala/com/avsystem/commons/analyzer/FinalValueClasses.scala

@@ -0,0 +1,21 @@
+package com.avsystem.commons
+package analyzer
+
+import scala.tools.nsc.Global
+
+class FinalValueClasses(g: Global) extends AnalyzerRule(g, "finalValueClasses", Level.Warn) {
+
+  import global.*
+
+  private lazy val anyValTpe = typeOf[AnyVal]
+
+  def analyze(unit: CompilationUnit): Unit = unit.body.foreach {
+    case cd: ClassDef if !cd.mods.hasFlag(Flag.FINAL) =>
+      val tpe = cd.symbol.typeSignature
+
+      if (tpe.baseClasses.contains(anyValTpe.typeSymbol) ) {
+        report(cd.pos, "Value classes should be marked as final")
+      }
+    case _ =>
+  }
+}

analyzer/src/main/scala/com/avsystem/commons/analyzer/ImplicitFunctionParams.scala

@@ -0,0 +1,24 @@
+package com.avsystem.commons
+package analyzer
+
+import scala.tools.nsc.Global
+
+class ImplicitFunctionParams(g: Global) extends AnalyzerRule(g, "implicitFunctionParams", Level.Warn) {
+
+  import global.*
+
+  def analyze(unit: CompilationUnit): Unit = unit.body.foreach {
+    case dd: DefDef =>
+      dd.vparamss.foreach { paramList =>
+        if (paramList.nonEmpty && paramList.head.mods.hasFlag(Flag.IMPLICIT)) {
+          paramList.foreach { param =>
+            val paramTpe = param.tpt.tpe
+            if (paramTpe != null && (definitions.isFunctionType(paramTpe) || definitions.isPartialFunctionType(paramTpe))) {
+              report(param.pos, "Implicit parameters should not have any function type")
+            }
+          }
+        }
+      }
+    case _ =>
+  }
+}

analyzer/src/main/scala/com/avsystem/commons/analyzer/ImplicitParamDefaults.scala

@@ -0,0 +1,23 @@
+package com.avsystem.commons
+package analyzer
+
+import scala.tools.nsc.Global
+
+class ImplicitParamDefaults(g: Global) extends AnalyzerRule(g, "implicitParamDefaults", Level.Warn) {
+
+  import global.*
+
+  def analyze(unit: CompilationUnit): Unit = unit.body.foreach {
+    case dd: DefDef =>
+      dd.vparamss.foreach { paramList =>
+        if (paramList.nonEmpty && paramList.head.mods.hasFlag(Flag.IMPLICIT)) {
+          paramList.foreach { param =>
+            if (param.rhs != EmptyTree) {
+              report(param.pos, "Do not declare default values for implicit parameters")
+            }
+          }
+        }
+      }
+    case _ =>
+  }
+}

analyzer/src/main/scala/com/avsystem/commons/analyzer/ImplicitValueClasses.scala

@@ -0,0 +1,53 @@
+package com.avsystem.commons
+package analyzer
+
+import scala.tools.nsc.Global
+
+class ImplicitValueClasses(g: Global) extends AnalyzerRule(g, "implicitValueClasses", Level.Warn) {
+
+  import global.*
+  import definitions.*
+
+  private lazy val defaultClasses = Set[Symbol](AnyClass, AnyValClass, ObjectClass)
+
+  private lazy val reportOnNestedClasses = argument match {
+    case null => false
+    case a => a.toBoolean
+  }
+
+  def analyze(unit: CompilationUnit): Unit = unit.body.foreach {
+    case cd: ClassDef if cd.mods.hasFlag(Flag.IMPLICIT) =>
+      val tpe = cd.symbol.typeSignature
+      val primaryCtor = tpe.member(termNames.CONSTRUCTOR).alternatives.find(_.asMethod.isPrimaryConstructor)
+      val paramLists = primaryCtor.map(_.asMethod.paramLists)
+      val inheritsAnyVal = tpe.baseClasses contains AnyValClass
+
+      def inheritsOtherClass = tpe.baseClasses.exists { cls =>
+        def isDefault = defaultClasses contains cls
+        def isUniversalTrait = cls.isTrait && cls.superClass == AnyClass
+
+        cls != cd.symbol && !isDefault && !isUniversalTrait
+      }
+      def hasExactlyOneParam = paramLists.exists(lists => lists.size == 1 && lists.head.size == 1)
+
+      def paramIsValueClass = paramLists.exists { lists =>
+        /* lists.nonEmpty && lists.head.nonEmpty && */
+        lists.head.head.typeSignature.typeSymbol.isDerivedValueClass
+      }
+
+      if (!inheritsAnyVal && !inheritsOtherClass && hasExactlyOneParam && !paramIsValueClass) {
+        val isNestedClass =
+          //implicit classes are always nested classes, so we want to check if the outer class's an object
+          /*cd.symbol.isNestedClass &&*/ !cd.symbol.isStatic
+
+        val message = "Implicit classes should always extend AnyVal to become value classes" +
+          (if (isNestedClass) ". Nested classes should be extracted to top-level objects" else "")
+
+        if (reportOnNestedClasses || !isNestedClass)
+          report(cd.pos, message)
+        else
+          report(cd.pos, message, level = Level.Info)
+      }
+    case _ =>
+  }
+}

analyzer/src/test/scala/com/avsystem/commons/analyzer/CatchThrowableTest.scala

@@ -0,0 +1,92 @@
+package com.avsystem.commons
+package analyzer
+
+import org.scalatest.funsuite.AnyFunSuite
+
+class CatchThrowableTest extends AnyFunSuite with AnalyzerTest {
+  test("catching Throwable should be rejected") {
+    assertErrors(1,
+      //language=Scala
+      """
+        |object Test {
+        |  def test(): Unit = {
+        |    try {
+        |      println("test")
+        |    } catch {
+        |      case t: Throwable => println(t)
+        |    }
+        |  }
+        |}
+      """.stripMargin)
+  }
+
+  test("catching specific exceptions should be allowed") {
+    assertNoErrors(
+      //language=Scala
+      """
+        |object Test {
+        |  def test(): Unit = {
+        |    try {
+        |      println("test")
+        |    } catch {
+        |      case e: Exception => println(e)
+        |      case e: RuntimeException => println(e)
+        |      case e: IllegalArgumentException => println(e)
+        |    }
+        |  }
+        |}
+      """.stripMargin)
+  }
+
+  test("catching Throwable with other exceptions should be rejected") {
+    assertErrors(1,
+      //language=Scala
+      """
+        |object Test {
+        |  def test(): Unit = {
+        |    try {
+        |      println("test")
+        |    } catch {
+        |      case e: IllegalArgumentException => println(e)
+        |      case t: Throwable => println(t)
+        |    }
+        |  }
+        |}
+      """.stripMargin)
+  }
+
+  test("catching Throwable in nested catch block should be rejected") {
+    assertErrors(1,
+      //language=Scala
+      """
+        |object Test {
+        |  def test(): Unit = {
+        |    try println("test")
+        |    catch {
+        |      case e: Exception => try println("test")
+        |      catch {
+        |        case e: Throwable => println(e)
+        |      }
+        |    }
+        |  }
+        |}
+      """.stripMargin)
+  }
+
+  test("catching Throwable using NonFatal should be allowed") {
+    assertNoErrors(
+      //language=Scala
+      """
+        |object Test extends com.avsystem.commons.CommonAliases {
+        |  def test(): Unit = {
+        |    try {
+        |      println("test")
+        |    } catch {
+        |      case NonFatal(t) => println(t)
+        |      case scala.util.control.NonFatal(t) => println(t)
+        |    }
+        |  }
+        |}
+      """.stripMargin)
+  }
+}

analyzer/src/test/scala/com/avsystem/commons/analyzer/FinalCaseClassesTest.scala

@@ -0,0 +1,59 @@
+package com.avsystem.commons
+package analyzer
+
+import org.scalatest.funsuite.AnyFunSuite
+
+class FinalCaseClassesTest extends AnyFunSuite with AnalyzerTest {
+  test("case classes should be marked as final") {
+    assertErrors(2,
+      //language=scala
+      """
+        |object whatever {
+        |  // This should pass - final case class
+        |  final case class GoodCaseClass(x: Int, y: String) {
+        |    def double: Int = x * 2
+        |  }
+        |
+        |  // This should fail - case class not marked as final
+        |  case class BadCaseClass1(x: Int, y: String) {
+        |    def double: Int = x * 2
+        |  }
+        |
+        |  // This should fail - another case class not marked as final
+        |  case class BadCaseClass2[T](x: T, y: String) {
+        |    def double: String = y * 2
+        |  }
+        |
+        |  // Regular class - should not be affected
+        |  class RegularClass(val x: Int, val y: String) {
+        |    def double: Int = x * 2
+        |  }
+        |
+        |  // Regular class with case-like constructor - should not be affected
+        |  class RegularClass2(x: Int, y: String) {
+        |    def double: Int = x * 2
+        |  }
+        |}
+      """.stripMargin)
+  }
+
+  // SI-4440 https://github.com/scala/bug/issues/4440
+  test("should not be affected due to SI-4440"){
+    assertNoErrors(
+      //language=scala
+      """
+        |trait Outer {
+        |  case class Inner(x: Int, y: String) {
+        |    def double: Int = x * 2
+        |  }
+        |}
+        |
+        |class Outer2 {
+        |  case class Inner(x: Int, y: String) {
+        |    def double: Int = x * 2
+        |  }
+        |}
+        """.stripMargin
+    )
+  }
+}