Merge pull request #9893 from som-snytt/topic/warn-nonunit-statement

Add `-Wnonunit-statement` to warn about discarded values in statement position

src/compiler/scala/tools/nsc/ast/parser/Parsers.scala

@@ -1617,8 +1617,22 @@ self =>
           val cond = condExpr()
           newLinesOpt()
           val thenp = expr()
-          val elsep = if (in.token == ELSE) { in.nextToken(); expr() }
-          else literalUnit
+          val elsep =
+            if (in.token == ELSE) {
+              in.nextToken()
+              expr()
+            }
+            else {
+              // user asked to silence warnings on unibranch if; also suppresses value discard
+              if (settings.warnNonUnitIf.isSetByUser && !settings.warnNonUnitIf.value) {
+                thenp match {
+                  case Block(_, res) => res.updateAttachment(TypedExpectingUnitAttachment)
+                  case _ => ()
+                }
+                thenp.updateAttachment(TypedExpectingUnitAttachment)
+              }
+              literalUnit
+            }
           If(cond, thenp, elsep)
         }
         parseIf

src/compiler/scala/tools/nsc/settings/Warnings.scala

@@ -113,6 +113,10 @@ trait Warnings {
     )
   ) withAbbreviation "-Ywarn-macros"
   val warnDeadCode         = BooleanSetting("-Wdead-code", "Warn when dead code is identified.") withAbbreviation "-Ywarn-dead-code"
+  val warnNonUnitIf        = BooleanSetting("-Wnonunit-if", "Warn when if statements are non-Unit expressions, enabled by -Wnonunit-statement.")
+  import scala.language.existentials
+  val warnNonUnitStatement = BooleanSetting("-Wnonunit-statement", "Warn when block statements are non-Unit expressions.")
+    .enablingIfNotSetByUser(warnNonUnitIf :: Nil)
   val warnValueDiscard     = BooleanSetting("-Wvalue-discard", "Warn when non-Unit expression results are unused.") withAbbreviation "-Ywarn-value-discard"
   val warnNumericWiden     = BooleanSetting("-Wnumeric-widen", "Warn when numerics are widened.") withAbbreviation "-Ywarn-numeric-widen"
   val warnOctalLiteral     = BooleanSetting("-Woctal-literal", "Warn on obsolete octal syntax.") withAbbreviation "-Ywarn-octal-literal"

src/compiler/scala/tools/nsc/typechecker/RefChecks.scala

@@ -1726,19 +1726,83 @@ abstract class RefChecks extends Transform {
     }
 
     // Verify classes extending AnyVal meet the requirements
-    private def checkAnyValSubclass(clazz: Symbol) = {
+    private def checkAnyValSubclass(clazz: Symbol) =
       if (clazz.isDerivedValueClass) {
         if (clazz.isTrait)
           reporter.error(clazz.pos, "Only classes (not traits) are allowed to extend AnyVal")
         else if (clazz.hasAbstractFlag)
           reporter.error(clazz.pos, "`abstract` modifier cannot be used with value classes")
       }
-    }
 
     private def checkUnexpandedMacro(t: Tree) =
       if (!t.isDef && t.hasSymbolField && t.symbol.isTermMacro)
         reporter.error(t.pos, "macro has not been expanded")
 
