Improve message for discarded pure non-Unit values

Fixes scala#18722
dotty-staging · Oct 19, 2023 · 23594b1 · 23594b1
1 parent f62429b
commit 23594b1
Show file tree

Hide file tree

Showing 8 changed files with 87 additions and 21 deletions.
diff --git a/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala b/compiler/src/dotty/tools/dotc/reporting/ErrorMessageID.scala
@@ -202,6 +202,7 @@ enum ErrorMessageID(val isActive: Boolean = true) extends java.lang.Enum[ErrorMe
   case ImplausiblePatternWarningID // erorNumber: 186
   case SynchronizedCallOnBoxedClassID // errorNumber: 187
   case VarArgsParamCannotBeGivenID // erorNumber: 188
+  case PureUnitExpressionID // erorNumber: 189
 
   def errorNumber = ordinal - 1
 

diff --git a/compiler/src/dotty/tools/dotc/reporting/messages.scala b/compiler/src/dotty/tools/dotc/reporting/messages.scala
@@ -2382,7 +2382,7 @@ class MemberWithSameNameAsStatic()(using Context)
   def explain(using Context) = ""
 }
 
-class PureExpressionInStatementPosition(stat: untpd.Tree, val exprOwner: Symbol)(using Context)
+class PureExpressionInStatementPosition(stat: untpd.Tree)(using Context)
   extends Message(PureExpressionInStatementPositionID) {
   def kind = MessageKind.PotentialIssue
   def msg(using Context) = "A pure expression does nothing in statement position; you may be omitting necessary parentheses"
@@ -2391,6 +2391,16 @@ class PureExpressionInStatementPosition(stat: untpd.Tree, val exprOwner: Symbol)
         |It can be removed without changing the semantics of the program. This may indicate an error."""
 }
 
+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 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.
+       |Therefore the expression is effectively equivalent to `()`."""
+}
+
 class UnqualifiedCallToAnyRefMethod(stat: untpd.Tree, method: Symbol)(using Context)
   extends Message(UnqualifiedCallToAnyRefMethodID) {
   def kind = MessageKind.PotentialIssue

diff --git a/compiler/src/dotty/tools/dotc/typer/Typer.scala b/compiler/src/dotty/tools/dotc/typer/Typer.scala
@@ -3332,7 +3332,7 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
         traverse(xtree :: rest)
       case stat :: rest =>
         val stat1 = typed(stat)(using ctx.exprContext(stat, exprOwner))
-        if !Linter.warnOnInterestingResultInStatement(stat1) then checkStatementPurity(stat1)(stat, exprOwner)
+        if !Linter.warnOnInterestingResultInStatement(stat1) then checkStatementPurity(stat1)(stat)
         buf += stat1
         traverse(rest)(using stat1.nullableContext)
       case nil =>
@@ -4212,7 +4212,7 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
         // local adaptation makes sure every adapted tree conforms to its pt
         // so will take the code path that decides on inlining
         val tree1 = adapt(tree, WildcardType, locked)
-        checkStatementPurity(tree1)(tree, ctx.owner)
+        checkStatementPurity(tree1)(tree, isUnitExpr = true)
         if (!ctx.isAfterTyper && !tree.isInstanceOf[Inlined] && ctx.settings.WvalueDiscard.value && !isThisTypeResult(tree)) {
           report.warning(ValueDiscarding(tree.tpe), tree.srcPos)
         }
@@ -4418,7 +4418,7 @@ class Typer(@constructorOnly nestingLevel: Int = 0) extends Namer
         typedExpr(cmp, defn.BooleanType)
       case _ =>
 
-  private def checkStatementPurity(tree: tpd.Tree)(original: untpd.Tree, exprOwner: Symbol)(using Context): Unit =
+  private def checkStatementPurity(tree: tpd.Tree)(original: untpd.Tree, isUnitExpr: Boolean = false)(using Context): Unit =
     if !tree.tpe.isErroneous
       && !ctx.isAfterTyper
       && !tree.isInstanceOf[Inlined]
@@ -4436,8 +4436,10 @@ 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 isUnitExpr =>
+        report.warning(PureUnitExpression(original, tree.tpe), original.srcPos)
       case _ =>
-        report.warning(PureExpressionInStatementPosition(original, exprOwner), original.srcPos)
+        report.warning(PureExpressionInStatementPosition(original), original.srcPos)
 
   /** Types the body Scala 2 macro declaration `def f = macro <body>` */
   protected def typedScala2MacroBody(call: untpd.Tree)(using Context): Tree =

diff --git a/tests/neg-custom-args/captures/real-try.check b/tests/neg-custom-args/captures/real-try.check
@@ -1,7 +1,7 @@
--- [E129] Potential Issue Warning: tests/neg-custom-args/captures/real-try.scala:36:4 ----------------------------------
+-- [E189] Potential Issue Warning: tests/neg-custom-args/captures/real-try.scala:36:4 ----------------------------------
 36 |  b.x
    |  ^^^
-   |  A pure expression does nothing in statement position; you may be omitting necessary parentheses
+   |  Discarded non-Unit value of type () -> Unit. You may want to use `()`.
    |
    | longer explanation available when compiling with `-explain`
 -- [E007] Type Mismatch Error: tests/neg-custom-args/captures/real-try.scala:19:4 --------------------------------------

diff --git a/tests/neg/i18722.check b/tests/neg/i18722.check
@@ -0,0 +1,44 @@
+-- [E189] Potential Issue Error: tests/neg/i18722.scala:3:15 -----------------------------------------------------------
+3 |def f1: Unit = null // error
+  |               ^^^^
+  |               Discarded non-Unit value of type Null. You may want to use `()`.
+  |---------------------------------------------------------------------------------------------------------------------
+  | Explanation (enabled by `-explain`)
+  |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+  | As this expression is not of type Unit, it is desugared into `{ null; () }`.
+  | Here the `null` expression is a pure statement that can be discarded.
+  | Therefore the expression is effectively equivalent to `()`.
+   ---------------------------------------------------------------------------------------------------------------------
+-- [E189] Potential Issue Error: tests/neg/i18722.scala:4:15 -----------------------------------------------------------
+4 |def f2: Unit = 1 // error
+  |               ^
+  |               Discarded non-Unit value of type Int. You may want to use `()`.
+  |---------------------------------------------------------------------------------------------------------------------
+  | Explanation (enabled by `-explain`)
+  |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+  | As this expression is not of type Unit, it is desugared into `{ 1; () }`.
+  | Here the `1` expression is a pure statement that can be discarded.
+  | Therefore the expression is effectively equivalent to `()`.
+   ---------------------------------------------------------------------------------------------------------------------
+-- [E189] Potential Issue Error: tests/neg/i18722.scala:5:15 -----------------------------------------------------------
+5 |def f3: Unit = "a" // error
+  |               ^^^
+  |               Discarded non-Unit value of type String. You may want to use `()`.
+  |---------------------------------------------------------------------------------------------------------------------
+  | Explanation (enabled by `-explain`)
+  |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+  | As this expression is not of type Unit, it is desugared into `{ "a"; () }`.
+  | Here the `"a"` expression is a pure statement that can be discarded.
+  | Therefore the expression is effectively equivalent to `()`.
+   ---------------------------------------------------------------------------------------------------------------------
+-- [E189] Potential Issue Error: tests/neg/i18722.scala:7:15 -----------------------------------------------------------
+7 |def f4: Unit = i // error
+  |               ^
+  |               Discarded non-Unit value of type Int. You may want to use `()`.
+  |---------------------------------------------------------------------------------------------------------------------
+  | Explanation (enabled by `-explain`)
+  |- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+  | As this expression is not of type Unit, it is desugared into `{ i; () }`.
+  | Here the `i` expression is a pure statement that can be discarded.
+  | Therefore the expression is effectively equivalent to `()`.
+   ---------------------------------------------------------------------------------------------------------------------
diff --git a/tests/neg/i18722.scala b/tests/neg/i18722.scala
@@ -0,0 +1,9 @@
+//> using options -Werror -explain
+
+def f1: Unit = null // error
+def f2: Unit = 1 // error
+def f3: Unit = "a" // error
+val i: Int = 1
+def f4: Unit = i // error
+val u: Unit = ()
+def f5: Unit = u
diff --git a/tests/neg/spaces-vs-tabs.check b/tests/neg/spaces-vs-tabs.check
@@ -28,9 +28,9 @@
    |	  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
--- [E129] Potential Issue Warning: tests/neg/spaces-vs-tabs.scala:13:7 -------------------------------------------------
+-- [E189] Potential Issue Warning: tests/neg/spaces-vs-tabs.scala:13:7 -------------------------------------------------
 13 |	      1
    |	      ^
-   |	      A pure expression does nothing in statement position; you may be omitting necessary parentheses
+   |	      Discarded non-Unit value of type Int. You may want to use `()`.
    |
    | longer explanation available when compiling with `-explain`
diff --git a/tests/neg/syntax-error-recovery.check b/tests/neg/syntax-error-recovery.check
@@ -94,45 +94,45 @@
    |          Not found: bam
    |
    | longer explanation available when compiling with `-explain`
--- [E129] Potential Issue Warning: tests/neg/syntax-error-recovery.scala:7:2 -------------------------------------------
+-- [E189] Potential Issue Warning: tests/neg/syntax-error-recovery.scala:7:2 -------------------------------------------
 6 |        2
 7 |  }
   |         ^
