Skip to content

Commit cc2b7fa

Browse files
author
Roman Janusz
authored
Merge pull request #423 from AVSystem/constants-analyzer-rule
Analyzer rule for Scala constant declarations
2 parents f002133 + 500ecf2 commit cc2b7fa

File tree

6 files changed

+146
-15
lines changed

6 files changed

+146
-15
lines changed

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,8 @@ final class AnalyzerPlugin(val global: Global) extends Plugin { plugin =>
5656
new Any2StringAdd(global),
5757
new ThrowableObjects(global),
5858
new DiscardedMonixTask(global),
59-
new BadSingletonComponent(global)
59+
new BadSingletonComponent(global),
60+
new ConstantDeclarations(global),
6061
)
6162

6263
private lazy val rulesByName = rules.map(r => (r.name, r)).toMap

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

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,8 @@ package com.avsystem.commons
22
package analyzer
33

44
import java.io.{PrintWriter, StringWriter}
5-
65
import scala.tools.nsc.Global
6+
import scala.tools.nsc.Reporting.WarningCategory
77
import scala.util.control.NonFatal
88

99
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:
2828

2929
private def adjustMsg(msg: String): String = s"[AVS] $msg"
3030

31-
protected def report(pos: Position, message: String): Unit =
31+
protected final def report(
32+
pos: Position,
33+
message: String,
34+
category: WarningCategory = WarningCategory.Lint,
35+
site: Symbol = NoSymbol
36+
): Unit =
3237
level match {
3338
case Level.Off =>
3439
case Level.Info => reporter.echo(pos, adjustMsg(message))
35-
case Level.Warn => reporter.warning(pos, adjustMsg(message))
40+
case Level.Warn => currentRun.reporting.warning(pos, adjustMsg(message), category, site)
3641
case Level.Error => reporter.error(pos, adjustMsg(message))
3742
}
3843

‎commons-analyzer/src/main/scala/com/avsystem/commons/analyzer/ConstantDeclarations.scala

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
package com.avsystem.commons
2+
package analyzer
3+
4+
import scala.tools.nsc.Global
5+
6+
class ConstantDeclarations(g: Global) extends AnalyzerRule(g, "constantDeclarations", Level.Off) {
7+
8+
import global._
9+
10+
def analyze(unit: CompilationUnit): Unit = unit.body.foreach {
11+
case t@ValDef(_, name, tpt, rhs)
12+
if t.symbol.hasGetter && t.symbol.owner.isEffectivelyFinal =>
13+
14+
val getter = t.symbol.getterIn(t.symbol.owner)
15+
if (getter.isPublic && getter.isStable && getter.overrides.isEmpty) {
16+
val constantValue = rhs.tpe match {
17+
case ConstantType(_) => true
18+
case _ => false
19+
}
20+
21+
def doReport(msg: String): Unit =
22+
report(t.pos, msg, site = t.symbol)
23+
24+
val firstChar = name.toString.charAt(0)
25+
if (constantValue && (firstChar.isLower || !getter.isFinal)) {
26+
doReport("a literal-valued constant should be declared as a `final val` with an UpperCamelCase name")
27+
}
28+
if (!constantValue && firstChar.isUpper && !getter.isFinal) {
29+
doReport("a constant with UpperCamelCase name should be declared as a `final val`")
30+
}
31+
if (getter.isFinal && constantValue && !(tpt.tpe =:= rhs.tpe)) {
32+
doReport("a constant with a literal value should not have an explicit type annotation")
33+
}
34+
}
35+
case _ =>
36+
}
37+
}

