Allow discarding "Discarded non-Unit" warnings with : Unit (#21927)

mbovel · web-flow · commit 8fb27f1a34d2 · 2024-11-22T14:17:47.000+01:00
Continuation of the work done by @nmcb and @rochala during the Oct 20th spree. This PR adds support for discarding "non-Unit" expressions by explicitly casting them to `Unit`: ```scala def test: Unit = (1 + 1): Unit // no warning 1 + 1 // warn ``` Closes #21557.
diff --git a/compiler/src/dotty/tools/dotc/reporting/messages.scala b/compiler/src/dotty/tools/dotc/reporting/messages.scala
@@ -2469,7 +2469,7 @@ class PureExpressionInStatementPosition(stat: untpd.Tree, val exprOwner: Symbol)
 class PureUnitExpression(stat: untpd.Tree, tpe: Type)(using Context)
   extends Message(PureUnitExpressionID) {
   def kind = MessageKind.PotentialIssue
-  def msg(using Context) = i"Discarded non-Unit value of type ${tpe.widen}. You may want to use `()`."
+  def msg(using Context) = i"Discarded non-Unit value of type ${tpe.widen}. Add `: Unit` to discard silently."
   def explain(using Context) =
     i"""As this expression is not of type Unit, it is desugared into `{ $stat; () }`.
        |Here the `$stat` expression is a pure statement that can be discarded.
@@ -3173,7 +3173,7 @@ class InlinedAnonClassWarning()(using Context)
 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 msg(using Context) = i"discarded non-Unit value of type ${tp.widen}. Add `: Unit` to discard silently."
     def explain(using Context) = ""
 
 class UnusedNonUnitValue(tp: Type)(using Context)
diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala
@@ -81,6 +81,9 @@ object Typer {
   /** Indicates that a definition was copied over from the parent refinements */
   val RefinementFromParent = new Property.StickyKey[Unit]
 
+  /** Indicates that an expression is explicitly ascribed to [[Unit]] type. */
+  val AscribedToUnit = new Property.StickyKey[Unit]
+
   /** An attachment on a Select node with an `apply` field indicating that the `apply`
    *  was inserted by the Typer.
    */
@@ -1193,7 +1196,10 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
         else tpt
       val expr1 =
         if isWildcard then tree.expr.withType(underlyingTreeTpe.tpe)
-        else typed(tree.expr, underlyingTreeTpe.tpe.widenSkolem)
+        else
+          if underlyingTreeTpe.tpe.isRef(defn.UnitClass) then
+            untpd.unsplice(tree.expr).putAttachment(AscribedToUnit, ())
+          typed(tree.expr, underlyingTreeTpe.tpe.widenSkolem)
       assignType(cpy.Typed(tree)(expr1, tpt), underlyingTreeTpe)
         .withNotNullInfo(expr1.notNullInfo)
     }
@@ -3374,7 +3380,7 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
       else if (ctx.mode.is(Mode.Pattern))
         typedUnApply(cpy.Apply(tree)(op, l :: r :: Nil), pt)
       else {
-        val app = typedApply(desugar.binop(l, op, r), pt)
+        val app = typedApply(desugar.binop(l, op, r).withAttachmentsFrom(tree), pt)
         if op.name.isRightAssocOperatorName && !ctx.mode.is(Mode.QuotedExprPattern) then
           val defs = new mutable.ListBuffer[Tree]
           def lift(app: Tree): Tree = (app: @unchecked) match
@@ -4578,9 +4584,14 @@ 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, isUnitExpr = true)
-        if (!ctx.isAfterTyper && !tree.isInstanceOf[Inlined] && ctx.settings.Whas.valueDiscard && !isThisTypeResult(tree)) {
+
+        if ctx.settings.Whas.valueDiscard
+           && !ctx.isAfterTyper
+           && !tree.isInstanceOf[Inlined]
+           && !isThisTypeResult(tree)
+           && !tree.hasAttachment(AscribedToUnit) then
           report.warning(ValueDiscarding(tree.tpe), tree.srcPos)
-        }
+
         return tpd.Block(tree1 :: Nil, unitLiteral)
       }
 
@@ -4858,6 +4869,9 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
         // sometimes we do not have the original anymore and use the transformed tree instead.
         // But taken together, the two criteria are quite accurate.
         missingArgs(tree, tree.tpe.widen)
+      case _ if tree.hasAttachment(AscribedToUnit) =>
+        // The tree was ascribed to `Unit` explicitly to silence the warning.
+        ()
       case _ if isUnitExpr =>
         report.warning(PureUnitExpression(original, tree.tpe), original.srcPos)
       case _ =>
diff --git a/tests/neg-custom-args/captures/real-try.check b/tests/neg-custom-args/captures/real-try.check
@@ -1,7 +1,7 @@
 -- [E190] Potential Issue Warning: tests/neg-custom-args/captures/real-try.scala:38:4 ----------------------------------
 38 |  b.x
    |  ^^^
-   |  Discarded non-Unit value of type () -> Unit. You may want to use `()`.
+   |  Discarded non-Unit value of type () -> Unit. Add `: Unit` to discard silently.
    |
    | longer explanation available when compiling with `-explain`
 -- Error: tests/neg-custom-args/captures/real-try.scala:14:2 -----------------------------------------------------------
diff --git a/tests/neg/i13091.check b/tests/neg/i13091.check
@@ -1,15 +1,9 @@
--- [E190] Potential Issue Warning: tests/neg/i13091.scala:7:17 ---------------------------------------------------------
-7 |def test: Unit = new Foo // error: class Foo is marked @experimental ...
-  |                 ^^^^^^^
-  |                 Discarded non-Unit value of type Foo. You may want to use `()`.
+-- Error: tests/neg/i13091.scala:7:16 ----------------------------------------------------------------------------------
+7 |def test = (new Foo): Unit // error: class Foo is marked @experimental ...
+  |                ^^^
+  |                class Foo is marked @experimental
   |
-  | longer explanation available when compiling with `-explain`
--- Error: tests/neg/i13091.scala:7:21 ----------------------------------------------------------------------------------
-7 |def test: Unit = new Foo // error: class Foo is marked @experimental ...
-  |                     ^^^
-  |                     class Foo is marked @experimental
-  |
-  |                     Experimental definition may only be used under experimental mode:
-  |                       1. in a definition marked as @experimental, or
-  |                       2. an experimental feature is imported at the package level, or
-  |                       3. compiling with the -experimental compiler flag.
+  |                Experimental definition may only be used under experimental mode:
+  |                  1. in a definition marked as @experimental, or
+  |                  2. an experimental feature is imported at the package level, or
+  |                  3. compiling with the -experimental compiler flag.
diff --git a/tests/neg/i13091.scala b/tests/neg/i13091.scala
@@ -4,4 +4,4 @@ import annotation.experimental
 
 @experimental class Foo
 
-def test: Unit = new Foo // error: class Foo is marked @experimental ...
+def test = (new Foo): Unit // error: class Foo is marked @experimental ...
diff --git a/tests/neg/i18408a.check b/tests/neg/i18408a.check
@@ -7,7 +7,7 @@
 -- [E190] Potential Issue Warning: tests/neg/i18408a.scala:3:15 --------------------------------------------------------
 3 |def test1 = fa(42)
   |               ^^
-  |               Discarded non-Unit value of type Int. You may want to use `()`.
+  |               Discarded non-Unit value of type Int. Add `: Unit` to discard silently.
   |
   | longer explanation available when compiling with `-explain`
 -- [E129] Potential Issue Warning: tests/neg/i18408a.scala:4:16 --------------------------------------------------------
diff --git a/tests/neg/i18408b.check b/tests/neg/i18408b.check
@@ -7,7 +7,7 @@
 -- [E190] Potential Issue Warning: tests/neg/i18408b.scala:3:15 --------------------------------------------------------
 3 |def test1 = fa(42)
   |               ^^
-  |               Discarded non-Unit value of type Int. You may want to use `()`.
+  |               Discarded non-Unit value of type Int. Add `: Unit` to discard silently.
   |
   | longer explanation available when compiling with `-explain`
 -- [E129] Potential Issue Warning: tests/neg/i18408b.scala:4:16 --------------------------------------------------------
diff --git a/tests/neg/i18408c.check b/tests/neg/i18408c.check
@@ -7,7 +7,7 @@
 -- [E190] Potential Issue Warning: tests/neg/i18408c.scala:3:15 --------------------------------------------------------
 3 |def test1 = fa(42)
   |               ^^
-  |               Discarded non-Unit value of type Int. You may want to use `()`.
+  |               Discarded non-Unit value of type Int. Add `: Unit` to discard silently.
   |
   | longer explanation available when compiling with `-explain`
 -- [E129] Potential Issue Warning: tests/neg/i18408c.scala:4:16 --------------------------------------------------------
diff --git a/tests/neg/spaces-vs-tabs.check b/tests/neg/spaces-vs-tabs.check
@@ -28,9 +28,3 @@
    |	  The start of this line does not match any of the previous indentation widths.
    |	  Indentation width of current line : 1 tab, 2 spaces
    |	  This falls between previous widths: 1 tab and 1 tab, 4 spaces
--- [E190] Potential Issue Warning: tests/neg/spaces-vs-tabs.scala:13:7 -------------------------------------------------
-13 |	      1
-   |	      ^
-   |	      Discarded non-Unit value of type Int. You may want to use `()`.
-   |
-   | longer explanation available when compiling with `-explain`
diff --git a/tests/neg/spaces-vs-tabs.scala b/tests/neg/spaces-vs-tabs.scala
@@ -10,6 +10,6 @@ object Test:
 	object Test2:
 
 	    if true then
-	      1
+	      ()
 	  else 2 // error
 
diff --git a/tests/warn/21557.check b/tests/warn/21557.check
@@ -0,0 +1,18 @@
+-- [E190] Potential Issue Warning: tests/warn/21557.scala:9:16 ---------------------------------------------------------
+9 |  val x: Unit = 1 + 1 // warn
+  |                ^^^^^
+  |                Discarded non-Unit value of type Int. Add `: Unit` to discard silently.
+  |
+  | longer explanation available when compiling with `-explain`
+-- [E176] Potential Issue Warning: tests/warn/21557.scala:10:2 ---------------------------------------------------------
+10 |  1 + 1 // warn
+   |  ^^^^^
+   |  unused value of type (2 : Int)
+-- [E175] Potential Issue Warning: tests/warn/21557.scala:15:52 --------------------------------------------------------
+15 |  val x1: Unit = new Assertion("another").shouldPass() // warn (enabled by -Wvalue-discard)
+   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |                 discarded non-Unit value of type Assertion. Add `: Unit` to discard silently.
+-- [E176] Potential Issue Warning: tests/warn/21557.scala:16:41 --------------------------------------------------------
+16 |  new Assertion("yet another").shouldPass() // warn (enabled by -Wnonunit-statement)
+   |  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |  unused value of type Assertion
diff --git a/tests/warn/21557.scala b/tests/warn/21557.scala
@@ -0,0 +1,19 @@
+//> using options -Wvalue-discard -Wnonunit-statement
+
+class Assertion(assert: => Any):
+  def shouldPass(): Assertion = ???
+
+def test: Unit =
+  1 + 1: Unit
+  (1 + 1): Unit
+  val x: Unit = 1 + 1 // warn
+  1 + 1 // warn
+  val y: Int = 1 + 1
+
+  new Assertion("").shouldPass(): Unit
+  (new Assertion("").shouldPass()): Unit
+  val x1: Unit = new Assertion("another").shouldPass() // warn (enabled by -Wvalue-discard)
+  new Assertion("yet another").shouldPass() // warn (enabled by -Wnonunit-statement)
+  val y1: Assertion = new Assertion("another other").shouldPass()
+
+  ()
diff --git a/tests/warn/i18722.check b/tests/warn/i18722.check
@@ -1,7 +1,7 @@
 -- [E190] Potential Issue Warning: tests/warn/i18722.scala:3:15 --------------------------------------------------------
 3 |def f1: Unit = null // warn
   |               ^^^^
-  |               Discarded non-Unit value of type Null. You may want to use `()`.
+  |               Discarded non-Unit value of type Null. Add `: Unit` to discard silently.
   |---------------------------------------------------------------------------------------------------------------------
   | Explanation (enabled by `-explain`)
   |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
@@ -12,7 +12,7 @@
 -- [E190] Potential Issue Warning: tests/warn/i18722.scala:4:15 --------------------------------------------------------
 4 |def f2: Unit = 1 // warn
   |               ^
-  |               Discarded non-Unit value of type Int. You may want to use `()`.
+  |               Discarded non-Unit value of type Int. Add `: Unit` to discard silently.
   |---------------------------------------------------------------------------------------------------------------------
   | Explanation (enabled by `-explain`)
   |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
@@ -23,7 +23,7 @@
 -- [E190] Potential Issue Warning: tests/warn/i18722.scala:5:15 --------------------------------------------------------
 5 |def f3: Unit = "a" // warn
   |               ^^^
-  |               Discarded non-Unit value of type String. You may want to use `()`.
+  |               Discarded non-Unit value of type String. Add `: Unit` to discard silently.
   |---------------------------------------------------------------------------------------------------------------------
   | Explanation (enabled by `-explain`)
   |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
@@ -34,7 +34,7 @@
 -- [E190] Potential Issue Warning: tests/warn/i18722.scala:7:15 --------------------------------------------------------
 7 |def f4: Unit = i // warn
   |               ^
-  |               Discarded non-Unit value of type Int. You may want to use `()`.
+  |               Discarded non-Unit value of type Int. Add `: Unit` to discard silently.
   |---------------------------------------------------------------------------------------------------------------------
   | Explanation (enabled by `-explain`)
   |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
diff --git a/tests/warn/nonunit-statement.check b/tests/warn/nonunit-statement.check
@@ -27,31 +27,31 @@
 -- [E175] Potential Issue Warning: tests/warn/nonunit-statement.scala:58:19 --------------------------------------------
 58 |    if (!isEmpty) f(a)      // warn (if)
    |                  ^^^^
-   |                  discarded non-Unit value of type U
+   |                  discarded non-Unit value of type U. Add `: Unit` to discard silently.
 -- [E175] Potential Issue Warning: tests/warn/nonunit-statement.scala:62:7 ---------------------------------------------
 62 |      f(a)                  // warn (if)
    |      ^^^^
-   |      discarded non-Unit value of type Boolean
+   |      discarded non-Unit value of type Boolean. Add `: Unit` to discard silently.
 -- [E175] Potential Issue Warning: tests/warn/nonunit-statement.scala:73:25 --------------------------------------------
 73 |    if (!fellback) action(z)  // warn (if)
    |                   ^^^^^^^^^
-   |                   discarded non-Unit value of type U
+   |                   discarded non-Unit value of type U. Add `: Unit` to discard silently.
 -- [E176] Potential Issue Warning: tests/warn/nonunit-statement.scala:79:6 ---------------------------------------------
 79 |      g   // warn block statement
    |      ^
    |      unused value of type (g : => Int)
 -- [E175] Potential Issue Warning: tests/warn/nonunit-statement.scala:81:6 ---------------------------------------------
 81 |      g   // warn (if)
    |      ^
-   |      discarded non-Unit value of type (g : => Int)
+   |      discarded non-Unit value of type Int. Add `: Unit` to discard silently.
 -- [E176] Potential Issue Warning: tests/warn/nonunit-statement.scala:84:6 ---------------------------------------------
 84 |      g   // warn
    |      ^
    |      unused value of type (g : => Int)
 -- [E175] Potential Issue Warning: tests/warn/nonunit-statement.scala:86:6 ---------------------------------------------
 86 |      g   // warn
    |      ^
-   |      discarded non-Unit value of type (g : => Int)
+   |      discarded non-Unit value of type Int. Add `: Unit` to discard silently.
 -- [E176] Potential Issue Warning: tests/warn/nonunit-statement.scala:96:4 ---------------------------------------------
  96 |    if (b) {          // warn, at least one branch looks interesting
     |    ^
@@ -70,20 +70,20 @@
 -- [E175] Potential Issue Warning: tests/warn/nonunit-statement.scala:126:37 -------------------------------------------
 126 |    if (start.length != 0) jsb.append(start) // warn (value-discard)
     |                           ^^^^^^^^^^^^^^^^^
-    |                           discarded non-Unit value of type StringBuilder
+    |                           discarded non-Unit value of type StringBuilder. Add `: Unit` to discard silently.
 -- [E175] Potential Issue Warning: tests/warn/nonunit-statement.scala:132:18 -------------------------------------------
 132 |        jsb.append(it.next()) // warn (value-discard)
     |        ^^^^^^^^^^^^^^^^^^^^^
-    |        discarded non-Unit value of type StringBuilder
+    |        discarded non-Unit value of type StringBuilder. Add `: Unit` to discard silently.
 -- [E175] Potential Issue Warning: tests/warn/nonunit-statement.scala:135:35 -------------------------------------------
 135 |    if (end.length != 0) jsb.append(end) // warn (value-discard)
     |                         ^^^^^^^^^^^^^^^
-    |                         discarded non-Unit value of type StringBuilder
+    |                         discarded non-Unit value of type StringBuilder. Add `: Unit` to discard silently.
 -- [E175] Potential Issue Warning: tests/warn/nonunit-statement.scala:141:14 -------------------------------------------
 141 |      b.append(it.next()) // warn (value-discard)
     |      ^^^^^^^^^^^^^^^^^^^
-    |      discarded non-Unit value of type StringBuilder
+    |      discarded non-Unit value of type StringBuilder. Add `: Unit` to discard silently.
 -- [E175] Potential Issue Warning: tests/warn/nonunit-statement.scala:146:30 -------------------------------------------
 146 |    while (it.hasNext) it.next()  // warn
     |                       ^^^^^^^^^
-    |                       discarded non-Unit value of type String
+    |                       discarded non-Unit value of type String. Add `: Unit` to discard silently.
diff --git a/tests/warn/warn-value-discard.check b/tests/warn/warn-value-discard.check
@@ -1,20 +1,20 @@
 -- [E175] Potential Issue Warning: tests/warn/warn-value-discard.scala:27:36 -------------------------------------------
 27 |    mutable.Set.empty[String].remove("") // warn
    |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-   |    discarded non-Unit value of type Boolean
+   |    discarded non-Unit value of type Boolean. Add `: Unit` to discard silently.
 -- [E175] Potential Issue Warning: tests/warn/warn-value-discard.scala:39:41 -------------------------------------------
 39 |    mutable.Set.empty[String].subtractOne("") // warn
    |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-   |    discarded non-Unit value of type scala.collection.mutable.Set[String]
+   |    discarded non-Unit value of type scala.collection.mutable.Set[String]. Add `: Unit` to discard silently.
 -- [E175] Potential Issue Warning: tests/warn/warn-value-discard.scala:59:4 --------------------------------------------
 59 |    mutable.Set.empty[String] += "" // warn
    |    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
-   |    discarded non-Unit value of type scala.collection.mutable.Set[String]
+   |    discarded non-Unit value of type scala.collection.mutable.Set[String]. Add `: Unit` to discard silently.
 -- [E175] Potential Issue Warning: tests/warn/warn-value-discard.scala:15:35 -------------------------------------------
 15 |  firstThing().map(_ => secondThing()) // warn
    |                        ^^^^^^^^^^^^^
-   |                        discarded non-Unit value of type Either[Failed, Unit]
+   |                        discarded non-Unit value of type Either[Failed, Unit]. Add `: Unit` to discard silently.
 -- [E175] Potential Issue Warning: tests/warn/warn-value-discard.scala:18:35 -------------------------------------------
 18 |  firstThing().map(_ => secondThing()) // warn
    |                        ^^^^^^^^^^^^^
-   |                        discarded non-Unit value of type Either[Failed, Unit]
+   |                        discarded non-Unit value of type Either[Failed, Unit]. Add `: Unit` to discard silently.

Original file line number	Diff line number	Diff line change
`@@ -1,7 +1,7 @@`
`1`	`1`	`-- [E190] Potential Issue Warning: tests/neg-custom-args/captures/real-try.scala:38:4 ----------------------------------`
`2`	`2`	`38 \| b.x`
`3`	`3`	`\| ^^^`
`4`		- \| Discarded non-Unit value of type () -> Unit. You may want to use `()`.
	`4`	+ \| Discarded non-Unit value of type () -> Unit. Add `: Unit` to discard silently.
`5`	`5`	`\|`
`6`	`6`	\| longer explanation available when compiling with `-explain`
`7`	`7`	`-- Error: tests/neg-custom-args/captures/real-try.scala:14:2 -----------------------------------------------------------`
Original file line number	Diff line number	Diff line change
`@@ -4,4 +4,4 @@ import annotation.experimental`
`4`	`4`
`5`	`5`	`@experimental class Foo`
`6`	`6`
`7`		`-def test: Unit = new Foo // error: class Foo is marked @experimental ...`
	`7`	`+def test = (new Foo): Unit // error: class Foo is marked @experimental ...`
Original file line number	Diff line number	Diff line change
`@@ -7,7 +7,7 @@`
`7`	`7`	`-- [E190] Potential Issue Warning: tests/neg/i18408a.scala:3:15 --------------------------------------------------------`
`8`	`8`	`3 \|def test1 = fa(42)`
`9`	`9`	`\| ^^`
`10`		- \| Discarded non-Unit value of type Int. You may want to use `()`.
	`10`	+ \| Discarded non-Unit value of type Int. Add `: Unit` to discard silently.
`11`	`11`	`\|`
`12`	`12`	\| longer explanation available when compiling with `-explain`
`13`	`13`	`-- [E129] Potential Issue Warning: tests/neg/i18408a.scala:4:16 --------------------------------------------------------`