-  |         A pure expression does nothing in statement position; you may be omitting necessary parentheses
+  |         Discarded non-Unit value of type Null. You may want to use `()`.
   |
   | longer explanation available when compiling with `-explain`
--- [E129] Potential Issue Warning: tests/neg/syntax-error-recovery.scala:15:2 ------------------------------------------
+-- [E189] Potential Issue Warning: tests/neg/syntax-error-recovery.scala:15:2 ------------------------------------------
 14 |        2
 15 |  println(baz) // error
    |         ^
-   |         A pure expression does nothing in statement position; you may be omitting necessary parentheses
+   |         Discarded non-Unit value of type Null. You may want to use `()`.
    |
    | longer explanation available when compiling with `-explain`
--- [E129] Potential Issue Warning: tests/neg/syntax-error-recovery.scala:27:2 ------------------------------------------
+-- [E189] Potential Issue Warning: tests/neg/syntax-error-recovery.scala:27:2 ------------------------------------------
 26 |      2
 27 |  println(bam) // error
    |       ^
-   |       A pure expression does nothing in statement position; you may be omitting necessary parentheses
+   |       Discarded non-Unit value of type Null. You may want to use `()`.
    |
    | longer explanation available when compiling with `-explain`
--- [E129] Potential Issue Warning: tests/neg/syntax-error-recovery.scala:36:2 ------------------------------------------
+-- [E189] Potential Issue Warning: tests/neg/syntax-error-recovery.scala:36:2 ------------------------------------------
 35 |        2
 36 |  }
    |         ^
-   |         A pure expression does nothing in statement position; you may be omitting necessary parentheses
+   |         Discarded non-Unit value of type Null. You may want to use `()`.
    |
    | longer explanation available when compiling with `-explain`
--- [E129] Potential Issue Warning: tests/neg/syntax-error-recovery.scala:44:2 ------------------------------------------
+-- [E189] Potential Issue Warning: tests/neg/syntax-error-recovery.scala:44:2 ------------------------------------------
 43 |        2
 44 |  println(baz) // error
    |         ^
-   |         A pure expression does nothing in statement position; you may be omitting necessary parentheses
+   |         Discarded non-Unit value of type Null. You may want to use `()`.
    |
    | longer explanation available when compiling with `-explain`
--- [E129] Potential Issue Warning: tests/neg/syntax-error-recovery.scala:56:2 ------------------------------------------
+-- [E189] Potential Issue Warning: tests/neg/syntax-error-recovery.scala:56:2 ------------------------------------------
 55 |      2
 56 |  println(bam) // error
    |       ^
-   |       A pure expression does nothing in statement position; you may be omitting necessary parentheses
+   |       Discarded non-Unit value of type Null. You may want to use `()`.
    |
    | longer explanation available when compiling with `-explain`