Implement -Wvalue-discard warning (#15975)

Warn when an expression `e` with non-Unit type is adapted by embedding
it into a block `{ e; () }` because the expected type is Unit.

In Scala 2.13 ([relevant code in
Typers.scala](https://github.com/scala/scala/blob/d578a02ea6b41b662072759c82c19f9309a15176/src/compiler/scala/tools/nsc/typechecker/Typers.scala#L1116))
there are two special cases:

1. The warning can be disabled by explicitly annotating an expression
with `: Unit`.
* In this PR I've elected not to implement this, as IMO it's not
necessary now we have `@nowarn`. It's just as easy to annotate with
`@nowarn` as with `: Unit`.
2. No warning is raised if the expression is an application of a
function that returns `this.type`, because this is a very common
operation when using mutable collections.
* The heuristic is actually slightly more subtle than that. See the
`warn-value-discard.scala` test for some examples.
* I have implemented this special case by copying the relevant code from
the `2.13.x` branch in scala/scala, so the behaviour should be
equivalent to Scala 2.13.

Author: KacperFKorban

compiler/src/dotty/tools/dotc/ast/TreeInfo.scala

@@ -110,6 +110,24 @@ trait TreeInfo[T <: Untyped] { self: Trees.Instance[T] =>
     case _ => 0
   }
 
+  /** The type arguments of a possibly curried call */
+  def typeArgss(tree: Tree): List[List[Tree]] =
+    @tailrec
+    def loop(tree: Tree, argss: List[List[Tree]]): List[List[Tree]] = tree match
+      case TypeApply(fn, args) => loop(fn, args :: argss)
+      case Apply(fn, args) => loop(fn, argss)
+      case _ => argss
+    loop(tree, Nil)
+
+  /** The term arguments of a possibly curried call */
+  def termArgss(tree: Tree): List[List[Tree]] =
+    @tailrec
+    def loop(tree: Tree, argss: List[List[Tree]]): List[List[Tree]] = tree match
+      case Apply(fn, args) => loop(fn, args :: argss)
+      case TypeApply(fn, args) => loop(fn, argss)
+      case _ => argss
+    loop(tree, Nil)
+
   /** All term arguments of an application in a single flattened list */
   def allArguments(tree: Tree): List[Tree] = unsplice(tree) match {
     case Apply(fn, args) => allArguments(fn) ::: args
@@ -308,6 +326,132 @@ trait TreeInfo[T <: Untyped] { self: Trees.Instance[T] =>
     case Block(_, expr) => forallResults(expr, p)
     case _ => p(tree)
   }
+
+  /** Applications in Scala can have one of the following shapes:
+   *
+   *    1) naked core: Ident(_) or Select(_, _) or basically anything else
+   *    2) naked core with targs: TypeApply(core, targs) or AppliedTypeTree(core, targs)
+   *    3) apply or several applies wrapping a core: Apply(core, _), or Apply(Apply(core, _), _), etc
+   *
+   *  This class provides different ways to decompose applications and simplifies their analysis.
+   *
+   *  ***Examples***
+   *  (TypeApply in the examples can be replaced with AppliedTypeTree)
+   *
+   *    Ident(foo):
+   *      * callee = Ident(foo)
+   *      * core = Ident(foo)
+   *      * targs = Nil
+   *      * argss = Nil
+   *
+   *    TypeApply(foo, List(targ1, targ2...))
+   *      * callee = TypeApply(foo, List(targ1, targ2...))
+   *      * core = foo
+   *      * targs = List(targ1, targ2...)
+   *      * argss = Nil
+   *
+   *    Apply(foo, List(arg1, arg2...))
+   *      * callee = foo
+   *      * core = foo
+   *      * targs = Nil
+   *      * argss = List(List(arg1, arg2...))
+   *
+   *    Apply(Apply(foo, List(arg21, arg22, ...)), List(arg11, arg12...))
+   *      * callee = foo
+   *      * core = foo
+   *      * targs = Nil
+   *      * argss = List(List(arg21, arg22, ...), List(arg11, arg12, ...))
+   *
+   *    Apply(Apply(TypeApply(foo, List(targs1, targs2, ...)), List(arg21, arg22, ...)), List(arg11, arg12...))
+   *      * callee = TypeApply(foo, List(targs1, targs2, ...))
+   *      * core = foo
+   *      * targs = Nil
+   *      * argss = List(List(arg21, arg22, ...), List(arg11, arg12, ...))
+   */
+  final class Applied(val tree: Tree) {
+    /** The tree stripped of the possibly nested applications.
+     *  The original tree if it's not an application.
+     */
+    def callee: Tree = stripApply(tree)
+
+    /** The `callee` unwrapped from type applications.
+     *  The original `callee` if it's not a type application.
+     */
+    def core: Tree = callee match {
+      case TypeApply(fn, _)       => fn
+      case AppliedTypeTree(fn, _) => fn
+      case tree                   => tree
+    }
+
+    /** The type arguments of the `callee`.
+     *  `Nil` if the `callee` is not a type application.
+     */
+    def targs: List[Tree] = callee match {
+      case TypeApply(_, args)       => args
+      case AppliedTypeTree(_, args) => args
+      case _                        => Nil
+    }
+
+    /** (Possibly multiple lists of) value arguments of an application.
+     *  `Nil` if the `callee` is not an application.
+     */
+    def argss: List[List[Tree]] = termArgss(tree)
+  }
+
+  /** Destructures applications into important subparts described in `Applied` class,
+   *  namely into: core, targs and argss (in the specified order).
+   *
+   *  Trees which are not applications are also accepted. Their callee and core will
+   *  be equal to the input, while targs and argss will be Nil.
+   *
+   *  The provided extractors don't expose all the API of the `Applied` class.
+   *  For advanced use, call `dissectApplied` explicitly and use its methods instead of pattern matching.
+   */
+  object Applied {
+    def apply(tree: Tree): Applied = new Applied(tree)
+
+    def unapply(applied: Applied): Some[(Tree, List[Tree], List[List[Tree]])] =
+      Some((applied.core, applied.targs, applied.argss))
+
+    def unapply(tree: Tree): Some[(Tree, List[Tree], List[List[Tree]])] =
+      unapply(new Applied(tree))
+  }
+
+  /** Is tree an application with result `this.type`?
+   *  Accept `b.addOne(x)` and also `xs(i) += x`
+   *  where the op is an assignment operator.
+   */
+  def isThisTypeResult(tree: Tree)(using Context): Boolean = tree match {
+    case Applied(fun @ Select(receiver, op), _, argss) =>
+      tree.tpe match {
+        case ThisType(tref) =>
+          tref.symbol == receiver.symbol
+        case tref: TermRef =>
+          tref.symbol == receiver.symbol || argss.exists(_.exists(tref.symbol == _.symbol))
+        case _ =>
+          def checkSingle(sym: Symbol): Boolean =
+            (sym == receiver.symbol) || {
+              receiver match {
+                case Apply(_, _) => op.isOpAssignmentName                    // xs(i) += x
+                case _ => receiver.symbol != NoSymbol &&
+                  (receiver.symbol.isGetter || receiver.symbol.isField)      // xs.addOne(x) for var xs
+              }
+            }
+          @tailrec def loop(mt: Type): Boolean = mt match {
+            case m: MethodType =>
+              m.resType match {
+                case ThisType(tref) => checkSingle(tref.symbol)
+                case tref: TermRef  => checkSingle(tref.symbol)
+                case restpe => loop(restpe)
+              }
+            case PolyType(_, restpe) => loop(restpe)
+            case _ => false
+          }
+          fun.symbol != NoSymbol && loop(fun.symbol.info)
+      }
+    case _ =>
+      tree.tpe.isInstanceOf[ThisType]
+  }
 }
 
 trait UntypedTreeInfo extends TreeInfo[Untyped] { self: Trees.Instance[Untyped] =>
@@ -683,24 +827,6 @@ trait TypedTreeInfo extends TreeInfo[Type] { self: Trees.Instance[Type] =>
     }
   }
 
-  /** The type arguments of a possibly curried call */
-  def typeArgss(tree: Tree): List[List[Tree]] =
-    @tailrec
-    def loop(tree: Tree, argss: List[List[Tree]]): List[List[Tree]] = tree match
-      case TypeApply(fn, args) => loop(fn, args :: argss)
-      case Apply(fn, args) => loop(fn, argss)
-      case _ => argss
-    loop(tree, Nil)
-
-  /** The term arguments of a possibly curried call */
-  def termArgss(tree: Tree): List[List[Tree]] =
-    @tailrec
-    def loop(tree: Tree, argss: List[List[Tree]]): List[List[Tree]] = tree match
-      case Apply(fn, args) => loop(fn, args :: argss)
-      case TypeApply(fn, args) => loop(fn, argss)
-      case _ => argss
-    loop(tree, Nil)
-
   /** The type and term arguments of a possibly curried call, in the order they are given */
   def allArgss(tree: Tree): List[List[Tree]] =
     @tailrec

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

@@ -159,6 +159,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 Wunused: Setting[List[ChoiceWithHelp[String]]] = MultiChoiceHelpSetting(
     name = "-Wunused",

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

@@ -188,6 +188,7 @@ enum ErrorMessageID(val isActive: Boolean = true) extends java.lang.Enum[ErrorMe
   case MissingImplicitArgumentID // errorNumber 172
   case CannotBeAccessedID // errorNumber 173
   case InlineGivenShouldNotBeFunctionID // errorNumber 174
+  case ValueDiscardingID // errorNumber 175
 
   def errorNumber = ordinal - 1
 

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

@@ -2790,3 +2790,8 @@ extends SyntaxMsg(InlineGivenShouldNotBeFunctionID):
        |      inline def apply(x: A) = x.toB
      """
 
+class ValueDiscarding(tp: Type)(using Context)
+  extends Message(ValueDiscardingID):
+    def kind = MessageKind.PotentialIssue
+    def msg(using Context) = i"discarded non-Unit value of type $tp"
+    def explain(using Context) = ""

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

@@ -3944,6 +3944,9 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
         // so will take the code path that decides on inlining
         val tree1 = adapt(tree, WildcardType, locked)
         checkStatementPurity(tree1)(tree, ctx.owner)
+        if (!ctx.isAfterTyper && !tree.isInstanceOf[Inlined] && ctx.settings.WvalueDiscard.value && !isThisTypeResult(tree)) {
+          report.warning(ValueDiscarding(tree.tpe), tree.srcPos)
+        }
         return tpd.Block(tree1 :: Nil, Literal(Constant(())))
       }
 

tests/neg/warn-value-discard.check

@@ -0,0 +1,20 @@
+-- [E175] Potential Issue Error: tests/neg/warn-value-discard.scala:15:35 ----------------------------------------------
+15 |  firstThing().map(_ => secondThing()) // error
+   |                        ^^^^^^^^^^^^^
+   |                        discarded non-Unit value of type Either[Failed, Unit]
+-- [E175] Potential Issue Error: tests/neg/warn-value-discard.scala:18:35 ----------------------------------------------
+18 |  firstThing().map(_ => secondThing()) // error
+   |                        ^^^^^^^^^^^^^
+   |                        discarded non-Unit value of type Either[Failed, Unit]
+-- [E175] Potential Issue Error: tests/neg/warn-value-discard.scala:27:36 ----------------------------------------------
+27 |    mutable.Set.empty[String].remove("") // error
+   |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |    discarded non-Unit value of type Boolean
+-- [E175] Potential Issue Error: tests/neg/warn-value-discard.scala:39:41 ----------------------------------------------
+39 |    mutable.Set.empty[String].subtractOne("") // error
+   |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |    discarded non-Unit value of type scala.collection.mutable.Set[String]
+-- [E175] Potential Issue Error: tests/neg/warn-value-discard.scala:59:4 -----------------------------------------------
+59 |    mutable.Set.empty[String] += "" // error
+   |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |    discarded non-Unit value of type scala.collection.mutable.Set[String]

tests/neg/warn-value-discard.scala

@@ -0,0 +1,66 @@
+// scalac: -Wvalue-discard -Werror
+
+import scala.util.{Either, Right, Left}
+import scala.collection.mutable
+
+case class Failed(msg: String)
+
+def firstThing(): Either[Failed, Unit] =
+  Right(())
+
+def secondThing(): Either[Failed, Unit] =
+  Left(Failed("whoops you should have flatMapped me"))
+
+def singleExpr(): Either[Failed, Unit] =
+  firstThing().map(_ => secondThing()) // error
+
+def block(): Either[Failed, Unit] = {
+  firstThing().map(_ => secondThing()) // error
+}
+
+class ValueDiscardTest:
+  val field = mutable.Set.empty[String]
+
+  def remove(): Unit =
+    // Set#remove returns a Boolean, not this.type
+    // --> Warning
+    mutable.Set.empty[String].remove("") // error
+
+  // TODO IMHO we don't need to support this,
+  // as it's just as easy to add a @nowarn annotation as a Unit ascription
+  //def removeAscribed(): Unit = {
+  //  mutable.Set.empty[String].remove(""): Unit    // nowarn
+  //}
+
+  def subtract(): Unit =
+    // - Set#subtractOne returns this.type
+    // - receiver is not a field or a local variable (not quite sure what you'd call it)
+    // --> Warning
+    mutable.Set.empty[String].subtractOne("") // error
+
+  def mutateLocalVariable(): Unit = {
+    // - Set#subtractOne returns this.type
+    // - receiver is a local variable
+    // --> No warning
+    val s: mutable.Set[String] = mutable.Set.empty[String]
+    s.subtractOne("")
+  }
+
+  def mutateField(): Unit =
+    // - Set#subtractOne returns this.type
+    // - receiver is a local variable
+    // --> No warning
+    field.subtractOne("")
+
+  def assignmentOperator(): Unit =
+    // - += returns this.type
+    // - receiver is not a field or a local variable
+    // --> Warning
+    mutable.Set.empty[String] += "" // error
+
+  def assignmentOperatorLocalVariable(): Unit =
+    // - += returns this.type
+    // - receiver is a local variable
+    // --> No warning
+    val s: mutable.Set[String] = mutable.Set.empty[String]
+    s += ""