Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Nov 22, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions compiler/src/dotty/tools/dotc/reporting/messages.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's create actionable diagnostics in the next spree for both diagnostics

def explain(using Context) = ""

class UnusedNonUnitValue(tp: Type)(using Context)
Expand Down
22 changes: 18 additions & 4 deletions compiler/src/dotty/tools/dotc/typer/Typer.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*/
Expand Down Expand Up @@ -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, ())
Copy link
Member Author

@mbovel mbovel Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

untpd.unsplice is needed to ignore parentheses (so that (1 + 1): Unit also silents the warning, not only 1 + 1: Unit).

typed(tree.expr, underlyingTreeTpe.tpe.widenSkolem)
assignType(cpy.Typed(tree)(expr1, tpt), underlyingTreeTpe)
.withNotNullInfo(expr1.notNullInfo)
}
Expand Down Expand Up @@ -3377,7 +3383,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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed to add withAttachmentsFrom(tree) here so that we don't throw away attachments when desugaring binary operations.

if op.name.isRightAssocOperatorName && !ctx.mode.is(Mode.QuotedExprPattern) then
val defs = new mutable.ListBuffer[Tree]
def lift(app: Tree): Tree = (app: @unchecked) match
Expand Down Expand Up @@ -4581,9 +4587,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)
}

Expand Down Expand Up @@ -4839,6 +4850,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 _ =>
Expand Down
2 changes: 1 addition & 1 deletion tests/neg-custom-args/captures/real-try.check
Original file line number Diff line number Diff line change
@@ -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 -----------------------------------------------------------
Expand Down
22 changes: 8 additions & 14 deletions tests/neg/i13091.check
Original file line number Diff line number Diff line change
@@ -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.
2 changes: 1 addition & 1 deletion tests/neg/i13091.scala
Original file line number Diff line number Diff line change
Expand Up @@ -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 ...
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoids the "non-Unit" warning, that is not directly related to this test.

2 changes: 1 addition & 1 deletion tests/neg/i18408a.check
Original file line number Diff line number Diff line change
Expand Up @@ -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 --------------------------------------------------------
Expand Down
2 changes: 1 addition & 1 deletion tests/neg/i18408b.check
Original file line number Diff line number Diff line change
Expand Up @@ -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 --------------------------------------------------------
Expand Down
2 changes: 1 addition & 1 deletion tests/neg/i18408c.check
Original file line number Diff line number Diff line change
Expand Up @@ -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 --------------------------------------------------------
Expand Down
6 changes: 0 additions & 6 deletions tests/neg/spaces-vs-tabs.check
Original file line number Diff line number Diff line change
Expand Up @@ -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`
2 changes: 1 addition & 1 deletion tests/neg/spaces-vs-tabs.scala
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,6 @@ object Test:
object Test2:

if true then
1
()
Copy link
Member Author

@mbovel mbovel Nov 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoids the "non-Unit" warning, that is not directly related to this test.

else 2 // error

18 changes: 18 additions & 0 deletions tests/warn/21557.check
Original file line number Diff line number Diff line change
@@ -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
19 changes: 19 additions & 0 deletions tests/warn/21557.scala
Original file line number Diff line number Diff line change
@@ -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()

()
8 changes: 4 additions & 4 deletions tests/warn/i18722.check
Original file line number Diff line number Diff line change
@@ -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`)
|- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
Expand All @@ -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`)
|- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
Expand All @@ -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`)
|- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
Expand All @@ -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`)
|- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
Expand Down
20 changes: 10 additions & 10 deletions tests/warn/nonunit-statement.check
Original file line number Diff line number Diff line change
Expand Up @@ -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
| ^
Expand All @@ -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.
10 changes: 5 additions & 5 deletions tests/warn/warn-value-discard.check
Original file line number Diff line number Diff line change
@@ -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.