From 3dd600de4cafa9926d4b17bba1050f1ce5196ade Mon Sep 17 00:00:00 2001 From: Kacper Korban Date: Mon, 21 Nov 2022 04:52:46 +0100 Subject: [PATCH] Port -Wnonunit-statement setting for dotty --- .../tools/dotc/config/ScalaSettings.scala | 1 + .../tools/dotc/reporting/ErrorMessageID.scala | 1 + .../dotty/tools/dotc/reporting/messages.scala | 6 + .../dotty/tools/dotc/typer/RefChecks.scala | 3 +- .../src/dotty/tools/dotc/typer/Typer.scala | 56 ++++- .../fatal-warnings/nonunit-statement.scala | 198 ++++++++++++++++++ 6 files changed, 262 insertions(+), 3 deletions(-) create mode 100644 tests/neg-custom-args/fatal-warnings/nonunit-statement.scala diff --git a/compiler/src/dotty/tools/dotc/config/ScalaSettings.scala b/compiler/src/dotty/tools/dotc/config/ScalaSettings.scala index 5ae99ec7e6fa..8606d72546ef 100644 --- a/compiler/src/dotty/tools/dotc/config/ScalaSettings.scala +++ b/compiler/src/dotty/tools/dotc/config/ScalaSettings.scala @@ -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", diff --git a/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala b/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala index 9f0d71645833..b72ec2a19d76 100644 --- a/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala +++ b/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala @@ -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 diff --git a/compiler/src/dotty/tools/dotc/reporting/messages.scala b/compiler/src/dotty/tools/dotc/reporting/messages.scala index f41d34b8c17c..5f42ac3b1452 100644 --- a/compiler/src/dotty/tools/dotc/reporting/messages.scala +++ b/compiler/src/dotty/tools/dotc/reporting/messages.scala @@ -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) = "" diff --git a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala index 3d53371e603e..4ca00ce6366f 100644 --- a/compiler/src/dotty/tools/dotc/typer/RefChecks.scala +++ b/compiler/src/dotty/tools/dotc/typer/RefChecks.scala @@ -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 diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala index 5752409c51c1..be4ebc11de26 100644 --- a/compiler/src/dotty/tools/dotc/typer/Typer.scala +++ b/compiler/src/dotty/tools/dotc/typer/Typer.scala @@ -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 @@ -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 => @@ -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 diff --git a/tests/neg-custom-args/fatal-warnings/nonunit-statement.scala b/tests/neg-custom-args/fatal-warnings/nonunit-statement.scala new file mode 100644 index 000000000000..399d132edfae --- /dev/null +++ b/tests/neg-custom-args/fatal-warnings/nonunit-statement.scala @@ -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) + () + } +}