Merge remote-tracking branch 'origin/master' into 500-handle-thrown-exception-outside-the-monadic-context-of-task

# Conflicts:
#	docs/Analyzer.md

Author: halotukozak

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

@@ -60,6 +60,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,31 @@
+package com.avsystem.commons
+package analyzer
+
+import scala.tools.nsc.Global
+
+class CatchThrowable(g: Global) extends AnalyzerRule(g, "catchThrowable", Level.Warn) {
+
+  import global.*
+
+  private lazy val throwableTpe = typeOf[Throwable]
+
+  private def isCustomExtractor(tree: Tree): Boolean = tree match {
+    case UnApply(Apply(Select(_, TermName("unapply")), _), _) => true
+    case _ => false
+  }
+
+  private def checkTree(pat: Tree): Unit = if (pat.tpe != null && pat.tpe =:= throwableTpe && !isCustomExtractor(pat)) {
+    report(pat.pos, "Catching Throwable is discouraged, catch specific exceptions instead")
+  }
+
+  def analyze(unit: CompilationUnit): Unit =
+    unit.body.foreach {
+      case t: Try =>
+        t.catches.foreach {
+          case CaseDef(Alternative(trees), _, _) => trees.foreach(checkTree)
+          case CaseDef(Bind(_, Alternative(trees)), _, _) => trees.foreach(checkTree)
+          case CaseDef(pat, _, _) => checkTree(pat)
+        }
+      case _ =>
+    }
+}

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

@@ -0,0 +1,24 @@
+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 | Flag.SEALED) && 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, "Implicit parameters should not have default values")
+            }
+          }
+        }
+      }
+    case _ =>
+  }
+}

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

@@ -0,0 +1,56 @@
+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 "all" => true
+    case "top-level-only" | null => false
+    case _ => throw new IllegalArgumentException(s"Unknown ImplicitValueClasses option: $argument")
+  }
+
+  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,115 @@
+package com.avsystem.commons
+package analyzer
+
+import org.scalatest.funsuite.AnyFunSuite
+
+final class CatchThrowableTest extends AnyFunSuite with AnalyzerTest {
+  test("catching Throwable should be rejected") {
+    assertErrors(1,
+      scala"""
+             |def test(): Unit = 
+             |  try {
+             |    println("test")
+             |  } catch {
+             |    case t: Throwable => println(t)
+             |  }
+             |""".stripMargin)
+  }
+
+  test("catching specific exceptions should be allowed") {
+    assertNoErrors(
+      scala"""
+             |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,
+      scala"""
+             |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,
+      scala"""
+             |def test(): Unit = {
+             |  try println("test")
+             |  catch {
+             |    case e: Exception => try println("test")
+             |    catch {
+             |      case e: Throwable => println(e)
+             |    }
+             |  }
+             |}
+             |""".stripMargin)
+  }
+
+  test("catching Throwable using custom extractor should be allowed") {
+    assertNoErrors(
+      scala"""
+             |import  com.avsystem.commons.CommonAliases._
+             |
+             |object custom {
+             |  def unapply(t: Throwable): Option[IllegalArgumentException] = t match {
+             |    case e: IllegalArgumentException => Some(e)
+             |    case _ => None
+             |  }
+             |}
+             |def test(): Unit = {
+             |  try {
+             |    println("test")
+             |  } catch {
+             |    case custom(t) => println(t)
+             |    case NonFatal(t) => println(t)
+             |    case scala.util.control.NonFatal(t) => println(t)
+             |  }
+             |}
+             |""".stripMargin)
+  }
+
+  test("catching non-Throwable with pattern match should be allowed") {
+    assertNoErrors(
+      scala"""
+             |def test(): Unit = {
+             |  try {
+             |    println("test")
+             |  } catch {
+             |    case _: IndexOutOfBoundsException | _: NullPointerException => println("OK!")
+             |  }
+             |  try {
+             |    println("test")
+             |  } catch {
+             |    case e@(_: IndexOutOfBoundsException | _: NullPointerException) => println("OK!")
+             |  }
+             |}
+             |""".stripMargin)
+  }
+
+  test("catching Throwable with pattern match should be rejected") {
+    assertErrors(1,
+      scala"""
+             |def test(): Unit = {
+             |  try {
+             |    println("test")
+             |  } catch {
+             |    case _: IndexOutOfBoundsException | _: Throwable => println("Not OK!")
+             |  }
+             |}
+             |""".stripMargin)
+  }
+}