Skip to content

Commit 847eccc

Browse files
Port -Wnonunit-statement setting for dotty (#16936)
`-Wnonunit-statement` warns about discarded values in statement positions (Somewhat a supplement to `-Wvalue-discard` and a stronger version of pure expression warnings)
2 parents 1821107 + 3dd600d commit 847eccc

File tree

6 files changed

+262
-3
lines changed

6 files changed

+262
-3
lines changed

compiler/src/dotty/tools/dotc/config/ScalaSettings.scala

+1
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,7 @@ private sealed trait WarningSettings:
160160
val Whelp: Setting[Boolean] = BooleanSetting("-W", "Print a synopsis of warning options.")
161161
val XfatalWarnings: Setting[Boolean] = BooleanSetting("-Werror", "Fail the compilation if there are any warnings.", aliases = List("-Xfatal-warnings"))
162162
val WvalueDiscard: Setting[Boolean] = BooleanSetting("-Wvalue-discard", "Warn when non-Unit expression results are unused.")
163+
val WNonUnitStatement = BooleanSetting("-Wnonunit-statement", "Warn when block statements are non-Unit expressions.")
163164

164165
val Wunused: Setting[List[ChoiceWithHelp[String]]] = MultiChoiceHelpSetting(
165166
name = "-Wunused",

compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala

+1
Original file line numberDiff line numberDiff line change
@@ -189,6 +189,7 @@ enum ErrorMessageID(val isActive: Boolean = true) extends java.lang.Enum[ErrorMe
189189
case CannotBeAccessedID // errorNumber 173
190190
case InlineGivenShouldNotBeFunctionID // errorNumber 174
191191
case ValueDiscardingID // errorNumber 175
192+
case UnusedNonUnitValueID // errorNumber 176
192193

193194
def errorNumber = ordinal - 1
194195

compiler/src/dotty/tools/dotc/reporting/messages.scala

+6
Original file line numberDiff line numberDiff line change
@@ -2817,3 +2817,9 @@ class ValueDiscarding(tp: Type)(using Context)
28172817
def kind = MessageKind.PotentialIssue
28182818
def msg(using Context) = i"discarded non-Unit value of type $tp"
28192819
def explain(using Context) = ""
2820+
2821+
class UnusedNonUnitValue(tp: Type)(using Context)
2822+
extends Message(UnusedNonUnitValueID):
2823+
def kind = MessageKind.PotentialIssue
2824+
def msg(using Context) = i"unused value of type $tp"
2825+
def explain(using Context) = ""

compiler/src/dotty/tools/dotc/typer/RefChecks.scala

+1-2
Original file line numberDiff line numberDiff line change
@@ -1163,8 +1163,7 @@ class RefChecks extends MiniPhase { thisPhase =>
11631163
checkAllOverrides(cls)
11641164
checkImplicitNotFoundAnnotation.template(cls.classDenot)
11651165
tree
1166-
}
1167-
catch {
1166+
} catch {
11681167
case ex: TypeError =>
11691168
report.error(ex, tree.srcPos)
11701169
tree

compiler/src/dotty/tools/dotc/typer/Typer.scala

+55-1
Original file line numberDiff line numberDiff line change
@@ -1102,6 +1102,7 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
11021102
val (stats1, exprCtx) = withoutMode(Mode.Pattern) {
11031103
typedBlockStats(tree.stats)
11041104
}
1105+
11051106
var expr1 = typedExpr(tree.expr, pt.dropIfProto)(using exprCtx)
11061107

11071108
// If unsafe nulls is enabled inside a block but not enabled outside
@@ -3128,7 +3129,7 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
31283129
traverse(xtree :: rest)
31293130
case stat :: rest =>
31303131
val stat1 = typed(stat)(using ctx.exprContext(stat, exprOwner))
3131-
checkStatementPurity(stat1)(stat, exprOwner)
3132+
if !checkInterestingResultInStatement(stat1) then checkStatementPurity(stat1)(stat, exprOwner)
31323133
buf += stat1
31333134
traverse(rest)(using stat1.nullableContext)
31343135
case nil =>
@@ -4215,6 +4216,59 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
42154216
typedExpr(cmp, defn.BooleanType)
42164217
case _ =>
42174218

4219+
private def checkInterestingResultInStatement(t: Tree)(using Context): Boolean = {
4220+
def isUninterestingSymbol(sym: Symbol): Boolean =
4221+
sym == NoSymbol ||
4222+
sym.isConstructor ||
4223+
sym.is(Package) ||
4224+
sym.isPackageObject ||
4225+
sym == defn.BoxedUnitClass ||
4226+
sym == defn.AnyClass ||
4227+
sym == defn.AnyRefAlias ||
4228+
sym == defn.AnyValClass
4229+
def isUninterestingType(tpe: Type): Boolean =
4230+
tpe == NoType ||
4231+
tpe.typeSymbol == defn.UnitClass ||
4232+
defn.isBottomClass(tpe.typeSymbol) ||
4233+
tpe =:= defn.UnitType ||
4234+
tpe.typeSymbol == defn.BoxedUnitClass ||
4235+
tpe =:= defn.AnyValType ||
4236+
tpe =:= defn.AnyType ||
4237+
tpe =:= defn.AnyRefType
4238+
def isJavaApplication(t: Tree): Boolean = t match {
4239+
case Apply(f, _) => f.symbol.is(JavaDefined) && !defn.ObjectClass.isSubClass(f.symbol.owner)
4240+
case _ => false
4241+
}
4242+
def checkInterestingShapes(t: Tree): Boolean = t match {
4243+
case If(_, thenpart, elsepart) => checkInterestingShapes(thenpart) || checkInterestingShapes(elsepart)
4244+
case Block(_, res) => checkInterestingShapes(res)
4245+
case Match(_, cases) => cases.exists(k => checkInterestingShapes(k.body))
4246+
case _ => checksForInterestingResult(t)
4247+
}
4248+
def checksForInterestingResult(t: Tree): Boolean = (
4249+
!t.isDef // ignore defs
4250+
&& !isUninterestingSymbol(t.symbol) // ctors, package, Unit, Any
4251+
&& !isUninterestingType(t.tpe) // bottom types, Unit, Any
4252+
&& !isThisTypeResult(t) // buf += x
4253+
&& !isSuperConstrCall(t) // just a thing
4254+
&& !isJavaApplication(t) // Java methods are inherently side-effecting
4255+
// && !treeInfo.hasExplicitUnit(t) // suppressed by explicit expr: Unit // TODO Should explicit `: Unit` be added as warning suppression?
4256+
)
4257+
if ctx.settings.WNonUnitStatement.value && !ctx.isAfterTyper && checkInterestingShapes(t) then
4258+
val where = t match {
4259+
case Block(_, res) => res
4260+
case If(_, thenpart, Literal(Constant(()))) =>
4261+
thenpart match {
4262+
case Block(_, res) => res
4263+
case _ => thenpart
4264+
}
4265+
case _ => t
4266+
}
4267+
report.warning(UnusedNonUnitValue(where.tpe), t.srcPos)
4268+
true
4269+
else false
4270+
}
4271+
42184272
private def checkStatementPurity(tree: tpd.Tree)(original: untpd.Tree, exprOwner: Symbol)(using Context): Unit =
42194273
if !tree.tpe.isErroneous
42204274
&& !ctx.isAfterTyper
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,198 @@
1+
// scalac: -Wnonunit-statement -Wvalue-discard
2+
import collection.ArrayOps
3+
import collection.mutable.{ArrayBuilder, LinkedHashSet, ListBuffer}
4+
import concurrent._
5+
import scala.reflect.ClassTag
6+
7+
class C {
8+
import ExecutionContext.Implicits._
9+
def c = {
10+
def improved = Future(42)
11+
def stale = Future(27)
12+
improved // error
13+
stale
14+
}
15+
}
16+
class D {
17+
def d = {
18+
class E
19+
new E().toString // error
20+
new E().toString * 2
21+
}
22+
}
23+
class F {
24+
import ExecutionContext.Implicits._
25+
Future(42) // error
26+
}
27+
// unused template expression uses synthetic method of class
28+
case class K(s: String) {
29+
copy() // error
30+
}
31+
// mutations returning this are ok
32+
class Mutate {
33+
val b = ListBuffer.empty[Int]
34+
b += 42 // nowarn, returns this.type
35+
val xs = List(42)
36+
27 +: xs // error
37+
38+
def f(x: Int): this.type = this
39+
def g(): Unit = f(42) // nowarn
40+
}
41+
// some uninteresting expressions may warn for other reasons
42+
class WhoCares {
43+
null // error for purity
44+
??? // nowarn for impurity
45+
}
46+
// explicit Unit ascription to opt out of warning, even for funky applies
47+
class Absolution {
48+
def f(i: Int): Int = i+1
49+
import ExecutionContext.Implicits._
50+
// Future(42): Unit // nowarn { F(42)(ctx) }: Unit where annot is on F(42)
51+
// f(42): Unit // nowarn
52+
}
53+
// warn uni-branched unless user disables it with -Wnonunit-if:false
54+
class Boxed[A](a: A) {
55+
def isEmpty = false
56+
def foreach[U](f: A => U): Unit =
57+
if (!isEmpty) f(a) // error (if)
58+
def forall(f: A => Boolean): Unit =
59+
if (!isEmpty) {
60+
println(".")
61+
f(a) // error (if)
62+
}
63+
def take(p: A => Boolean): Option[A] = {
64+
while (isEmpty || !p(a)) ()
65+
Some(a).filter(p)
66+
}
67+
}
68+
class Unibranch[A, B] {
69+
def runWith[U](action: B => U): A => Boolean = { x =>
70+
val z = null.asInstanceOf[B]
71+
val fellback = false
72+
if (!fellback) action(z) // error (if)
73+
!fellback
74+
}
75+
def f(i: Int): Int = {
76+
def g = 17
77+
if (i < 42) {
78+
g // error block statement
79+
println("uh oh")
80+
g // error (if)
81+
}
82+
while (i < 42) {
83+
g // error
84+
println("uh oh")
85+
g // error
86+
}
87+
42
88+
}
89+
}
90+
class Dibranch {
91+
def i: Int = ???
92+
def j: Int = ???
93+
def f(b: Boolean): Int = {
94+
// if-expr might have an uninteresting LUB
95+
if (b) { // error, at least one branch looks interesting
96+
println("true")
97+
i
98+
}
99+
else {
100+
println("false")
101+
j
102+
}
103+
42
104+
}
105+
}
106+
class Next[A] {
107+
val all = ListBuffer.empty[A]
108+
def f(it: Iterator[A], g: A => A): Unit =
109+
while (it.hasNext)
110+
all += g(it.next()) // nowarn
111+
}
112+
class Setting[A] {
113+
def set = LinkedHashSet.empty[A]
114+
def f(a: A): Unit = {
115+
set += a // error because cannot know whether the `set` was supposed to be consumed or assigned
116+
println(set)
117+
}
118+
}
119+
// neither StringBuilder warns, because either append is Java method or returns this.type
120+
// while loop looks like if branch with block1(block2, jump to label), where block2 typed as non-unit
121+
class Strung {
122+
def iterator = Iterator.empty[String]
123+
def addString(b: StringBuilder, start: String, sep: String, end: String): StringBuilder = {
124+
val jsb = b.underlying
125+
if (start.length != 0) jsb.append(start) // error (value-discard)
126+
val it = iterator
127+
if (it.hasNext) {
128+
jsb.append(it.next())
129+
while (it.hasNext) {
130+
jsb.append(sep) // nowarn (java)
131+
jsb.append(it.next()) // error (value-discard)
132+
}
133+
}
134+
if (end.length != 0) jsb.append(end) // error (value-discard)
135+
b
136+
}
137+
def f(b: java.lang.StringBuilder, it: Iterator[String]): String = {
138+
while (it.hasNext) {
139+
b.append("\n") // nowarn (java)
140+
b.append(it.next()) // error (value-discard)
141+
}
142+
b.toString
143+
}
144+
def g(b: java.lang.StringBuilder, it: Iterator[String]): String = {
145+
while (it.hasNext) it.next() // error
146+
b.toString
147+
}
148+
}
149+
class J {
150+
import java.util.Collections
151+
def xs: java.util.List[Int] = ???
152+
def f(): Int = {
153+
Collections.checkedList[Int](xs, classOf[Int])
154+
42
155+
}
156+
}
157+
class Variant {
158+
var bs = ListBuffer.empty[Int]
159+
val xs = ListBuffer.empty[Int]
160+
private[this] val ys = ListBuffer.empty[Int]
161+
private[this] var zs = ListBuffer.empty[Int]
162+
def f(i: Int): Unit = {
163+
bs.addOne(i)
164+
xs.addOne(i)
165+
ys.addOne(i)
166+
zs.addOne(i)
167+
println("done")
168+
}
169+
}
170+
final class ArrayOops[A](private val xs: Array[A]) extends AnyVal {
171+
def other: ArrayOps[A] = ???
172+
def transpose[B](implicit asArray: A => Array[B]): Array[Array[B]] = {
173+
val aClass = xs.getClass.getComponentType
174+
val bb = new ArrayBuilder.ofRef[Array[B]]()(ClassTag[Array[B]](aClass))
175+
if (xs.length == 0) bb.result()
176+
else {
177+
def mkRowBuilder() = ArrayBuilder.make[B](ClassTag[B](aClass.getComponentType))
178+
val bs = new ArrayOps(asArray(xs(0))).map((x: B) => mkRowBuilder())
179+
for (xs <- other) {
180+
var i = 0
181+
for (x <- new ArrayOps(asArray(xs))) {
182+
bs(i) += x
183+
i += 1
184+
}
185+
}
186+
for (b <- new ArrayOps(bs)) bb += b.result()
187+
bb.result()
188+
}
189+
}
190+
}
191+
class Depends {
192+
def f[A](a: A): a.type = a
193+
def g() = {
194+
val d = new Depends
195+
f(d)
196+
()
197+
}
198+
}

0 commit comments

Comments
 (0)