‎commons-analyzer/src/test/scala/com/avsystem/commons/analyzer/CheckMacroPrivateTest.scala

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ class CheckMacroPrivateTest extends AnyFunSuite with AnalyzerTest {
5050
|object test {
5151
| @macroPrivate def macroPrivateMethod = { println("whatever"); 5 }
5252
| @macroPrivate object macroPrivateObject {
53-
| val x = 42
53+
| final val X = 42
5454
| }
5555
|}
5656
""".stripMargin

‎commons-analyzer/src/test/scala/com/avsystem/commons/analyzer/ConstantDeclarationsTest.scala

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
package com.avsystem.commons
2+
package analyzer
3+
4+
import org.scalatest.funsuite.AnyFunSuite
5+
6+
class ConstantDeclarationsTest extends AnyFunSuite with AnalyzerTest {
7+
test("literal-valued constants should be non-lazy final vals with UpperCamelCase and no type annotation") {
8+
assertErrors(4,
9+
"""
10+
|object Whatever {
11+
| // bad
12+
| val a = 10
13+
| val B = 10
14+
| final val c = 10
15+
| final val D: Int = 10
16+
|
17+
| // good
18+
| final val E = 10
19+
|}
20+
""".stripMargin)
21+
}
22+
23+
test("effectively final, non-literal UpperCamelCase vals should be final") {
24+
assertErrors(1,
25+
"""
26+
|object Whatever {
27+
| // bad
28+
| val A = "foo".trim
29+
|
30+
| // good
31+
| final val B = "foo".trim
32+
| val c = "foo".trim
33+
|}
34+
""".stripMargin)
35+
}
36+
37+
test("no constant checking in traits or non-final classes") {
38+
assertNoErrors(
39+
"""
40+
|trait Whatever {
41+
| val a = 10
42+
| val B = 10
43+
| final val c = 10
44+
| final val D: Int = 10
45+
| val A = "foo".trim
46+
|}
47+
|
48+
|class Stuff {
49+
| val a = 10
50+
| val B = 10
51+
| final val c = 10
52+
| final val D: Int = 10
53+
| val A = "foo".trim
54+
|}
55+
""".stripMargin)
56+
}
57+
58+
test("no constant checking for overrides") {
59+
assertNoErrors(
60+
"""
61+
|trait Whatever {
62+
| def a: Int
63+
|}
64+
|
65+
|object Stuff extends Whatever {
66+
| val a: Int = 42
67+
|}
68+
""".stripMargin)
69+
}
70+
71+
test("no constant checking for privates") {
72+
assertNoErrors(
73+
"""
74+
|object Whatever {
75+
| private val a = 10
76+
| private val B = 10
77+
| private final val c = 10
78+
| private final val D: Int = 10
79+
| private val A = "foo".trim
80+
|}
81+
""".stripMargin)
82+
}
83+
}

‎docs/Analyzer.md

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -16,26 +16,31 @@ and [silencer](https://github.com/ghik/silencer) plugin for warning suppression.
1616

1717
Here's a list of currently supported rules:
1818

19-
| Name | Default level | Description |
20-
| --- | --- | --- |
21-
| `importJavaUtil` | warning | Rejects direct imports of `java.util` package. |
19+
| Name | Default level | Description |
20+
|----------------------------| --- |------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
21+
| `importJavaUtil` | warning | Rejects direct imports of `java.util` package. |
2222
| `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). |
23-
| `any2stringadd` | off | Disables `any2stringadd` (concatenating arbitrary values with strings using `+`). |
24-
| `bincompat` | warning | Enables [`@bincompat`](https://github.com/AVSystem/scala-commons/blob/master/commons-core/src/main/scala/com/avsystem/commons/annotation/bincompat.scala) checking |
25-
| `showAst` | error | Handles [`@showAst`](https://github.com/AVSystem/scala-commons/blob/master/commons-core/src/main/scala/com/avsystem/commons/annotation/showAst.scala). |
26-
| `findUsages` | warning | Issues a message for every occurrence of any of the symbols listed in the argument of this rule |
27-
| `varargsAtLeast` | warning | Enables [`@atLeast`](https://github.com/AVSystem/scala-commons/blob/master/commons-core/src/main/scala/com/avsystem/commons/annotation/atLeast.scala) checking |
28-
| `macroPrivate` | warning | Enables [`@macroPrivate`](https://github.com/AVSystem/scala-commons/blob/master/commons-core/src/main/scala/com/avsystem/commons/annotation/macroPrivate.scala) checking |
29-
| `explicitGenerics` | warning | Enables [`@explicitGenerics`](https://github.com/AVSystem/scala-commons/blob/master/commons-core/src/main/scala/com/avsystem/commons/annotation/explicitGenerics.scala) checking |
23+
| `any2stringadd` | off | Disables `any2stringadd` (concatenating arbitrary values with strings using `+`). |
24+
| `bincompat` | warning | Enables [`@bincompat`](https://github.com/AVSystem/scala-commons/blob/master/commons-core/src/main/scala/com/avsystem/commons/annotation/bincompat.scala) checking |
25+
| `showAst` | error | Handles [`@showAst`](https://github.com/AVSystem/scala-commons/blob/master/commons-core/src/main/scala/com/avsystem/commons/annotation/showAst.scala). |
26+
| `findUsages` | warning | Issues a message for every occurrence of any of the symbols listed in the argument of this rule |
27+
| `varargsAtLeast` | warning | Enables [`@atLeast`](https://github.com/AVSystem/scala-commons/blob/master/commons-core/src/main/scala/com/avsystem/commons/annotation/atLeast.scala) checking |
28+
| `macroPrivate` | warning | Enables [`@macroPrivate`](https://github.com/AVSystem/scala-commons/blob/master/commons-core/src/main/scala/com/avsystem/commons/annotation/macroPrivate.scala) checking |
29+
| `explicitGenerics` | warning | Enables [`@explicitGenerics`](https://github.com/AVSystem/scala-commons/blob/master/commons-core/src/main/scala/com/avsystem/commons/annotation/explicitGenerics.scala) checking |
30+
| `discardedMonixTask` | warning | Makes sure that expressions evaluating to Monix `Task`s are not accidentally discarded by conversion to `Unit` |
31+
| `throwableObjects` | warning | Makes sure that objects are never used as `Throwable`s (unless they have stack traces disabled) |
32+
| `constantDeclarations` | warning | Checks if constants are always declared as `final val`s with `UpperCamelCase` and no explicit type annotation for literal values |
3033

3134
Rules may be enabled and disabled in `build.sbt` with Scala compiler options:
3235

3336
```scala
3437
scalacOptions += "-P:AVSystemAnalyzer:<level><ruleName>,<level><ruleName>,..."
3538
```
39+
3640
`<name>` must be one of the rule names listed above or `_` to apply to all rules.
3741

3842
`<level>` may be one of:
43+
3944
* `-` to disable rule
4045
* `*` for "info" level
4146
* _empty_ for "warning" level

0 commit comments

Comments
 (0)