From 2a98bf6ecc691ab8d3769b66ec058ef4095990e7 Mon Sep 17 00:00:00 2001 From: Roman Janusz Date: Wed, 10 Aug 2022 16:30:17 +0200 Subject: [PATCH 1/5] analyzer rule that checks if Scala constants are properly declared --- .../commons/analyzer/AnalyzerPlugin.scala | 3 +- .../analyzer/ConstantDeclarations.scala | 34 ++++++++ .../analyzer/CheckMacroPrivateTest.scala | 2 +- .../analyzer/ConstantDeclarationsTest.scala | 83 +++++++++++++++++++ 4 files changed, 120 insertions(+), 2 deletions(-) create mode 100644 commons-analyzer/src/main/scala/com/avsystem/commons/analyzer/ConstantDeclarations.scala create mode 100644 commons-analyzer/src/test/scala/com/avsystem/commons/analyzer/ConstantDeclarationsTest.scala diff --git a/commons-analyzer/src/main/scala/com/avsystem/commons/analyzer/AnalyzerPlugin.scala b/commons-analyzer/src/main/scala/com/avsystem/commons/analyzer/AnalyzerPlugin.scala index 6f5101261..28d1411c2 100644 --- a/commons-analyzer/src/main/scala/com/avsystem/commons/analyzer/AnalyzerPlugin.scala +++ b/commons-analyzer/src/main/scala/com/avsystem/commons/analyzer/AnalyzerPlugin.scala @@ -56,7 +56,8 @@ final class AnalyzerPlugin(val global: Global) extends Plugin { plugin => new Any2StringAdd(global), new ThrowableObjects(global), new DiscardedMonixTask(global), - new BadSingletonComponent(global) + new BadSingletonComponent(global), + new ConstantDeclarations(global), ) private lazy val rulesByName = rules.map(r => (r.name, r)).toMap diff --git a/commons-analyzer/src/main/scala/com/avsystem/commons/analyzer/ConstantDeclarations.scala b/commons-analyzer/src/main/scala/com/avsystem/commons/analyzer/ConstantDeclarations.scala new file mode 100644 index 000000000..c0771beaa --- /dev/null +++ b/commons-analyzer/src/main/scala/com/avsystem/commons/analyzer/ConstantDeclarations.scala @@ -0,0 +1,34 @@ +package com.avsystem.commons +package analyzer + +import scala.tools.nsc.Global + +class ConstantDeclarations(g: Global) extends AnalyzerRule(g, "constantDeclarations") { + + import global._ + + def analyze(unit: CompilationUnit): Unit = unit.body.foreach { + case t@ValDef(mods, name, tpt, rhs) + if t.symbol.hasGetter && t.symbol.owner.isEffectivelyFinal => + + val getter = t.symbol.getterIn(t.symbol.owner) + if (getter.isPublic && getter.isStable && getter.overrides.isEmpty) { + val constantValue = rhs.tpe match { + case ConstantType(_) => true + case _ => false + } + + val firstChar = name.toString.charAt(0) + if (constantValue && (firstChar.isLower || !getter.isFinal)) { + report(t.pos, "a constant should be declared as a `final val` with an UpperCamelCase name") + } + if (firstChar.isUpper && !getter.isFinal) { + report(t.pos, "a constant with UpperCamelCase name should be declared as a `final val`") + } + if (getter.isFinal && constantValue && !(tpt.tpe =:= rhs.tpe)) { + report(t.pos, "a constant with a literal value should not have an explicit type annotation") + } + } + case _ => + } +} diff --git a/commons-analyzer/src/test/scala/com/avsystem/commons/analyzer/CheckMacroPrivateTest.scala b/commons-analyzer/src/test/scala/com/avsystem/commons/analyzer/CheckMacroPrivateTest.scala index 6363d0dfe..dd5765c62 100644 --- a/commons-analyzer/src/test/scala/com/avsystem/commons/analyzer/CheckMacroPrivateTest.scala +++ b/commons-analyzer/src/test/scala/com/avsystem/commons/analyzer/CheckMacroPrivateTest.scala @@ -50,7 +50,7 @@ class CheckMacroPrivateTest extends AnyFunSuite with AnalyzerTest { |object test { | @macroPrivate def macroPrivateMethod = { println("whatever"); 5 } | @macroPrivate object macroPrivateObject { - | val x = 42 + | final val X = 42 | } |} """.stripMargin diff --git a/commons-analyzer/src/test/scala/com/avsystem/commons/analyzer/ConstantDeclarationsTest.scala b/commons-analyzer/src/test/scala/com/avsystem/commons/analyzer/ConstantDeclarationsTest.scala new file mode 100644 index 000000000..a4e52edd4 --- /dev/null +++ b/commons-analyzer/src/test/scala/com/avsystem/commons/analyzer/ConstantDeclarationsTest.scala @@ -0,0 +1,83 @@ +package com.avsystem.commons +package analyzer + +import org.scalatest.funsuite.AnyFunSuite + +class ConstantDeclarationsTest extends AnyFunSuite with AnalyzerTest { + test("literal-valued constants should be non-lazy final vals with UpperCamelCase and no type annotation") { + assertErrors(4, + """ + |object Whatever { + | // bad + | val a = 10 + | val B = 10 + | final val c = 10 + | final val D: Int = 10 + | + | // good + | final val E = 10 + |} + """.stripMargin) + } + + test("effectively final, non-literal UpperCamelCase vals should be final") { + assertErrors(1, + """ + |object Whatever { + | // bad + | val A = "foo".trim + | + | // good + | final val B = "foo".trim + | val c = "foo".trim + |} + """.stripMargin) + } + + test("no constant checking in traits or non-final classes") { + assertNoErrors( + """ + |trait Whatever { + | val a = 10 + | val B = 10 + | final val c = 10 + | final val D: Int = 10 + | val A = "foo".trim + |} + | + |class Stuff { + | val a = 10 + | val B = 10 + | final val c = 10 + | final val D: Int = 10 + | val A = "foo".trim + |} + """.stripMargin) + } + + test("no constant checking for overrides") { + assertNoErrors( + """ + |trait Whatever { + | def a: Int + |} + | + |object Stuff extends Whatever { + | val a: Int = 42 + |} + """.stripMargin) + } + + test("no constant checking for privates") { + assertNoErrors( + """ + |object Whatever { + | private val a = 10 + | private val B = 10 + | private final val c = 10 + | private final val D: Int = 10 + | private val A = "foo".trim + |} + """.stripMargin) + } +} From e114e2c2b9c61e1a3a6e64c11e9f30b827d7ec44 Mon Sep 17 00:00:00 2001 From: Roman Janusz Date: Wed, 10 Aug 2022 16:39:27 +0200 Subject: [PATCH 2/5] updated analyzer doc --- docs/Analyzer.md | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/docs/Analyzer.md b/docs/Analyzer.md index 8c0cbc8b9..bc1cb4c05 100644 --- a/docs/Analyzer.md +++ b/docs/Analyzer.md @@ -16,26 +16,31 @@ and [silencer](https://github.com/ghik/silencer) plugin for warning suppression. Here's a list of currently supported rules: -| Name | Default level | Description | -| --- | --- | --- | -| `importJavaUtil` | warning | Rejects direct imports of `java.util` package. | +| Name | Default level | Description | +|----------------------------| --- |------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------| +| `importJavaUtil` | warning | Rejects direct imports of `java.util` package. | | `valueEnumExhaustiveMatch` | warning | Enables (limited) exhaustive pattern match checking for [`ValueEnum`s](https://github.com/AVSystem/scala-commons/blob/master/commons-core/src/main/scala/com/avsystem/commons/misc/ValueEnum.scala). | -| `any2stringadd` | off | Disables `any2stringadd` (concatenating arbitrary values with strings using `+`). | -| `bincompat` | warning | Enables [`@bincompat`](https://github.com/AVSystem/scala-commons/blob/master/commons-core/src/main/scala/com/avsystem/commons/annotation/bincompat.scala) checking | -| `showAst` | error | Handles [`@showAst`](https://github.com/AVSystem/scala-commons/blob/master/commons-core/src/main/scala/com/avsystem/commons/annotation/showAst.scala). | -| `findUsages` | warning | Issues a message for every occurrence of any of the symbols listed in the argument of this rule | -| `varargsAtLeast` | warning | Enables [`@atLeast`](https://github.com/AVSystem/scala-commons/blob/master/commons-core/src/main/scala/com/avsystem/commons/annotation/atLeast.scala) checking | -| `macroPrivate` | warning | Enables [`@macroPrivate`](https://github.com/AVSystem/scala-commons/blob/master/commons-core/src/main/scala/com/avsystem/commons/annotation/macroPrivate.scala) checking | -| `explicitGenerics` | warning | Enables [`@explicitGenerics`](https://github.com/AVSystem/scala-commons/blob/master/commons-core/src/main/scala/com/avsystem/commons/annotation/explicitGenerics.scala) checking | +| `any2stringadd` | off | Disables `any2stringadd` (concatenating arbitrary values with strings using `+`). | +| `bincompat` | warning | Enables [`@bincompat`](https://github.com/AVSystem/scala-commons/blob/master/commons-core/src/main/scala/com/avsystem/commons/annotation/bincompat.scala) checking | +| `showAst` | error | Handles [`@showAst`](https://github.com/AVSystem/scala-commons/blob/master/commons-core/src/main/scala/com/avsystem/commons/annotation/showAst.scala). | +| `findUsages` | warning | Issues a message for every occurrence of any of the symbols listed in the argument of this rule | +| `varargsAtLeast` | warning | Enables [`@atLeast`](https://github.com/AVSystem/scala-commons/blob/master/commons-core/src/main/scala/com/avsystem/commons/annotation/atLeast.scala) checking | +| `macroPrivate` | warning | Enables [`@macroPrivate`](https://github.com/AVSystem/scala-commons/blob/master/commons-core/src/main/scala/com/avsystem/commons/annotation/macroPrivate.scala) checking | +| `explicitGenerics` | warning | Enables [`@explicitGenerics`](https://github.com/AVSystem/scala-commons/blob/master/commons-core/src/main/scala/com/avsystem/commons/annotation/explicitGenerics.scala) checking | +| `discardedMonixTask` | warning | Makes sure that expressions evaluating to Monix `Task`s are not accidentally discarded by conversion to `Unit` | +| `throwableObjects` | warning | Makes sure that objects are never used as `Throwable`s (unless they have stack traces disabled) | +| `constantDeclarations` | warning | Checks if constants are always declared as `final val`s with `UpperCamelCase` and no explicit type annotation for literal values | Rules may be enabled and disabled in `build.sbt` with Scala compiler options: ```scala scalacOptions += "-P:AVSystemAnalyzer:,,..." ``` + `` must be one of the rule names listed above or `_` to apply to all rules. `` may be one of: + * `-` to disable rule * `*` for "info" level * _empty_ for "warning" level From 906fe526085f860b2ada555fc719e82a37807bcc Mon Sep 17 00:00:00 2001 From: Roman Janusz Date: Wed, 10 Aug 2022 17:54:29 +0200 Subject: [PATCH 3/5] cosmetic --- .../com/avsystem/commons/analyzer/ConstantDeclarations.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/commons-analyzer/src/main/scala/com/avsystem/commons/analyzer/ConstantDeclarations.scala b/commons-analyzer/src/main/scala/com/avsystem/commons/analyzer/ConstantDeclarations.scala index c0771beaa..b27b7d042 100644 --- a/commons-analyzer/src/main/scala/com/avsystem/commons/analyzer/ConstantDeclarations.scala +++ b/commons-analyzer/src/main/scala/com/avsystem/commons/analyzer/ConstantDeclarations.scala @@ -20,9 +20,9 @@ class ConstantDeclarations(g: Global) extends AnalyzerRule(g, "constantDeclarati val firstChar = name.toString.charAt(0) if (constantValue && (firstChar.isLower || !getter.isFinal)) { - report(t.pos, "a constant should be declared as a `final val` with an UpperCamelCase name") + report(t.pos, "a literal-valued constant should be declared as a `final val` with an UpperCamelCase name") } - if (firstChar.isUpper && !getter.isFinal) { + if (!constantValue && firstChar.isUpper && !getter.isFinal) { report(t.pos, "a constant with UpperCamelCase name should be declared as a `final val`") } if (getter.isFinal && constantValue && !(tpt.tpe =:= rhs.tpe)) { From 2a943b80a2bf15e162d3710eb5406b725960c4eb Mon Sep 17 00:00:00 2001 From: Roman Janusz Date: Thu, 11 Aug 2022 00:25:24 +0200 Subject: [PATCH 4/5] analyzer reports warnings that can be suppressed with @nowarn --- .../com/avsystem/commons/analyzer/AnalyzerRule.scala | 11 ++++++++--- .../commons/analyzer/ConstantDeclarations.scala | 11 +++++++---- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/commons-analyzer/src/main/scala/com/avsystem/commons/analyzer/AnalyzerRule.scala b/commons-analyzer/src/main/scala/com/avsystem/commons/analyzer/AnalyzerRule.scala index 87f306d44..d5b42f0f0 100644 --- a/commons-analyzer/src/main/scala/com/avsystem/commons/analyzer/AnalyzerRule.scala +++ b/commons-analyzer/src/main/scala/com/avsystem/commons/analyzer/AnalyzerRule.scala @@ -2,8 +2,8 @@ package com.avsystem.commons package analyzer import java.io.{PrintWriter, StringWriter} - import scala.tools.nsc.Global +import scala.tools.nsc.Reporting.WarningCategory import scala.util.control.NonFatal abstract class AnalyzerRule(val global: Global, val name: String, defaultLevel: Level = Level.Warn) { @@ -28,11 +28,16 @@ abstract class AnalyzerRule(val global: Global, val name: String, defaultLevel: private def adjustMsg(msg: String): String = s"[AVS] $msg" - protected def report(pos: Position, message: String): Unit = + protected final def report( + pos: Position, + message: String, + category: WarningCategory = WarningCategory.Lint, + site: Symbol = NoSymbol + ): Unit = level match { case Level.Off => case Level.Info => reporter.echo(pos, adjustMsg(message)) - case Level.Warn => reporter.warning(pos, adjustMsg(message)) + case Level.Warn => currentRun.reporting.warning(pos, adjustMsg(message), category, site) case Level.Error => reporter.error(pos, adjustMsg(message)) } diff --git a/commons-analyzer/src/main/scala/com/avsystem/commons/analyzer/ConstantDeclarations.scala b/commons-analyzer/src/main/scala/com/avsystem/commons/analyzer/ConstantDeclarations.scala index b27b7d042..a667368d9 100644 --- a/commons-analyzer/src/main/scala/com/avsystem/commons/analyzer/ConstantDeclarations.scala +++ b/commons-analyzer/src/main/scala/com/avsystem/commons/analyzer/ConstantDeclarations.scala @@ -8,7 +8,7 @@ class ConstantDeclarations(g: Global) extends AnalyzerRule(g, "constantDeclarati import global._ def analyze(unit: CompilationUnit): Unit = unit.body.foreach { - case t@ValDef(mods, name, tpt, rhs) + case t@ValDef(_, name, tpt, rhs) if t.symbol.hasGetter && t.symbol.owner.isEffectivelyFinal => val getter = t.symbol.getterIn(t.symbol.owner) @@ -18,15 +18,18 @@ class ConstantDeclarations(g: Global) extends AnalyzerRule(g, "constantDeclarati case _ => false } + def doReport(msg: String): Unit = + report(t.pos, msg, site = t.symbol) + val firstChar = name.toString.charAt(0) if (constantValue && (firstChar.isLower || !getter.isFinal)) { - report(t.pos, "a literal-valued constant should be declared as a `final val` with an UpperCamelCase name") + doReport("a literal-valued constant should be declared as a `final val` with an UpperCamelCase name") } if (!constantValue && firstChar.isUpper && !getter.isFinal) { - report(t.pos, "a constant with UpperCamelCase name should be declared as a `final val`") + doReport("a constant with UpperCamelCase name should be declared as a `final val`") } if (getter.isFinal && constantValue && !(tpt.tpe =:= rhs.tpe)) { - report(t.pos, "a constant with a literal value should not have an explicit type annotation") + doReport("a constant with a literal value should not have an explicit type annotation") } } case _ => From 500ecf24516e59ea4cf9fbf831ea5d8448153d9e Mon Sep 17 00:00:00 2001 From: Roman Janusz Date: Thu, 11 Aug 2022 11:13:39 +0200 Subject: [PATCH 5/5] changed default level of ConstantDeclarations to Off --- .../com/avsystem/commons/analyzer/ConstantDeclarations.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/commons-analyzer/src/main/scala/com/avsystem/commons/analyzer/ConstantDeclarations.scala b/commons-analyzer/src/main/scala/com/avsystem/commons/analyzer/ConstantDeclarations.scala index a667368d9..7911fb9dd 100644 --- a/commons-analyzer/src/main/scala/com/avsystem/commons/analyzer/ConstantDeclarations.scala +++ b/commons-analyzer/src/main/scala/com/avsystem/commons/analyzer/ConstantDeclarations.scala @@ -3,7 +3,7 @@ package analyzer import scala.tools.nsc.Global -class ConstantDeclarations(g: Global) extends AnalyzerRule(g, "constantDeclarations") { +class ConstantDeclarations(g: Global) extends AnalyzerRule(g, "constantDeclarations", Level.Off) { import global._