+    // if expression in statement position (of template or block)
+    // looks like a useful value that should not be ignored, warn and return true
+    // User specifies that an expression is boring by ascribing `e: Unit`.
+    // The subtree `e` will bear an attachment, but may be wrapped in adaptations.
+    private def checkInterestingResultInStatement(t: Tree): Boolean = {
+      def isUninterestingSymbol(sym: Symbol): Boolean =
+        sym != null && (
+          sym.isConstructor ||
+          sym.hasPackageFlag ||
+          sym.isPackageObjectOrClass ||
+          sym == BoxedUnitClass ||
+          sym == AnyClass ||
+          sym == AnyRefClass ||
+          sym == AnyValClass
+        )
+      def isUninterestingType(tpe: Type): Boolean =
+        tpe != null && (
+          isUnitType(tpe) ||
+          tpe.typeSymbol.isBottomClass ||
+          tpe =:= UnitTpe ||
+          tpe =:= BoxedUnitTpe ||
+          isTrivialTopType(tpe)
+        )
+      // java lacks this.type idiom to distinguish side-effecting method, so ignore result of invoking java method.
+      def isJavaApplication(t: Tree): Boolean = t match {
+        case Apply(f, _) => f.symbol.isJavaDefined && !isUniversalMember(f.symbol)
+        case _ => false
+      }
+      // The quirk of typechecking if is that the LUB often results in boring types.
+      // Parser adds suppressing attachment on `if (b) expr` when user has `-Wnonunit-if:false`.
+      def checkInterestingShapes(t: Tree): Boolean =
+        t match {
+          case If(_, thenpart, elsepart) => checkInterestingShapes(thenpart) || checkInterestingShapes(elsepart) // either or
+          //case Block(_, Apply(label, Nil)) if label.symbol != null && nme.isLoopHeaderLabel(label.symbol.name) => false
+          case Block(_, res) => checkInterestingShapes(res)
+          case Match(_, cases) => cases.exists(k => checkInterestingShapes(k.body))
+          case _ => checksForInterestingResult(t)
+        }
+      // tests for various flavors of blandness in expressions.
+      def checksForInterestingResult(t: Tree): Boolean = (
+           !t.isDef && !treeInfo.isPureDef(t)     // ignore defs
+        && !isUninterestingSymbol(t.symbol)       // ctors, package, Unit, Any
+        && !isUninterestingType(t.tpe)            // bottom types, Unit, Any
+        && !treeInfo.isThisTypeResult(t)          // buf += x
+        && !treeInfo.isSuperConstrCall(t)         // just a thing
+        && !treeInfo.hasExplicitUnit(t)           // suppressed by explicit expr: Unit
+        && !isJavaApplication(t)                  // Java methods are inherently side-effecting
+      )
+      // begin checkInterestingResultInStatement
+      settings.warnNonUnitStatement.value && checkInterestingShapes(t) && {
+        val where = t match {
+          case Block(_, res) => res
+          case If(_, thenpart, Literal(Constant(()))) =>
+            thenpart match {
+              case Block(_, res) => res
+              case _ => thenpart
+            }
+          case _ => t
+        }
+        def msg = s"unused value of type ${where.tpe} (add `: Unit` to discard silently)"
+        refchecksWarning(where.pos, msg, WarningCategory.OtherPureStatement)
+        true
+      }
+    } // end checkInterestingResultInStatement
+
     override def transform(tree: Tree): Tree = {
       val savedLocalTyper = localTyper
       val savedCurrentApplication = currentApplication
@@ -1778,14 +1842,12 @@ abstract class RefChecks extends Transform {
 
           case Template(parents, self, body) =>
             localTyper = localTyper.atOwner(tree, currentOwner)
-            for (stat <- body) {
-              if (treeInfo.isPureExprForWarningPurposes(stat)) {
+            for (stat <- body)
+              if (!checkInterestingResultInStatement(stat) && treeInfo.isPureExprForWarningPurposes(stat)) {
                 val msg = "a pure expression does nothing in statement position"
                 val clause = if (body.lengthCompare(1) > 0) "; multiline expressions may require enclosing parentheses" else ""
                 refchecksWarning(stat.pos, s"$msg$clause", WarningCategory.OtherPureStatement)
               }
-            }
-
             validateBaseTypes(currentOwner)
             checkOverloadedRestrictions(currentOwner, currentOwner)
             // scala/bug#7870 default getters for constructors live in the companion module
@@ -1852,7 +1914,8 @@ abstract class RefChecks extends Transform {
                            // probably not, until we allow parameterised extractors
             tree
 
-          case Block(stats, expr) =>
+          case blk @ Block(stats, expr) =>
+            // diagnostic info
             val (count, result0, adapted) =
               expr match {
                 case Block(expr :: Nil, Literal(Constant(()))) => (1, expr, true)
@@ -1862,7 +1925,7 @@ abstract class RefChecks extends Transform {
             val isMultiline = stats.lengthCompare(1 - count) > 0
 
             def checkPure(t: Tree, supple: Boolean): Unit =
-              if (!analyzer.explicitlyUnit(t) && treeInfo.isPureExprForWarningPurposes(t)) {
+              if (!treeInfo.hasExplicitUnit(t) && treeInfo.isPureExprForWarningPurposes(t)) {
                 val msg = "a pure expression does nothing in statement position"
                 val parens = if (isMultiline) "multiline expressions might require enclosing parentheses" else ""
                 val discard = if (adapted) "; a value can be silently discarded when Unit is expected" else ""
@@ -1871,8 +1934,8 @@ abstract class RefChecks extends Transform {
                   else if (!parens.isEmpty) s"$msg; $parens" else msg
                 refchecksWarning(t.pos, text, WarningCategory.OtherPureStatement)
               }
-            // check block for unintended expr placement
-            stats.foreach(checkPure(_, supple = false))
+            // check block for unintended "expression in statement position"
+            stats.foreach { t => if (!checkInterestingResultInStatement(t)) checkPure(t, supple = false) }
             if (result0.nonEmpty) checkPure(result0, supple = true)
 
             def checkImplicitlyAdaptedBlockResult(t: Tree): Unit =

src/compiler/scala/tools/nsc/typechecker/StdAttachments.scala

@@ -196,10 +196,6 @@ trait StdAttachments {
    * track of other adapted trees.
    */
   case class OriginalTreeAttachment(original: Tree)
-
-  /** Marks a Typed tree with Unit tpt. */
-  case object TypedExpectingUnitAttachment
-  def explicitlyUnit(tree: Tree): Boolean = tree.hasAttachment[TypedExpectingUnitAttachment.type]
 }
 
 

src/compiler/scala/tools/nsc/typechecker/Typers.scala

@@ -750,7 +750,7 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
         if (immediate) {
           action()
         } else {
-          unit.toCheck += (() => action())
+          unit.toCheck += (() => action(): Unit)
           true
         }
       }
@@ -1112,15 +1112,9 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
           @inline def tpdPos(transformed: Tree) = typedPos(tree.pos, mode, pt)(transformed)
           @inline def tpd(transformed: Tree)    = typed(transformed, mode, pt)
 
-          @inline def warnValueDiscard(): Unit = if (!isPastTyper && settings.warnValueDiscard.value) {
-            def isThisTypeResult = (tree, tree.tpe) match {
-              case (Apply(Select(receiver, _), _), SingleType(_, sym)) => sym == receiver.symbol
-              case _ => false
-            }
-            if (!isThisTypeResult && !explicitlyUnit(tree))
-              context.warning(tree.pos, s"discarded non-Unit value of type ${tree.tpe}",
-                WarningCategory.WFlagValueDiscard)
-          }
+          @inline def warnValueDiscard(): Unit =
+            if (!isPastTyper && settings.warnValueDiscard.value && !treeInfo.isThisTypeResult(tree) && !treeInfo.hasExplicitUnit(tree))
+              context.warning(tree.pos, s"discarded non-Unit value of type ${tree.tpe}", WarningCategory.WFlagValueDiscard)
           @inline def warnNumericWiden(tpSym: Symbol, ptSym: Symbol): Unit =
             if (!isPastTyper) {
               val isInharmonic = (
@@ -1245,7 +1239,7 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
         case ct @ FoldableConstantType(value) if mode.inNone(TYPEmode | FUNmode) && (ct <:< pt) && canAdaptConstantTypeToLiteral => // (0)
           adaptConstant(value)
         case OverloadedType(_, _) if !mode.inFunMode => // (1)
-          inferExprAlternative(tree, pt)
+          inferExprAlternative(tree, pt): Unit
           adaptAfterOverloadResolution(tree, mode, pt, original)
         case NullaryMethodType(restpe) => // (2)
           if (hasUndets && settings.lintUniversalMethods && (isCastSymbol(tree.symbol) || isTypeTestSymbol(tree.symbol)) && context.undetparams.exists(_.owner == tree.symbol))
@@ -4722,7 +4716,7 @@ trait Typers extends Adaptations with Tags with TypersTracking with PatternTyper
 
       def typedIf(tree: If): If = {
         val cond1 = checkDead(context, typedByValueExpr(tree.cond, BooleanTpe))
-        // One-legged ifs don't need a lot of analysis
+        // Unibranch if normally has unit value else, but synthetic code may emit empty else.
         if (tree.elsep.isEmpty)
           return treeCopy.If(tree, cond1, typed(tree.thenp, UnitTpe), tree.elsep) setType UnitTpe
 

src/reflect/scala/reflect/api/Internals.scala

@@ -530,7 +530,7 @@ trait Internals { self: Universe =>
     /** Set symbol's type signature to given type.
      *  @return the symbol itself
      */
-    def setInfo[S <: Symbol](sym: S, tpe: Type): S
+    def setInfo[S <: Symbol](sym: S, tpe: Type): sym.type
 
     /** Set symbol's annotations to given annotations `annots`.
      */

src/reflect/scala/reflect/internal/ReificationSupport.scala

@@ -60,7 +60,7 @@ trait ReificationSupport { self: SymbolTable =>
     def setAnnotations[S <: Symbol](sym: S, annots: List[AnnotationInfo]): S =
       sym.setAnnotations(annots)
 
-    def setInfo[S <: Symbol](sym: S, tpe: Type): S =
+    def setInfo[S <: Symbol](sym: S, tpe: Type): sym.type =
       sym.setInfo(tpe).markAllCompleted()
 
     def mkThis(sym: Symbol): Tree = self.This(sym)

src/reflect/scala/reflect/internal/StdAttachments.scala

@@ -145,4 +145,8 @@ trait StdAttachments {
 
   // Use of _root_ is in correct leading position of selection
   case object RootSelection extends PlainAttachment
+
+  /** Marks a Typed tree with Unit tpt. */
+  case object TypedExpectingUnitAttachment
+  def explicitlyUnit(tree: Tree): Boolean = tree.hasAttachment[TypedExpectingUnitAttachment.type]
 }

src/reflect/scala/reflect/internal/TreeInfo.scala

@@ -351,6 +351,42 @@ abstract class TreeInfo {
     case _                                    => false
   }
 
+  /** 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): Boolean = tree match {
+    case Applied(fun @ Select(receiver, op), _, argss) =>
+      tree.tpe match {
+        case ThisType(sym) =>
+          sym == receiver.symbol
+        case SingleType(p, sym) =>
+          sym == receiver.symbol || argss.exists(_.exists(sym == _.symbol))
+        case _ =>
+          def checkSingle(sym: Symbol): Boolean =
+            (sym == receiver.symbol) || {
+              receiver match {
+                case Apply(_, _) => Precedence(op.decoded).level == 0         // xs(i) += x
+                case _ => receiver.symbol != null &&
+                  (receiver.symbol.isGetter || receiver.symbol.isField)       // xs.addOne(x) for var xs
+              }
+            }
+          @tailrec def loop(mt: Type): Boolean = mt match {
+            case MethodType(_, restpe) =>
+              restpe match {
+                case ThisType(sym) => checkSingle(sym)
+                case SingleType(_, sym) => checkSingle(sym)
+                case _ => loop(restpe)
+              }
+            case PolyType(_, restpe) => loop(restpe)
+            case _ => false
+          }
+          fun.symbol != null && loop(fun.symbol.info)
+      }
+    case _ =>
+      tree.tpe.isInstanceOf[ThisType]
+  }
+
   /**
    * Named arguments can transform a constructor call into a block, e.g.
    *   <init>(b = foo, a = bar)
@@ -746,6 +782,16 @@ abstract class TreeInfo {
     ((sym ne null) && sym.initialize.isTrait)
   }
 
+  def hasExplicitUnit(tree: Tree): Boolean =
+    explicitlyUnit(tree) || {
+      tree match {
+        case Apply(f, _)           => hasExplicitUnit(f)
+        case TypeApply(f, _)       => hasExplicitUnit(f)
+        case AppliedTypeTree(f, _) => hasExplicitUnit(f)
+        case _                     => false
+      }
+    }
+
   /** Applications in Scala can have one of the following shapes:
    *
    *    1) naked core: Ident(_) or Select(_, _) or basically anything else

src/reflect/scala/reflect/runtime/JavaUniverseForce.scala

@@ -77,6 +77,7 @@ trait JavaUniverseForce { self: runtime.JavaUniverse  =>
     this.ChangeOwnerAttachment
     this.InterpolatedString
     this.RootSelection
+    this.TypedExpectingUnitAttachment
     this.noPrint
     this.typeDebug
     // inaccessible: this.posAssigner

test/files/neg/nonunit-if.check

@@ -0,0 +1,57 @@
+nonunit-if.scala:58: warning: discarded non-Unit value of type U
+    if (!isEmpty) f(a)      // warn, check is on
+                   ^
+nonunit-if.scala:62: warning: discarded non-Unit value of type Boolean
+      f(a)                  // warn, check is on
+       ^
+nonunit-if.scala:13: warning: unused value of type scala.concurrent.Future[Int] (add `: Unit` to discard silently)
+    improved              // warn
+    ^
+nonunit-if.scala:20: warning: unused value of type String (add `: Unit` to discard silently)
+    new E().toString      // warn
+            ^
+nonunit-if.scala:26: warning: unused value of type scala.concurrent.Future[Int] (add `: Unit` to discard silently)
+  Future(42)              // warn
+        ^
+nonunit-if.scala:30: warning: unused value of type K (add `: Unit` to discard silently)
+  copy()                  // warn
+      ^
+nonunit-if.scala:37: warning: unused value of type List[Int] (add `: Unit` to discard silently)
+  27 +: xs                // warn
+     ^
+nonunit-if.scala:44: warning: a pure expression does nothing in statement position; multiline expressions may require enclosing parentheses
+  null                    // warn for purity
+  ^
+nonunit-if.scala:58: warning: unused value of type U (add `: Unit` to discard silently)
+    if (!isEmpty) f(a)      // warn, check is on
+                   ^
+nonunit-if.scala:62: warning: unused value of type Boolean (add `: Unit` to discard silently)
+      f(a)                  // warn, check is on
+       ^
+nonunit-if.scala:73: warning: unused value of type U (add `: Unit` to discard silently)
+    if (!fellback) action(z)  // warn, check is on
+                         ^
+nonunit-if.scala:81: warning: unused value of type Int (add `: Unit` to discard silently)
+      g   // warn, check is on
+      ^
+nonunit-if.scala:79: warning: unused value of type Int (add `: Unit` to discard silently)
+      g   // warn block statement
+      ^
+nonunit-if.scala:86: warning: unused value of type Int (add `: Unit` to discard silently)
+      g   // warn
+      ^
+nonunit-if.scala:84: warning: unused value of type Int (add `: Unit` to discard silently)
+      g   // warn
+      ^
+nonunit-if.scala:96: warning: unused value of type Int (add `: Unit` to discard silently)
+    if (b) {          // warn, at least one branch looks interesting
+    ^
+nonunit-if.scala:116: warning: unused value of type scala.collection.mutable.LinkedHashSet[A] (add `: Unit` to discard silently)
+    set += a     // warn because cannot know whether the `set` was supposed to be consumed or assigned
+        ^
+nonunit-if.scala:146: warning: unused value of type String (add `: Unit` to discard silently)
+    while (it.hasNext) it.next()  // warn
+                              ^
+error: No warnings can be incurred under -Werror.
+18 warnings
+1 error