Skip to content

Commit

Permalink
Port -Wnonunit-statement setting for dotty
Browse files Browse the repository at this point in the history
  • Loading branch information
KacperFKorban committed Feb 20, 2023
1 parent ce1ca44 commit 3dd600d
Show file tree
Hide file tree
Showing 6 changed files with 262 additions and 3 deletions.
1 change: 1 addition & 0 deletions compiler/src/dotty/tools/dotc/config/ScalaSettings.scala
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ private sealed trait WarningSettings:
val Whelp: Setting[Boolean] = BooleanSetting("-W", "Print a synopsis of warning options.")
val XfatalWarnings: Setting[Boolean] = BooleanSetting("-Werror", "Fail the compilation if there are any warnings.", aliases = List("-Xfatal-warnings"))
val WvalueDiscard: Setting[Boolean] = BooleanSetting("-Wvalue-discard", "Warn when non-Unit expression results are unused.")
val WNonUnitStatement = BooleanSetting("-Wnonunit-statement", "Warn when block statements are non-Unit expressions.")

val Wunused: Setting[List[ChoiceWithHelp[String]]] = MultiChoiceHelpSetting(
name = "-Wunused",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,7 @@ enum ErrorMessageID(val isActive: Boolean = true) extends java.lang.Enum[ErrorMe
case CannotBeAccessedID // errorNumber 173
case InlineGivenShouldNotBeFunctionID // errorNumber 174
case ValueDiscardingID // errorNumber 175
case UnusedNonUnitValueID // errorNumber 176

def errorNumber = ordinal - 1

Expand Down
6 changes: 6 additions & 0 deletions compiler/src/dotty/tools/dotc/reporting/messages.scala
Original file line number Diff line number Diff line change
Expand Up @@ -2806,3 +2806,9 @@ class ValueDiscarding(tp: Type)(using Context)
def kind = MessageKind.PotentialIssue
def msg(using Context) = i"discarded non-Unit value of type $tp"
def explain(using Context) = ""

class UnusedNonUnitValue(tp: Type)(using Context)
extends Message(UnusedNonUnitValueID):
def kind = MessageKind.PotentialIssue
def msg(using Context) = i"unused value of type $tp"
def explain(using Context) = ""
3 changes: 1 addition & 2 deletions compiler/src/dotty/tools/dotc/typer/RefChecks.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1163,8 +1163,7 @@ class RefChecks extends MiniPhase { thisPhase =>
checkAllOverrides(cls)
checkImplicitNotFoundAnnotation.template(cls.classDenot)
tree
}
catch {
} catch {
case ex: TypeError =>
report.error(ex, tree.srcPos)
tree
Expand Down
56 changes: 55 additions & 1 deletion compiler/src/dotty/tools/dotc/typer/Typer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -1102,6 +1102,7 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
val (stats1, exprCtx) = withoutMode(Mode.Pattern) {
typedBlockStats(tree.stats)
}

var expr1 = typedExpr(tree.expr, pt.dropIfProto)(using exprCtx)

// If unsafe nulls is enabled inside a block but not enabled outside
Expand Down Expand Up @@ -3128,7 +3129,7 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
traverse(xtree :: rest)
case stat :: rest =>
val stat1 = typed(stat)(using ctx.exprContext(stat, exprOwner))
checkStatementPurity(stat1)(stat, exprOwner)
if !checkInterestingResultInStatement(stat1) then checkStatementPurity(stat1)(stat, exprOwner)
buf += stat1
traverse(rest)(using stat1.nullableContext)
case nil =>
Expand Down Expand Up @@ -4212,6 +4213,59 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
typedExpr(cmp, defn.BooleanType)
case _ =>

private def checkInterestingResultInStatement(t: Tree)(using Context): Boolean = {
def isUninterestingSymbol(sym: Symbol): Boolean =
sym == NoSymbol ||
sym.isConstructor ||
sym.is(Package) ||
sym.isPackageObject ||
sym == defn.BoxedUnitClass ||
sym == defn.AnyClass ||
sym == defn.AnyRefAlias ||
sym == defn.AnyValClass
def isUninterestingType(tpe: Type): Boolean =
tpe == NoType ||
tpe.typeSymbol == defn.UnitClass ||
defn.isBottomClass(tpe.typeSymbol) ||
tpe =:= defn.UnitType ||
tpe.typeSymbol == defn.BoxedUnitClass ||
tpe =:= defn.AnyValType ||
tpe =:= defn.AnyType ||
tpe =:= defn.AnyRefType
def isJavaApplication(t: Tree): Boolean = t match {
case Apply(f, _) => f.symbol.is(JavaDefined) && !defn.ObjectClass.isSubClass(f.symbol.owner)
case _ => false
}
def checkInterestingShapes(t: Tree): Boolean = t match {
case If(_, thenpart, elsepart) => checkInterestingShapes(thenpart) || checkInterestingShapes(elsepart)
case Block(_, res) => checkInterestingShapes(res)
case Match(_, cases) => cases.exists(k => checkInterestingShapes(k.body))
case _ => checksForInterestingResult(t)
}
def checksForInterestingResult(t: Tree): Boolean = (
!t.isDef // ignore defs
&& !isUninterestingSymbol(t.symbol) // ctors, package, Unit, Any
&& !isUninterestingType(t.tpe) // bottom types, Unit, Any
&& !isThisTypeResult(t) // buf += x
&& !isSuperConstrCall(t) // just a thing
&& !isJavaApplication(t) // Java methods are inherently side-effecting
// && !treeInfo.hasExplicitUnit(t) // suppressed by explicit expr: Unit // TODO Should explicit `: Unit` be added as warning suppression?
)
if ctx.settings.WNonUnitStatement.value && !ctx.isAfterTyper && checkInterestingShapes(t) then
val where = t match {
case Block(_, res) => res
case If(_, thenpart, Literal(Constant(()))) =>
thenpart match {
case Block(_, res) => res
case _ => thenpart
}
case _ => t
}
report.warning(UnusedNonUnitValue(where.tpe), t.srcPos)
true
else false
}

private def checkStatementPurity(tree: tpd.Tree)(original: untpd.Tree, exprOwner: Symbol)(using Context): Unit =
if !tree.tpe.isErroneous
&& !ctx.isAfterTyper
Expand Down
198 changes: 198 additions & 0 deletions tests/neg-custom-args/fatal-warnings/nonunit-statement.scala
Original file line number Diff line number Diff line change
@@ -0,0 +1,198 @@
// scalac: -Wnonunit-statement -Wvalue-discard
import collection.ArrayOps
import collection.mutable.{ArrayBuilder, LinkedHashSet, ListBuffer}
import concurrent._
import scala.reflect.ClassTag

class C {
import ExecutionContext.Implicits._
def c = {
def improved = Future(42)
def stale = Future(27)
improved // error
stale
}
}
class D {
def d = {
class E
new E().toString // error
new E().toString * 2
}
}
class F {
import ExecutionContext.Implicits._
Future(42) // error
}
// unused template expression uses synthetic method of class
case class K(s: String) {
copy() // error
}
// mutations returning this are ok
class Mutate {
val b = ListBuffer.empty[Int]
b += 42 // nowarn, returns this.type
val xs = List(42)
27 +: xs // error

def f(x: Int): this.type = this
def g(): Unit = f(42) // nowarn
}
// some uninteresting expressions may warn for other reasons
class WhoCares {
null // error for purity
??? // nowarn for impurity
}
// explicit Unit ascription to opt out of warning, even for funky applies
class Absolution {
def f(i: Int): Int = i+1
import ExecutionContext.Implicits._
// Future(42): Unit // nowarn { F(42)(ctx) }: Unit where annot is on F(42)
// f(42): Unit // nowarn
}
// warn uni-branched unless user disables it with -Wnonunit-if:false
class Boxed[A](a: A) {
def isEmpty = false
def foreach[U](f: A => U): Unit =
if (!isEmpty) f(a) // error (if)
def forall(f: A => Boolean): Unit =
if (!isEmpty) {
println(".")
f(a) // error (if)
}
def take(p: A => Boolean): Option[A] = {
while (isEmpty || !p(a)) ()
Some(a).filter(p)
}
}
class Unibranch[A, B] {
def runWith[U](action: B => U): A => Boolean = { x =>
val z = null.asInstanceOf[B]
val fellback = false
if (!fellback) action(z) // error (if)
!fellback
}
def f(i: Int): Int = {
def g = 17
if (i < 42) {
g // error block statement
println("uh oh")
g // error (if)
}
while (i < 42) {
g // error
println("uh oh")
g // error
}
42
}
}
class Dibranch {
def i: Int = ???
def j: Int = ???
def f(b: Boolean): Int = {
// if-expr might have an uninteresting LUB
if (b) { // error, at least one branch looks interesting
println("true")
i
}
else {
println("false")
j
}
42
}
}
class Next[A] {
val all = ListBuffer.empty[A]
def f(it: Iterator[A], g: A => A): Unit =
while (it.hasNext)
all += g(it.next()) // nowarn
}
class Setting[A] {
def set = LinkedHashSet.empty[A]
def f(a: A): Unit = {
set += a // error because cannot know whether the `set` was supposed to be consumed or assigned
println(set)
}
}
// neither StringBuilder warns, because either append is Java method or returns this.type
// while loop looks like if branch with block1(block2, jump to label), where block2 typed as non-unit
class Strung {
def iterator = Iterator.empty[String]
def addString(b: StringBuilder, start: String, sep: String, end: String): StringBuilder = {
val jsb = b.underlying
if (start.length != 0) jsb.append(start) // error (value-discard)
val it = iterator
if (it.hasNext) {
jsb.append(it.next())
while (it.hasNext) {
jsb.append(sep) // nowarn (java)
jsb.append(it.next()) // error (value-discard)
}
}
if (end.length != 0) jsb.append(end) // error (value-discard)
b
}
def f(b: java.lang.StringBuilder, it: Iterator[String]): String = {
while (it.hasNext) {
b.append("\n") // nowarn (java)
b.append(it.next()) // error (value-discard)
}
b.toString
}
def g(b: java.lang.StringBuilder, it: Iterator[String]): String = {
while (it.hasNext) it.next() // error
b.toString
}
}
class J {
import java.util.Collections
def xs: java.util.List[Int] = ???
def f(): Int = {
Collections.checkedList[Int](xs, classOf[Int])
42
}
}
class Variant {
var bs = ListBuffer.empty[Int]
val xs = ListBuffer.empty[Int]
private[this] val ys = ListBuffer.empty[Int]
private[this] var zs = ListBuffer.empty[Int]
def f(i: Int): Unit = {
bs.addOne(i)
xs.addOne(i)
ys.addOne(i)
zs.addOne(i)
println("done")
}
}
final class ArrayOops[A](private val xs: Array[A]) extends AnyVal {
def other: ArrayOps[A] = ???
def transpose[B](implicit asArray: A => Array[B]): Array[Array[B]] = {
val aClass = xs.getClass.getComponentType
val bb = new ArrayBuilder.ofRef[Array[B]]()(ClassTag[Array[B]](aClass))
if (xs.length == 0) bb.result()
else {
def mkRowBuilder() = ArrayBuilder.make[B](ClassTag[B](aClass.getComponentType))
val bs = new ArrayOps(asArray(xs(0))).map((x: B) => mkRowBuilder())
for (xs <- other) {
var i = 0
for (x <- new ArrayOps(asArray(xs))) {
bs(i) += x
i += 1
}
}
for (b <- new ArrayOps(bs)) bb += b.result()
bb.result()
}
}
}
class Depends {
def f[A](a: A): a.type = a
def g() = {
val d = new Depends
f(d)
()
}
}

0 comments on commit 3dd600d

Please sign in to comment.