Improve message for discarded pure non-Unit values (#18723)

Fixes #18408
Fixes #18722

Author: nicolasstucki

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

@@ -199,10 +199,11 @@ enum ErrorMessageID(val isActive: Boolean = true) extends java.lang.Enum[ErrorMe
   case ClosureCannotHaveInternalParameterDependenciesID // errorNumber: 183
   case MatchTypeNoCasesID // errorNumber: 184
   case UnimportedAndImportedID // errorNumber: 185
-  case ImplausiblePatternWarningID // erorNumber: 186
+  case ImplausiblePatternWarningID // errorNumber: 186
   case SynchronizedCallOnBoxedClassID // errorNumber: 187
-  case VarArgsParamCannotBeGivenID // erorNumber: 188
+  case VarArgsParamCannotBeGivenID // errorNumber: 188
   case ExtractorNotFoundID // errorNumber: 189
+  case PureUnitExpressionID // errorNumber: 190
 
   def errorNumber = ordinal - 1
 

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

@@ -2403,12 +2403,22 @@ class MemberWithSameNameAsStatic()(using Context)
 class PureExpressionInStatementPosition(stat: untpd.Tree, val exprOwner: Symbol)(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"
+  def msg(using Context) = "A pure expression does nothing in statement position"
   def explain(using Context) =
     i"""The pure expression $stat doesn't have any side effect and its result is not assigned elsewhere.
         |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

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

@@ -4226,7 +4226,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, ctx.owner, isUnitExpr = true)
         if (!ctx.isAfterTyper && !tree.isInstanceOf[Inlined] && ctx.settings.WvalueDiscard.value && !isThisTypeResult(tree)) {
           report.warning(ValueDiscarding(tree.tpe), tree.srcPos)
         }
@@ -4432,7 +4432,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, exprOwner: Symbol, isUnitExpr: Boolean = false)(using Context): Unit =
     if !tree.tpe.isErroneous
       && !ctx.isAfterTyper
       && !tree.isInstanceOf[Inlined]
@@ -4450,6 +4450,8 @@ 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)
 

compiler/test-resources/repl/nowarn.scala

@@ -27,7 +27,7 @@ scala> def f = { 1; 2 }
 -- [E129] Potential Issue Warning: ---------------------------------------------
 1 | def f = { 1; 2 }
   |           ^
-  |A pure expression does nothing in statement position; you may be omitting necessary parentheses
+  |           A pure expression does nothing in statement position
   |
   | longer explanation available when compiling with `-explain`
 def f: Int

language-server/test/dotty/tools/languageserver/DiagnosticsTest.scala

@@ -30,7 +30,7 @@ class DiagnosticsTest {
           |}"""
      .diagnostics(m1,
        (m1 to m2,
-        "A pure expression does nothing in statement position; you may be omitting necessary parentheses",
+        "A pure expression does nothing in statement position",
         Warning, Some(PureExpressionInStatementPositionID)))
 
   @Test def diagnosticWorksheetPureExpression: Unit =

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 ----------------------------------
+-- [E190] 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 --------------------------------------

tests/neg-macros/annot-suspend-cycle.check

@@ -1,7 +1,7 @@
 -- [E129] Potential Issue Warning: tests/neg-macros/annot-suspend-cycle/Macro.scala:7:4 --------------------------------
 7 |    new Foo
   |    ^^^^^^^
-  |    A pure expression does nothing in statement position; you may be omitting necessary parentheses
+  |    A pure expression does nothing in statement position
   |
   | longer explanation available when compiling with `-explain`
 Cyclic macro dependencies in tests/neg-macros/annot-suspend-cycle/Test.scala.

tests/neg/i18408a.check

@@ -0,0 +1,18 @@
+-- [E103] Syntax Error: tests/neg/i18408a.scala:2:0 --------------------------------------------------------------------
+2 |fa(42) // error
+  |^^
+  |Illegal start of toplevel definition
+  |
+  | longer explanation available when compiling with `-explain`
+-- [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 `()`.
+  |
+  | longer explanation available when compiling with `-explain`
+-- [E129] Potential Issue Warning: tests/neg/i18408a.scala:4:16 --------------------------------------------------------
+4 |def test2 = fa({42; ()})
+  |                ^^
+  |                A pure expression does nothing in statement position
+  |
+  | longer explanation available when compiling with `-explain`

tests/neg/i18408a.scala

@@ -0,0 +1,4 @@
+def fa(f: String ?=> Unit): Unit = ???
+fa(42) // error
+def test1 = fa(42)
+def test2 = fa({42; ()})

tests/neg/i18408b.check

@@ -0,0 +1,18 @@
+-- [E103] Syntax Error: tests/neg/i18408b.scala:2:0 --------------------------------------------------------------------
+2 |fa(42) // error
+  |^^
+  |Illegal start of toplevel definition
+  |
+  | longer explanation available when compiling with `-explain`
+-- [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 `()`.
+  |
+  | longer explanation available when compiling with `-explain`
+-- [E129] Potential Issue Warning: tests/neg/i18408b.scala:4:16 --------------------------------------------------------
+4 |def test2 = fa({42; ()})
+  |                ^^
+  |                A pure expression does nothing in statement position
+  |
+  | longer explanation available when compiling with `-explain`